Re: [PATCH 4/8] iio: adc: ad9467: fix scale setting

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2023-12-06 at 17:01 -0600, David Lechner wrote:
> On Tue, Dec 5, 2023 at 11:06 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > When reading in_voltage_scale we can get something like:
> > 
> > root@analog:/sys/bus/iio/devices/iio:device2# cat in_voltage_scale
> > 0.038146
> > 
> > However, when reading the available options:
> > 
> > root@analog:/sys/bus/iio/devices/iio:device2# cat
> > in_voltage_scale_available
> > 2000.000000 2100.000006 2200.000007 2300.000008 2400.000009 2500.000010
> > 
> > which does not make sense. Moreover, when trying to set a new scale we
> > get an error because there's no call to __ad9467_get_scale() to give us
> > values as given when reading in_voltage_scale. Fix it by computing the
> > available scales during probe and properly pass the list when
> > .read_available() is called.
> > 
> > While at it, change to use .read_available() from iio_info. Also note
> > that to properly fix this, adi-axi-adc.c has to be changed accordingly.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/ad9467.c            | 47 +++++++++++++++++++++++
> >  drivers/iio/adc/adi-axi-adc.c       | 74 ++++++++-----------------------------
> >  include/linux/iio/adc/adi-axi-adc.h |  4 ++
> >  3 files changed, 66 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index badbef2ce9f8..3c8bd6c821a4 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -120,6 +120,7 @@ struct ad9467_state {
> >         struct spi_device               *spi;
> >         struct clk                      *clk;
> >         unsigned int                    output_mode;
> > +       unsigned int                    (*scales)[2];
> > 
> >         struct gpio_desc                *pwrdown_gpio;
> >         /* ensure consistent state obtained on multiple related accesses */
> > @@ -216,6 +217,7 @@ static void __ad9467_get_scale(struct adi_axi_adc_conv *conv,
> > int index,
> >         .channel = _chan,                                               \
> >         .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
> >                 BIT(IIO_CHAN_INFO_SAMP_FREQ),                           \
> > +       .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
> >         .scan_index = _si,                                              \
> >         .scan_type = {                                                  \
> >                 .sign = _sign,                                          \
> > @@ -370,6 +372,26 @@ static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
> >         }
> >  }
> > 
> > +static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
> > +                            struct iio_chan_spec const *chan,
> > +                            const int **vals, int *type, int *length,
> > +                            long mask)
> > +{
> > +       const struct adi_axi_adc_chip_info *info = conv->chip_info;
> > +       struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> > +
> > +       switch (mask) {
> > +       case IIO_CHAN_INFO_SCALE:
> > +               *vals = (const int *)st->scales;
> > +               *type = IIO_VAL_INT_PLUS_MICRO;
> > +               /* Values are stored in a 2D matrix */
> > +               *length = info->num_scales * 2;
> 
> Maybe use ARRAY_SIZE(*info->scales) here instead of hard-coding 2?
> 
> > +               return IIO_AVAIL_LIST;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +}
> > +
> >  static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
> >  {
> >         int ret;
> > @@ -382,6 +404,26 @@ static int ad9467_outputmode_set(struct spi_device *spi,
> > unsigned int mode)
> >                                 AN877_ADC_TRANSFER_SYNC);
> >  }
> > 
> > +static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
> > +{
> > +       const struct adi_axi_adc_chip_info *info = conv->chip_info;
> > +       struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> > +       unsigned int i, val1, val2;
> > +
> > +       st->scales = devm_kcalloc(&st->spi->dev, ARRAY_SIZE(*st->scales),
> > +                                 info->num_scales, GFP_KERNEL);
> 
> If I'm reading this correctly, it says to allocate an array with n=2
> elements (ARRAY_SIZE(*st->scales) == 2) and the elements have
> size=info->num_scales bytes.
> 

Hmm, you're completely right! I'm pretty sure I tested this so I'm wondering how it
worked. Maybe I did a last minute (stupid) change.
 
> It seems like this should be:
> 
>         st->scales = devm_kmalloc_array(&st->spi->dev, info->num_scales,
>                                         sizeof(*st->scales), GFP_KERNEL);
> 
> Which allocates n=info->num_scales elements and the elements have
> size=8 bytes (sizeof(*st->scales) == 8).
> 
> (also changed to devm_kmalloc_array() since it doesn't look like it
> needs to be zeroed since all values are assigned below)
> 
> > +       if (!st->scales)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < info->num_scales * 2; i++) {
> 
> Is `* 2` correct here? Assuming only info->num_scales elements were allocated.

No, don't think so... This should actually lead to an out of bounds access.

- Nuno Sá
>