| 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 |
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 > >
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á >
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á
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á > >
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 --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,