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

[Tomas Melin <tomas.melin@vaisala.com>]

Hi Andy,

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


>