* [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 18:03 ` Jonathan Cameron
2023-12-06 22:09 ` David Lechner
2023-12-05 17:06 ` [PATCH 2/8] iio: adc: ad9467: don't ignore error codes Nuno Sa via B4 Relay
` (6 subsequent siblings)
7 siblings, 2 replies; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
The reset gpio was being requested with GPIOD_OUT_LOW which means, not
asserted. Then it was being asserted but never de-asserted which means
the devices was left in reset. Fix it by de-asserting the gpio.
While at it, moved the handling to it's own function and dropped
'reset_gpio' from the 'struct ad9467_state' as we only need it during
probe. On top of that, refactored things so that we now request the gpio
asserted (i.e in reset) and then de-assert it. Also note that we now use
gpiod_set_value_cansleep() instead of gpiod_direction_output() as we
already request the pin as output.
Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 39eccc28debe..5ecf486bf5d1 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -121,7 +121,6 @@ struct ad9467_state {
unsigned int output_mode;
struct gpio_desc *pwrdown_gpio;
- struct gpio_desc *reset_gpio;
};
static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
@@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
return ad9467_outputmode_set(st->spi, st->output_mode);
}
+static int ad9467_reset(struct device *dev)
+{
+ struct gpio_desc *gpio;
+
+ gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(gpio))
+ return PTR_ERR(gpio);
+ if (!gpio)
+ return 0;
+
+ fsleep(1);
+ gpiod_set_value_cansleep(gpio, 0);
+ fsleep(10 * USEC_PER_MSEC);
+
+ return 0;
+}
+
static int ad9467_probe(struct spi_device *spi)
{
const struct ad9467_chip_info *info;
@@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
if (IS_ERR(st->pwrdown_gpio))
return PTR_ERR(st->pwrdown_gpio);
- st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
- GPIOD_OUT_LOW);
- if (IS_ERR(st->reset_gpio))
- return PTR_ERR(st->reset_gpio);
-
- if (st->reset_gpio) {
- udelay(1);
- ret = gpiod_direction_output(st->reset_gpio, 1);
- if (ret)
- return ret;
- mdelay(10);
- }
+ ret = ad9467_reset(&spi->dev);
+ if (ret)
+ return ret;
conv->chip_info = &info->axi_adc_info;
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling
2023-12-05 17:06 ` [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling Nuno Sa via B4 Relay
@ 2023-12-06 18:03 ` Jonathan Cameron
2023-12-07 9:02 ` Nuno Sá
2023-12-06 22:09 ` David Lechner
1 sibling, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2023-12-06 18:03 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, Michael Hennerich, Lars-Peter Clausen
On Tue, 05 Dec 2023 18:06:41 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> asserted. Then it was being asserted but never de-asserted which means
> the devices was left in reset. Fix it by de-asserting the gpio.
If I understand this correctly, it's really just inverted polarity
compared to the expectation? If so just call it that rather than
discussing what happens in detail.
>
> While at it, moved the handling to it's own function and dropped
> 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> probe. On top of that, refactored things so that we now request the gpio
> asserted (i.e in reset) and then de-assert it. Also note that we now use
> gpiod_set_value_cansleep() instead of gpiod_direction_output() as we
> already request the pin as output.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
> drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 39eccc28debe..5ecf486bf5d1 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -121,7 +121,6 @@ struct ad9467_state {
> unsigned int output_mode;
>
> struct gpio_desc *pwrdown_gpio;
> - struct gpio_desc *reset_gpio;
> };
>
> static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
> return ad9467_outputmode_set(st->spi, st->output_mode);
> }
>
> +static int ad9467_reset(struct device *dev)
> +{
> + struct gpio_desc *gpio;
> +
> + gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(gpio))
> + return PTR_ERR(gpio);
> + if (!gpio)
> + return 0;
> +
> + fsleep(1);
> + gpiod_set_value_cansleep(gpio, 0);
> + fsleep(10 * USEC_PER_MSEC);
> +
> + return 0;
> +}
> +
> static int ad9467_probe(struct spi_device *spi)
> {
> const struct ad9467_chip_info *info;
> @@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
> if (IS_ERR(st->pwrdown_gpio))
> return PTR_ERR(st->pwrdown_gpio);
>
> - st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> - GPIOD_OUT_LOW);
> - if (IS_ERR(st->reset_gpio))
> - return PTR_ERR(st->reset_gpio);
> -
> - if (st->reset_gpio) {
> - udelay(1);
> - ret = gpiod_direction_output(st->reset_gpio, 1);
> - if (ret)
> - return ret;
> - mdelay(10);
> - }
> + ret = ad9467_reset(&spi->dev);
> + if (ret)
> + return ret;
>
> conv->chip_info = &info->axi_adc_info;
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling
2023-12-06 18:03 ` Jonathan Cameron
@ 2023-12-07 9:02 ` Nuno Sá
0 siblings, 0 replies; 26+ messages in thread
From: Nuno Sá @ 2023-12-07 9:02 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, Michael Hennerich, Lars-Peter Clausen
On Wed, 2023-12-06 at 18:03 +0000, Jonathan Cameron wrote:
> On Tue, 05 Dec 2023 18:06:41 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> > From: Nuno Sa <nuno.sa@analog.com>
> >
> > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > asserted. Then it was being asserted but never de-asserted which means
> > the devices was left in reset. Fix it by de-asserting the gpio.
>
> If I understand this correctly, it's really just inverted polarity
> compared to the expectation? If so just call it that rather than
> discussing what happens in detail.
>
Hmm, honestly I do not know but likely you're right. The driver was just playing
games with polarity and expecting people made things "right" in devicetree.
Will change the message...
- Nuno Sá
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling
2023-12-05 17:06 ` [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling Nuno Sa via B4 Relay
2023-12-06 18:03 ` Jonathan Cameron
@ 2023-12-06 22:09 ` David Lechner
2023-12-07 9:04 ` Nuno Sá
1 sibling, 1 reply; 26+ messages in thread
From: David Lechner @ 2023-12-06 22:09 UTC (permalink / raw)
To: nuno.sa; +Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
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>
>
> The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> asserted. Then it was being asserted but never de-asserted which means
> the devices was left in reset. Fix it by de-asserting the gpio.
>
> While at it, moved the handling to it's own function and dropped
> 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> probe. On top of that, refactored things so that we now request the gpio
> asserted (i.e in reset) and then de-assert it. Also note that we now use
> gpiod_set_value_cansleep() instead of gpiod_direction_output() as we
> already request the pin as output.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
> drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 39eccc28debe..5ecf486bf5d1 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -121,7 +121,6 @@ struct ad9467_state {
> unsigned int output_mode;
>
> struct gpio_desc *pwrdown_gpio;
> - struct gpio_desc *reset_gpio;
> };
>
> static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
> return ad9467_outputmode_set(st->spi, st->output_mode);
> }
>
> +static int ad9467_reset(struct device *dev)
> +{
> + struct gpio_desc *gpio;
> +
> + gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(gpio))
> + return PTR_ERR(gpio);
> + if (!gpio)
> + return 0;
same comment as before about replacing two ifs with one:
if (IS_ERR_OR_NULL(gpio))
return PTR_ERR_OR_ZERO(gpio);
> +
> + fsleep(1);
> + gpiod_set_value_cansleep(gpio, 0);
> + fsleep(10 * USEC_PER_MSEC);
> +
> + return 0;
> +}
> +
> static int ad9467_probe(struct spi_device *spi)
> {
> const struct ad9467_chip_info *info;
> @@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
> if (IS_ERR(st->pwrdown_gpio))
> return PTR_ERR(st->pwrdown_gpio);
>
> - st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> - GPIOD_OUT_LOW);
> - if (IS_ERR(st->reset_gpio))
> - return PTR_ERR(st->reset_gpio);
> -
> - if (st->reset_gpio) {
> - udelay(1);
> - ret = gpiod_direction_output(st->reset_gpio, 1);
> - if (ret)
> - return ret;
> - mdelay(10);
> - }
> + ret = ad9467_reset(&spi->dev);
> + if (ret)
> + return ret;
>
> conv->chip_info = &info->axi_adc_info;
>
>
> --
> 2.43.0
>
>
With the suggested change above:
Reviewed-by: David Lechner <dlechner@baylibre.com>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling
2023-12-06 22:09 ` David Lechner
@ 2023-12-07 9:04 ` Nuno Sá
0 siblings, 0 replies; 26+ messages in thread
From: Nuno Sá @ 2023-12-07 9:04 UTC (permalink / raw)
To: David Lechner, nuno.sa
Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
On Wed, 2023-12-06 at 16:09 -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>
> >
> > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > asserted. Then it was being asserted but never de-asserted which means
> > the devices was left in reset. Fix it by de-asserting the gpio.
> >
> > While at it, moved the handling to it's own function and dropped
> > 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> > probe. On top of that, refactored things so that we now request the gpio
> > asserted (i.e in reset) and then de-assert it. Also note that we now use
> > gpiod_set_value_cansleep() instead of gpiod_direction_output() as we
> > already request the pin as output.
> >
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> > drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 39eccc28debe..5ecf486bf5d1 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -121,7 +121,6 @@ struct ad9467_state {
> > unsigned int output_mode;
> >
> > struct gpio_desc *pwrdown_gpio;
> > - struct gpio_desc *reset_gpio;
> > };
> >
> > static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv
> > *conv)
> > return ad9467_outputmode_set(st->spi, st->output_mode);
> > }
> >
> > +static int ad9467_reset(struct device *dev)
> > +{
> > + struct gpio_desc *gpio;
> > +
> > + gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(gpio))
> > + return PTR_ERR(gpio);
> > + if (!gpio)
> > + return 0;
>
> same comment as before about replacing two ifs with one:
>
> if (IS_ERR_OR_NULL(gpio))
> return PTR_ERR_OR_ZERO(gpio);
>
Bah, forgot about that one... will do as suggested.
- Nuno Sá
>
> > +
> > + fsleep(1);
> > + gpiod_set_value_cansleep(gpio, 0);
> > + fsleep(10 * USEC_PER_MSEC);
> > +
> > + return 0;
> > +}
> > +
> > static int ad9467_probe(struct spi_device *spi)
> > {
> > const struct ad9467_chip_info *info;
> > @@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
> > if (IS_ERR(st->pwrdown_gpio))
> > return PTR_ERR(st->pwrdown_gpio);
> >
> > - st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> > - GPIOD_OUT_LOW);
> > - if (IS_ERR(st->reset_gpio))
> > - return PTR_ERR(st->reset_gpio);
> > -
> > - if (st->reset_gpio) {
> > - udelay(1);
> > - ret = gpiod_direction_output(st->reset_gpio, 1);
> > - if (ret)
> > - return ret;
> > - mdelay(10);
> > - }
> > + ret = ad9467_reset(&spi->dev);
> > + if (ret)
> > + return ret;
> >
> > conv->chip_info = &info->axi_adc_info;
> >
> >
> > --
> > 2.43.0
> >
> >
>
> With the suggested change above:
>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/8] iio: adc: ad9467: don't ignore error codes
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
2023-12-05 17:06 ` [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 22:14 ` David Lechner
2023-12-05 17:06 ` [PATCH 3/8] iio: adc: ad9467: add mutex to struct ad9467_state Nuno Sa via B4 Relay
` (5 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
Make sure functions that return errors are not ignored.
Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 5ecf486bf5d1..0f2dce730a0a 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -162,9 +162,10 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
if (readval == NULL) {
ret = ad9467_spi_write(spi, reg, writeval);
- ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
- AN877_ADC_TRANSFER_SYNC);
- return ret;
+ if (ret)
+ return ret;
+ return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
}
ret = ad9467_spi_read(spi, reg);
@@ -272,10 +273,13 @@ static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
const struct ad9467_chip_info *info1 = to_ad9467_chip_info(info);
struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
unsigned int i, vref_val;
+ int ret;
- vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
+ ret = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
+ if (ret < 0)
+ return ret;
- vref_val &= info1->vref_mask;
+ vref_val = ret & info1->vref_mask;
for (i = 0; i < info->num_scales; i++) {
if (vref_val == info->scale_table[i][1])
@@ -296,6 +300,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
unsigned int scale_val[2];
unsigned int i;
+ int ret;
if (val != 0)
return -EINVAL;
@@ -305,11 +310,13 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
if (scale_val[0] != val || scale_val[1] != val2)
continue;
- ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
- info->scale_table[i][1]);
- ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
- AN877_ADC_TRANSFER_SYNC);
- return 0;
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
+ info->scale_table[i][1]);
+ if (ret < 0)
+ return ret;
+
+ return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
}
return -EINVAL;
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 3/8] iio: adc: ad9467: add mutex to struct ad9467_state
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
2023-12-05 17:06 ` [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling Nuno Sa via B4 Relay
2023-12-05 17:06 ` [PATCH 2/8] iio: adc: ad9467: don't ignore error codes Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 22:27 ` David Lechner
2023-12-05 17:06 ` [PATCH 4/8] iio: adc: ad9467: fix scale setting Nuno Sa via B4 Relay
` (4 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
are done which means we need to properly protect the whole operation so
we are sure we will be in a sane state if two concurrent calls occur.
Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 0f2dce730a0a..badbef2ce9f8 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -4,8 +4,9 @@
*
* Copyright 2012-2020 Analog Devices Inc.
*/
-
+#include <linux/cleanup.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/slab.h>
@@ -121,6 +122,8 @@ struct ad9467_state {
unsigned int output_mode;
struct gpio_desc *pwrdown_gpio;
+ /* ensure consistent state obtained on multiple related accesses */
+ struct mutex lock;
};
static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
@@ -161,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
int ret;
if (readval == NULL) {
+ guard(mutex)(&st->lock);
ret = ad9467_spi_write(spi, reg, writeval);
if (ret)
return ret;
@@ -310,6 +314,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
if (scale_val[0] != val || scale_val[1] != val2)
continue;
+ guard(mutex)(&st->lock);
ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
info->scale_table[i][1]);
if (ret < 0)
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/8] iio: adc: ad9467: add mutex to struct ad9467_state
2023-12-05 17:06 ` [PATCH 3/8] iio: adc: ad9467: add mutex to struct ad9467_state Nuno Sa via B4 Relay
@ 2023-12-06 22:27 ` David Lechner
2023-12-07 9:10 ` Nuno Sá
0 siblings, 1 reply; 26+ messages in thread
From: David Lechner @ 2023-12-06 22:27 UTC (permalink / raw)
To: nuno.sa; +Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
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 calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> are done which means we need to properly protect the whole operation so
> we are sure we will be in a sane state if two concurrent calls occur.
ad9467_outputmode_set() also has multiple calls to ad9467_spi_write().
Does it need similar protection?
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
> drivers/iio/adc/ad9467.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 0f2dce730a0a..badbef2ce9f8 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -4,8 +4,9 @@
> *
> * Copyright 2012-2020 Analog Devices Inc.
> */
> -
> +#include <linux/cleanup.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> @@ -121,6 +122,8 @@ struct ad9467_state {
> unsigned int output_mode;
>
> struct gpio_desc *pwrdown_gpio;
> + /* ensure consistent state obtained on multiple related accesses */
> + struct mutex lock;
> };
>
> static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -161,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
> int ret;
>
> if (readval == NULL) {
> + guard(mutex)(&st->lock);
> ret = ad9467_spi_write(spi, reg, writeval);
> if (ret)
> return ret;
> @@ -310,6 +314,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
> if (scale_val[0] != val || scale_val[1] != val2)
> continue;
>
> + guard(mutex)(&st->lock);
Why is the guard inside of the for loop instead of outside?
__ad9467_get_scale() called in this loop calls ad9467_spi_read() too,
but since this is not a read-modify-write, I guess I can see why that
might not be inside of the gurad.
> ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> info->scale_table[i][1]);
> if (ret < 0)
>
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/8] iio: adc: ad9467: add mutex to struct ad9467_state
2023-12-06 22:27 ` David Lechner
@ 2023-12-07 9:10 ` Nuno Sá
0 siblings, 0 replies; 26+ messages in thread
From: Nuno Sá @ 2023-12-07 9:10 UTC (permalink / raw)
To: David Lechner, nuno.sa
Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
On Wed, 2023-12-06 at 16:27 -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 calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > are done which means we need to properly protect the whole operation so
> > we are sure we will be in a sane state if two concurrent calls occur.
>
>
> ad9467_outputmode_set() also has multiple calls to ad9467_spi_write().
> Does it need similar protection?
>
Just called during probe (of the axi-adc driver) before registering the IIO device.
We should not need the lock in there (for now).
> >
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> > drivers/iio/adc/ad9467.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 0f2dce730a0a..badbef2ce9f8 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -4,8 +4,9 @@
> > *
> > * Copyright 2012-2020 Analog Devices Inc.
> > */
> > -
> > +#include <linux/cleanup.h>
> > #include <linux/module.h>
> > +#include <linux/mutex.h>
> > #include <linux/device.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > @@ -121,6 +122,8 @@ struct ad9467_state {
> > unsigned int output_mode;
> >
> > struct gpio_desc *pwrdown_gpio;
> > + /* ensure consistent state obtained on multiple related accesses */
> > + struct mutex lock;
> > };
> >
> > static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > @@ -161,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > unsigned int reg,
> > int ret;
> >
> > if (readval == NULL) {
> > + guard(mutex)(&st->lock);
> > ret = ad9467_spi_write(spi, reg, writeval);
> > if (ret)
> > return ret;
> > @@ -310,6 +314,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > int val, int val2)
> > if (scale_val[0] != val || scale_val[1] != val2)
> > continue;
> >
> > + guard(mutex)(&st->lock);
>
> Why is the guard inside of the for loop instead of outside?
>
> __ad9467_get_scale() called in this loop calls ad9467_spi_read() too,
Hmm, am I missing something? __ad9467_get_scale() is not doing any spi access, is it?
I think you made confusion with the version without underscore...
- Nuno Sá
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/8] iio: adc: ad9467: fix scale setting
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
` (2 preceding siblings ...)
2023-12-05 17:06 ` [PATCH 3/8] iio: adc: ad9467: add mutex to struct ad9467_state Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 23:01 ` David Lechner
2023-12-05 17:06 ` [PATCH 5/8] iio: adc: ad9467: use spi_get_device_match_data() Nuno Sa via B4 Relay
` (3 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
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;
+ 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,
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 4/8] iio: adc: ad9467: fix scale setting
2023-12-05 17:06 ` [PATCH 4/8] iio: adc: ad9467: fix scale setting Nuno Sa via B4 Relay
@ 2023-12-06 23:01 ` David Lechner
2023-12-07 9:21 ` Nuno Sá
2023-12-07 10:02 ` Nuno Sá
0 siblings, 2 replies; 26+ messages in thread
From: David Lechner @ 2023-12-06 23:01 UTC (permalink / raw)
To: nuno.sa; +Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
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
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 4/8] iio: adc: ad9467: fix scale setting
2023-12-06 23:01 ` David Lechner
@ 2023-12-07 9:21 ` Nuno Sá
2023-12-07 10:02 ` Nuno Sá
1 sibling, 0 replies; 26+ messages in thread
From: Nuno Sá @ 2023-12-07 9:21 UTC (permalink / raw)
To: David Lechner, nuno.sa
Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
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á
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 4/8] iio: adc: ad9467: fix scale setting
2023-12-06 23:01 ` David Lechner
2023-12-07 9:21 ` Nuno Sá
@ 2023-12-07 10:02 ` Nuno Sá
2023-12-10 11:27 ` Jonathan Cameron
1 sibling, 1 reply; 26+ messages in thread
From: Nuno Sá @ 2023-12-07 10:02 UTC (permalink / raw)
To: David Lechner, nuno.sa
Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
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á
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 4/8] iio: adc: ad9467: fix scale setting
2023-12-07 10:02 ` Nuno Sá
@ 2023-12-10 11:27 ` Jonathan Cameron
2023-12-11 9:32 ` Nuno Sá
0 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2023-12-10 11:27 UTC (permalink / raw)
To: Nuno Sá
Cc: David Lechner, nuno.sa, linux-iio, Michael Hennerich,
Lars-Peter Clausen
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á
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 4/8] iio: adc: ad9467: fix scale setting
2023-12-10 11:27 ` Jonathan Cameron
@ 2023-12-11 9:32 ` Nuno Sá
0 siblings, 0 replies; 26+ messages in thread
From: Nuno Sá @ 2023-12-11 9:32 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, nuno.sa, linux-iio, Michael Hennerich,
Lars-Peter Clausen
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á
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/8] iio: adc: ad9467: use spi_get_device_match_data()
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
` (3 preceding siblings ...)
2023-12-05 17:06 ` [PATCH 4/8] iio: adc: ad9467: fix scale setting Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 23:03 ` David Lechner
2023-12-05 17:06 ` [PATCH 6/8] iio: adc: ad9467: use chip_info variables instead of array Nuno Sa via B4 Relay
` (2 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
Make use of spi_get_device_match_data() to simplify things.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 3c8bd6c821a4..6bdbd0ea8d6c 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -456,9 +456,7 @@ static int ad9467_probe(struct spi_device *spi)
unsigned int id;
int ret;
- info = of_device_get_match_data(&spi->dev);
- if (!info)
- info = (void *)spi_get_device_id(spi)->driver_data;
+ info = spi_get_device_match_data(spi);
if (!info)
return -ENODEV;
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 6/8] iio: adc: ad9467: use chip_info variables instead of array
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
` (4 preceding siblings ...)
2023-12-05 17:06 ` [PATCH 5/8] iio: adc: ad9467: use spi_get_device_match_data() Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 23:10 ` David Lechner
2023-12-05 17:06 ` [PATCH 7/8] iio: adc: ad9467: use the more common !val NULL check Nuno Sa via B4 Relay
2023-12-05 17:06 ` [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap Nuno Sa via B4 Relay
7 siblings, 1 reply; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
Instead of having an array and keeping IDs for each entry of the array,
just have a chip_info struct per device.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467.c | 89 +++++++++++++++++++++++-------------------------
1 file changed, 43 insertions(+), 46 deletions(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 6bdbd0ea8d6c..0d075e6b6248 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -101,12 +101,6 @@
#define AD9467_DEF_OUTPUT_MODE 0x08
#define AD9467_REG_VREF_MASK 0x0F
-enum {
- ID_AD9265,
- ID_AD9434,
- ID_AD9467,
-};
-
struct ad9467_chip_info {
struct adi_axi_adc_chip_info axi_adc_info;
unsigned int default_output_mode;
@@ -234,43 +228,46 @@ static const struct iio_chan_spec ad9467_channels[] = {
AD9467_CHAN(0, 0, 16, 'S'),
};
-static const struct ad9467_chip_info ad9467_chip_tbl[] = {
- [ID_AD9265] = {
- .axi_adc_info = {
- .id = CHIPID_AD9265,
- .max_rate = 125000000UL,
- .scale_table = ad9265_scale_table,
- .num_scales = ARRAY_SIZE(ad9265_scale_table),
- .channels = ad9467_channels,
- .num_channels = ARRAY_SIZE(ad9467_channels),
- },
- .default_output_mode = AD9265_DEF_OUTPUT_MODE,
- .vref_mask = AD9265_REG_VREF_MASK,
+static const struct ad9467_chip_info ad9467_chip_tbl = {
+ .axi_adc_info = {
+ .name = "ad9467",
+ .id = CHIPID_AD9467,
+ .max_rate = 250000000UL,
+ .scale_table = ad9467_scale_table,
+ .num_scales = ARRAY_SIZE(ad9467_scale_table),
+ .channels = ad9467_channels,
+ .num_channels = ARRAY_SIZE(ad9467_channels),
},
- [ID_AD9434] = {
- .axi_adc_info = {
- .id = CHIPID_AD9434,
- .max_rate = 500000000UL,
- .scale_table = ad9434_scale_table,
- .num_scales = ARRAY_SIZE(ad9434_scale_table),
- .channels = ad9434_channels,
- .num_channels = ARRAY_SIZE(ad9434_channels),
- },
- .default_output_mode = AD9434_DEF_OUTPUT_MODE,
- .vref_mask = AD9434_REG_VREF_MASK,
+ .default_output_mode = AD9467_DEF_OUTPUT_MODE,
+ .vref_mask = AD9467_REG_VREF_MASK,
+};
+
+static const struct ad9467_chip_info ad9434_chip_tbl = {
+ .axi_adc_info = {
+ .name = "ad9434",
+ .id = CHIPID_AD9434,
+ .max_rate = 500000000UL,
+ .scale_table = ad9434_scale_table,
+ .num_scales = ARRAY_SIZE(ad9434_scale_table),
+ .channels = ad9434_channels,
+ .num_channels = ARRAY_SIZE(ad9434_channels),
},
- [ID_AD9467] = {
- .axi_adc_info = {
- .id = CHIPID_AD9467,
- .max_rate = 250000000UL,
- .scale_table = ad9467_scale_table,
- .num_scales = ARRAY_SIZE(ad9467_scale_table),
- .channels = ad9467_channels,
- .num_channels = ARRAY_SIZE(ad9467_channels),
- },
- .default_output_mode = AD9467_DEF_OUTPUT_MODE,
- .vref_mask = AD9467_REG_VREF_MASK,
+ .default_output_mode = AD9434_DEF_OUTPUT_MODE,
+ .vref_mask = AD9434_REG_VREF_MASK,
+};
+
+static const struct ad9467_chip_info ad9265_chip_tbl = {
+ .axi_adc_info = {
+ .name = "ad9265",
+ .id = CHIPID_AD9265,
+ .max_rate = 125000000UL,
+ .scale_table = ad9265_scale_table,
+ .num_scales = ARRAY_SIZE(ad9265_scale_table),
+ .channels = ad9467_channels,
+ .num_channels = ARRAY_SIZE(ad9467_channels),
},
+ .default_output_mode = AD9265_DEF_OUTPUT_MODE,
+ .vref_mask = AD9265_REG_VREF_MASK,
};
static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
@@ -506,17 +503,17 @@ static int ad9467_probe(struct spi_device *spi)
}
static const struct of_device_id ad9467_of_match[] = {
- { .compatible = "adi,ad9265", .data = &ad9467_chip_tbl[ID_AD9265], },
- { .compatible = "adi,ad9434", .data = &ad9467_chip_tbl[ID_AD9434], },
- { .compatible = "adi,ad9467", .data = &ad9467_chip_tbl[ID_AD9467], },
+ { .compatible = "adi,ad9265", .data = &ad9265_chip_tbl, },
+ { .compatible = "adi,ad9434", .data = &ad9434_chip_tbl, },
+ { .compatible = "adi,ad9467", .data = &ad9467_chip_tbl, },
{}
};
MODULE_DEVICE_TABLE(of, ad9467_of_match);
static const struct spi_device_id ad9467_ids[] = {
- { "ad9265", (kernel_ulong_t)&ad9467_chip_tbl[ID_AD9265] },
- { "ad9434", (kernel_ulong_t)&ad9467_chip_tbl[ID_AD9434] },
- { "ad9467", (kernel_ulong_t)&ad9467_chip_tbl[ID_AD9467] },
+ { "ad9265", (kernel_ulong_t)&ad9265_chip_tbl },
+ { "ad9434", (kernel_ulong_t)&ad9434_chip_tbl },
+ { "ad9467", (kernel_ulong_t)&ad9467_chip_tbl },
{}
};
MODULE_DEVICE_TABLE(spi, ad9467_ids);
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 7/8] iio: adc: ad9467: use the more common !val NULL check
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
` (5 preceding siblings ...)
2023-12-05 17:06 ` [PATCH 6/8] iio: adc: ad9467: use chip_info variables instead of array Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 23:11 ` David Lechner
2023-12-05 17:06 ` [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap Nuno Sa via B4 Relay
7 siblings, 1 reply; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
Check !val instead of directing checking for NULL (val == NULL).
No functional changes intended.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 0d075e6b6248..637823de712f 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -158,7 +158,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
struct spi_device *spi = st->spi;
int ret;
- if (readval == NULL) {
+ if (!readval) {
guard(mutex)(&st->lock);
ret = ad9467_spi_write(spi, reg, writeval);
if (ret)
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap
2023-12-05 17:06 [PATCH 0/8] iio: ad9467 and axi-adc cleanups Nuno Sa via B4 Relay
` (6 preceding siblings ...)
2023-12-05 17:06 ` [PATCH 7/8] iio: adc: ad9467: use the more common !val NULL check Nuno Sa via B4 Relay
@ 2023-12-05 17:06 ` Nuno Sa via B4 Relay
2023-12-06 18:05 ` Jonathan Cameron
2023-12-06 23:18 ` David Lechner
7 siblings, 2 replies; 26+ messages in thread
From: Nuno Sa via B4 Relay @ 2023-12-05 17:06 UTC (permalink / raw)
To: linux-iio; +Cc: Michael Hennerich, Jonathan Cameron, Lars-Peter Clausen
From: Nuno Sa <nuno.sa@analog.com>
Use MMIO regmap interface. It makes things easier for manipulating bits.
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/adi-axi-adc.c | 85 ++++++++++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 33 deletions(-)
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index ae83ada7f9f2..c247ff1541d2 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -14,6 +14,7 @@
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/property.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/iio/iio.h>
@@ -62,7 +63,7 @@ struct adi_axi_adc_state {
struct mutex lock;
struct adi_axi_adc_client *client;
- void __iomem *regs;
+ struct regmap *regmap;
};
struct adi_axi_adc_client {
@@ -90,19 +91,6 @@ void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
}
EXPORT_SYMBOL_NS_GPL(adi_axi_adc_conv_priv, IIO_ADI_AXI);
-static void adi_axi_adc_write(struct adi_axi_adc_state *st,
- unsigned int reg,
- unsigned int val)
-{
- iowrite32(val, st->regs + reg);
-}
-
-static unsigned int adi_axi_adc_read(struct adi_axi_adc_state *st,
- unsigned int reg)
-{
- return ioread32(st->regs + reg);
-}
-
static int adi_axi_adc_config_dma_buffer(struct device *dev,
struct iio_dev *indio_dev)
{
@@ -163,17 +151,20 @@ static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
{
struct adi_axi_adc_state *st = iio_priv(indio_dev);
struct adi_axi_adc_conv *conv = &st->client->conv;
- unsigned int i, ctrl;
+ unsigned int i;
+ int ret;
for (i = 0; i < conv->chip_info->num_channels; i++) {
- ctrl = adi_axi_adc_read(st, ADI_AXI_REG_CHAN_CTRL(i));
-
if (test_bit(i, scan_mask))
- ctrl |= ADI_AXI_REG_CHAN_CTRL_ENABLE;
+ ret = regmap_set_bits(st->regmap,
+ ADI_AXI_REG_CHAN_CTRL(i),
+ ADI_AXI_REG_CHAN_CTRL_ENABLE);
else
- ctrl &= ~ADI_AXI_REG_CHAN_CTRL_ENABLE;
-
- adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i), ctrl);
+ ret = regmap_clear_bits(st->regmap,
+ ADI_AXI_REG_CHAN_CTRL(i),
+ ADI_AXI_REG_CHAN_CTRL_ENABLE);
+ if (ret)
+ return ret;
}
return 0;
@@ -310,21 +301,32 @@ static int adi_axi_adc_setup_channels(struct device *dev,
}
for (i = 0; i < conv->chip_info->num_channels; i++) {
- adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i),
- ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
+ ret = regmap_write(st->regmap, ADI_AXI_REG_CHAN_CTRL(i),
+ ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
+ if (ret)
+ return ret;
}
return 0;
}
-static void axi_adc_reset(struct adi_axi_adc_state *st)
+static int axi_adc_reset(struct adi_axi_adc_state *st)
{
- adi_axi_adc_write(st, ADI_AXI_REG_RSTN, 0);
+ int ret;
+
+ ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
+ if (ret)
+ return ret;
+
mdelay(10);
- adi_axi_adc_write(st, ADI_AXI_REG_RSTN, ADI_AXI_REG_RSTN_MMCM_RSTN);
+ ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN,
+ ADI_AXI_REG_RSTN_MMCM_RSTN);
+ if (ret)
+ return ret;
+
mdelay(10);
- adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
- ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
+ return regmap_write(st->regmap, ADI_AXI_REG_RSTN,
+ ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
}
static void adi_axi_adc_cleanup(void *data)
@@ -335,12 +337,20 @@ static void adi_axi_adc_cleanup(void *data)
module_put(cl->dev->driver->owner);
}
+static const struct regmap_config axi_adc_regmap_config = {
+ .val_bits = 32,
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .max_register = 0x0800,
+};
+
static int adi_axi_adc_probe(struct platform_device *pdev)
{
struct adi_axi_adc_conv *conv;
struct iio_dev *indio_dev;
struct adi_axi_adc_client *cl;
struct adi_axi_adc_state *st;
+ void __iomem *base;
unsigned int ver;
int ret;
@@ -361,15 +371,24 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
cl->state = st;
mutex_init(&st->lock);
- st->regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(st->regs))
- return PTR_ERR(st->regs);
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ st->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+ &axi_adc_regmap_config);
+ if (IS_ERR(st->regmap))
+ return PTR_ERR(st->regmap);
conv = &st->client->conv;
- axi_adc_reset(st);
+ ret = axi_adc_reset(st);
+ if (ret)
+ return ret;
- ver = adi_axi_adc_read(st, ADI_AXI_REG_VERSION);
+ ret = regmap_read(st->regmap, ADI_AXI_REG_VERSION, &ver);
+ if (ret)
+ return ret;
if (cl->info->version > ver) {
dev_err(&pdev->dev,
--
2.43.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap
2023-12-05 17:06 ` [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap Nuno Sa via B4 Relay
@ 2023-12-06 18:05 ` Jonathan Cameron
2023-12-06 23:18 ` David Lechner
1 sibling, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2023-12-06 18:05 UTC (permalink / raw)
To: Nuno Sa via B4 Relay
Cc: nuno.sa, linux-iio, Michael Hennerich, Lars-Peter Clausen
On Tue, 05 Dec 2023 18:06:48 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> From: Nuno Sa <nuno.sa@analog.com>
>
> Use MMIO regmap interface. It makes things easier for manipulating bits.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Rest of series including this one look fine to me, but we should leave
them on list for a day or two before applying them. If we agree on
language for patch 1 description I can tidy that up whilst applying.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap
2023-12-05 17:06 ` [PATCH 8/8] iio: adc: adi-axi-adc: convert to regmap Nuno Sa via B4 Relay
2023-12-06 18:05 ` Jonathan Cameron
@ 2023-12-06 23:18 ` David Lechner
1 sibling, 0 replies; 26+ messages in thread
From: David Lechner @ 2023-12-06 23:18 UTC (permalink / raw)
To: nuno.sa; +Cc: linux-iio, Michael Hennerich, Jonathan Cameron,
Lars-Peter Clausen
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>
>
> Use MMIO regmap interface. It makes things easier for manipulating bits.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>
^ permalink raw reply [flat|nested] 26+ messages in thread