Re: [PATCH v2 04/11] iio: dac: ad5686: fix powerdown control on dual-channel devices

[Rodrigo Alencar]

On 26/04/27 09:02PM, Andy Shevchenko wrote:
> On Mon, Apr 27, 2026 at 12:30:11PM +0100, Rodrigo Alencar via B4 Relay wrote:
> 
> > Fix powerdown control by using a proper bit shift for the powerdown mask
> > values. During initialization, powerdown bits are initialized so that
> > unused bits are set to 1 and the correct bit shift is used. Dual-channel
> > devices use one-hot encoding in the address and that reflects on the
> > position of the powerdown bits, which are not channel-index based
> > for that case. Quad-channel devices also use one-hot encoding for the
> > channel address but the result of log2(address) coincides with the channel
> > index value. The issue was introduced when first adding support for
> > dual-channel devices, which overlooked powerdown control differences.
> 
> ...
> 
> > +static inline unsigned int ad5686_pd_mask_shift(const struct iio_chan_spec *chan)
> > +{
> > +	if (chan->channel == chan->address)
> > +		return chan->channel * 2;
> > +
> > +	/* one-hot encoding is used in dual/quad channel devices */
> > +	return __ffs(chan->address) * 2;
> 
> If channel->address guaranteed never be 0? This is UB otherwise.

All the possible values are managed within this driver, so this would not be 0,
if so, it would fall into the if statement above.
 
> > +}
> 
> ...
> 
> > +	st->pwr_down_mode &= ~(0x3 << shift);
> > +	st->pwr_down_mode |= ((mode + 1) << shift);
> 
> Now too many parentheses. Also do you guarantee that mode won't ever saturate
> the bits outside of the given mask?

ad5686_powerdown_modes enum has num_items = 3, so max value for mode is 2.

...

-- 
Kind regards,

Rodrigo Alencar