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

[Andy Shevchenko <andy@kernel.org>]

On Wed, Apr 23, 2025 at 07:50:51AM +0000, Paller, Kim Seer wrote:
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Monday, April 21, 2025 9:48 PM
> > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > On Mon, 21 Apr 2025 12:24:54 +0800
> > Kim Seer Paller <kimseer.paller@analog.com> 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 Best Regards,
Andy Shevchenko