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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aS75V9Zb3xu5RTU6@smile.fi.intel.com>
Date: Tue, 2 Dec 2025 16:36:07 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Tomas Melin <tomas.melin@...sala.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Nuno Sa <nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Andy Shevchenko <andy@...nel.org>,
	Alexandru Ardelean <alexandru.ardelean@...log.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: ad9467: support write/read offset

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@...sala.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