diff mbox series

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

Message ID 20231205-iio-backend-prep-v1-4-7c9bc18d612b@analog.com (mailing list archive)
State Changes Requested
Headers show
Series iio: ad9467 and axi-adc cleanups | expand

Commit Message

Nuno Sa via B4 Relay Dec. 5, 2023, 5:06 p.m. UTC 
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(-)

Comments

David Lechner Dec. 6, 2023, 11:01 p.m. UTC | #1 
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.

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.

> +               __ad9467_get_scale(conv, i, &val1, &val2);
> +               st->scales[i][0] = val1;
> +               st->scales[i][1] = val2;
> +       }
> +
> +       return 0;
> +}
> +
>  static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
>  {
>         struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> @@ -442,6 +484,10 @@ static int ad9467_probe(struct spi_device *spi)
>
>         conv->chip_info = &info->axi_adc_info;
>
> +       ret = ad9467_scale_fill(conv);
> +       if (ret)
> +               return ret;
> +
>         id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
>         if (id != conv->chip_info->id) {
>                 dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
> @@ -452,6 +498,7 @@ static int ad9467_probe(struct spi_device *spi)
>         conv->reg_access = ad9467_reg_access;
>         conv->write_raw = ad9467_write_raw;
>         conv->read_raw = ad9467_read_raw;
> +       conv->read_avail = ad9467_read_avail;
>         conv->preenable_setup = ad9467_preenable_setup;
>
>         st->output_mode = info->default_output_mode |
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index aff0532a974a..ae83ada7f9f2 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -144,6 +144,20 @@ static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
>         return conv->write_raw(conv, chan, val, val2, mask);
>  }
>
> +static int adi_axi_adc_read_avail(struct iio_dev *indio_dev,
> +                                 struct iio_chan_spec const *chan,
> +                                 const int **vals, int *type, int *length,
> +                                 long mask)
> +{
> +       struct adi_axi_adc_state *st = iio_priv(indio_dev);
> +       struct adi_axi_adc_conv *conv = &st->client->conv;
> +
> +       if (!conv->read_avail)
> +               return -EOPNOTSUPP;
> +
> +       return conv->read_avail(conv, chan, vals, type, length, mask);
> +}
> +
>  static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
>                                         const unsigned long *scan_mask)
>  {
> @@ -228,69 +242,11 @@ struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_adi_axi_adc_conv_register, IIO_ADI_AXI);
>
> -static ssize_t in_voltage_scale_available_show(struct device *dev,
> -                                              struct device_attribute *attr,
> -                                              char *buf)
> -{
> -       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -       struct adi_axi_adc_state *st = iio_priv(indio_dev);
> -       struct adi_axi_adc_conv *conv = &st->client->conv;
> -       size_t len = 0;
> -       int i;
> -
> -       for (i = 0; i < conv->chip_info->num_scales; i++) {
> -               const unsigned int *s = conv->chip_info->scale_table[i];
> -
> -               len += scnprintf(buf + len, PAGE_SIZE - len,
> -                                "%u.%06u ", s[0], s[1]);
> -       }
> -       buf[len - 1] = '\n';
> -
> -       return len;
> -}
> -
> -static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
> -
> -enum {
> -       ADI_AXI_ATTR_SCALE_AVAIL,
> -};
> -
> -#define ADI_AXI_ATTR(_en_, _file_)                     \
> -       [ADI_AXI_ATTR_##_en_] = &iio_dev_attr_##_file_.dev_attr.attr
> -
> -static struct attribute *adi_axi_adc_attributes[] = {
> -       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> -       NULL
> -};
> -
> -static umode_t axi_adc_attr_is_visible(struct kobject *kobj,
> -                                      struct attribute *attr, int n)
> -{
> -       struct device *dev = kobj_to_dev(kobj);
> -       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -       struct adi_axi_adc_state *st = iio_priv(indio_dev);
> -       struct adi_axi_adc_conv *conv = &st->client->conv;
> -
> -       switch (n) {
> -       case ADI_AXI_ATTR_SCALE_AVAIL:
> -               if (!conv->chip_info->num_scales)
> -                       return 0;
> -               return attr->mode;
> -       default:
> -               return attr->mode;
> -       }
> -}
> -
> -static const struct attribute_group adi_axi_adc_attribute_group = {
> -       .attrs = adi_axi_adc_attributes,
> -       .is_visible = axi_adc_attr_is_visible,
> -};
> -
>  static const struct iio_info adi_axi_adc_info = {
>         .read_raw = &adi_axi_adc_read_raw,
>         .write_raw = &adi_axi_adc_write_raw,
> -       .attrs = &adi_axi_adc_attribute_group,
>         .update_scan_mode = &adi_axi_adc_update_scan_mode,
> +       .read_avail = &adi_axi_adc_read_avail,
>  };
>
>  static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
> diff --git a/include/linux/iio/adc/adi-axi-adc.h b/include/linux/iio/adc/adi-axi-adc.h
> index 52620e5b8052..b7904992d561 100644
> --- a/include/linux/iio/adc/adi-axi-adc.h
> +++ b/include/linux/iio/adc/adi-axi-adc.h
> @@ -41,6 +41,7 @@ struct adi_axi_adc_chip_info {
>   * @reg_access         IIO debugfs_reg_access hook for the client ADC
>   * @read_raw           IIO read_raw hook for the client ADC
>   * @write_raw          IIO write_raw hook for the client ADC
> + * @read_avail         IIO read_avail hook for the client ADC
>   */
>  struct adi_axi_adc_conv {
>         const struct adi_axi_adc_chip_info              *chip_info;
> @@ -54,6 +55,9 @@ struct adi_axi_adc_conv {
>         int (*write_raw)(struct adi_axi_adc_conv *conv,
>                          struct iio_chan_spec const *chan,
>                          int val, int val2, long mask);
> +       int (*read_avail)(struct adi_axi_adc_conv *conv,
> +                         struct iio_chan_spec const *chan,
> +                         const int **val, int *type, int *length, long mask);
>  };
>
>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>
> --
> 2.43.0
>
>
Nuno Sá Dec. 7, 2023, 9:21 a.m. UTC | #2 
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á
>
Nuno Sá Dec. 7, 2023, 10:02 a.m. UTC | #3 
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?

Not to keen on that as the comment should already say everything about the hardcoded
2.

- Nuno Sá
Jonathan Cameron Dec. 10, 2023, 11:27 a.m. UTC | #4 
On Thu, 07 Dec 2023 11:02:36 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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?  
> 
> Not to keen on that as the comment should already say everything about the hardcoded
> 2.
> 
I don't mind either way, but the comment only says it's a 2D matrix, not that the dimension
is 2 * num_scales - it could easily be 14 * num_scales

> - Nuno Sá
> 
>
Nuno Sá Dec. 11, 2023, 9:32 a.m. UTC | #5 
On Sun, 2023-12-10 at 11:27 +0000, Jonathan Cameron wrote:
> On Thu, 07 Dec 2023 11:02:36 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > 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?  
> > 
> > Not to keen on that as the comment should already say everything about the
> > hardcoded
> > 2.
> > 
> I don't mind either way, but the comment only says it's a 2D matrix, not that
> the dimension
> is 2 * num_scales - it could easily be 14 * num_scales

Oops, totally right! For some reason I was seeing the comment as "2 columns" in
the matrix which is obviously not true (but I suspect that was the intent of the
comment)

- Nuno Sá
diff mbox series

Patch

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;
+		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 (!st->scales)
+		return -ENOMEM;
+
+	for (i = 0; i < info->num_scales * 2; i++) {
+		__ad9467_get_scale(conv, i, &val1, &val2);
+		st->scales[i][0] = val1;
+		st->scales[i][1] = val2;
+	}
+
+	return 0;
+}
+
 static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
 {
 	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
@@ -442,6 +484,10 @@  static int ad9467_probe(struct spi_device *spi)
 
 	conv->chip_info = &info->axi_adc_info;
 
+	ret = ad9467_scale_fill(conv);
+	if (ret)
+		return ret;
+
 	id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
 	if (id != conv->chip_info->id) {
 		dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
@@ -452,6 +498,7 @@  static int ad9467_probe(struct spi_device *spi)
 	conv->reg_access = ad9467_reg_access;
 	conv->write_raw = ad9467_write_raw;
 	conv->read_raw = ad9467_read_raw;
+	conv->read_avail = ad9467_read_avail;
 	conv->preenable_setup = ad9467_preenable_setup;
 
 	st->output_mode = info->default_output_mode |
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index aff0532a974a..ae83ada7f9f2 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -144,6 +144,20 @@  static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
 	return conv->write_raw(conv, chan, val, val2, mask);
 }
 
+static int adi_axi_adc_read_avail(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+
+	if (!conv->read_avail)
+		return -EOPNOTSUPP;
+
+	return conv->read_avail(conv, chan, vals, type, length, mask);
+}
+
 static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
 					const unsigned long *scan_mask)
 {
@@ -228,69 +242,11 @@  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_adi_axi_adc_conv_register, IIO_ADI_AXI);
 
-static ssize_t in_voltage_scale_available_show(struct device *dev,
-					       struct device_attribute *attr,
-					       char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-	size_t len = 0;
-	int i;
-
-	for (i = 0; i < conv->chip_info->num_scales; i++) {
-		const unsigned int *s = conv->chip_info->scale_table[i];
-
-		len += scnprintf(buf + len, PAGE_SIZE - len,
-				 "%u.%06u ", s[0], s[1]);
-	}
-	buf[len - 1] = '\n';
-
-	return len;
-}
-
-static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
-
-enum {
-	ADI_AXI_ATTR_SCALE_AVAIL,
-};
-
-#define ADI_AXI_ATTR(_en_, _file_)			\
-	[ADI_AXI_ATTR_##_en_] = &iio_dev_attr_##_file_.dev_attr.attr
-
-static struct attribute *adi_axi_adc_attributes[] = {
-	ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
-	NULL
-};
-
-static umode_t axi_adc_attr_is_visible(struct kobject *kobj,
-				       struct attribute *attr, int n)
-{
-	struct device *dev = kobj_to_dev(kobj);
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	switch (n) {
-	case ADI_AXI_ATTR_SCALE_AVAIL:
-		if (!conv->chip_info->num_scales)
-			return 0;
-		return attr->mode;
-	default:
-		return attr->mode;
-	}
-}
-
-static const struct attribute_group adi_axi_adc_attribute_group = {
-	.attrs = adi_axi_adc_attributes,
-	.is_visible = axi_adc_attr_is_visible,
-};
-
 static const struct iio_info adi_axi_adc_info = {
 	.read_raw = &adi_axi_adc_read_raw,
 	.write_raw = &adi_axi_adc_write_raw,
-	.attrs = &adi_axi_adc_attribute_group,
 	.update_scan_mode = &adi_axi_adc_update_scan_mode,
+	.read_avail = &adi_axi_adc_read_avail,
 };
 
 static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
diff --git a/include/linux/iio/adc/adi-axi-adc.h b/include/linux/iio/adc/adi-axi-adc.h
index 52620e5b8052..b7904992d561 100644
--- a/include/linux/iio/adc/adi-axi-adc.h
+++ b/include/linux/iio/adc/adi-axi-adc.h
@@ -41,6 +41,7 @@  struct adi_axi_adc_chip_info {
  * @reg_access		IIO debugfs_reg_access hook for the client ADC
  * @read_raw		IIO read_raw hook for the client ADC
  * @write_raw		IIO write_raw hook for the client ADC
+ * @read_avail		IIO read_avail hook for the client ADC
  */
 struct adi_axi_adc_conv {
 	const struct adi_axi_adc_chip_info		*chip_info;
@@ -54,6 +55,9 @@  struct adi_axi_adc_conv {
 	int (*write_raw)(struct adi_axi_adc_conv *conv,
 			 struct iio_chan_spec const *chan,
 			 int val, int val2, long mask);
+	int (*read_avail)(struct adi_axi_adc_conv *conv,
+			  struct iio_chan_spec const *chan,
+			  const int **val, int *type, int *length, long mask);
 };
 
 struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,