[PATCH 0/3] iio: adc: ad7124: Make it work on de10-nano

[PATCH 0/3] iio: adc: ad7124: Make it work on de10-nano

[Uwe Kleine-König]

Hello,

the ad7124 driver was initially brought up on a raspberry pi. I tried to
do the same on a terasIC de10-nano board and that was much more bumpy
and time consuming than expected. There were two issues:

a) The GPIO controller used on the de10-nano detects an edge event on
   the monitored line even when the irq is masked. Together with the
   "feature" of the ad7124 that the irq is reported on the spi MISO line
   this means that nearly every spi transfer results in a pending irq.
   So when ad_sigma_delta_single_conversion() calls enable_irq(), the
   irq immediately triggers without the ADC having performed its work
   and so non-sense data is read.

   To fix that, patch #2 adds a possibility to double check in the irq
   handler if the DOUT/̅R̅D̅Y line is really low and only report success if
   yes. That needs a different representation in the device tree, so the
   binding docs need adaption. That's implemented in patch #1.

   Note that while it's annoying to handle that sensitivity of the gpio
   controller, this is actually a sane behaviour for a gpio controller.
   (The problem with the rpi one's is: If the process is preempted long
   enough that the ADC is already done when enable_irq() is called, the
   event is missed and the measurement times out even though hardware
   wise everything is fine.)

b) The reset default for the CHANNEL0 register is 0x8001, that means
   that channel is enabled by default. So if the first measurement is on
   (say) channel 5, the chip generates two interrupts. One when the
   channel 0 measurement is done and another when the channel 5
   measurement is done. The driver however only expects a single irq and
   so reports the first data read as channel 5 data to the upper layers.
   (Maybe the second irq then happens when the irq is currently enabled
   and so might trigger another spurious irq.)

   To fix that, patch #3 disables all channels at probe time to sync
   reality to the driver's expectations.

   This doesn't hurt if the first measurement is done on channel 0.

I wondered if I should add a "Fixes:" line for patch #2. Not entirely
sure because it's not needed for every setup. If yes, it would be for
the initial ad7124 commit (i.e. b3af341bbd96 ("iio: adc: Add ad7124
support")). Additionally that patch doesn't cure the symptom without a
device tree change.

I'd recommend backporting at least patch #3 to the stable branches, but
I didn't add the respective footers for that. I let the applying
maintainer decide if they want to add it (and for which patches).

Best regards
Uwe

Uwe Kleine-König (3):
  dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line
  iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
  iio: adc: ad7124: Disable all channels at probe time

 .../bindings/iio/adc/adi,ad7124.yaml          | 13 +++++--
 drivers/iio/adc/ad7124.c                      |  3 ++
 drivers/iio/adc/ad_sigma_delta.c              | 36 +++++++++++++++----
 include/linux/iio/adc/ad_sigma_delta.h        |  1 +
 4 files changed, 44 insertions(+), 9 deletions(-)

base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
-- 
2.45.2

[PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Uwe Kleine-König]

For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
pin as the spi MISO output (DOUT) and so reading a register might
trigger an interrupt. For correct operation it's critical that the
actual state of the pin can be read to judge if an interrupt event is a
real one or just a spurious one triggered by toggling the line in its
MISO mode.

Allow specification of an "interrupt-gpios" property instead of a plain
interrupt. The semantic is that the GPIO's interrupt is to be used as
event source and reading the GPIO can be used to differentiate between a
real event and one triggered by MISO.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7124.yaml     | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
index 35ed04350e28..feb3a41a148e 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
@@ -37,6 +37,9 @@ properties:
     description: IRQ line for the ADC
     maxItems: 1
 
+  interrupt-gpios:
+    description: GPIO reading the interrupt line
+
   '#address-cells':
     const: 1
 
@@ -57,7 +60,12 @@ required:
   - reg
   - clocks
   - clock-names
-  - interrupts
+
+oneOf:
+  - required:
+      - interrupts
+  - required:
+      - interrupt-gpios
 
 patternProperties:
   "^channel@([0-9]|1[0-5])$":
@@ -119,8 +127,7 @@ examples:
         compatible = "adi,ad7124-4";
         reg = <0>;
         spi-max-frequency = <5000000>;
-        interrupts = <25 2>;
-        interrupt-parent = <&gpio>;
+        interrupt-gpios = <&gpio 25 2>;
         refin1-supply = <&adc_vref>;
         clocks = <&ad7124_mclk>;
         clock-names = "mclk";
-- 
2.45.2

[PATCH 2/3] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

[Uwe Kleine-König]

Some of the ADCs by Analog signal their irq condition on the MISO line.
So typically that line is connected to an SPI controller and a GPIO. The
GPIO is used as input and the respective interrupt is enabled when the
last SPI transfer is completed.

Depending on the GPIO controller the toggling MISO line might make the
interrupt pending even while it's masked. In that case the irq handler
is called immediately after irq_enable() and so before the device
actually pulls that line low which results in non-sense values being
reported to the upper layers.

The only way to find out if the line was actually pulled low is to read
the GPIO. (There is a flag in AD7124's status register that also signals
if an interrupt was asserted, but reading that register toggles the MISO
line and so might trigger another spurious interrupt.)

Add the possibility to specify an interrupt GPIO in the machine
description instead of a plain interrupt. This GPIO is used as interrupt
source and to check if the irq line is actually active in the irq
handler.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 36 +++++++++++++++++++++-----
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index e2bed2d648f2..d35602cfb093 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -539,12 +539,29 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
 {
 	struct ad_sigma_delta *sigma_delta = private;
 
-	complete(&sigma_delta->completion);
-	disable_irq_nosync(irq);
-	sigma_delta->irq_dis = true;
-	iio_trigger_poll(sigma_delta->trig);
+	/*
+	 * AD7124 and a few others use the same physical line for interrupt
+	 * reporting (nRDY) and MISO.
+	 * As MISO toggles when reading a register, this likely results in a
+	 * pending interrupt. This has two consequences: a) The irq might
+	 * trigger immediately after it's enabled even though the conversion
+	 * isn't done yet; and b) checking the STATUS register's nRDY flag is
+	 * off-limits as reading that would trigger another irq event.
+	 *
+	 * So read the MOSI line as GPIO (if available) and only trigger the irq
+	 * if the line is active.
+	 */
 
-	return IRQ_HANDLED;
+	if (!sigma_delta->irq_gpiod || gpiod_get_value(sigma_delta->irq_gpiod)) {
+		complete(&sigma_delta->completion);
+		disable_irq_nosync(irq);
+		sigma_delta->irq_dis = true;
+		iio_trigger_poll(sigma_delta->trig);
+
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
 }
 
 /**
@@ -676,8 +693,15 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 
 	if (info->irq_line)
 		sigma_delta->irq_line = info->irq_line;
-	else
+	else if (spi->irq)
 		sigma_delta->irq_line = spi->irq;
+	else {
+		sigma_delta->irq_gpiod = devm_gpiod_get(&spi->dev, "interrupt", GPIOD_IN);
+		if (IS_ERR(sigma_delta->irq_gpiod))
+			return dev_err_probe(&spi->dev, PTR_ERR(sigma_delta->irq_gpiod),
+					     "Failed to find interrupt gpio\n");
+		sigma_delta->irq_line = gpiod_to_irq(sigma_delta->irq_gpiod);
+	}
 
 	iio_device_set_drvdata(indio_dev, sigma_delta);
 
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index f8c1d2505940..fc0141e0f0ef 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -96,6 +96,7 @@ struct ad_sigma_delta {
 	unsigned int		active_slots;
 	unsigned int		current_slot;
 	unsigned int		num_slots;
+	struct gpio_desc	*irq_gpiod;
 	int		irq_line;
 	bool			status_appended;
 	/* map slots to channels in order to know what to expect from devices */
-- 
2.45.2

[PATCH 3/3] iio: adc: ad7124: Disable all channels at probe time

[Uwe Kleine-König]

When during a measurement two channels are enabled, two measurements are
done that are reported sequencially in the DATA register. As the code
triggered by reading one of the sysfs properties expects that only one
channel is enabled it only reads the first data set which might or might
not belong to the intended channel.

To prevent this situation disable all channels during probe. This fixes
a problem in practise because the reset default for channel 0 is
enabled. So all measurements before the first measurement on channel 0
(which disables channel 0 at the end) might report wrong values.

Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index a5d91933f505..912ba6592560 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st)
 		 * set all channels to this default value.
 		 */
 		ad7124_set_channel_odr(st, i, 10);
+
+		/* Disable all channels to prevent unintended conversions. */
+		ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0x0001);
 	}
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
-- 
2.45.2

Re: [PATCH 3/3] iio: adc: ad7124: Disable all channels at probe time

[Jonathan Cameron]

On Thu, 24 Oct 2024 19:17:05 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> When during a measurement two channels are enabled, two measurements are
> done that are reported sequencially in the DATA register. As the code
> triggered by reading one of the sysfs properties expects that only one
> channel is enabled it only reads the first data set which might or might
> not belong to the intended channel.
> 
> To prevent this situation disable all channels during probe. This fixes
> a problem in practise because the reset default for channel 0 is
> enabled. So all measurements before the first measurement on channel 0
> (which disables channel 0 at the end) might report wrong values.
> 
> Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Makes sense in general, but one comment inline.

> ---
>  drivers/iio/adc/ad7124.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index a5d91933f505..912ba6592560 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st)
>  		 * set all channels to this default value.
>  		 */
>  		ad7124_set_channel_odr(st, i, 10);
> +
> +		/* Disable all channels to prevent unintended conversions. */
> +		ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0x0001);
Why 1?  Build that default up from the register definitions rather than a magic
constant.


>  	}
>  
>  	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);

Re: [PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Jonathan Cameron]

On Thu, 24 Oct 2024 19:17:03 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> pin as the spi MISO output (DOUT) and so reading a register might
> trigger an interrupt. For correct operation it's critical that the
> actual state of the pin can be read to judge if an interrupt event is a
> real one or just a spurious one triggered by toggling the line in its
> MISO mode.

This text should note that this is a limitation with the interrupt controller.
The IRQ is disabled when those reads are going on, yet the controller is
still detecting the interrupt and reporting it on reenable.
I'm not an expert in what the kernel IRQ subsystem requires so maybe
this is a valid implementation.

> 
> Allow specification of an "interrupt-gpios" property instead of a plain
> interrupt. The semantic is that the GPIO's interrupt is to be used as
> event source and reading the GPIO can be used to differentiate between a
> real event and one triggered by MISO.

This sort of hack is a bit nasty and if we are going to do it we should
allow for double wiring - so to separate GPIO and interrupt pins on the
host wired to single pin on the device.

The binding does that by allowing both interrupts and interrupt-gpio
but we need to make that explicit in this text. Arguably even when
they are the same pin the binding should treat them as independent
and the driver should get the gpio from one, and the interrupt from
the other.

I also definitely need input from Analog Devices folk on this series.

Jonathan

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7124.yaml     | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> index 35ed04350e28..feb3a41a148e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> @@ -37,6 +37,9 @@ properties:
>      description: IRQ line for the ADC
>      maxItems: 1
>  
> +  interrupt-gpios:
> +    description: GPIO reading the interrupt line
> +
>    '#address-cells':
>      const: 1
>  
> @@ -57,7 +60,12 @@ required:
>    - reg
>    - clocks
>    - clock-names
> -  - interrupts
> +
> +oneOf:
> +  - required:
> +      - interrupts
> +  - required:
> +      - interrupt-gpios
>  
>  patternProperties:
>    "^channel@([0-9]|1[0-5])$":
> @@ -119,8 +127,7 @@ examples:
>          compatible = "adi,ad7124-4";
>          reg = <0>;
>          spi-max-frequency = <5000000>;
> -        interrupts = <25 2>;
> -        interrupt-parent = <&gpio>;
> +        interrupt-gpios = <&gpio 25 2>;
>          refin1-supply = <&adc_vref>;
>          clocks = <&ad7124_mclk>;
>          clock-names = "mclk";

Re: [PATCH 2/3] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

[Jonathan Cameron]

On Thu, 24 Oct 2024 19:17:04 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Some of the ADCs by Analog signal their irq condition on the MISO line.
> So typically that line is connected to an SPI controller and a GPIO. The
> GPIO is used as input and the respective interrupt is enabled when the
> last SPI transfer is completed.
> 
> Depending on the GPIO controller the toggling MISO line might make the
> interrupt pending even while it's masked. In that case the irq handler
> is called immediately after irq_enable() and so before the device
> actually pulls that line low which results in non-sense values being
> reported to the upper layers.
> 
> The only way to find out if the line was actually pulled low is to read
> the GPIO. (There is a flag in AD7124's status register that also signals
> if an interrupt was asserted, but reading that register toggles the MISO
> line and so might trigger another spurious interrupt.)
> 
> Add the possibility to specify an interrupt GPIO in the machine
> description instead of a plain interrupt. This GPIO is used as interrupt
> source and to check if the irq line is actually active in the irq
> handler.

Maybe we should just implement polling instead and never enable the interrupt
for cases like this?

I'm not sure if it is the device assumptions that are wrong wrt to the Linux
interrupt model, or the GPIO/IRQ chip you have doing something it shouldn't.
I am worried this solution is fragile.

Polling was never in the driver because the interrupt is always wired
but if we need it to get around the fact it is wired, but to a pin we can't
really use as an interrupt, then maybe it is time to add polling.
(probably just a fixed delay and check the status register).

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 36 +++++++++++++++++++++-----
>  include/linux/iio/adc/ad_sigma_delta.h |  1 +
>  2 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index e2bed2d648f2..d35602cfb093 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -539,12 +539,29 @@ static irqreturn_t ad_sd_data_rdy_trig_poll(int irq, void *private)
>  {
>  	struct ad_sigma_delta *sigma_delta = private;
>  
> -	complete(&sigma_delta->completion);
> -	disable_irq_nosync(irq);
> -	sigma_delta->irq_dis = true;
> -	iio_trigger_poll(sigma_delta->trig);
> +	/*
> +	 * AD7124 and a few others use the same physical line for interrupt
> +	 * reporting (nRDY) and MISO.

This is pretty common, so I'd drop the 'and a few others' bit.

> +	 * As MISO toggles when reading a register, this likely results in a
> +	 * pending interrupt. This has two consequences: a) The irq might
> +	 * trigger immediately after it's enabled even though the conversion
> +	 * isn't done yet; and b) checking the STATUS register's nRDY flag is
> +	 * off-limits as reading that would trigger another irq event.
> +	 *
> +	 * So read the MOSI line as GPIO (if available) and only trigger the irq
> +	 * if the line is active.
> +	 */
>  
> -	return IRQ_HANDLED;
> +	if (!sigma_delta->irq_gpiod || gpiod_get_value(sigma_delta->irq_gpiod)) {
> +		complete(&sigma_delta->completion);
> +		disable_irq_nosync(irq);
> +		sigma_delta->irq_dis = true;
> +		iio_trigger_poll(sigma_delta->trig);
> +
> +		return IRQ_HANDLED;
> +	} else {
> +		return IRQ_NONE;
> +	}
>  }
>  
>  /**
> @@ -676,8 +693,15 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>  
>  	if (info->irq_line)
>  		sigma_delta->irq_line = info->irq_line;
> -	else
> +	else if (spi->irq)
>  		sigma_delta->irq_line = spi->irq;
> +	else {
> +		sigma_delta->irq_gpiod = devm_gpiod_get(&spi->dev, "interrupt", GPIOD_IN);
> +		if (IS_ERR(sigma_delta->irq_gpiod))
> +			return dev_err_probe(&spi->dev, PTR_ERR(sigma_delta->irq_gpiod),
> +					     "Failed to find interrupt gpio\n");
> +		sigma_delta->irq_line = gpiod_to_irq(sigma_delta->irq_gpiod);

As I mentioned on the binding if this is something we are going to do there should be
no assumption of the gpio and the irq being the same pin.

> +	}
>  
>  	iio_device_set_drvdata(indio_dev, sigma_delta);
>  
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index f8c1d2505940..fc0141e0f0ef 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -96,6 +96,7 @@ struct ad_sigma_delta {
>  	unsigned int		active_slots;
>  	unsigned int		current_slot;
>  	unsigned int		num_slots;
> +	struct gpio_desc	*irq_gpiod;
>  	int		irq_line;
>  	bool			status_appended;
>  	/* map slots to channels in order to know what to expect from devices */

Re: [PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Krzysztof Kozlowski]

On Thu, Oct 24, 2024 at 07:17:03PM +0200, Uwe Kleine-König wrote:
> For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> pin as the spi MISO output (DOUT) and so reading a register might
> trigger an interrupt. For correct operation it's critical that the
> actual state of the pin can be read to judge if an interrupt event is a
> real one or just a spurious one triggered by toggling the line in its
> MISO mode.
> 
> Allow specification of an "interrupt-gpios" property instead of a plain
> interrupt. The semantic is that the GPIO's interrupt is to be used as
> event source and reading the GPIO can be used to differentiate between a
> real event and one triggered by MISO.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7124.yaml     | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> index 35ed04350e28..feb3a41a148e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> @@ -37,6 +37,9 @@ properties:
>      description: IRQ line for the ADC
>      maxItems: 1
>  
> +  interrupt-gpios:
> +    description: GPIO reading the interrupt line

Missing constraints, maxItems

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Rob Herring]

On Thu, Oct 24, 2024 at 07:17:03PM +0200, Uwe Kleine-König wrote:
> For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> pin as the spi MISO output (DOUT) and so reading a register might
> trigger an interrupt. For correct operation it's critical that the
> actual state of the pin can be read to judge if an interrupt event is a
> real one or just a spurious one triggered by toggling the line in its
> MISO mode.
> 
> Allow specification of an "interrupt-gpios" property instead of a plain
> interrupt. The semantic is that the GPIO's interrupt is to be used as
> event source and reading the GPIO can be used to differentiate between a
> real event and one triggered by MISO.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7124.yaml     | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> index 35ed04350e28..feb3a41a148e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> @@ -37,6 +37,9 @@ properties:
>      description: IRQ line for the ADC
>      maxItems: 1
>  
> +  interrupt-gpios:

Name it for the pin/signal, not how you are going to use it: rdy-gpios

Rob

Re: [PATCH 3/3] iio: adc: ad7124: Disable all channels at probe time

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

On Sun, Oct 27, 2024 at 11:42:28AM +0000, Jonathan Cameron wrote:
> On Thu, 24 Oct 2024 19:17:05 +0200
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > When during a measurement two channels are enabled, two measurements are
> > done that are reported sequencially in the DATA register. As the code
> > triggered by reading one of the sysfs properties expects that only one
> > channel is enabled it only reads the first data set which might or might
> > not belong to the intended channel.
> > 
> > To prevent this situation disable all channels during probe. This fixes
> > a problem in practise because the reset default for channel 0 is
> > enabled. So all measurements before the first measurement on channel 0
> > (which disables channel 0 at the end) might report wrong values.
> > 
> > Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Makes sense in general, but one comment inline.
> 
> > ---
> >  drivers/iio/adc/ad7124.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> > index a5d91933f505..912ba6592560 100644
> > --- a/drivers/iio/adc/ad7124.c
> > +++ b/drivers/iio/adc/ad7124.c
> > @@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st)
> >  		 * set all channels to this default value.
> >  		 */
> >  		ad7124_set_channel_odr(st, i, 10);
> > +
> > +		/* Disable all channels to prevent unintended conversions. */
> > +		ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0x0001);
> Why 1?  Build that default up from the register definitions rather than a magic
> constant.

I picked 0x0001 because that's the documented reset default values for
the channels > 0. But agreed. Will fix in a v2.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2318 bytes --]

Hello,

[dropped Alexandru Tachici from Cc:, the address bounces]

On Sun, Oct 27, 2024 at 11:54:09AM +0000, Jonathan Cameron wrote:
> On Thu, 24 Oct 2024 19:17:03 +0200
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> > pin as the spi MISO output (DOUT) and so reading a register might
> > trigger an interrupt. For correct operation it's critical that the
> > actual state of the pin can be read to judge if an interrupt event is a
> > real one or just a spurious one triggered by toggling the line in its
> > MISO mode.
> 
> This text should note that this is a limitation with the interrupt controller.
> The IRQ is disabled when those reads are going on, yet the controller is
> still detecting the interrupt and reporting it on reenable.
> I'm not an expert in what the kernel IRQ subsystem requires so maybe
> this is a valid implementation.

This is even the saner option and a controller not triggering might miss
irqs. Consider the process that triggered a conversion and then calls
enable_irq() is preempted long enough that the conversion is already
done when enable_irq() is called. The completion would just time out and
no measurement reported.

> > Allow specification of an "interrupt-gpios" property instead of a plain
> > interrupt. The semantic is that the GPIO's interrupt is to be used as
> > event source and reading the GPIO can be used to differentiate between a
> > real event and one triggered by MISO.
> 
> This sort of hack is a bit nasty and if we are going to do it we should
> allow for double wiring - so to separate GPIO and interrupt pins on the
> host wired to single pin on the device.
> 
> The binding does that by allowing both interrupts and interrupt-gpio
> but we need to make that explicit in this text. Arguably even when
> they are the same pin the binding should treat them as independent
> and the driver should get the gpio from one, and the interrupt from
> the other.

This would also need a code update because currently the interrupt-gpio
is only used if no interrupt is specified.

> I also definitely need input from Analog Devices folk on this series.

Good candidates to comment are still on Cc:

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

Hello Rob,

On Sun, Oct 27, 2024 at 04:26:22PM -0500, Rob Herring wrote:
> On Thu, Oct 24, 2024 at 07:17:03PM +0200, Uwe Kleine-König wrote:
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> > index 35ed04350e28..feb3a41a148e 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> > @@ -37,6 +37,9 @@ properties:
> >      description: IRQ line for the ADC
> >      maxItems: 1
> >  
> > +  interrupt-gpios:
> 
> Name it for the pin/signal, not how you are going to use it: rdy-gpios

Good idea. The line is called ̅̅R̅D̅Y, is rdy-gpios still right? I'd
consider nRDY-gpios.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line

[Jonathan Cameron]

On Sun, 27 Oct 2024 22:53:39 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> [dropped Alexandru Tachici from Cc:, the address bounces]
> 
> On Sun, Oct 27, 2024 at 11:54:09AM +0000, Jonathan Cameron wrote:
> > On Thu, 24 Oct 2024 19:17:03 +0200
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> >   
> > > For the AD7124 chip the logical irq line (̅R̅D̅Y) is physically on the same
> > > pin as the spi MISO output (DOUT) and so reading a register might
> > > trigger an interrupt. For correct operation it's critical that the
> > > actual state of the pin can be read to judge if an interrupt event is a
> > > real one or just a spurious one triggered by toggling the line in its
> > > MISO mode.  
> > 
> > This text should note that this is a limitation with the interrupt controller.
> > The IRQ is disabled when those reads are going on, yet the controller is
> > still detecting the interrupt and reporting it on reenable.
> > I'm not an expert in what the kernel IRQ subsystem requires so maybe
> > this is a valid implementation.  
> 
> This is even the saner option and a controller not triggering might miss
> irqs. Consider the process that triggered a conversion and then calls
> enable_irq() is preempted long enough that the conversion is already
> done when enable_irq() is called. The completion would just time out and
> no measurement reported.

True enough - that race is a possibility, but not all interrupt inputs
are capable of gpio usage whilst setup to received interrupts.

> 
> > > Allow specification of an "interrupt-gpios" property instead of a plain
> > > interrupt. The semantic is that the GPIO's interrupt is to be used as
> > > event source and reading the GPIO can be used to differentiate between a
> > > real event and one triggered by MISO.  
> > 
> > This sort of hack is a bit nasty and if we are going to do it we should
> > allow for double wiring - so to separate GPIO and interrupt pins on the
> > host wired to single pin on the device.
> > 
> > The binding does that by allowing both interrupts and interrupt-gpio
> > but we need to make that explicit in this text. Arguably even when
> > they are the same pin the binding should treat them as independent
> > and the driver should get the gpio from one, and the interrupt from
> > the other.  
> 
> This would also need a code update because currently the interrupt-gpio
> is only used if no interrupt is specified.

Absolutely.  It would require slightly different code.


> 
> > I also definitely need input from Analog Devices folk on this series.  
> 
> Good candidates to comment are still on Cc:
Yeah. I was poking them  :)

> 
> Best regards
> Uwe