Re: [PATCH 2/2] iio: adc: ad9467: support write/read offset

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Dec 02, 2025 at 12:03:21PM +0200, Tomas Melin wrote:
> On 01/12/2025 15:59, Andy Shevchenko wrote:
> > On Mon, Dec 1, 2025 at 2:00 PM Tomas Melin <tomas.melin@vaisala.com> wrote:

...

> >> +#define AD9434_CHAN(_chan, avai_mask, _si, _bits, _sign)               \
> >> +{                                                                      \
> >> +       .type = IIO_VOLTAGE,                                            \
> >> +       .indexed = 1,                                                   \
> >> +       .channel = _chan,                                               \
> >> +       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
> >> +               BIT(IIO_CHAN_INFO_SAMP_FREQ) |                          \
> >> +               BIT(IIO_CHAN_INFO_OFFSET),                              \
> >> +       .info_mask_shared_by_type_available = avai_mask,                \
> > 
> > Okay, this macro uses parameterized avai_mask (which should be spelled
> > correctly and probably in parentheses, but it's not the point) and
> > it's being called only once. Why can't we just embed it for now?
> 
> Sure, I'm ok with embedding if that is preferred.

I meant to embed the one of the _chan/_si parameter, not the entire macro.
And with comment about index vs. channel, it seems one of them is not needed
at all.

> >> +       .scan_index = _si,                                              \
> >> +       .scan_type = {                                                  \
> >> +               .sign = _sign,                                          \
> >> +               .realbits = _bits,                                      \
> >> +               .storagebits = 16,                                      \
> >> +       },                                                              \
> >> +}
> > 
> > Also, looking at the existing macro below, I think you should have a
> > common, parameterised macro and then 3 different on top of it for this
> > case, and for the existing two.
> > 
> > Does it make sense?
> Keeping this and embedding the ad9434 declaration.

Answered to v2. I think the macro approach is better due to consistency.

> >>  #define AD9467_CHAN(_chan, avai_mask, _si, _bits, _sign)               \

...

> >>  static const struct iio_chan_spec ad9434_channels[] = {
> >> -       AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> >> +       AD9434_CHAN(0, BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> >> +                   0, 12, 's'),
> >>  };
> > 
> > Also the first and third parameters are identical in all cases, can we
> > for now just make them using a single parameter?

...

> >> +       if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
> >> +               return -EINVAL;
> > 
> > Wondering if at some point we can switch to in_range(). And we perhaps
> > need a new generic macro to supply start/end instead of start/size.

> Atleast drop-in usage of in_range() does not seems feasible?

I know, that's why it starts with "wondering" and continues with "perhaps".

-- 
With Best Regards,
Andy Shevchenko