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

Rodrigo Alencar
2026-05-05T14:13:47Z

On 26/05/05 04:17PM, Andy Shevchenko wrote:
> 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);

We cannot do this as the mask would depend on the shift too. AD5686_PD_MSK is
being defined with no shift, so that any shift needs to be applied at runtime.

> > +}
> > +
> > +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.

Same here...

> > +}
>
> ...
>
> > - 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.

What would be the difference compared to this:

if (readin)
ad5686_pd_field_set(chan, &st->pwr_down_mask, AD5686_PD_MSK_PWR_DOWN);
else
ad5686_pd_field_set(chan, &st->pwr_down_mask, AD5686_PD_MSK_PWR_UP);

> --
> With Best Regards,
> Andy Shevchenko
>
>

--
Kind regards,

Rodrigo Alencar