Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
low-power with monitor capabilities SAR ADCs. Each variant of the family
differs in speed and resolution, resulting in different scan types and
spi word sizes, that are matched by the compatible with the chip_info.
The device contains one input (cnv) and two outputs (gp0, gp1).
The outputs can be configured for range of options, such as threshold
and data ready.
The spi-max-frequency refers to the configuration mode maximum access
speed. The ADC mode speed depends on the vio input voltage.
Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
.../devicetree/bindings/iio/adc/adi,ad4052.yaml | 167 +++++++++++++++++++++
MAINTAINERS | 6 +
include/dt-bindings/iio/adc/adi,ad4052.h | 17 +++
3 files changed, 190 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..2cf197e2d872d9a3d4f7210121a1e38f784f92dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
@@ -0,0 +1,167 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2025 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4052.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4052 ADC family device driver
+
+maintainers:
+ - Jorge Marques <jorge.marques@analog.com>
+
+description: |
+ Analog Devices AD4052 Single Channel Precision SAR ADC family
+
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050-ad4056.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052-ad4058.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,ad4050
+ - adi,ad4052
+ - adi,ad4056
+ - adi,ad4058
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ items:
+ - description: Signal coming from the GP0 pin.
+ - description: Signal coming from the GP1 pin.
+
+ interrupt-names:
+ items:
+ - const: gp0
+ - const: gp1
+
+ cnv-gpios:
+ description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
+ maxItems: 1
+
+ pwms:
+ maxItems: 1
+ description: PWM connected to the CNV pin.
+
+ trigger-sources:
+ minItems: 1
+ maxItems: 2
+ description:
+ Describes the output pin and event associated.
+
+ "#trigger-source-cells":
+ const: 2
+ description: |
+ Output pins used as trigger source.
+
+ Cell 0 defines the event:
+ * 0 = Data ready
+ * 1 = Min threshold
+ * 2 = Max threshold
+ * 3 = Either threshold
+ * 4 = CHOP control
+ * 5 = Device enable
+ * 6 = Device ready (only GP1)
+
+ Cell 1 defines which pin:
+ * 0 = GP0
+ * 1 = GP1
+
+ For convenience, macros for these values are available in
+ dt-bindings/iio/adc/adi,ad4052.h.
+
+ spi-max-frequency:
+ maximum: 83333333
+
+ vdd-supply:
+ description: Analog power supply.
+
+ vio-supply:
+ description: Digital interface logic power supply.
+
+ ref-supply:
+ description: |
+ Reference voltage to set the ADC full-scale range. If not present,
+ vdd-supply is used as the reference voltage.
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+ - vio-supply
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/iio/adc/adi,ad4052.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,ad4052";
+ reg = <0>;
+ vdd-supply = <&vdd>;
+ vio-supply = <&vio>;
+ ref-supply = <&ref>;
+ spi-max-frequency = <83333333>;
+
+ #trigger-source-cells = <2>;
+ trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
+ AD4052_TRIGGER_PIN_GP0
+ &adc AD4052_TRIGGER_EVENT_DATA_READY
+ AD4052_TRIGGER_PIN_GP1>;
+ interrupt-parent = <&gpio>;
+ interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
+ <0 1 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-names = "gp0", "gp1";
+ cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ };
+ };
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/iio/adc/adi,ad4052.h>
+
+ rx_dma {
+ #dma-cells = <1>;
+ };
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dmas = <&rx_dma 0>;
+ dma-names = "offload0-rx";
+ trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
+ AD4052_TRIGGER_PIN_GP1>;
+
+ adc@0 {
+ compatible = "adi,ad4052";
+ reg = <0>;
+ vdd-supply = <&vdd>;
+ vio-supply = <&vio>;
+ spi-max-frequency = <83333333>;
+ pwms = <&adc_trigger 0 10000 0>;
+
+ #trigger-source-cells = <2>;
+ trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
+ AD4052_TRIGGER_PIN_GP0
+ &adc AD4052_TRIGGER_EVENT_DATA_READY
+ AD4052_TRIGGER_PIN_GP1>;
+ interrupt-parent = <&gpio>;
+ interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
+ <0 1 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-names = "gp0", "gp1";
+ cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index c02d83560058f7ea75e24509b4d87ef293df6773..d000c7de7ff9eba390f87593bc2b1847f966f48b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1337,6 +1337,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
F: Documentation/iio/ad4030.rst
F: drivers/iio/adc/ad4030.c
+ANALOG DEVICES INC AD4052 DRIVER
+M: Jorge Marques <jorge.marques@analog.com>
+S: Supported
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
+
ANALOG DEVICES INC AD4080 DRIVER
M: Antoniu Miclaus <antoniu.miclaus@analog.com>
L: linux-iio@vger.kernel.org
diff --git a/include/dt-bindings/iio/adc/adi,ad4052.h b/include/dt-bindings/iio/adc/adi,ad4052.h
new file mode 100644
index 0000000000000000000000000000000000000000..37db5d9d10e788d5e7fb715c4ba9077e555131d5
--- /dev/null
+++ b/include/dt-bindings/iio/adc/adi,ad4052.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_ADI_AD4052_H
+#define _DT_BINDINGS_ADI_AD4052_H
+
+#define AD4052_TRIGGER_EVENT_DATA_READY 0
+#define AD4052_TRIGGER_EVENT_MIN_THRESH 1
+#define AD4052_TRIGGER_EVENT_MAX_THRESH 2
+#define AD4052_TRIGGER_EVENT_EITHER_THRESH 3
+#define AD4052_TRIGGER_EVENT_CHOP 4
+#define AD4052_TRIGGER_EVENT_DEV_ENABLED 5
+#define AD4052_TRIGGER_EVENT_DEV_READY 6
+
+#define AD4052_TRIGGER_PIN_GP0 0
+#define AD4052_TRIGGER_PIN_GP1 1
+
+#endif /* _DT_BINDINGS_ADI_AD4052_H */
--
2.49.0On Tue, 10 Jun 2025 09:34:35 +0200
Jorge Marques <jorge.marques@analog.com> wrote:
> Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
> low-power with monitor capabilities SAR ADCs. Each variant of the family
> differs in speed and resolution, resulting in different scan types and
> spi word sizes, that are matched by the compatible with the chip_info.
The bit about what the drive does with this doesn't really belong in a DT
binding patch description. Stick to just something like.
Each variant of the family differs in speed and resolution, reflected
in word size for SPI messages.
> The device contains one input (cnv) and two outputs (gp0, gp1).
> The outputs can be configured for range of options, such as threshold
> and data ready.
> The spi-max-frequency refers to the configuration mode maximum access
> speed. The ADC mode speed depends on the vio input voltage.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
> .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 167 +++++++++++++++++++++
> MAINTAINERS | 6 +
> include/dt-bindings/iio/adc/adi,ad4052.h | 17 +++
> 3 files changed, 190 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..2cf197e2d872d9a3d4f7210121a1e38f784f92dc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> @@ -0,0 +1,167 @@
> +
> + interrupts:
> + items:
> + - description: Signal coming from the GP0 pin.
Description would be better in interrupt-names than here.
> + - description: Signal coming from the GP1 pin.
Also minItems should be specified to allow for just one of these
being wired I think.
> +
> + interrupt-names:
> + items:
> + - const: gp0
> + - const: gp1
> +
> + cnv-gpios:
> + description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
> + maxItems: 1
> +
> + pwms:
> + maxItems: 1
> + description: PWM connected to the CNV pin.
> +
> + trigger-sources:
> + minItems: 1
> + maxItems: 2
> + description:
> + Describes the output pin and event associated.
> +
> + "#trigger-source-cells":
> + const: 2
> + description: |
> + Output pins used as trigger source.
> +
> + Cell 0 defines the event:
> + * 0 = Data ready
> + * 1 = Min threshold
> + * 2 = Max threshold
> + * 3 = Either threshold
> + * 4 = CHOP control
> + * 5 = Device enable
> + * 6 = Device ready (only GP1)
Hmm. I'm a bit dubious on why 'what the offload trigger is'
is a DT thing? Is that because the IP needs to comprehend
this? I guess only data ready is actually supported in
practice?
What would the use of Device enable or device ready or chop
control actually be?
The thresholds are unusual but those I can sort of understand.
Jonathan
> +
> + Cell 1 defines which pin:
> + * 0 = GP0
> + * 1 = GP1
> +
> + For convenience, macros for these values are available in
> + dt-bindings/iio/adc/adi,ad4052.h.
> +
> + spi-max-frequency:
> + maximum: 83333333
> +
> + vdd-supply:
> + description: Analog power supply.
> +
> + vio-supply:
> + description: Digital interface logic power supply.
> +
> + ref-supply:
> + description: |
Don't need the | as no need to preserve anything about formatting of
a single paragraph like this.
> + Reference voltage to set the ADC full-scale range. If not present,
> + vdd-supply is used as the reference voltage.
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> + - vio-supply
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@0 {
> + compatible = "adi,ad4052";
> + reg = <0>;
> + vdd-supply = <&vdd>;
> + vio-supply = <&vio>;
> + ref-supply = <&ref>;
> + spi-max-frequency = <83333333>;
> +
> + #trigger-source-cells = <2>;
> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> + AD4052_TRIGGER_PIN_GP0
> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> + AD4052_TRIGGER_PIN_GP1>;
> + interrupt-parent = <&gpio>;
> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-names = "gp0", "gp1";
> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> + };
> + };
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> +
> + rx_dma {
> + #dma-cells = <1>;
> + };
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dmas = <&rx_dma 0>;
> + dma-names = "offload0-rx";
> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
> + AD4052_TRIGGER_PIN_GP1>;
> +
> + adc@0 {
> + compatible = "adi,ad4052";
> + reg = <0>;
> + vdd-supply = <&vdd>;
> + vio-supply = <&vio>;
> + spi-max-frequency = <83333333>;
> + pwms = <&adc_trigger 0 10000 0>;
> +
> + #trigger-source-cells = <2>;
> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> + AD4052_TRIGGER_PIN_GP0
> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> + AD4052_TRIGGER_PIN_GP1>;
> + interrupt-parent = <&gpio>;
> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-names = "gp0", "gp1";
> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c02d83560058f7ea75e24509b4d87ef293df6773..d000c7de7ff9eba390f87593bc2b1847f966f48b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1337,6 +1337,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> F: Documentation/iio/ad4030.rst
> F: drivers/iio/adc/ad4030.c
>
> +ANALOG DEVICES INC AD4052 DRIVER
> +M: Jorge Marques <jorge.marques@analog.com>
> +S: Supported
> +W: https://ez.analog.com/linux-software-drivers
> +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> +
> ANALOG DEVICES INC AD4080 DRIVER
> M: Antoniu Miclaus <antoniu.miclaus@analog.com>
> L: linux-iio@vger.kernel.org
> diff --git a/include/dt-bindings/iio/adc/adi,ad4052.h b/include/dt-bindings/iio/adc/adi,ad4052.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..37db5d9d10e788d5e7fb715c4ba9077e555131d5
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/adi,ad4052.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +
> +#ifndef _DT_BINDINGS_ADI_AD4052_H
> +#define _DT_BINDINGS_ADI_AD4052_H
> +
> +#define AD4052_TRIGGER_EVENT_DATA_READY 0
> +#define AD4052_TRIGGER_EVENT_MIN_THRESH 1
> +#define AD4052_TRIGGER_EVENT_MAX_THRESH 2
> +#define AD4052_TRIGGER_EVENT_EITHER_THRESH 3
> +#define AD4052_TRIGGER_EVENT_CHOP 4
> +#define AD4052_TRIGGER_EVENT_DEV_ENABLED 5
> +#define AD4052_TRIGGER_EVENT_DEV_READY 6
> +
> +#define AD4052_TRIGGER_PIN_GP0 0
> +#define AD4052_TRIGGER_PIN_GP1 1
> +
> +#endif /* _DT_BINDINGS_ADI_AD4052_H */
>
On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:35 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
>
> > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
> > low-power with monitor capabilities SAR ADCs. Each variant of the family
> > differs in speed and resolution, resulting in different scan types and
> > spi word sizes, that are matched by the compatible with the chip_info.
>
> The bit about what the drive does with this doesn't really belong in a DT
> binding patch description. Stick to just something like.
>
> Each variant of the family differs in speed and resolution, reflected
> in word size for SPI messages.
>
> > The device contains one input (cnv) and two outputs (gp0, gp1).
> > The outputs can be configured for range of options, such as threshold
> > and data ready.
> > The spi-max-frequency refers to the configuration mode maximum access
> > speed. The ADC mode speed depends on the vio input voltage.
> >
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> > .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 167 +++++++++++++++++++++
> > MAINTAINERS | 6 +
> > include/dt-bindings/iio/adc/adi,ad4052.h | 17 +++
> > 3 files changed, 190 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..2cf197e2d872d9a3d4f7210121a1e38f784f92dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> > @@ -0,0 +1,167 @@
> > +
> > + interrupts:
> > + items:
> > + - description: Signal coming from the GP0 pin.
> Description would be better in interrupt-names than here.
> > + - description: Signal coming from the GP1 pin.
>
> Also minItems should be specified to allow for just one of these
> being wired I think.
>
Ack, then maxItems is also required, or dt_binding_check complains
> "interrupts: [[...], [...]] is too long"
since I don't have the items to infer length, that means, the updated
yaml is:
interrupts:
minItems: 1
maxItems: 2
interrupt-names:
items:
- const: gp0
description: Signal coming from the GP0 pin.
- const: gp1
description: Signal coming from the GP1 pin.
> > +
> > + interrupt-names:
> > + items:
> > + - const: gp0
> > + - const: gp1
> > +
> > + cnv-gpios:
> > + description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
> > + maxItems: 1
> > +
> > + pwms:
> > + maxItems: 1
> > + description: PWM connected to the CNV pin.
> > +
> > + trigger-sources:
> > + minItems: 1
> > + maxItems: 2
> > + description:
> > + Describes the output pin and event associated.
> > +
> > + "#trigger-source-cells":
> > + const: 2
> > + description: |
> > + Output pins used as trigger source.
> > +
> > + Cell 0 defines the event:
> > + * 0 = Data ready
> > + * 1 = Min threshold
> > + * 2 = Max threshold
> > + * 3 = Either threshold
> > + * 4 = CHOP control
> > + * 5 = Device enable
> > + * 6 = Device ready (only GP1)
>
> Hmm. I'm a bit dubious on why 'what the offload trigger is'
> is a DT thing? Is that because the IP needs to comprehend
> this? I guess only data ready is actually supported in
> practice?
A trigger can be connected to trigger something other than a spi
offload, it is in the DT because it describes how the device is
connected. When using spi offload, the trigger-source at the spi handle
describes which gpio and event is routed to the offload trigger input.
At the ADC node, trigger-source-cells describe the source gpio and event
for the device driver.
In practice, in this series, one gpio is Data ready, triggering offload
when buffer enabled, and raw reads, when disabled. And the other is
Either threshold, propagated as an IIO event. Fancy logic can be added
to the driver in future patches to allow other combinations.
It is also worth to mention that the trigger-source is duplicated for
each node that uses it, as seen in the second dts example:
&adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
Is repeated on both adc and spi node.
One last thing, on the driver, for v3, I should handle -ENOENT:
ret = of_parse_phandle_with_args(np, "trigger-sources",
"#trigger-source-cells", i,
&trigger_sources);
if (ret)
return ret == -ENOENT ? 0 : ret;
To assert only when present, since the nodes are not required.
Or, in the driver,
require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
(I would go with the first option).
>
> What would the use of Device enable or device ready or chop
> control actually be?
>
Device enable is the default register value, it could signal the
conclusion of the reset or exit of low power mode. The CHOP signal is
used to synchronize an auto-zero amplifier's chopping and the adc's
sampling and is an example of when the trigger is connected to something
other than spi. In that particular topology, the user would then set at
the devicetree to use the gpio for that, at the hypothetical chop
consumer node. Still, in the driver of this series, only the Data ready
and Either threshold is supported.
Best regards,
Jorge
> The thresholds are unusual but those I can sort of understand.
>
> Jonathan
>
> > +
> > + Cell 1 defines which pin:
> > + * 0 = GP0
> > + * 1 = GP1
> > +
> > + For convenience, macros for these values are available in
> > + dt-bindings/iio/adc/adi,ad4052.h.
> > +
> > + spi-max-frequency:
> > + maximum: 83333333
> > +
> > + vdd-supply:
> > + description: Analog power supply.
> > +
> > + vio-supply:
> > + description: Digital interface logic power supply.
> > +
> > + ref-supply:
> > + description: |
>
> Don't need the | as no need to preserve anything about formatting of
> a single paragraph like this.
>
Ack.
>
> > + Reference voltage to set the ADC full-scale range. If not present,
> > + vdd-supply is used as the reference voltage.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - vdd-supply
> > + - vio-supply
> > +
> > +allOf:
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/iio/adc/adi,ad4052.h>
> > +
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + adc@0 {
> > + compatible = "adi,ad4052";
> > + reg = <0>;
> > + vdd-supply = <&vdd>;
> > + vio-supply = <&vio>;
> > + ref-supply = <&ref>;
> > + spi-max-frequency = <83333333>;
> > +
> > + #trigger-source-cells = <2>;
> > + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> > + AD4052_TRIGGER_PIN_GP0
> > + &adc AD4052_TRIGGER_EVENT_DATA_READY
> > + AD4052_TRIGGER_PIN_GP1>;
> > + interrupt-parent = <&gpio>;
> > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > + interrupt-names = "gp0", "gp1";
> > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > + };
> > + };
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/iio/adc/adi,ad4052.h>
> > +
> > + rx_dma {
> > + #dma-cells = <1>;
> > + };
> > +
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + dmas = <&rx_dma 0>;
> > + dma-names = "offload0-rx";
> > + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
> > + AD4052_TRIGGER_PIN_GP1>;
> > +
> > + adc@0 {
> > + compatible = "adi,ad4052";
> > + reg = <0>;
> > + vdd-supply = <&vdd>;
> > + vio-supply = <&vio>;
> > + spi-max-frequency = <83333333>;
> > + pwms = <&adc_trigger 0 10000 0>;
> > +
> > + #trigger-source-cells = <2>;
> > + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> > + AD4052_TRIGGER_PIN_GP0
> > + &adc AD4052_TRIGGER_EVENT_DATA_READY
> > + AD4052_TRIGGER_PIN_GP1>;
> > + interrupt-parent = <&gpio>;
> > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > + interrupt-names = "gp0", "gp1";
> > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > + };
> > + };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c02d83560058f7ea75e24509b4d87ef293df6773..d000c7de7ff9eba390f87593bc2b1847f966f48b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1337,6 +1337,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> > F: Documentation/iio/ad4030.rst
> > F: drivers/iio/adc/ad4030.c
> >
> > +ANALOG DEVICES INC AD4052 DRIVER
> > +M: Jorge Marques <jorge.marques@analog.com>
> > +S: Supported
> > +W: https://ez.analog.com/linux-software-drivers
> > +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> > +
> > ANALOG DEVICES INC AD4080 DRIVER
> > M: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > L: linux-iio@vger.kernel.org
> > diff --git a/include/dt-bindings/iio/adc/adi,ad4052.h b/include/dt-bindings/iio/adc/adi,ad4052.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..37db5d9d10e788d5e7fb715c4ba9077e555131d5
> > --- /dev/null
> > +++ b/include/dt-bindings/iio/adc/adi,ad4052.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +
> > +#ifndef _DT_BINDINGS_ADI_AD4052_H
> > +#define _DT_BINDINGS_ADI_AD4052_H
> > +
> > +#define AD4052_TRIGGER_EVENT_DATA_READY 0
> > +#define AD4052_TRIGGER_EVENT_MIN_THRESH 1
> > +#define AD4052_TRIGGER_EVENT_MAX_THRESH 2
> > +#define AD4052_TRIGGER_EVENT_EITHER_THRESH 3
> > +#define AD4052_TRIGGER_EVENT_CHOP 4
> > +#define AD4052_TRIGGER_EVENT_DEV_ENABLED 5
> > +#define AD4052_TRIGGER_EVENT_DEV_READY 6
> > +
> > +#define AD4052_TRIGGER_PIN_GP0 0
> > +#define AD4052_TRIGGER_PIN_GP1 1
> > +
> > +#endif /* _DT_BINDINGS_ADI_AD4052_H */
> >
>
On 6/12/25 5:11 AM, Jorge Marques wrote:
> On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
>> On Tue, 10 Jun 2025 09:34:35 +0200
>> Jorge Marques <jorge.marques@analog.com> wrote:
>>
...
>>> + trigger-sources:
>>> + minItems: 1
>>> + maxItems: 2
>>> + description:
>>> + Describes the output pin and event associated.
trigger-sources would be an input pin connected to an external trigger.
For example, the CNV pin could be connected to a trigger-source
provider to trigger a conversion. But there aren't any other digital
inputs, so I don't know what the 2nd source would be here.
As an example, see [1]. We could potentially use the same gpio
trigger-source for the conversion pin here. There is already
a similar binding for pwm triggers, so we could drop the separate
pwms binding as well an just have a single trigger-sources
property for the CNV pin that works for both gpio and pwm.
[1]: https://lore.kernel.org/linux-iio/cover.1749569957.git.Jonathan.Santos@analog.com/
>>> +
>>> + "#trigger-source-cells":
>>> + const: 2
>>> + description: |
>>> + Output pins used as trigger source.
>>> +
>>> + Cell 0 defines the event:
>>> + * 0 = Data ready
>>> + * 1 = Min threshold
>>> + * 2 = Max threshold
>>> + * 3 = Either threshold
>>> + * 4 = CHOP control
>>> + * 5 = Device enable
>>> + * 6 = Device ready (only GP1)
>>
>> Hmm. I'm a bit dubious on why 'what the offload trigger is'
>> is a DT thing? Is that because the IP needs to comprehend
>> this? I guess only data ready is actually supported in
>> practice?
>
> A trigger can be connected to trigger something other than a spi
> offload, it is in the DT because it describes how the device is
> connected. When using spi offload, the trigger-source at the spi handle
> describes which gpio and event is routed to the offload trigger input.
> At the ADC node, trigger-source-cells describe the source gpio and event
> for the device driver.
>
> In practice, in this series, one gpio is Data ready, triggering offload
> when buffer enabled, and raw reads, when disabled. And the other is
> Either threshold, propagated as an IIO event. Fancy logic can be added
> to the driver in future patches to allow other combinations.
>
> It is also worth to mention that the trigger-source is duplicated for
> each node that uses it, as seen in the second dts example:
>
> &adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
>
> Is repeated on both adc and spi node.
That sounds wrong. This would only make sense if an output of the
ADC was wired back to itself.
>
> One last thing, on the driver, for v3, I should handle -ENOENT:
>
> ret = of_parse_phandle_with_args(np, "trigger-sources",
> "#trigger-source-cells", i,
> &trigger_sources);
> if (ret)
> return ret == -ENOENT ? 0 : ret;
>
> To assert only when present, since the nodes are not required.
> Or, in the driver,
> require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
> require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
> (I would go with the first option).
>>
,,,
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/gpio/gpio.h>
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
>>> +
>>> + spi {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + adc@0 {
>>> + compatible = "adi,ad4052";
>>> + reg = <0>;
>>> + vdd-supply = <&vdd>;
>>> + vio-supply = <&vio>;
>>> + ref-supply = <&ref>;
>>> + spi-max-frequency = <83333333>;
>>> +
>>> + #trigger-source-cells = <2>;
>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
>>> + AD4052_TRIGGER_PIN_GP0
>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
>>> + AD4052_TRIGGER_PIN_GP1>;
This doesn't make sense for the reason given above. These outputs
aren't wired back to inputs on the ADC. They are wired to interrupts
on the MCU, which is already described below.
>>> + interrupt-parent = <&gpio>;
>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
>>> + interrupt-names = "gp0", "gp1";
>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
>>> + };
>>> + };
>>> + - |
>>> + #include <dt-bindings/gpio/gpio.h>
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
>>> +
>>> + rx_dma {
>>> + #dma-cells = <1>;
>>> + };
>>> +
>>> + spi {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + dmas = <&rx_dma 0>;
>>> + dma-names = "offload0-rx";
The dmas aren't related to the ADC, so can be left out of the example.
>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
>>> + AD4052_TRIGGER_PIN_GP1>;
>>> +
>>> + adc@0 {
>>> + compatible = "adi,ad4052";
>>> + reg = <0>;
>>> + vdd-supply = <&vdd>;
>>> + vio-supply = <&vio>;
>>> + spi-max-frequency = <83333333>;
>>> + pwms = <&adc_trigger 0 10000 0>;
>>> +
>>> + #trigger-source-cells = <2>;
>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
>>> + AD4052_TRIGGER_PIN_GP0
>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
>>> + AD4052_TRIGGER_PIN_GP1>;
Same as above - the GP pins aren't wired back to the ADC itself.
>>> + interrupt-parent = <&gpio>;
>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
>>> + interrupt-names = "gp0", "gp1";
>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
>>> + };
>>> + };
Hi David,
thank you for chiming in
On Thu, Jun 12, 2025 at 10:03:37AM -0500, David Lechner wrote:
> On 6/12/25 5:11 AM, Jorge Marques wrote:
> > On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
> >> On Tue, 10 Jun 2025 09:34:35 +0200
> >> Jorge Marques <jorge.marques@analog.com> wrote:
> >>
>
> ...
>
> >>> + trigger-sources:
> >>> + minItems: 1
> >>> + maxItems: 2
> >>> + description:
> >>> + Describes the output pin and event associated.
>
> trigger-sources would be an input pin connected to an external trigger.
> For example, the CNV pin could be connected to a trigger-source
> provider to trigger a conversion. But there aren't any other digital
> inputs, so I don't know what the 2nd source would be here.
>
> As an example, see [1]. We could potentially use the same gpio
> trigger-source for the conversion pin here. There is already
> a similar binding for pwm triggers, so we could drop the separate
> pwms binding as well an just have a single trigger-sources
> property for the CNV pin that works for both gpio and pwm.
>
> [1]: https://lore.kernel.org/linux-iio/cover.1749569957.git.Jonathan.Santos@analog.com/
>
Quick summary to familiarize myself with this part and driver.
On ad7768-1:
ad7768-1.SYNC_OUT is a digital output, ad7768-1.SYNC_IN input, and
ad7768-1.GPIO3 (START) configured as input. ad7768-1.GPIO[0..3] are
configurable GPIO, GPIO3 as START, or in PIN control mode, the input
GPIO[3:0] sets the power mode and modulator freq (MODEx).
On that thread:
https://lore.kernel.org/linux-iio/8abca580f43cb31d7088d07a7414b5f7efe91ead.1749569957.git.Jonathan.Santos@analog.com/
exposes GPIO[0..3] through gpio_chip if gpio-controller in dt.
https://lore.kernel.org/linux-iio/713fd786010c75858700efaec8bb285274e7057e.1749569957.git.Jonathan.Santos@analog.com/
trigger-sources-cells: the cell define the type of signal but *not* its
origin, because {DRDY, SYNC_OUT, GPIO3(START)} are dedicated pins, *so
there is no need to do so*.
> >>> +
> >>> + "#trigger-source-cells":
> >>> + const: 2
> >>> + description: |
> >>> + Output pins used as trigger source.
> >>> +
> >>> + Cell 0 defines the event:
> >>> + * 0 = Data ready
> >>> + * 1 = Min threshold
> >>> + * 2 = Max threshold
> >>> + * 3 = Either threshold
> >>> + * 4 = CHOP control
> >>> + * 5 = Device enable
> >>> + * 6 = Device ready (only GP1)
> >>
> >> Hmm. I'm a bit dubious on why 'what the offload trigger is'
> >> is a DT thing? Is that because the IP needs to comprehend
> >> this? I guess only data ready is actually supported in
> >> practice?
> >
> > A trigger can be connected to trigger something other than a spi
> > offload, it is in the DT because it describes how the device is
> > connected. When using spi offload, the trigger-source at the spi handle
> > describes which gpio and event is routed to the offload trigger input.
> > At the ADC node, trigger-source-cells describe the source gpio and event
> > for the device driver.
> >
> > In practice, in this series, one gpio is Data ready, triggering offload
> > when buffer enabled, and raw reads, when disabled. And the other is
> > Either threshold, propagated as an IIO event. Fancy logic can be added
> > to the driver in future patches to allow other combinations.
> >
> > It is also worth to mention that the trigger-source is duplicated for
> > each node that uses it, as seen in the second dts example:
> >
> > &adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
> >
> > Is repeated on both adc and spi node.
>
> That sounds wrong. This would only make sense if an output of the
> ADC was wired back to itself.
>
The issue is the lack of way of describing to the driver the function of
each gpio, when configurable. Perhaps it is better to use
trigger-source-cells to only describe the topology at that node
receiving the trigger, e.g.
trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
Below I continue the discussion.
> >
> > One last thing, on the driver, for v3, I should handle -ENOENT:
> >
> > ret = of_parse_phandle_with_args(np, "trigger-sources",
> > "#trigger-source-cells", i,
> > &trigger_sources);
> > if (ret)
> > return ret == -ENOENT ? 0 : ret;
> >
> > To assert only when present, since the nodes are not required.
> > Or, in the driver,
> > require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
> > require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
> > (I would go with the first option).
> >>
>
> ,,,
>
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/gpio/gpio.h>
> >>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> >>> +
> >>> + spi {
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> +
> >>> + adc@0 {
> >>> + compatible = "adi,ad4052";
> >>> + reg = <0>;
> >>> + vdd-supply = <&vdd>;
> >>> + vio-supply = <&vio>;
> >>> + ref-supply = <&ref>;
> >>> + spi-max-frequency = <83333333>;
> >>> +
> >>> + #trigger-source-cells = <2>;
> >>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> >>> + AD4052_TRIGGER_PIN_GP0
> >>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> >>> + AD4052_TRIGGER_PIN_GP1>;
>
> This doesn't make sense for the reason given above. These outputs
> aren't wired back to inputs on the ADC. They are wired to interrupts
> on the MCU, which is already described below.
>
Below.
> >>> + interrupt-parent = <&gpio>;
> >>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> >>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> >>> + interrupt-names = "gp0", "gp1";
> >>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> >>> + };
> >>> + };
> >>> + - |
> >>> + #include <dt-bindings/gpio/gpio.h>
> >>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> >>> +
> >>> + rx_dma {
> >>> + #dma-cells = <1>;
> >>> + };
> >>> +
> >>> + spi {
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> +
> >>> + dmas = <&rx_dma 0>;
> >>> + dma-names = "offload0-rx";
>
> The dmas aren't related to the ADC, so can be left out of the example.
>
Ack.
> >>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
> >>> + AD4052_TRIGGER_PIN_GP1>;
> >>> +
> >>> + adc@0 {
> >>> + compatible = "adi,ad4052";
> >>> + reg = <0>;
> >>> + vdd-supply = <&vdd>;
> >>> + vio-supply = <&vio>;
> >>> + spi-max-frequency = <83333333>;
> >>> + pwms = <&adc_trigger 0 10000 0>;
> >>> +
> >>> + #trigger-source-cells = <2>;
> >>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> >>> + AD4052_TRIGGER_PIN_GP0
> >>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> >>> + AD4052_TRIGGER_PIN_GP1>;
>
> Same as above - the GP pins aren't wired back to the ADC itself.
>
> >>> + interrupt-parent = <&gpio>;
> >>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> >>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> >>> + interrupt-names = "gp0", "gp1";
> >>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> >>> + };
> >>> + };
Considering the discussion above. As is, in this series GP0 is event
Either threshold and GP1 Data ready. A future series would aim to make
it truly configurable.
For this series then, do we then drop the second cell of trigger cell
and do not provide a way of describing the function of each gpio? e.g.
- |
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/iio/adc/adi,ad4052.h>
rx_dma {
#dma-cells = <1>;
};
spi {
#address-cells = <1>;
#size-cells = <0>;
trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
adc@0 {
compatible = "adi,ad4052";
reg = <0>;
vdd-supply = <&vdd>;
vio-supply = <&vio>;
spi-max-frequency = <83333333>;
pwms = <&adc_trigger 0 10000 0>;
// --- Other thought ------
//adi,gpio-role = <AD4052_TRIGGER_EVENT_EITHER_THRESH
// AD4052_TRIGGER_EVENT_DATA_READY>;
// ------------------------
interrupt-parent = <&gpio>;
interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
<0 1 IRQ_TYPE_EDGE_FALLING>;
interrupt-names = "gp0", "gp1";
cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
};
};
Other thought is to add an "adi,gpio-role" property to define gpio
function (as commented in the example above, matched with index of
interrupts-names). If no interrupt-name.gp0 but trigger-source.GP0,
assume role Data ready (no irq for raw read, only buffer offload).
What is your opinion on this?
Best regards,
Jorge
On 6/12/25 2:42 PM, Jorge Marques wrote:
> Hi David,
>
> thank you for chiming in
>
> On Thu, Jun 12, 2025 at 10:03:37AM -0500, David Lechner wrote:
>> On 6/12/25 5:11 AM, Jorge Marques wrote:
>>> On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
>>>> On Tue, 10 Jun 2025 09:34:35 +0200
>>>> Jorge Marques <jorge.marques@analog.com> wrote:
>>>>
>>
>> ...
>>
>>>>> + trigger-sources:
>>>>> + minItems: 1
>>>>> + maxItems: 2
>>>>> + description:
>>>>> + Describes the output pin and event associated.
>>
>> trigger-sources would be an input pin connected to an external trigger.
>> For example, the CNV pin could be connected to a trigger-source
>> provider to trigger a conversion. But there aren't any other digital
>> inputs, so I don't know what the 2nd source would be here.
>>
>> As an example, see [1]. We could potentially use the same gpio
>> trigger-source for the conversion pin here. There is already
>> a similar binding for pwm triggers, so we could drop the separate
>> pwms binding as well an just have a single trigger-sources
>> property for the CNV pin that works for both gpio and pwm.
>>
>> [1]: https://lore.kernel.org/linux-iio/cover.1749569957.git.Jonathan.Santos@analog.com/
>>
>
> Quick summary to familiarize myself with this part and driver.
>
> On ad7768-1:
> ad7768-1.SYNC_OUT is a digital output, ad7768-1.SYNC_IN input, and
> ad7768-1.GPIO3 (START) configured as input. ad7768-1.GPIO[0..3] are
> configurable GPIO, GPIO3 as START, or in PIN control mode, the input
> GPIO[3:0] sets the power mode and modulator freq (MODEx).
>
> On that thread:
> https://lore.kernel.org/linux-iio/8abca580f43cb31d7088d07a7414b5f7efe91ead.1749569957.git.Jonathan.Santos@analog.com/
> exposes GPIO[0..3] through gpio_chip if gpio-controller in dt.
>
> https://lore.kernel.org/linux-iio/713fd786010c75858700efaec8bb285274e7057e.1749569957.git.Jonathan.Santos@analog.com/
> trigger-sources-cells: the cell define the type of signal but *not* its
> origin, because {DRDY, SYNC_OUT, GPIO3(START)} are dedicated pins, *so
> there is no need to do so*.
>
>>>>> +
>>>>> + "#trigger-source-cells":
>>>>> + const: 2
>>>>> + description: |
>>>>> + Output pins used as trigger source.
>>>>> +
>>>>> + Cell 0 defines the event:
>>>>> + * 0 = Data ready
>>>>> + * 1 = Min threshold
>>>>> + * 2 = Max threshold
>>>>> + * 3 = Either threshold
>>>>> + * 4 = CHOP control
>>>>> + * 5 = Device enable
>>>>> + * 6 = Device ready (only GP1)
>>>>
>>>> Hmm. I'm a bit dubious on why 'what the offload trigger is'
>>>> is a DT thing? Is that because the IP needs to comprehend
>>>> this? I guess only data ready is actually supported in
>>>> practice?
>>>
>>> A trigger can be connected to trigger something other than a spi
>>> offload, it is in the DT because it describes how the device is
>>> connected. When using spi offload, the trigger-source at the spi handle
>>> describes which gpio and event is routed to the offload trigger input.
>>> At the ADC node, trigger-source-cells describe the source gpio and event
>>> for the device driver.
>>>
>>> In practice, in this series, one gpio is Data ready, triggering offload
>>> when buffer enabled, and raw reads, when disabled. And the other is
>>> Either threshold, propagated as an IIO event. Fancy logic can be added
>>> to the driver in future patches to allow other combinations.
>>>
>>> It is also worth to mention that the trigger-source is duplicated for
>>> each node that uses it, as seen in the second dts example:
>>>
>>> &adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
>>>
>>> Is repeated on both adc and spi node.
>>
>> That sounds wrong. This would only make sense if an output of the
>> ADC was wired back to itself.
>>
>
> The issue is the lack of way of describing to the driver the function of
> each gpio, when configurable. Perhaps it is better to use
> trigger-source-cells to only describe the topology at that node
> receiving the trigger, e.g.
>
> trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
>
> Below I continue the discussion.
>>>
>>> One last thing, on the driver, for v3, I should handle -ENOENT:
>>>
>>> ret = of_parse_phandle_with_args(np, "trigger-sources",
>>> "#trigger-source-cells", i,
>>> &trigger_sources);
>>> if (ret)
>>> return ret == -ENOENT ? 0 : ret;
>>>
>>> To assert only when present, since the nodes are not required.
>>> Or, in the driver,
>>> require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
>>> require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
>>> (I would go with the first option).
>>>>
>>
>> ,,,
>>
>>>>> +examples:
>>>>> + - |
>>>>> + #include <dt-bindings/gpio/gpio.h>
>>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
>>>>> +
>>>>> + spi {
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> +
>>>>> + adc@0 {
>>>>> + compatible = "adi,ad4052";
>>>>> + reg = <0>;
>>>>> + vdd-supply = <&vdd>;
>>>>> + vio-supply = <&vio>;
>>>>> + ref-supply = <&ref>;
>>>>> + spi-max-frequency = <83333333>;
>>>>> +
>>>>> + #trigger-source-cells = <2>;
>>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
>>>>> + AD4052_TRIGGER_PIN_GP0
>>>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
>>>>> + AD4052_TRIGGER_PIN_GP1>;
>>
>> This doesn't make sense for the reason given above. These outputs
>> aren't wired back to inputs on the ADC. They are wired to interrupts
>> on the MCU, which is already described below.
>>
> Below.
>>>>> + interrupt-parent = <&gpio>;
>>>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
>>>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
>>>>> + interrupt-names = "gp0", "gp1";
>>>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
>>>>> + };
>>>>> + };
>>>>> + - |
>>>>> + #include <dt-bindings/gpio/gpio.h>
>>>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
>>>>> +
>>>>> + rx_dma {
>>>>> + #dma-cells = <1>;
>>>>> + };
>>>>> +
>>>>> + spi {
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> +
>>>>> + dmas = <&rx_dma 0>;
>>>>> + dma-names = "offload0-rx";
>>
>> The dmas aren't related to the ADC, so can be left out of the example.
>>
> Ack.
>>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
>>>>> + AD4052_TRIGGER_PIN_GP1>;
>>>>> +
>>>>> + adc@0 {
>>>>> + compatible = "adi,ad4052";
>>>>> + reg = <0>;
>>>>> + vdd-supply = <&vdd>;
>>>>> + vio-supply = <&vio>;
>>>>> + spi-max-frequency = <83333333>;
>>>>> + pwms = <&adc_trigger 0 10000 0>;
>>>>> +
>>>>> + #trigger-source-cells = <2>;
>>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
>>>>> + AD4052_TRIGGER_PIN_GP0
>>>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
>>>>> + AD4052_TRIGGER_PIN_GP1>;
>>
>> Same as above - the GP pins aren't wired back to the ADC itself.
>>
>>>>> + interrupt-parent = <&gpio>;
>>>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
>>>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
>>>>> + interrupt-names = "gp0", "gp1";
>>>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
>>>>> + };
>>>>> + };
>
> Considering the discussion above. As is, in this series GP0 is event
> Either threshold and GP1 Data ready. A future series would aim to make
> it truly configurable.
>
> For this series then, do we then drop the second cell of trigger cell
> and do not provide a way of describing the function of each gpio? e.g.
The bindings can't be changed later, so no, don't drop the 2nd cell
if we are going to add it back later.
But considering Jonathan's feedback, I am now questioning if we need
the 2nd cell at all. The way trigger-source consumers work currently
is that they request a trigger of a certain generic type, like "data
ready". So this information could be used to determine what function
needs to be assigned to the pin without having to define that in the
devicetree.
>
> - |
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/interrupt-controller/irq.h>
> #include <dt-bindings/iio/adc/adi,ad4052.h>
>
> rx_dma {
> #dma-cells = <1>;
> };
>
> spi {
> #address-cells = <1>;
> #size-cells = <0>;
>
> trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
>
> adc@0 {
> compatible = "adi,ad4052";
> reg = <0>;
> vdd-supply = <&vdd>;
> vio-supply = <&vio>;
> spi-max-frequency = <83333333>;
> pwms = <&adc_trigger 0 10000 0>;
>
> // --- Other thought ------
> //adi,gpio-role = <AD4052_TRIGGER_EVENT_EITHER_THRESH
> // AD4052_TRIGGER_EVENT_DATA_READY>;
> // ------------------------
> interrupt-parent = <&gpio>;
> interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> <0 1 IRQ_TYPE_EDGE_FALLING>;
> interrupt-names = "gp0", "gp1";
> cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> };
> };
>
> Other thought is to add an "adi,gpio-role" property to define gpio
> function (as commented in the example above, matched with index of
> interrupts-names). If no interrupt-name.gp0 but trigger-source.GP0,
> assume role Data ready (no irq for raw read, only buffer offload).
>
> What is your opinion on this?
Usually, we just have the devicetree describe how things are wired up.
Then the driver looks at how things are wired up and decides how to
best make use of the available resources. I.e. in the driver add some
variables in the driver state struct that keeps track of the function
assigned to each GP pin and use that to make decisions.
In the driver, we would want to make sure to handle triggers first
since those are less flexible (so set up SPI offload first). This
would cause one of the GP pins to be assigned to the /RDY function.
It doesn't matter which one.
Then later, parse the interrupts property. If we see that one of
the GP pins is already assigned to /RDY, then we know we have to
use that pin for the /RDY interrupt as well. If both pins are still
available, then an arbitrary one can be assigned for /RDY.
Then if there is still an unused GP pin left that is actually
wired up to an interrupt, that can be used for the events interrupt.
Or we could even consider to have everything on one pin since the
/RDY signal would never be needed at the same time as events as long
as the events are only ever used in monitor mode.
If we find that there is some case though where the driver really
can't figure out what to do with the available information, then
we could probably justify adding a property like you suggested.
It seems like we could possibly do without it at this point though.
Hi David,
On Thu, Jun 12, 2025 at 03:20:40PM -0500, David Lechner wrote:
> On 6/12/25 2:42 PM, Jorge Marques wrote:
> > Hi David,
> >
> > thank you for chiming in
> >
> > On Thu, Jun 12, 2025 at 10:03:37AM -0500, David Lechner wrote:
> >> On 6/12/25 5:11 AM, Jorge Marques wrote:
> >>> On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
> >>>> On Tue, 10 Jun 2025 09:34:35 +0200
> >>>> Jorge Marques <jorge.marques@analog.com> wrote:
> >>>>
> >>
> >> ...
> >>
> >>>>> + trigger-sources:
> >>>>> + minItems: 1
> >>>>> + maxItems: 2
> >>>>> + description:
> >>>>> + Describes the output pin and event associated.
> >>
> >> trigger-sources would be an input pin connected to an external trigger.
> >> For example, the CNV pin could be connected to a trigger-source
> >> provider to trigger a conversion. But there aren't any other digital
> >> inputs, so I don't know what the 2nd source would be here.
> >>
> >> As an example, see [1]. We could potentially use the same gpio
> >> trigger-source for the conversion pin here. There is already
> >> a similar binding for pwm triggers, so we could drop the separate
> >> pwms binding as well an just have a single trigger-sources
> >> property for the CNV pin that works for both gpio and pwm.
> >>
> >> [1]: https://lore.kernel.org/linux-iio/cover.1749569957.git.Jonathan.Santos@analog.com/
> >>
> >
> > Quick summary to familiarize myself with this part and driver.
> >
> > On ad7768-1:
> > ad7768-1.SYNC_OUT is a digital output, ad7768-1.SYNC_IN input, and
> > ad7768-1.GPIO3 (START) configured as input. ad7768-1.GPIO[0..3] are
> > configurable GPIO, GPIO3 as START, or in PIN control mode, the input
> > GPIO[3:0] sets the power mode and modulator freq (MODEx).
> >
> > On that thread:
> > https://lore.kernel.org/linux-iio/8abca580f43cb31d7088d07a7414b5f7efe91ead.1749569957.git.Jonathan.Santos@analog.com/
> > exposes GPIO[0..3] through gpio_chip if gpio-controller in dt.
> >
> > https://lore.kernel.org/linux-iio/713fd786010c75858700efaec8bb285274e7057e.1749569957.git.Jonathan.Santos@analog.com/
> > trigger-sources-cells: the cell define the type of signal but *not* its
> > origin, because {DRDY, SYNC_OUT, GPIO3(START)} are dedicated pins, *so
> > there is no need to do so*.
> >
> >>>>> +
> >>>>> + "#trigger-source-cells":
> >>>>> + const: 2
> >>>>> + description: |
> >>>>> + Output pins used as trigger source.
> >>>>> +
> >>>>> + Cell 0 defines the event:
> >>>>> + * 0 = Data ready
> >>>>> + * 1 = Min threshold
> >>>>> + * 2 = Max threshold
> >>>>> + * 3 = Either threshold
> >>>>> + * 4 = CHOP control
> >>>>> + * 5 = Device enable
> >>>>> + * 6 = Device ready (only GP1)
> >>>>
> >>>> Hmm. I'm a bit dubious on why 'what the offload trigger is'
> >>>> is a DT thing? Is that because the IP needs to comprehend
> >>>> this? I guess only data ready is actually supported in
> >>>> practice?
> >>>
> >>> A trigger can be connected to trigger something other than a spi
> >>> offload, it is in the DT because it describes how the device is
> >>> connected. When using spi offload, the trigger-source at the spi handle
> >>> describes which gpio and event is routed to the offload trigger input.
> >>> At the ADC node, trigger-source-cells describe the source gpio and event
> >>> for the device driver.
> >>>
> >>> In practice, in this series, one gpio is Data ready, triggering offload
> >>> when buffer enabled, and raw reads, when disabled. And the other is
> >>> Either threshold, propagated as an IIO event. Fancy logic can be added
> >>> to the driver in future patches to allow other combinations.
> >>>
> >>> It is also worth to mention that the trigger-source is duplicated for
> >>> each node that uses it, as seen in the second dts example:
> >>>
> >>> &adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
> >>>
> >>> Is repeated on both adc and spi node.
> >>
> >> That sounds wrong. This would only make sense if an output of the
> >> ADC was wired back to itself.
> >>
> >
> > The issue is the lack of way of describing to the driver the function of
> > each gpio, when configurable. Perhaps it is better to use
> > trigger-source-cells to only describe the topology at that node
> > receiving the trigger, e.g.
> >
> > trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
> >
> > Below I continue the discussion.
> >>>
> >>> One last thing, on the driver, for v3, I should handle -ENOENT:
> >>>
> >>> ret = of_parse_phandle_with_args(np, "trigger-sources",
> >>> "#trigger-source-cells", i,
> >>> &trigger_sources);
> >>> if (ret)
> >>> return ret == -ENOENT ? 0 : ret;
> >>>
> >>> To assert only when present, since the nodes are not required.
> >>> Or, in the driver,
> >>> require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
> >>> require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
> >>> (I would go with the first option).
> >>>>
> >>
> >> ,,,
> >>
> >>>>> +examples:
> >>>>> + - |
> >>>>> + #include <dt-bindings/gpio/gpio.h>
> >>>>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> >>>>> +
> >>>>> + spi {
> >>>>> + #address-cells = <1>;
> >>>>> + #size-cells = <0>;
> >>>>> +
> >>>>> + adc@0 {
> >>>>> + compatible = "adi,ad4052";
> >>>>> + reg = <0>;
> >>>>> + vdd-supply = <&vdd>;
> >>>>> + vio-supply = <&vio>;
> >>>>> + ref-supply = <&ref>;
> >>>>> + spi-max-frequency = <83333333>;
> >>>>> +
> >>>>> + #trigger-source-cells = <2>;
> >>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> >>>>> + AD4052_TRIGGER_PIN_GP0
> >>>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> >>>>> + AD4052_TRIGGER_PIN_GP1>;
> >>
> >> This doesn't make sense for the reason given above. These outputs
> >> aren't wired back to inputs on the ADC. They are wired to interrupts
> >> on the MCU, which is already described below.
> >>
> > Below.
> >>>>> + interrupt-parent = <&gpio>;
> >>>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> >>>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> >>>>> + interrupt-names = "gp0", "gp1";
> >>>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> >>>>> + };
> >>>>> + };
> >>>>> + - |
> >>>>> + #include <dt-bindings/gpio/gpio.h>
> >>>>> + #include <dt-bindings/interrupt-controller/irq.h>
> >>>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> >>>>> +
> >>>>> + rx_dma {
> >>>>> + #dma-cells = <1>;
> >>>>> + };
> >>>>> +
> >>>>> + spi {
> >>>>> + #address-cells = <1>;
> >>>>> + #size-cells = <0>;
> >>>>> +
> >>>>> + dmas = <&rx_dma 0>;
> >>>>> + dma-names = "offload0-rx";
> >>
> >> The dmas aren't related to the ADC, so can be left out of the example.
> >>
> > Ack.
> >>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
> >>>>> + AD4052_TRIGGER_PIN_GP1>;
> >>>>> +
> >>>>> + adc@0 {
> >>>>> + compatible = "adi,ad4052";
> >>>>> + reg = <0>;
> >>>>> + vdd-supply = <&vdd>;
> >>>>> + vio-supply = <&vio>;
> >>>>> + spi-max-frequency = <83333333>;
> >>>>> + pwms = <&adc_trigger 0 10000 0>;
> >>>>> +
> >>>>> + #trigger-source-cells = <2>;
> >>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> >>>>> + AD4052_TRIGGER_PIN_GP0
> >>>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> >>>>> + AD4052_TRIGGER_PIN_GP1>;
> >>
> >> Same as above - the GP pins aren't wired back to the ADC itself.
> >>
> >>>>> + interrupt-parent = <&gpio>;
> >>>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> >>>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> >>>>> + interrupt-names = "gp0", "gp1";
> >>>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> >>>>> + };
> >>>>> + };
> >
> > Considering the discussion above. As is, in this series GP0 is event
> > Either threshold and GP1 Data ready. A future series would aim to make
> > it truly configurable.
> >
> > For this series then, do we then drop the second cell of trigger cell
> > and do not provide a way of describing the function of each gpio? e.g.
>
> The bindings can't be changed later, so no, don't drop the 2nd cell
> if we are going to add it back later.
>
> But considering Jonathan's feedback, I am now questioning if we need
> the 2nd cell at all. The way trigger-source consumers work currently
> is that they request a trigger of a certain generic type, like "data
> ready". So this information could be used to determine what function
> needs to be assigned to the pin without having to define that in the
> devicetree.
>
Useful for assertion. It is odd to be used for requesting of a certain
type (gpio role) instead of telling how things are wired.
> >
> > - |
> > #include <dt-bindings/gpio/gpio.h>
> > #include <dt-bindings/interrupt-controller/irq.h>
> > #include <dt-bindings/iio/adc/adi,ad4052.h>
> >
> > rx_dma {
> > #dma-cells = <1>;
> > };
> >
> > spi {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
> >
> > adc@0 {
> > compatible = "adi,ad4052";
> > reg = <0>;
> > vdd-supply = <&vdd>;
> > vio-supply = <&vio>;
> > spi-max-frequency = <83333333>;
> > pwms = <&adc_trigger 0 10000 0>;
> >
> > // --- Other thought ------
> > //adi,gpio-role = <AD4052_TRIGGER_EVENT_EITHER_THRESH
> > // AD4052_TRIGGER_EVENT_DATA_READY>;
> > // ------------------------
> > interrupt-parent = <&gpio>;
> > interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > <0 1 IRQ_TYPE_EDGE_FALLING>;
> > interrupt-names = "gp0", "gp1";
> > cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > };
> > };
> >
> > Other thought is to add an "adi,gpio-role" property to define gpio
> > function (as commented in the example above, matched with index of
> > interrupts-names). If no interrupt-name.gp0 but trigger-source.GP0,
> > assume role Data ready (no irq for raw read, only buffer offload).
> >
> > What is your opinion on this?
>
>
> Usually, we just have the devicetree describe how things are wired up.
> Then the driver looks at how things are wired up and decides how to
> best make use of the available resources. I.e. in the driver add some
> variables in the driver state struct that keeps track of the function
> assigned to each GP pin and use that to make decisions.
>
> In the driver, we would want to make sure to handle triggers first
> since those are less flexible (so set up SPI offload first). This
> would cause one of the GP pins to be assigned to the /RDY function.
> It doesn't matter which one.
>
I will default drdy_gp to g0, until offload request overwrites it,
either gp0 or gp1.
> Then later, parse the interrupts property. If we see that one of
> the GP pins is already assigned to /RDY, then we know we have to
> use that pin for the /RDY interrupt as well. If both pins are still
> available, then an arbitrary one can be assigned for /RDY.
based on drdy_gp, set that gp as drdy, and the remaining is threshold
either. the interrupt is optional, but setup device gp regardless, since
the irq may be consumed by other device.
>
> Then if there is still an unused GP pin left that is actually
> wired up to an interrupt, that can be used for the events interrupt.
>
> Or we could even consider to have everything on one pin since the
> /RDY signal would never be needed at the same time as events as long
> as the events are only ever used in monitor mode.
>
The threshold event occurs on the rising edge, the data ready on the
falling edge (it is actually BUSY). Mixing both has a lot of nuances
involved.
> If we find that there is some case though where the driver really
> can't figure out what to do with the available information, then
> we could probably justify adding a property like you suggested.
> It seems like we could possibly do without it at this point though.
With the proposed above, I don't need the cell 0 of trigger-sources. But
I will keep for assertion since we are inferring
has?trigger-sources-> -then-> drdy.
Best regards,
Jorge
On Fri, 13 Jun 2025 13:17:46 +0200
Jorge Marques <gastmaier@gmail.com> wrote:
> Hi David,
> On Thu, Jun 12, 2025 at 03:20:40PM -0500, David Lechner wrote:
> > On 6/12/25 2:42 PM, Jorge Marques wrote:
> > > Hi David,
> > >
> > > thank you for chiming in
> > >
> > > On Thu, Jun 12, 2025 at 10:03:37AM -0500, David Lechner wrote:
> > >> On 6/12/25 5:11 AM, Jorge Marques wrote:
> > >>> On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
> > >>>> On Tue, 10 Jun 2025 09:34:35 +0200
> > >>>> Jorge Marques <jorge.marques@analog.com> wrote:
> > >>>>
> > >>
> > >> ...
> > >>
> > >>>>> + trigger-sources:
> > >>>>> + minItems: 1
> > >>>>> + maxItems: 2
> > >>>>> + description:
> > >>>>> + Describes the output pin and event associated.
> > >>
> > >> trigger-sources would be an input pin connected to an external trigger.
> > >> For example, the CNV pin could be connected to a trigger-source
> > >> provider to trigger a conversion. But there aren't any other digital
> > >> inputs, so I don't know what the 2nd source would be here.
> > >>
> > >> As an example, see [1]. We could potentially use the same gpio
> > >> trigger-source for the conversion pin here. There is already
> > >> a similar binding for pwm triggers, so we could drop the separate
> > >> pwms binding as well an just have a single trigger-sources
> > >> property for the CNV pin that works for both gpio and pwm.
> > >>
> > >> [1]: https://lore.kernel.org/linux-iio/cover.1749569957.git.Jonathan.Santos@analog.com/
> > >>
> > >
> > > Quick summary to familiarize myself with this part and driver.
> > >
> > > On ad7768-1:
> > > ad7768-1.SYNC_OUT is a digital output, ad7768-1.SYNC_IN input, and
> > > ad7768-1.GPIO3 (START) configured as input. ad7768-1.GPIO[0..3] are
> > > configurable GPIO, GPIO3 as START, or in PIN control mode, the input
> > > GPIO[3:0] sets the power mode and modulator freq (MODEx).
> > >
> > > On that thread:
> > > https://lore.kernel.org/linux-iio/8abca580f43cb31d7088d07a7414b5f7efe91ead.1749569957.git.Jonathan.Santos@analog.com/
> > > exposes GPIO[0..3] through gpio_chip if gpio-controller in dt.
> > >
> > > https://lore.kernel.org/linux-iio/713fd786010c75858700efaec8bb285274e7057e.1749569957.git.Jonathan.Santos@analog.com/
> > > trigger-sources-cells: the cell define the type of signal but *not* its
> > > origin, because {DRDY, SYNC_OUT, GPIO3(START)} are dedicated pins, *so
> > > there is no need to do so*.
> > >
> > >>>>> +
> > >>>>> + "#trigger-source-cells":
> > >>>>> + const: 2
> > >>>>> + description: |
> > >>>>> + Output pins used as trigger source.
> > >>>>> +
> > >>>>> + Cell 0 defines the event:
> > >>>>> + * 0 = Data ready
> > >>>>> + * 1 = Min threshold
> > >>>>> + * 2 = Max threshold
> > >>>>> + * 3 = Either threshold
> > >>>>> + * 4 = CHOP control
> > >>>>> + * 5 = Device enable
> > >>>>> + * 6 = Device ready (only GP1)
> > >>>>
> > >>>> Hmm. I'm a bit dubious on why 'what the offload trigger is'
> > >>>> is a DT thing? Is that because the IP needs to comprehend
> > >>>> this? I guess only data ready is actually supported in
> > >>>> practice?
> > >>>
> > >>> A trigger can be connected to trigger something other than a spi
> > >>> offload, it is in the DT because it describes how the device is
> > >>> connected. When using spi offload, the trigger-source at the spi handle
> > >>> describes which gpio and event is routed to the offload trigger input.
> > >>> At the ADC node, trigger-source-cells describe the source gpio and event
> > >>> for the device driver.
> > >>>
> > >>> In practice, in this series, one gpio is Data ready, triggering offload
> > >>> when buffer enabled, and raw reads, when disabled. And the other is
> > >>> Either threshold, propagated as an IIO event. Fancy logic can be added
> > >>> to the driver in future patches to allow other combinations.
> > >>>
> > >>> It is also worth to mention that the trigger-source is duplicated for
> > >>> each node that uses it, as seen in the second dts example:
> > >>>
> > >>> &adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
> > >>>
> > >>> Is repeated on both adc and spi node.
> > >>
> > >> That sounds wrong. This would only make sense if an output of the
> > >> ADC was wired back to itself.
> > >>
> > >
> > > The issue is the lack of way of describing to the driver the function of
> > > each gpio, when configurable. Perhaps it is better to use
> > > trigger-source-cells to only describe the topology at that node
> > > receiving the trigger, e.g.
> > >
> > > trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
> > >
> > > Below I continue the discussion.
> > >>>
> > >>> One last thing, on the driver, for v3, I should handle -ENOENT:
> > >>>
> > >>> ret = of_parse_phandle_with_args(np, "trigger-sources",
> > >>> "#trigger-source-cells", i,
> > >>> &trigger_sources);
> > >>> if (ret)
> > >>> return ret == -ENOENT ? 0 : ret;
> > >>>
> > >>> To assert only when present, since the nodes are not required.
> > >>> Or, in the driver,
> > >>> require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
> > >>> require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
> > >>> (I would go with the first option).
> > >>>>
> > >>
> > >> ,,,
> > >>
> > >>>>> +examples:
> > >>>>> + - |
> > >>>>> + #include <dt-bindings/gpio/gpio.h>
> > >>>>> + #include <dt-bindings/interrupt-controller/irq.h>
> > >>>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> > >>>>> +
> > >>>>> + spi {
> > >>>>> + #address-cells = <1>;
> > >>>>> + #size-cells = <0>;
> > >>>>> +
> > >>>>> + adc@0 {
> > >>>>> + compatible = "adi,ad4052";
> > >>>>> + reg = <0>;
> > >>>>> + vdd-supply = <&vdd>;
> > >>>>> + vio-supply = <&vio>;
> > >>>>> + ref-supply = <&ref>;
> > >>>>> + spi-max-frequency = <83333333>;
> > >>>>> +
> > >>>>> + #trigger-source-cells = <2>;
> > >>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> > >>>>> + AD4052_TRIGGER_PIN_GP0
> > >>>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> > >>>>> + AD4052_TRIGGER_PIN_GP1>;
> > >>
> > >> This doesn't make sense for the reason given above. These outputs
> > >> aren't wired back to inputs on the ADC. They are wired to interrupts
> > >> on the MCU, which is already described below.
> > >>
> > > Below.
> > >>>>> + interrupt-parent = <&gpio>;
> > >>>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > >>>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > >>>>> + interrupt-names = "gp0", "gp1";
> > >>>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > >>>>> + };
> > >>>>> + };
> > >>>>> + - |
> > >>>>> + #include <dt-bindings/gpio/gpio.h>
> > >>>>> + #include <dt-bindings/interrupt-controller/irq.h>
> > >>>>> + #include <dt-bindings/iio/adc/adi,ad4052.h>
> > >>>>> +
> > >>>>> + rx_dma {
> > >>>>> + #dma-cells = <1>;
> > >>>>> + };
> > >>>>> +
> > >>>>> + spi {
> > >>>>> + #address-cells = <1>;
> > >>>>> + #size-cells = <0>;
> > >>>>> +
> > >>>>> + dmas = <&rx_dma 0>;
> > >>>>> + dma-names = "offload0-rx";
> > >>
> > >> The dmas aren't related to the ADC, so can be left out of the example.
> > >>
> > > Ack.
> > >>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
> > >>>>> + AD4052_TRIGGER_PIN_GP1>;
> > >>>>> +
> > >>>>> + adc@0 {
> > >>>>> + compatible = "adi,ad4052";
> > >>>>> + reg = <0>;
> > >>>>> + vdd-supply = <&vdd>;
> > >>>>> + vio-supply = <&vio>;
> > >>>>> + spi-max-frequency = <83333333>;
> > >>>>> + pwms = <&adc_trigger 0 10000 0>;
> > >>>>> +
> > >>>>> + #trigger-source-cells = <2>;
> > >>>>> + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> > >>>>> + AD4052_TRIGGER_PIN_GP0
> > >>>>> + &adc AD4052_TRIGGER_EVENT_DATA_READY
> > >>>>> + AD4052_TRIGGER_PIN_GP1>;
> > >>
> > >> Same as above - the GP pins aren't wired back to the ADC itself.
> > >>
> > >>>>> + interrupt-parent = <&gpio>;
> > >>>>> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > >>>>> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > >>>>> + interrupt-names = "gp0", "gp1";
> > >>>>> + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > >>>>> + };
> > >>>>> + };
> > >
> > > Considering the discussion above. As is, in this series GP0 is event
> > > Either threshold and GP1 Data ready. A future series would aim to make
> > > it truly configurable.
> > >
> > > For this series then, do we then drop the second cell of trigger cell
> > > and do not provide a way of describing the function of each gpio? e.g.
> >
> > The bindings can't be changed later, so no, don't drop the 2nd cell
> > if we are going to add it back later.
> >
> > But considering Jonathan's feedback, I am now questioning if we need
> > the 2nd cell at all. The way trigger-source consumers work currently
> > is that they request a trigger of a certain generic type, like "data
> > ready". So this information could be used to determine what function
> > needs to be assigned to the pin without having to define that in the
> > devicetree.
> >
> Useful for assertion. It is odd to be used for requesting of a certain
> type (gpio role) instead of telling how things are wired.
> > >
> > > - |
> > > #include <dt-bindings/gpio/gpio.h>
> > > #include <dt-bindings/interrupt-controller/irq.h>
> > > #include <dt-bindings/iio/adc/adi,ad4052.h>
> > >
> > > rx_dma {
> > > #dma-cells = <1>;
> > > };
> > >
> > > spi {
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
> > >
> > > adc@0 {
> > > compatible = "adi,ad4052";
> > > reg = <0>;
> > > vdd-supply = <&vdd>;
> > > vio-supply = <&vio>;
> > > spi-max-frequency = <83333333>;
> > > pwms = <&adc_trigger 0 10000 0>;
> > >
> > > // --- Other thought ------
> > > //adi,gpio-role = <AD4052_TRIGGER_EVENT_EITHER_THRESH
> > > // AD4052_TRIGGER_EVENT_DATA_READY>;
> > > // ------------------------
> > > interrupt-parent = <&gpio>;
> > > interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > > <0 1 IRQ_TYPE_EDGE_FALLING>;
> > > interrupt-names = "gp0", "gp1";
> > > cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > > };
> > > };
> > >
> > > Other thought is to add an "adi,gpio-role" property to define gpio
> > > function (as commented in the example above, matched with index of
> > > interrupts-names). If no interrupt-name.gp0 but trigger-source.GP0,
> > > assume role Data ready (no irq for raw read, only buffer offload).
> > >
> > > What is your opinion on this?
> >
> >
> > Usually, we just have the devicetree describe how things are wired up.
> > Then the driver looks at how things are wired up and decides how to
> > best make use of the available resources. I.e. in the driver add some
> > variables in the driver state struct that keeps track of the function
> > assigned to each GP pin and use that to make decisions.
> >
> > In the driver, we would want to make sure to handle triggers first
> > since those are less flexible (so set up SPI offload first). This
> > would cause one of the GP pins to be assigned to the /RDY function.
> > It doesn't matter which one.
> >
> I will default drdy_gp to g0, until offload request overwrites it,
> either gp0 or gp1.
> > Then later, parse the interrupts property. If we see that one of
> > the GP pins is already assigned to /RDY, then we know we have to
> > use that pin for the /RDY interrupt as well. If both pins are still
> > available, then an arbitrary one can be assigned for /RDY.
> based on drdy_gp, set that gp as drdy, and the remaining is threshold
> either. the interrupt is optional, but setup device gp regardless, since
> the irq may be consumed by other device.
> >
> > Then if there is still an unused GP pin left that is actually
> > wired up to an interrupt, that can be used for the events interrupt.
> >
> > Or we could even consider to have everything on one pin since the
> > /RDY signal would never be needed at the same time as events as long
> > as the events are only ever used in monitor mode.
> >
>
> The threshold event occurs on the rising edge, the data ready on the
> falling edge (it is actually BUSY). Mixing both has a lot of nuances
> involved.
Ok. Mixing them might not make sense - but overall the decision on what
to do with any line that is just wired device to host interrupt is
a driver problem. If it's also wired to another device (including
offload engine) and that requires a specific setting (e.g. data ready)
then that is fair enough to have in DT.
I think that's roughly where this discussion ended up but just wanted
to confirm that.
Jonathan
> > If we find that there is some case though where the driver really
> > can't figure out what to do with the available information, then
> > we could probably justify adding a property like you suggested.
> > It seems like we could possibly do without it at this point though.
>
> With the proposed above, I don't need the cell 0 of trigger-sources. But
> I will keep for assertion since we are inferring
> has?trigger-sources-> -then-> drdy.
>
> Best regards,
> Jorge
© 2016 - 2025 Red Hat, Inc.