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

Andy Shevchenko
2026-05-05T15:27:49Z

On Tue, May 05, 2026 at 03:13:43PM +0100, Rodrigo Alencar wrote:
> 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:

...

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

In my proposal I thought of

#define AD5686_PD_MSK(shift) (GENMASK(...) << (shift))

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

But maybe it's over engineered, and your version is okay... Maybe we can even
switch to
field_get()/field_prep()/u32_replace_bits()/u32_encode_bits()/u32_get_bits()
from bitfield.h.

> > > +}

...

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

That's what I thought about...

--
With Best Regards,
Andy Shevchenko