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: <CAHp75VdaYR5S7MmVEMBH1e6g-0bQim_d7m48=X2ybpf3hBAfTA@mail.gmail.com>
Date: Mon, 1 Dec 2025 15:59:50 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Tomas Melin <tomas.melin@...sala.com>
Cc: 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 Mon, Dec 1, 2025 at 2:00 PM Tomas Melin <tomas.melin@...sala.com> wrote:
>
> Support configuring output offset value. Among the devices
> currently supported by this driver, this setting is specific to
> ad9434.

...

> +#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?


> +       .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?

>  #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?

...

> +static int ad9467_get_offset(struct ad9467_state *st, int *val)
> +{
> +       *val = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
> +       if (*val < 0)
> +               return *val;

The standard pattern is to avoid polluting the output in case of known
errors. Hence

   int ret;

       ret = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
       if (ret < 0)
               return ret;

       *val = ret;

> +       return IIO_VAL_INT;
> +}


...

> +       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.

-- 
With Best Regards,
Andy Shevchenko