Re: [PATCH v6 09/12] iio: dac: ad5686: add helpers to handle powerdown masks

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

On Tue, May 05, 2026 at 01:35:10PM +0100, Rodrigo Alencar via B4 Relay wrote:

> Add ad5686_pd_field_set() and ad5686_pd_field_get() helpers to cleanup
> powerdown mask control. Define AD5686_PD_* constants, e.g. AD5686_PD_MSK
> to hold powerdown mask value for a single channel. AD5686_LDAC_PWRDN_*
> macros are replaced by AD5686_PD_MODE_*, because they are unused and the
> LDAC feature for async load of DAC channel values is not related to power
> down control.

...

> +static inline void ad5686_pd_field_set(const struct iio_chan_spec *chan,
> +				       unsigned int *pd, unsigned int val)
> +{
> +	unsigned int shift = ad5686_pd_mask_shift(chan);
> +
> +	*pd = (*pd & ~(AD5686_PD_MSK << shift)) | ((val & AD5686_PD_MSK) << shift);

Just noticed that semantically this is more like _field_modify().
Besides that I would consider adding a shifted mask variable or definition


	*pd = (*pd & ~AD5686_PD_MSK) | ((val << shift) & AD5686_PD_MSK);

> +}
> +
> +static inline unsigned int ad5686_pd_field_get(const struct iio_chan_spec *chan,
> +					       unsigned int pd)
> +{
> +	unsigned int shift = ad5686_pd_mask_shift(chan);
> +
> +	return (pd >> shift) & AD5686_PD_MSK;

	return (pd & AD5686_PD_MSK) >> shift;

accordingly.

> +}

...

> -	if (readin)
> -		st->pwr_down_mask |= 0x3U << ad5686_pd_mask_shift(chan);
> -	else
> -		st->pwr_down_mask &= ~(0x3U << ad5686_pd_mask_shift(chan));
> +	ad5686_pd_field_set(chan, &st->pwr_down_mask,
> +			    readin ? AD5686_PD_MSK_PWR_DOWN : AD5686_PD_MSK_PWR_UP);

TBH, I would leave the if-else untouched, only branches to change.

-- 
With Best Regards,
Andy Shevchenko