Re: [PATCH 12/22] iio: dac: ad5686: fix powerdown control

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

On Wed, Apr 22, 2026 at 03:45:46PM +0100, Rodrigo Alencar via B4 Relay wrote:

> This patch fixes powerdown control issues by protecting the cached

Please, read Submitting Patches documentation. It clearly says to use
imperative mood.

> powerdown states with mutex access, and 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 encondig 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 encondig for the
> channel address but the result of log2(address) coincides with the channel
> index value.

When I see a word "fix" and no Fixes tag I got confused. Please, align commit
message and tags (either make it just refactor, or a real fix, or explain at
bare minimum in the comments block why there is "fix" w/o Fixes tag).

...

>  {
>  	struct ad5686_state *st = iio_priv(indio_dev);
> +	int val, shift = ad5686_pd_mask_shift(chan);
>  
> -	return sysfs_emit(buf, "%d\n", !!(st->pwr_down_mask &
> -				       (0x3 << (chan->channel * 2))));
> +	mutex_lock(&st->lock);
> +	val = !!(st->pwr_down_mask & (0x3 << shift));
> +	mutex_unlock(&st->lock);
> +
> +	return sysfs_emit(buf, "%d\n", val);
>  }

Why not use guard()() from day 1?

...

>  	struct ad5686_state *st;
> -	int ret, i;
> +	int ret, i, shift;

Do 'i' and 'shift' need to be signed?

-- 
With Best Regards,
Andy Shevchenko