| Message ID | 20241024171703.201436-6-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7124: Make it work on de10-nano | expand |
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";
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
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
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
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
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
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";
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(-)