On Tue, Dec 5, 2023 at 11:06 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
>
> From: Nuno Sa <nuno.sa@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
>
>
