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

[Rodrigo Alencar]

On 26/05/17 03:25PM, Jonathan Cameron wrote:
> On Tue, 5 May 2026 17:31:48 +0100
> Rodrigo Alencar <455.rodrigo.alencar@xxxxxxxxx> wrote:
> 
> > On 26/05/05 04:50PM, Rodrigo Alencar wrote:
> > > On 26/05/05 06:27PM, Andy Shevchenko wrote:  
> > > > 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.  
> > > 
> > > That is indeed better, thanks! will adjust...   
> > 
> > Now I see that the "over engineering" is that field_get()/field_prep() recomputes
> > the shift based on the provided mask, which was manually shifted. The u32_*_bits()
> > variants seem to rely on constant masks, as I get compile-time errors.
> 
> I still have these stalled on the fixes making it up stream (and into an rc)
> but in meantime can you confirm that I read this right and plan is leave this
> patch as it stands?

Yeah, I suppose we would not need to use field_get()/field_prep() because
they recompute the shift based on the mask, but we already create the mask
from the shift. I think the helpers are clear enough and there is no
real benefit on the change.

-- 
Kind regards,

Rodrigo Alencar