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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 01 May 2026 10:14:57 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> 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.
> 
> Fixes: 7dc8faeab3e3 ("iio: dac: ad5686: add support for AD5338R")
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>

Just for reference I think the sashiko bug report on this one is spurious
due to -fwrapv (signed integer overflow being defined for kernel).

It is non obvious though!  So this one is fine but the guard() I'm suggesting
in the previous patch review to protect get_powerdown_mode() against races
would add some mess to this one so I won't apply it until that's addressed
(or argued against!)
> ---
>  drivers/iio/dac/ad5686.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 69358dd66cbc..c607251b82a0 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -25,24 +25,35 @@ static const char * const ad5686_powerdown_modes[] = {
>  	"three_state"
>  };
>  
> +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;
> +}
> +
>  static int ad5686_get_powerdown_mode(struct iio_dev *indio_dev,
>  				     const struct iio_chan_spec *chan)
>  {
> +	unsigned int shift = ad5686_pd_mask_shift(chan);
>  	struct ad5686_state *st = iio_priv(indio_dev);
>  
> -	return ((st->pwr_down_mode >> (chan->channel * 2)) & 0x3) - 1;
> +	return ((st->pwr_down_mode >> shift) & 0x3) - 1;
>  }
>  
>  static int ad5686_set_powerdown_mode(struct iio_dev *indio_dev,
>  				     const struct iio_chan_spec *chan,
>  				     unsigned int mode)
>  {
> +	unsigned int shift = ad5686_pd_mask_shift(chan);
>  	struct ad5686_state *st = iio_priv(indio_dev);
>  
>  	guard(mutex)(&st->lock);
>  
> -	st->pwr_down_mode &= ~(0x3 << (chan->channel * 2));
> -	st->pwr_down_mode |= ((mode + 1) << (chan->channel * 2));
> +	st->pwr_down_mode &= ~(0x3 << shift);
The bug reported is that 0x3 is singed and shift can be 30 which
would overflow. However that should be fine in the kernel.
Bit odd though so maybe force that 0x3 to be unsigned.


> +	st->pwr_down_mode |= (mode + 1) << shift;
>  
>  	return 0;
>  }