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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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.

> +}

...

> +	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?

...

> +	return sysfs_emit(buf, "%d\n", !!(st->pwr_down_mask & (0x3 << shift)));

At some point (I understand that this is a fix and probably we want to be less
intrusive) I think we want to see that 0x3 magic being defined and reused. This
GENMASK(1, 0) << shift seems an idiom that may deserve it's own macro / helper
function.

...

> +	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);

Too many parentheses.

> +	}

-- 
With Best Regards,
Andy Shevchenko