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] [thread-next>] [day] [month] [year] [list]

Message-ID: <44a38485-ce29-45c8-8892-5000abb8d44e@vaisala.com>
Date: Tue, 2 Dec 2025 12:03:21 +0200
From: Tomas Melin <tomas.melin@...sala.com>
To: Andy Shevchenko <andy.shevchenko@...il.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

Hi Andy,

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

Sure, I'm ok with embedding if that is preferred.


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

> 
>>  #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;
ok, will change to this format.

> 
>> +       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.
Atleast drop-in usage of in_range() does not seems feasible?

Thanks,
Tomas


>