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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Apr 2026 18:02:19 +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>
Backed this one out again for now.  See sashiko review.
https://sashiko.dev/#/patchset/20260428-ad5686-fixes-v3-0-9cff7bd67a15%40analog.com

I took a look at the ad5310r (unhelpfully called ad5310 in the
majority of the driver - something it would be nice to fix!)
and there are some real oddities when comparing what we send
to that device with the datasheet.

Understanding not helped by the bit numbering changing on the
datasheet between tables 10 and 11 :( Feel free to suggest they fix that!

Anyhow, I think sashiko is correct and the stray 1111b will corrupt
the command bits and maybe even the reset.

I haven't yet asked for sashiko reviews to just be sent to the mailing
list, as I'm still personally getting comfortable with how accurate they
are - but I'm thinking about it. In meantime please do take a look
at them as they catch things like this (assuming it is real!)

Jonathan


> ---
>  drivers/iio/dac/ad5686.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 69358dd66cbc..b31d0ec5415c 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);
> +	st->pwr_down_mode |= (mode + 1) << shift;
>  
>  	return 0;
>  }
> @@ -57,10 +68,10 @@ static const struct iio_enum ad5686_powerdown_mode_enum = {
>  static ssize_t ad5686_read_dac_powerdown(struct iio_dev *indio_dev,
>  		uintptr_t private, const struct iio_chan_spec *chan, char *buf)
>  {
> +	unsigned int shift = ad5686_pd_mask_shift(chan);
>  	struct ad5686_state *st = iio_priv(indio_dev);
>  
> -	return sysfs_emit(buf, "%d\n", !!(st->pwr_down_mask &
> -				       (0x3 << (chan->channel * 2))));
> +	return sysfs_emit(buf, "%d\n", !!(st->pwr_down_mask & (0x3 << shift)));
>  }
>  
>  static ssize_t ad5686_write_dac_powerdown(struct iio_dev *indio_dev,
> @@ -82,9 +93,9 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev *indio_dev,
>  	guard(mutex)(&st->lock);
>  
>  	if (readin)
> -		st->pwr_down_mask |= (0x3 << (chan->channel * 2));
> +		st->pwr_down_mask |= 0x3 << ad5686_pd_mask_shift(chan);
>  	else
> -		st->pwr_down_mask &= ~(0x3 << (chan->channel * 2));
> +		st->pwr_down_mask &= ~(0x3 << ad5686_pd_mask_shift(chan));
>  
>  	switch (st->chip_info->regmap_type) {
>  	case AD5310_REGMAP:
> @@ -464,7 +475,7 @@ int ad5686_probe(struct device *dev,
>  {
>  	struct ad5686_state *st;
>  	struct iio_dev *indio_dev;
> -	unsigned int val, ref_bit_msk;
> +	unsigned int val, ref_bit_msk, shift;
>  	bool has_external_vref;
>  	u8 cmd;
>  	int ret, i;
> @@ -489,8 +500,14 @@ int ad5686_probe(struct device *dev,
>  	st->vref_mv = has_external_vref ? ret / 1000 : st->chip_info->int_vref_mv;
>  
>  	/* Set all the power down mode for all channels to 1K pulldown */
> -	for (i = 0; i < st->chip_info->num_channels; i++)
> -		st->pwr_down_mode |= (0x01 << (i * 2));
> +	st->pwr_down_mode = ~0U;
> +	st->pwr_down_mask = ~0U;
> +	for (i = 0; i < st->chip_info->num_channels; i++) {
> +		shift = ad5686_pd_mask_shift(&st->chip_info->channels[i]);
> +		st->pwr_down_mask &= ~(0x3 << shift); /* powered up state */
> +		st->pwr_down_mode &= ~(0x3 << shift);
> +		st->pwr_down_mode |= 0x01 << shift;
> +	}
>  
>  	indio_dev->name = name;
>  	indio_dev->info = &ad5686_info;
>