Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Jonathan Cameron]

On Wed, 23 Apr 2025 19:52:13 +0300
Andy Shevchenko <andy@xxxxxxxxxx> wrote:

> On Wed, Apr 23, 2025 at 07:50:51AM +0000, Paller, Kim Seer wrote:
> > > From: Jonathan Cameron <jic23@xxxxxxxxxx>
> > > Sent: Monday, April 21, 2025 9:48 PM
> > > To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>
> > > On Mon, 21 Apr 2025 12:24:54 +0800
> > > Kim Seer Paller <kimseer.paller@xxxxxxxxxx> wrote:  
> 
> ...
> 
> > > > +	mask = GENMASK(chan->address + 1, chan->address);  
> > > 
> > > I think maybe we need a macro to get the mask from the channel number?
> > > Using address for this seems overkill given how simple that maths is.
> > > Ideally that macro could perhaps be used in the code below to avoid
> > > all the defines I suggested.  
> > 
> > The motivation for using the chan->address field was to hide the calculation a bit.
> > However, would using a macro like 
> > #define AD3530R_OP_MODE_CHAN_MSK(chan)	GENMASK(2 * chan + 1, 2 * chan) 
> > be a good approach in this case? This drops the need for the address field and
> > can also be used to explicitly set the operating mode for the 4 fields of the register.
> > What do you think?  
> 
> Please, note that doing GENMASK(foo + X, foo) is highly discouraged as it may
> give a very bad generated code (although I haven't checked recently if it's
> still the case). The preferred way is GENMASK(X, 0) << foo. Where X is a
> compile time constant.
> 

With what Andy suggested as the implementation, this sort of macro
looks like a good solution to me.

Jonathan