[PATCH v3 0/8] Add support for AD4052 device family

[PATCH v3 0/8] Add support for AD4052 device family

[Jorge Marques]

The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
successive approximation register (SAR) analog-to-digital converter (ADC).

The scan_type depends if the oversampling feature is enabled, since the
16-bit device increases the SPI word size from 16-bit to 24-bit. Also
due to this, the spi message optimization is balanced on the buffer ops,
instead of once per probe.

The device has autonomous monitoring capabilities, that are exposed as
IIO events. Since register access requires leaving the monitoring state
and returning, device access is blocked until the IIO event is disabled.

The device contains two required outputs:

* gp0: Threshold event interrupt on the rising edge.
* gp1: ADC conversion ready signal on the falling edge.
       The user should either invert the signal or set the IRQ as falling edge.

And one optional input:

* cnv: Triggers a conversion, can be replaced by shortening the CNV and
  SPI CS trace.

The devices utilizes PM to enter the low power mode.

The driver can be used with SPI controllers with and without offload support.

A FPGA design is available:
https://analogdevicesinc.github.io/hdl/projects/ad4052_ardz/

The devices datasheet:
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

The unique monitoring capabilities and multiple GPIOs where the decision factor
to have a standalone driver for this device family.

Non-implemented features:

* Status word: First byte of the SPI transfer aligned to the register
  address.
* Averaging mode: Similar to burst averaging mode used in the
  oversampling, but requiring a sequence of CNV triggers for each
  conversion.
* Trigger mode: Similar to monitor mode used in the monitoring mode, but
  exits to configuration mode on event.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
Changes in v3:
Documentation/ABI/testing/sysfs-bus-iio:
- oversampling_frequency and oversampling_frequency_available: target
  6.17 instead of 6.15

dt-bindings/adi,ad4052.yaml:
- add trigger-sources
- add sample dts with spi engine support

dt-bidings/ad4052.h (new to series):
- add header file with event and gpio values

ad4052.c:
- rework regmap and fields. always prefix field with reg name
  - create custom regmap bus to bound spi speed in configuration mode
- rework scantype:
  - drop offload scantype
  - use realbits to compute spi xfer length
  - set storagebits as 32-bits (fixed SPI Engine offload width)
      - tools use storagebits to compute the number of samples.
- reconsider spi speeds:
  - devicetree node: sets regmap spi speed, used for configuration
    access.
  - vio voltage: sets the adc access speed (higher than configuration
    mode).
- explicitily support only signed, removes conditionals checking if
  signed.
- add calibscale to configure scaling (MON_VAL), value 1 disable
  scaling.
- add get scale
- on sleep mode exit, sleep for 4ms (time required to power-on adc side
  and not trigger NOT_RDY_ERROR)

general:
- Break driver and doc commit into three: base driver, offload support,
  and events support, to be easier to review.
- Drop commit that sets get_current_scan_type as const.

- Link to v2: https://lore.kernel.org/r/20250422-iio-driver-ad4052-v2-0-638af47e9eb3@analog.com

Changes in v2:
dt-bindings:
- commit message: describe io, how each device differ, remove driver
  specifics.
- add interrupt names, format descriptions
- fix datasheet link
- add vdd/vio supply

documentation (new to series):
- add oversampling_frequency in sysfs-bus-iio

documentation/ad4052:
- rename sample_rate to conversion_frequency
- extend threshold event description

ad4052:
- use oversampling_frequency in burst_averaging_mode
- name the defines with register and label names, not only label
- remove defines that are used once, or may hard to understand, instead, have logic where they are used.
- due to the topology:
  - set spi_offload_trigger_config.type from PERIODIC to DATA_READY
  - handle the pwm_device on the driver.
- add oversampling_frequency and events_frequency to store distinct conversion_frequency
  and to write accordingly when entering monitor_mode or burst_averaging_mode
- set sampling frequency as the pwm_device frequency
- update production IDs values with the ones from the released parts
- use production IDs to obtain device grade.
- set chip info static
- remove ad4052_iio_device_claim_direct, and solve unbalances
- add missing rd_table, wr_table to regmap_config
- replace PTR_ERR_OR_ZERO with if IS_ERR return PTR_ERR
- rename ad4052_set_non_defaults with a ad4052_setup (more usual naming convention)
- reorder pm_runtime autosuspend to after enabling the pm

- Link to v1: https://lore.kernel.org/r/20250306-iio-driver-ad4052-v1-0-2badad30116c@analog.com

---
Jorge Marques (8):
      Documentation: ABI: add oversampling frequency in sysfs-bus-iio
      dt-bindings: iio: adc: Add adi,ad4052
      docs: iio: New docs for ad4052 driver
      iio: adc: Add support for ad4052
      docs: iio: ad4052: Add offload support documentation
      iio: adc: Add offload support for ad4052
      docs: iio: ad4052: Add event documentation
      iio: adc: Add events support to ad4052

 Documentation/ABI/testing/sysfs-bus-iio            |   17 +
 .../devicetree/bindings/iio/adc/adi,ad4052.yaml    |  167 ++
 Documentation/iio/ad4052.rst                       |  133 ++
 Documentation/iio/index.rst                        |    1 +
 MAINTAINERS                                        |    8 +
 drivers/iio/adc/Kconfig                            |   15 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/ad4052.c                           | 1692 ++++++++++++++++++++
 include/dt-bindings/iio/adc/adi,ad4052.h           |   17 +
 9 files changed, 2051 insertions(+)
---
base-commit: 8bd4d29e36cd44abe95e1b289994bcda47e011ee
change-id: 20250306-iio-driver-ad4052-a4acc3bb11b3

Best regards,
-- 
Jorge Marques <jorge.marques@analog.com>

[PATCH v3 1/8] Documentation: ABI: add oversampling frequency in sysfs-bus-iio

[Jorge Marques]

Some devices have an internal clock used to space out the conversion
trigger for the oversampling filter, Consider an ADC with conversion and
data ready pins topology:

  Sampling trigger |       |       |       |       |
  ADC conversion   ++++    ++++    ++++    ++++    ++++
  ADC data ready      *       *       *       *       *

With the oversampling frequency, conversions are spaced:

  Sampling trigger |       |       |       |       |
  ADC conversion   + + + + + + + + + + + + + + + + + + + +
  ADC data ready         *       *       *       *       *

In some devices and ranges, this internal clock can be used to evenly
space the conversions between the sampling edge. In other devices the
oversampling frequency is fixed or is computed based on the sampling
frequency parameter, and the parameter is read only.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index ef52c427a015cf47bb9847782e13afbee01e9f31..e60367255be89a9acc827ec1a749b729735f60e6 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -139,6 +139,23 @@ Contact:	linux-iio@vger.kernel.org
 Description:
 		Hardware dependent values supported by the oversampling filter.
 
+What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency
+KernelVersion:	6.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Some devices have internal clocks for oversampling.
+		Sets the resulting frequency in Hz to trigger a conversion used by
+		the oversampling filter.
+		If the device has a fixed internal clock or is computed based on
+		the sampling frequency parameter, the parameter is read only.
+
+What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency_available
+KernelVersion:	6.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Hardware dependent values supported by the oversampling
+		frequency.
+
 What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw

-- 
2.49.0

Re: [PATCH v3 1/8] Documentation: ABI: add oversampling frequency in sysfs-bus-iio

[Jonathan Cameron]

On Tue, 10 Jun 2025 09:34:34 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

Trivial: 
In the patch title use the actual file name of the new ABI.

add oversampling_frequency in sysfs-bus-iio


> Some devices have an internal clock used to space out the conversion
> trigger for the oversampling filter, Consider an ADC with conversion and
> data ready pins topology:
> 
>   Sampling trigger |       |       |       |       |
>   ADC conversion   ++++    ++++    ++++    ++++    ++++
>   ADC data ready      *       *       *       *       *
> 
> With the oversampling frequency, conversions are spaced:
> 
>   Sampling trigger |       |       |       |       |
>   ADC conversion   + + + + + + + + + + + + + + + + + + + +
>   ADC data ready         *       *       *       *       *
> 
> In some devices and ranges, this internal clock can be used to evenly
> space the conversions between the sampling edge. In other devices the
> oversampling frequency is fixed or is computed based on the sampling
> frequency parameter, and the parameter is read only.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index ef52c427a015cf47bb9847782e13afbee01e9f31..e60367255be89a9acc827ec1a749b729735f60e6 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -139,6 +139,23 @@ Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Hardware dependent values supported by the oversampling filter.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency
> +KernelVersion:	6.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Some devices have internal clocks for oversampling.

Wrapping on each sentence is unusual. David pointed this out in v2.
Wrap at 80 chars as a single paragraph.

> +		Sets the resulting frequency in Hz to trigger a conversion used by
> +		the oversampling filter.
> +		If the device has a fixed internal clock or is computed based on
> +		the sampling frequency parameter, the parameter is read only.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency_available
> +KernelVersion:	6.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Hardware dependent values supported by the oversampling
> +		frequency.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw
> 

Re: [PATCH v3 1/8] Documentation: ABI: add oversampling frequency in sysfs-bus-iio

[Jorge Marques]

On Wed, Jun 11, 2025 at 06:02:52PM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:34 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
> Trivial: 
> In the patch title use the actual file name of the new ABI.
> 
> add oversampling_frequency in sysfs-bus-iio
> 

Ack.
> 
> > Some devices have an internal clock used to space out the conversion
> > trigger for the oversampling filter, Consider an ADC with conversion and
> > data ready pins topology:
> > 
> >   Sampling trigger |       |       |       |       |
> >   ADC conversion   ++++    ++++    ++++    ++++    ++++
> >   ADC data ready      *       *       *       *       *
> > 
> > With the oversampling frequency, conversions are spaced:
> > 
> >   Sampling trigger |       |       |       |       |
> >   ADC conversion   + + + + + + + + + + + + + + + + + + + +
> >   ADC data ready         *       *       *       *       *
> > 
> > In some devices and ranges, this internal clock can be used to evenly
> > space the conversions between the sampling edge. In other devices the
> > oversampling frequency is fixed or is computed based on the sampling
> > frequency parameter, and the parameter is read only.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index ef52c427a015cf47bb9847782e13afbee01e9f31..e60367255be89a9acc827ec1a749b729735f60e6 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -139,6 +139,23 @@ Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		Hardware dependent values supported by the oversampling filter.
> >  
> > +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency
> > +KernelVersion:	6.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Some devices have internal clocks for oversampling.
> 
> Wrapping on each sentence is unusual. David pointed this out in v2.
> Wrap at 80 chars as a single paragraph.
> 

Yep, sorry.
Hopefully I develop muscle memory for vim's Vgq soon.

Best regards,
Jorge

> > +		Sets the resulting frequency in Hz to trigger a conversion used by
> > +		the oversampling filter.
> > +		If the device has a fixed internal clock or is computed based on
> > +		the sampling frequency parameter, the parameter is read only.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency_available
> > +KernelVersion:	6.17
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Hardware dependent values supported by the oversampling
> > +		frequency.
> > +
> >  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_raw
> >  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_raw
> >  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_raw
> > 
> 

[PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

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.0

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[Jonathan Cameron]

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.

> +
> +  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 */
> 

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

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 */
> > 
> 

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

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>;
>>> +        };
>>> +    };

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

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

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

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.

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

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

Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[Jonathan Cameron]

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

[PATCH v3 3/8] docs: iio: New docs for ad4052 driver

[Jorge Marques]

This adds a new page to document how to use the ad4052 ADC driver.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 Documentation/iio/ad4052.rst | 71 ++++++++++++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst  |  1 +
 MAINTAINERS                  |  1 +
 3 files changed, 73 insertions(+)

diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
new file mode 100644
index 0000000000000000000000000000000000000000..25e55eb72e167bd2b2195ba789b45ce402869b0f
--- /dev/null
+++ b/Documentation/iio/ad4052.rst
@@ -0,0 +1,71 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=============
+AD4052 driver
+=============
+
+ADC driver for Analog Devices Inc. AD4052 and similar devices. The module name
+is ``ad4052``.
+
+Supported devices
+=================
+
+The following chips are supported by this driver:
+
+* `AD4050 <https://www.analog.com/AD4050>`_
+* `AD4052 <https://www.analog.com/AD4052>`_
+* `AD4056 <https://www.analog.com/AD4056>`_
+* `AD4058 <https://www.analog.com/AD4058>`_
+
+Wiring modes
+============
+
+The ADC uses SPI 4-wire mode, and contain two programmable GPIOs and a CNV pin.
+
+The CNV pin is exposed as the ``cnv-gpios`` and triggers an ADC conversion. GP1
+is ADC conversion ready signal and GP0 Threshold event interrupt, both exposed
+as interrupts.
+
+Omit ``cnv-gpios`` and tie CNV and CS together to use the rising edge of the CS
+as the CNV signal.
+
+Device attributes
+=================
+
+The ADC contains only one channel with following attributes:
+
+.. list-table:: Channel attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``in_voltage_calibscale``
+     - Scale factor to multiply the raw value.
+   * - ``in_voltage_raw``
+     - Raw ADC voltage value
+   * - ``in_voltage_oversampling_ratio``
+     - Enable the device's burst averaging mode to over sample using the
+       internal sample rate.
+   * - ``in_voltage_oversampling_ratio_available``
+     - List of available oversampling values. Value 1 disable the burst
+       averaging mode.
+
+Also contain the following device attributes:
+
+.. list-table:: Device attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``oversamling_frequency``
+     - Frequency used in the burst averaging mode, sets the device internal
+       sample rate when the mode is activated.
+   * - ``oversampling_frequency_available``
+     - List of available sample rates.
+
+Low-power mode
+==============
+
+The device enters low-power mode on idle to save power. Enabling an event puts
+the device out of the low-power since the ADC autonomously samples to assert
+the event condition.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 2d6afc5a8ed54a90cd8d5723f0dc5212b8593d16..ef03022b8a6374c14d7b1b2e51e5c487e1d37df9 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -22,6 +22,7 @@ Industrial I/O Kernel Drivers
    ad3552r
    ad4000
    ad4030
+   ad4052
    ad4695
    ad7191
    ad7380
diff --git a/MAINTAINERS b/MAINTAINERS
index d000c7de7ff9eba390f87593bc2b1847f966f48b..32afd568b68e45616c291d689a35ab905d421cd1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1342,6 +1342,7 @@ 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
+F:	Documentation/iio/ad4052.rst
 
 ANALOG DEVICES INC AD4080 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>

-- 
2.49.0

[PATCH v3 4/8] iio: adc: Add support for ad4052

[Jorge Marques]

The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
approximation register (SAR) analog-to-digital converter (ADC) that
enables low-power, high-density data acquisition solutions without
sacrificing precision. This ADC offers a unique balance of performance
and power efficiency, plus innovative features for seamlessly switching
between high-resolution and low-power modes tailored to the immediate
needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
battery-powered, compact data acquisition and edge sensing applications.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 MAINTAINERS              |    1 +
 drivers/iio/adc/Kconfig  |   15 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4052.c | 1083 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1100 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 32afd568b68e45616c291d689a35ab905d421cd1..d83ac902e515575e1419df7505d2e8a96f69244a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1343,6 +1343,7 @@ S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
 F:	Documentation/iio/ad4052.rst
+F:	drivers/iio/adc/ad4052.c
 
 ANALOG DEVICES INC AD4080 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 3bd03df9a9761f20e2b1b71b51ebe43ba7f51b60..1fe14f939a4d9473b697b40f022124a2ae0b3245 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -55,6 +55,21 @@ config AD4030
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4030.
 
+config AD4052
+	tristate "Analog Devices AD4052 Driver"
+	depends on SPI
+	depends on GPIOLIB
+	select SPI_OFFLOAD
+	select IIO_BUFFER
+	select IIO_BUFFER_DMAENGINE
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD4052 SPI analog
+	  to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4052.
+
 config AD4080
 	tristate "Analog Devices AD4080 high speed ADC"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 6516ccb4d63bcfe2e2d56c39da7b906fdbe62319..dc3cd61df815fe7a7e77fad85630794b8be11f86 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4030) += ad4030.o
+obj-$(CONFIG_AD4052) += ad4052.o
 obj-$(CONFIG_AD4080) += ad4080.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD4695) += ad4695.o
diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
new file mode 100644
index 0000000000000000000000000000000000000000..842f5972a1c58701addf5243e7b87da9c26c773f
--- /dev/null
+++ b/drivers/iio/adc/ad4052.c
@@ -0,0 +1,1083 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AD4052 SPI ADC driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/units.h>
+#include <linux/unaligned.h>
+#include <dt-bindings/iio/adc/adi,ad4052.h>
+
+#define AD4052_REG_INTERFACE_CONFIG_A			0x00
+#define AD4052_REG_DEVICE_CONFIG			0x02
+#define     AD4052_REG_DEVICE_CONFIG_POWER_MODE_MSK	GENMASK(1, 0)
+#define     AD4052_REG_DEVICE_CONFIG_LOW_POWER_MODE	3
+#define AD4052_REG_PROD_ID_1				0x05
+#define AD4052_REG_DEVICE_GRADE				0x06
+#define AD4052_REG_SCRATCH_PAD				0x0A
+#define AD4052_REG_VENDOR_H				0x0D
+#define AD4052_REG_STREAM_MODE				0x0E
+#define AD4052_REG_INTERFACE_STATUS			0x11
+#define     AD4052_REG_INTERFACE_STATUS_NOT_RDY		BIT(7)
+#define AD4052_REG_MODE_SET				0x20
+#define     AD4052_REG_MODE_SET_ENTER_ADC		BIT(0)
+#define AD4052_REG_ADC_MODES				0x21
+#define     AD4052_REG_ADC_MODES_MODE_MSK		GENMASK(1, 0)
+#define     AD4052_REG_ADC_MODES_DATA_FORMAT		BIT(7)
+#define AD4052_REG_ADC_CONFIG				0x22
+#define     AD4052_REG_ADC_CONFIG_REF_EN_MSK		BIT(5)
+#define     AD4052_REG_ADC_CONFIG_SCALE_EN_MSK		BIT(4)
+#define AD4052_REG_AVG_CONFIG				0x23
+#define AD4052_REG_GP_CONF				0x24
+#define     AD4052_REG_GP_CONF_MODE_MSK_0		GENMASK(2, 0)
+#define     AD4052_REG_GP_CONF_MODE_MSK_1		GENMASK(6, 4)
+#define AD4052_REG_INTR_CONF				0x25
+#define     AD4052_REG_INTR_CONF_EN_MSK_0		GENMASK(1, 0)
+#define     AD4052_REG_INTR_CONF_EN_MSK_1		GENMASK(5, 4)
+#define AD4052_REG_TIMER_CONFIG				0x27
+#define     AD4052_REG_TIMER_CONFIG_FS_MASK		GENMASK(7, 4)
+#define     AD4052_REG_TIMER_CONFIG_300KSPS		0x2
+#define AD4052_REG_MAX_LIMIT				0x29
+#define AD4052_REG_MIN_LIMIT				0x2B
+#define AD4052_REG_MAX_HYST				0x2C
+#define AD4052_REG_MIN_HYST				0x2D
+#define AD4052_REG_MON_VAL				0x2F
+#define AD4052_REG_FUSE_CRC				0x40
+#define AD4052_REG_DEVICE_STATUS			0x41
+#define     AD4052_REG_DEVICE_STATUS_DEVICE_RDY		BIT(7)
+#define     AD4052_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
+#define AD4052_REG_MIN_SAMPLE				0x45
+#define AD4052_MAX_REG					0x45
+
+#define AD4052_SPI_VENDOR	0x0456
+
+#define AD4050_MAX_AVG		0x7
+#define AD4052_MAX_AVG		0xB
+#define AD4052_MAX_RATE(x)	((x) == AD4052_500KSPS ? 500000 : 2000000)
+#define AD4052_FS_OFFSET(g)	((g) == AD4052_500KSPS ? 2 : 0)
+#define AD4052_FS(g)		(&ad4052_conversion_freqs[AD4052_FS_OFFSET(g)])
+#define AD4052_FS_LEN(g)	(ARRAY_SIZE(ad4052_conversion_freqs) - (AD4052_FS_OFFSET(g)))
+#define AD4052_MON_VAL_MAX_GAIN		1999970
+#define AD4052_MON_VAL_MIDDLE_POINT	0x8000
+#define AD4052_T_CNVH_NS	10
+#define AD4052_VIO_3V3		3300000
+#define AD4052_SPI_MAX_ADC_XFER_SPEED(x)	((x) >= AD4052_VIO_3V3 ? 83333333 : 58823529)
+#define AD4052_SPI_MAX_REG_XFER_SPEED		16000000
+
+enum ad4052_grade {
+	AD4052_2MSPS,
+	AD4052_500KSPS,
+};
+
+enum ad4052_operation_mode {
+	AD4052_SAMPLE_MODE = 0,
+	AD4052_BURST_AVERAGING_MODE = 1,
+	AD4052_MONITOR_MODE = 3,
+};
+
+enum ad4052_gp_mode {
+	AD4052_GP_DISABLED,
+	AD4052_GP_INTR,
+	AD4052_GP_DRDY,
+};
+
+enum ad4052_interrupt_en {
+	AD4052_INTR_EN_NEITHER,
+	AD4052_INTR_EN_MIN,
+	AD4052_INTR_EN_MAX,
+	AD4052_INTR_EN_EITHER,
+};
+
+struct ad4052_chip_info {
+	const struct iio_chan_spec channels[1];
+	const char *name;
+	u16 prod_id;
+	u8 max_avg;
+	u8 grade;
+};
+
+enum {
+	AD4052_SCAN_TYPE_SAMPLE,
+	AD4052_SCAN_TYPE_BURST_AVG,
+};
+
+static const struct iio_scan_type ad4052_scan_type_12_s[] = {
+	[AD4052_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+};
+
+static const struct iio_scan_type ad4052_scan_type_16_s[] = {
+	[AD4052_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+};
+
+struct ad4052_state {
+	const struct ad4052_bus_ops *ops;
+	const struct ad4052_chip_info *chip;
+	enum ad4052_operation_mode mode;
+	struct spi_device *spi;
+	struct pwm_device *cnv_pwm;
+	struct pwm_state pwm_st;
+	struct spi_transfer xfer;
+	struct gpio_desc *cnv_gp;
+	struct completion completion;
+	struct regmap *regmap;
+	u16 oversampling_frequency;
+	int gp1_irq;
+	int vio_uv;
+	int vref_uv;
+	u8 reg_tx[3];
+	u8 reg_rx[3];
+	u8 raw[4] __aligned(IIO_DMA_MINALIGN);
+	u8 buf_reset_pattern[18];
+};
+
+static int ad4052_spi_read(void *context, const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	int ret;
+	struct ad4052_state *st = context;
+	struct spi_transfer xfer = {
+		.tx_buf = st->reg_tx,
+		.rx_buf = st->reg_rx,
+		.len = reg_size + val_size,
+		.speed_hz = AD4052_SPI_MAX_REG_XFER_SPEED,
+	};
+
+	if (xfer.len > sizeof(st->reg_tx) ||
+	    xfer.len > sizeof(st->reg_rx))
+		return  -EINVAL;
+
+	memset(st->reg_tx, 0, sizeof(st->reg_tx));
+	memcpy(st->reg_tx, reg, reg_size);
+
+	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	if (ret)
+		return ret;
+
+	memcpy(val, &st->reg_rx[reg_size], val_size);
+
+	return 0;
+}
+
+static int ad4052_spi_write(void *context, const void *data, size_t count)
+{
+	struct ad4052_state *st = context;
+	struct spi_transfer xfer = {
+		.tx_buf = st->reg_tx,
+		.len = count,
+		.speed_hz = AD4052_SPI_MAX_REG_XFER_SPEED,
+	};
+
+	if (count > sizeof(st->reg_tx))
+		return  -EINVAL;
+
+	memcpy(st->reg_tx, data, count);
+
+	return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+/* To limit the configuration mode access speed */
+static const struct regmap_bus ad4052_regmap_bus = {
+	.read = ad4052_spi_read,
+	.write = ad4052_spi_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static const struct regmap_range ad4052_regmap_rd_ranges[] = {
+	regmap_reg_range(AD4052_REG_INTERFACE_CONFIG_A, AD4052_REG_DEVICE_GRADE),
+	regmap_reg_range(AD4052_REG_SCRATCH_PAD, AD4052_REG_INTERFACE_STATUS),
+	regmap_reg_range(AD4052_REG_MODE_SET, AD4052_REG_MON_VAL),
+	regmap_reg_range(AD4052_REG_FUSE_CRC, AD4052_REG_MIN_SAMPLE),
+};
+
+static const struct regmap_access_table ad4052_regmap_rd_table = {
+	.yes_ranges = ad4052_regmap_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_rd_ranges),
+};
+
+static const struct regmap_range ad4052_regmap_wr_ranges[] = {
+	regmap_reg_range(AD4052_REG_INTERFACE_CONFIG_A, AD4052_REG_DEVICE_CONFIG),
+	regmap_reg_range(AD4052_REG_SCRATCH_PAD, AD4052_REG_SCRATCH_PAD),
+	regmap_reg_range(AD4052_REG_STREAM_MODE, AD4052_REG_INTERFACE_STATUS),
+	regmap_reg_range(AD4052_REG_MODE_SET, AD4052_REG_MON_VAL),
+	regmap_reg_range(AD4052_REG_FUSE_CRC, AD4052_REG_DEVICE_STATUS),
+};
+
+static const struct regmap_access_table ad4052_regmap_wr_table = {
+	.yes_ranges = ad4052_regmap_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_wr_ranges),
+};
+
+static const char *const ad4052_conversion_freqs[] = {
+	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
+	"33300", "10000", "3000", "500",		/*  4 -  7 */
+	"333", "250", "200", "166",			/*  8 - 11 */
+	"140", "124", "111",				/* 12 - 15 */
+};
+
+static int ad4052_conversion_frequency_set(struct ad4052_state *st, u8 val)
+{
+	val += AD4052_FS_OFFSET(st->chip->grade);
+	return regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
+			    FIELD_PREP(AD4052_REG_TIMER_CONFIG_FS_MASK, val));
+}
+
+static int ad4052_oversampling_frequency_get(struct iio_dev *indio_dev,
+					     const struct iio_chan_spec *chan)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	return st->oversampling_frequency - AD4052_FS_OFFSET(st->chip->grade);
+}
+
+static int ad4052_oversampling_frequency_set(struct iio_dev *indio_dev,
+					     const struct iio_chan_spec *chan,
+					     unsigned int val)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	val += AD4052_FS_OFFSET(st->chip->grade);
+	st->oversampling_frequency = val;
+
+	return 0;
+}
+
+static const struct iio_enum AD4052_500KSPS_conversion_freq_enum = {
+	.items = AD4052_FS(AD4052_500KSPS),
+	.num_items = AD4052_FS_LEN(AD4052_500KSPS),
+	.set = ad4052_oversampling_frequency_set,
+	.get = ad4052_oversampling_frequency_get,
+};
+
+static const struct iio_enum AD4052_2MSPS_conversion_freq_enum = {
+	.items = AD4052_FS(AD4052_2MSPS),
+	.num_items = AD4052_FS_LEN(AD4052_2MSPS),
+	.set = ad4052_oversampling_frequency_set,
+	.get = ad4052_oversampling_frequency_get,
+};
+
+#define AD4052_EXT_INFO(grade)						\
+static struct iio_chan_spec_ext_info grade##_ext_info[] = {		\
+	IIO_ENUM("oversampling_frequency", IIO_SHARED_BY_ALL,		\
+		 &grade##_conversion_freq_enum),			\
+	IIO_ENUM_AVAILABLE("oversampling_frequency", IIO_SHARED_BY_ALL,	\
+			   &grade##_conversion_freq_enum),		\
+	{ }								\
+}
+
+AD4052_EXT_INFO(AD4052_2MSPS);
+AD4052_EXT_INFO(AD4052_500KSPS);
+
+#define AD4052_CHAN(bits, grade) {							\
+	.type = IIO_VOLTAGE,								\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
+				    BIT(IIO_CHAN_INFO_SCALE) |				\
+				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
+				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+	.indexed = 1,									\
+	.channel = 0,									\
+	.has_ext_scan_type = 1,								\
+	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
+	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
+	.ext_info = grade##_ext_info,							\
+}
+
+#define AD4052_OFFLOAD_CHAN(bits, grade) {						\
+	.type = IIO_VOLTAGE,								\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
+				    BIT(IIO_CHAN_INFO_SCALE) |				\
+				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
+				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |		\
+				    BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+	.indexed = 1,									\
+	.channel = 0,									\
+	.has_ext_scan_type = 1,								\
+	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
+	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
+	.ext_info = grade##_ext_info,							\
+}
+
+static const struct ad4052_chip_info ad4050_chip_info = {
+	.name = "ad4050",
+	.channels = { AD4052_CHAN(12, AD4052_2MSPS) },
+	.prod_id = 0x70,
+	.max_avg = AD4050_MAX_AVG,
+	.grade = AD4052_2MSPS,
+};
+
+static const struct ad4052_chip_info ad4052_chip_info = {
+	.name = "ad4052",
+	.channels = { AD4052_CHAN(16, AD4052_2MSPS) },
+	.prod_id = 0x72,
+	.max_avg = AD4052_MAX_AVG,
+	.grade = AD4052_2MSPS,
+};
+
+static const struct ad4052_chip_info ad4056_chip_info = {
+	.name = "ad4056",
+	.channels = { AD4052_CHAN(12, AD4052_500KSPS) },
+	.prod_id = 0x76,
+	.max_avg = AD4050_MAX_AVG,
+	.grade = AD4052_500KSPS,
+};
+
+static const struct ad4052_chip_info ad4058_chip_info = {
+	.name = "ad4058",
+	.channels = { AD4052_CHAN(16, AD4052_500KSPS) },
+	.prod_id = 0x78,
+	.max_avg = AD4052_MAX_AVG,
+	.grade = AD4052_500KSPS,
+};
+
+static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+	struct spi_transfer *xfer = &st->xfer;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	xfer->rx_buf = st->raw;
+	xfer->bits_per_word = scan_type->realbits;
+	xfer->len = scan_type->realbits == 24 ? 4 : 2;
+	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
+
+	return 0;
+}
+
+static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
+					 const struct iio_chan_spec *chan,
+					 unsigned int val)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (val < 1 || val > BIT(st->chip->max_avg + 1))
+		return -EINVAL;
+
+	/* 1 disables oversampling */
+	if (val == 1) {
+		st->mode = AD4052_SAMPLE_MODE;
+	} else {
+		val = ilog2(val);
+		st->mode = AD4052_BURST_AVERAGING_MODE;
+		ret = regmap_write(st->regmap, AD4052_REG_AVG_CONFIG, val - 1);
+		if (ret)
+			return ret;
+	}
+
+	return ad4052_update_xfer_raw(indio_dev, chan);
+}
+
+static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
+					 unsigned int *val)
+{
+	int ret, buf;
+
+	if (st->mode == AD4052_SAMPLE_MODE) {
+		*val = 1;
+		return 0;
+	}
+
+	ret = regmap_read(st->regmap, AD4052_REG_AVG_CONFIG, &buf);
+	if (ret)
+		return ret;
+
+	*val = BIT(buf + 1);
+
+	return 0;
+}
+
+static int ad4052_check_ids(struct ad4052_state *st)
+{
+	int ret;
+	u16 val;
+
+	ret = regmap_bulk_read(st->regmap, AD4052_REG_PROD_ID_1, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	val = get_unaligned_be16(st->raw);
+	if (val != st->chip->prod_id)
+		dev_warn(&st->spi->dev,
+			 "Production ID x%x does not match known values", val);
+
+	ret = regmap_bulk_read(st->regmap, AD4052_REG_VENDOR_H, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	val = get_unaligned_be16(st->raw);
+	if (val != AD4052_SPI_VENDOR)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int ad4052_exit_command(struct ad4052_state *st)
+{
+	struct spi_device *spi = st->spi;
+	const u8 val = 0xA8;
+
+	return spi_write(spi, &val, 1);
+}
+
+static int ad4052_set_operation_mode(struct ad4052_state *st,
+				     enum ad4052_operation_mode mode)
+{
+	int ret;
+
+	if (mode == AD4052_BURST_AVERAGING_MODE) {
+		ret = ad4052_conversion_frequency_set(st, st->oversampling_frequency);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_ADC_MODES,
+				 AD4052_REG_ADC_MODES_MODE_MSK, mode);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4052_REG_MODE_SET,
+			    AD4052_REG_MODE_SET_ENTER_ADC);
+}
+
+static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
+{
+	const u32 start = 1;
+
+	if (!in_range(freq, start, AD4052_MAX_RATE(st->chip->grade)))
+		return -EINVAL;
+
+	st->pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
+	return pwm_apply_might_sleep(st->cnv_pwm, &st->pwm_st);
+}
+
+static int ad4052_soft_reset(struct ad4052_state *st)
+{
+	int ret;
+
+	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
+	st->buf_reset_pattern[5] = 0xFE;
+	st->buf_reset_pattern[11] = 0xFE;
+	st->buf_reset_pattern[17] = 0xFE;
+
+	ret = spi_write(st->spi, st->buf_reset_pattern,
+			sizeof(st->buf_reset_pattern));
+	if (ret)
+		return ret;
+
+	/* Wait AD4052 reset delay */
+	fsleep(5000);
+
+	return 0;
+}
+
+static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			const bool *ref_sel)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+	int ret;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	u8 val = FIELD_PREP(AD4052_REG_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
+		 FIELD_PREP(AD4052_REG_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
+				 AD4052_REG_GP_CONF_MODE_MSK_1 | AD4052_REG_GP_CONF_MODE_MSK_0,
+				 val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4052_REG_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
+	      FIELD_PREP(AD4052_REG_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
+
+	if (st->chip->grade == AD4052_500KSPS) {
+		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
+				   FIELD_PREP(AD4052_REG_TIMER_CONFIG_FS_MASK,
+					      AD4052_REG_TIMER_CONFIG_300KSPS));
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_ADC_MODES,
+				 AD4052_REG_ADC_CONFIG_REF_EN_MSK,
+				 FIELD_PREP(AD4052_REG_ADC_CONFIG_REF_EN_MSK,
+					    *ref_sel));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
+			   AD4052_REG_DEVICE_STATUS_DEVICE_RESET);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
+				  AD4052_REG_INTR_CONF_EN_MSK_0 | AD4052_REG_INTR_CONF_EN_MSK_1,
+				  val);
+}
+
+static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
+{
+	struct ad4052_state *st = private;
+
+	complete(&st->completion);
+	return IRQ_HANDLED;
+}
+
+static int ad4052_request_irq(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	int ret;
+
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
+	if (ret > 0) {
+		ret = devm_request_threaded_irq(dev, ret, NULL,
+						ad4052_irq_handler_drdy,
+						IRQF_ONESHOT, indio_dev->name,
+						st);
+		st->gp1_irq = ret;
+	} else if (ret == -EPROBE_DEFER) {
+		return ret;
+	}
+	return 0;
+}
+
+static const int ad4052_oversampling_avail[] = {
+	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
+};
+
+static int ad4052_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, const int **vals,
+			     int *type, int *len, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = ad4052_oversampling_avail;
+		*len = ARRAY_SIZE(ad4052_oversampling_avail);
+		*type = IIO_VAL_INT;
+
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4052_get_samp_freq(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val,
+				int *val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
+	return IIO_VAL_INT;
+}
+
+static int ad4052_get_chan_scale(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val,
+				 int *val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+
+	scan_type = iio_get_current_scan_type(indio_dev, st->chip->channels);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	*val = (st->vref_uv * 2) / MILLI;
+
+	*val2 = scan_type->realbits;
+
+	return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+static int ad4052_get_chan_calibscale(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      int *val,
+				      int *val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	u16 gain;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4052_REG_MON_VAL, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	gain = get_unaligned_be16(&st->raw);
+
+	/* From datasheet: code out = code in × mon_val/0x8000 */
+	*val = gain / AD4052_MON_VAL_MIDDLE_POINT;
+	*val2 = mul_u64_u32_div(gain % AD4052_MON_VAL_MIDDLE_POINT, NANO,
+				AD4052_MON_VAL_MIDDLE_POINT);
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static int ad4052_set_chan_calibscale(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      int gain_int,
+				      int gain_frac)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	u64 gain;
+	int ret;
+
+	if (gain_int < 0 || gain_frac < 0)
+		return -EINVAL;
+
+	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
+
+	if (gain > AD4052_MON_VAL_MAX_GAIN)
+		return -EINVAL;
+
+	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4052_MON_VAL_MIDDLE_POINT,
+						 MICRO),
+			   &st->raw);
+
+	ret = regmap_bulk_write(st->regmap, AD4052_REG_MON_VAL, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, AD4052_REG_ADC_MODES,
+				  AD4052_REG_ADC_CONFIG_SCALE_EN_MSK,
+				  FIELD_PREP(AD4052_REG_ADC_CONFIG_SCALE_EN_MSK,
+					     !(gain_int == 1 && gain_frac == 0)));
+}
+
+static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
+{
+	struct spi_device *spi = st->spi;
+	struct spi_transfer t_cnv = {};
+	int ret;
+
+	reinit_completion(&st->completion);
+
+	if (st->cnv_gp) {
+		gpiod_set_value_cansleep(st->cnv_gp, 1);
+		gpiod_set_value_cansleep(st->cnv_gp, 0);
+	} else {
+		ret = spi_sync_transfer(spi, &t_cnv, 1);
+		if (ret)
+			return ret;
+	}
+	/*
+	 * Single sample read should be used only for oversampling and
+	 * sampling frequency pairs that take less than 1 sec.
+	 */
+	if (st->gp1_irq) {
+		ret = wait_for_completion_timeout(&st->completion,
+						  msecs_to_jiffies(1000));
+		if (!ret)
+			return -ETIMEDOUT;
+	}
+
+	ret = spi_sync_transfer(spi, &st->xfer, 1);
+	if (ret)
+		return ret;
+
+	if (st->xfer.len == 2)
+		*val = sign_extend32(*(u16 *)(st->raw), 15);
+	else
+		*val = sign_extend32(*(u32 *)(st->raw), 23);
+
+	return ret;
+}
+
+static int ad4052_read_chan_raw(struct iio_dev *indio_dev, int *val)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&st->spi->dev);
+	if (ret)
+		return ret;
+
+	ret = ad4052_set_operation_mode(st, st->mode);
+	if (ret)
+		goto out_error;
+
+	ret = __ad4052_read_chan_raw(st, val);
+	if (ret)
+		goto out_error;
+
+	ret = ad4052_exit_command(st);
+
+out_error:
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+	return ret;
+}
+
+static int ad4052_read_raw_dispatch(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, int *val,
+				    int *val2, long info)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		return ad4052_read_chan_raw(indio_dev, val);
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4052_get_chan_calibscale(indio_dev, chan, val, val2);
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4052_get_oversampling_ratio(st, val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4052_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	int ret;
+
+	if (info ==  IIO_CHAN_INFO_SAMP_FREQ)
+		return ad4052_get_samp_freq(indio_dev, chan, val, val2);
+	else if (info == IIO_CHAN_INFO_SCALE)
+		return ad4052_get_chan_scale(indio_dev, chan, val, val2);
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = ad4052_read_raw_dispatch(indio_dev, chan, val, val2, info);
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int ad4052_write_raw_dispatch(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan, int val,
+				     int val2, long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4052_set_oversampling_ratio(indio_dev, chan, val);
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4052_set_chan_calibscale(indio_dev, chan, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+};
+
+static int ad4052_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long info)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4052_set_sampling_freq(st, val);
+	}
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = ad4052_write_raw_dispatch(indio_dev, chan, val, val2, info);
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				     unsigned int writeval, unsigned int *readval)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (readval)
+		ret = regmap_read(st->regmap, reg, readval);
+	else
+		ret = regmap_write(st->regmap, reg, writeval);
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4052_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	return st->mode == AD4052_BURST_AVERAGING_MODE ?
+			   AD4052_SCAN_TYPE_BURST_AVG :
+			   AD4052_SCAN_TYPE_SAMPLE;
+}
+
+static const struct iio_info ad4052_info = {
+	.read_raw = ad4052_read_raw,
+	.write_raw = ad4052_write_raw,
+	.read_avail = ad4052_read_avail,
+	.get_current_scan_type = &ad4052_get_current_scan_type,
+	.debugfs_reg_access = &ad4052_debugfs_reg_access,
+};
+
+static const struct regmap_config ad4052_regmap_config = {
+	.name = "ad4052",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AD4052_MAX_REG,
+	.rd_table = &ad4052_regmap_rd_table,
+	.wr_table = &ad4052_regmap_wr_table,
+	.read_flag_mask = BIT(7),
+	.can_sleep = true,
+};
+
+static int __ad4052_validate_trigger_sources(struct of_phandle_args *trigger_sources)
+{
+	switch (trigger_sources->args[1]) {
+	case AD4052_TRIGGER_PIN_GP1:
+		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_DATA_READY ?
+		       0 : -EINVAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4052_validate_trigger_sources(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct of_phandle_args trigger_sources;
+	struct device_node *np;
+	int ret;
+
+	np = st->spi->dev.of_node;
+	ret = of_parse_phandle_with_args(np, "trigger-sources",
+					 "#trigger-source-cells", 0,
+					 &trigger_sources);
+	if (ret)
+		return ret;
+
+	ret = __ad4052_validate_trigger_sources(&trigger_sources);
+	of_node_put(trigger_sources.np);
+	return ret;
+}
+
+static int ad4052_regulators_get(struct ad4052_state *st, bool *ref_sel)
+{
+	struct device *dev = &st->spi->dev;
+	int uv;
+
+	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
+	if (st->vio_uv < 0)
+		return dev_err_probe(dev, st->vio_uv,
+				     "Failed to enable and read vio voltage\n");
+
+	uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (uv < 0)
+		return dev_err_probe(dev, uv,
+				     "Failed to enable vdd regulator\n");
+
+	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+	*ref_sel = st->vref_uv == -ENODEV;
+	if (st->vref_uv == -ENODEV)
+		st->vref_uv = uv;
+	else if (st->vref_uv < 0)
+		return dev_err_probe(dev, st->vref_uv,
+				     "Failed to enable and read ref voltage\n");
+	return 0;
+}
+
+static int ad4052_probe(struct spi_device *spi)
+{
+	const struct ad4052_chip_info *chip;
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad4052_state *st;
+	bool ref_sel;
+	int ret;
+
+	chip = spi_get_device_match_data(spi);
+	if (!chip)
+		return dev_err_probe(dev, -ENODEV,
+				     "Could not find chip info data\n");
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+	spi_set_drvdata(spi, st);
+	init_completion(&st->completion);
+
+	ret = ad4052_regulators_get(st, &ref_sel);
+	if (ret)
+		return ret;
+
+	st->regmap = devm_regmap_init(dev, &ad4052_regmap_bus, st,
+				      &ad4052_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to initialize regmap\n");
+
+	st->mode = AD4052_SAMPLE_MODE;
+	st->chip = chip;
+	st->oversampling_frequency = AD4052_FS_OFFSET(st->chip->grade);
+
+	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
+	if (IS_ERR(st->cnv_gp))
+		return dev_err_probe(dev, PTR_ERR(st->cnv_gp),
+				     "Failed to get cnv gpio\n");
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ad4052_info;
+	indio_dev->name = chip->name;
+
+	ret = ad4052_validate_trigger_sources(indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to validate trigger sources\n");
+
+	ret = ad4052_soft_reset(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "AD4052 failed to soft reset\n");
+
+	ret = ad4052_check_ids(st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "AD4052 fields assertions failed\n");
+
+	ret = ad4052_setup(indio_dev, indio_dev->channels, &ref_sel);
+	if (ret)
+		return ret;
+
+	ret = ad4052_request_irq(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_active(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable pm_runtime\n");
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int ad4052_runtime_suspend(struct device *dev)
+{
+	struct ad4052_state *st = dev_get_drvdata(dev);
+
+	return regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
+			    FIELD_PREP(AD4052_REG_DEVICE_CONFIG_POWER_MODE_MSK,
+				       AD4052_REG_DEVICE_CONFIG_LOW_POWER_MODE));
+}
+
+static int ad4052_runtime_resume(struct device *dev)
+{
+	struct ad4052_state *st = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regmap_clear_bits(st->regmap, AD4052_REG_DEVICE_CONFIG,
+				AD4052_REG_DEVICE_CONFIG_POWER_MODE_MSK);
+
+	fsleep(4000);
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ad4052_pm_ops, ad4052_runtime_suspend,
+				 ad4052_runtime_resume, NULL);
+
+static const struct spi_device_id ad4052_id_table[] = {
+	{"ad4050", (kernel_ulong_t)&ad4050_chip_info },
+	{"ad4052", (kernel_ulong_t)&ad4052_chip_info },
+	{"ad4056", (kernel_ulong_t)&ad4056_chip_info },
+	{"ad4058", (kernel_ulong_t)&ad4058_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad4052_id_table);
+
+static const struct of_device_id ad4052_of_match[] = {
+	{ .compatible = "adi,ad4050", .data = &ad4050_chip_info },
+	{ .compatible = "adi,ad4052", .data = &ad4052_chip_info },
+	{ .compatible = "adi,ad4056", .data = &ad4056_chip_info },
+	{ .compatible = "adi,ad4058", .data = &ad4058_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad4052_of_match);
+
+static struct spi_driver ad4052_driver = {
+	.driver = {
+		.name = "ad4052",
+		.of_match_table = ad4052_of_match,
+		.pm = pm_ptr(&ad4052_pm_ops),
+	},
+	.probe = ad4052_probe,
+	.id_table = ad4052_id_table,
+};
+module_spi_driver(ad4052_driver);
+
+MODULE_AUTHOR("Jorge Marques <jorge.marques@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4052");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_DMAENGINE_BUFFER");

-- 
2.49.0

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Jonathan Cameron]

On Tue, 10 Jun 2025 09:34:37 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
> approximation register (SAR) analog-to-digital converter (ADC) that
> enables low-power, high-density data acquisition solutions without
> sacrificing precision. This ADC offers a unique balance of performance
> and power efficiency, plus innovative features for seamlessly switching
> between high-resolution and low-power modes tailored to the immediate
> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
> battery-powered, compact data acquisition and edge sensing applications.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jorge,

Various minor comments inline.

Jonathan

> diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..842f5972a1c58701addf5243e7b87da9c26c773f
> --- /dev/null
> +++ b/drivers/iio/adc/ad4052.c
> @@ -0,0 +1,1083 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices AD4052 SPI ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>

whilst I've asked you to drop the of specific code, the
absences of an appropriate header here makes me thing you
should take another look at these.  

> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +#include <linux/unaligned.h>
> +#include <dt-bindings/iio/adc/adi,ad4052.h>


> +
> +struct ad4052_chip_info {
> +	const struct iio_chan_spec channels[1];

As below. If it's always one drop the array.

> +	const char *name;
> +	u16 prod_id;
> +	u8 max_avg;
> +	u8 grade;
> +};

> +};
> +
> +struct ad4052_state {
> +	const struct ad4052_bus_ops *ops;
> +	const struct ad4052_chip_info *chip;
> +	enum ad4052_operation_mode mode;
> +	struct spi_device *spi;
> +	struct pwm_device *cnv_pwm;
> +	struct pwm_state pwm_st;
> +	struct spi_transfer xfer;
> +	struct gpio_desc *cnv_gp;
> +	struct completion completion;
> +	struct regmap *regmap;
> +	u16 oversampling_frequency;
> +	int gp1_irq;
> +	int vio_uv;
> +	int vref_uv;
> +	u8 reg_tx[3];

These look to also be used in spi transactions?  As such don't the
need to be IIO_DMA_MINALIGN?   Fine to have all these with just one
marking but it needs to be on the first one used in this fashion and you need
to be sure that they are all protected by an appropriate lock.
SYSFS access can race and the bus lock isn't sufficient as it only protects
the actual bus, not the buffers passed.  Also don't rely on core locking
(i.e. claim direct etc) because whether they prevent multiple users is an
implementation detail of the core and shouldn't be relied on by users.
They are only guaranteed to hold the buffer / direct state.

> +	u8 reg_rx[3];
> +	u8 raw[4] __aligned(IIO_DMA_MINALIGN);
> +	u8 buf_reset_pattern[18];
> +};


> +
> +#define AD4052_CHAN(bits, grade) {							\
> +	.type = IIO_VOLTAGE,								\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
> +				    BIT(IIO_CHAN_INFO_SCALE) |				\
> +				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> +				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +	.indexed = 1,									\
> +	.channel = 0,									\
> +	.has_ext_scan_type = 1,								\
> +	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
> +	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
> +	.ext_info = grade##_ext_info,							\
> +}
> +
> +#define AD4052_OFFLOAD_CHAN(bits, grade) {						\

Not used in this patch I think.  Push it to the later one.

> +	.type = IIO_VOLTAGE,								\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
> +				    BIT(IIO_CHAN_INFO_SCALE) |				\
> +				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> +				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |		\
> +				    BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +	.indexed = 1,									\
> +	.channel = 0,									\
> +	.has_ext_scan_type = 1,								\
> +	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
> +	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
> +	.ext_info = grade##_ext_info,							\
> +}
> +
> +static const struct ad4052_chip_info ad4050_chip_info = {
> +	.name = "ad4050",
> +	.channels = { AD4052_CHAN(12, AD4052_2MSPS) },

Are we ever going to have more than 1?  Maybe just use channel and drop the array part.

> +	.prod_id = 0x70,
> +	.max_avg = AD4050_MAX_AVG,
> +	.grade = AD4052_2MSPS,
> +};
> +
> +static const struct ad4052_chip_info ad4052_chip_info = {
> +	.name = "ad4052",
> +	.channels = { AD4052_CHAN(16, AD4052_2MSPS) },
> +	.prod_id = 0x72,
> +	.max_avg = AD4052_MAX_AVG,
> +	.grade = AD4052_2MSPS,
> +};
> +
> +static const struct ad4052_chip_info ad4056_chip_info = {
> +	.name = "ad4056",
> +	.channels = { AD4052_CHAN(12, AD4052_500KSPS) },
> +	.prod_id = 0x76,
> +	.max_avg = AD4050_MAX_AVG,
> +	.grade = AD4052_500KSPS,
> +};
> +
> +static const struct ad4052_chip_info ad4058_chip_info = {
> +	.name = "ad4058",
> +	.channels = { AD4052_CHAN(16, AD4052_500KSPS) },
> +	.prod_id = 0x78,
> +	.max_avg = AD4052_MAX_AVG,
> +	.grade = AD4052_500KSPS,
> +};
> +
> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> +				   struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	struct spi_transfer *xfer = &st->xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	xfer->rx_buf = st->raw;
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;

This is a little odd. I'm not sure what happens with len not dividing
into a whole number of bits per word chunks.
Maybe a comment?

> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> +
> +	return 0;
> +}


> +static int ad4052_exit_command(struct ad4052_state *st)
> +{
> +	struct spi_device *spi = st->spi;
> +	const u8 val = 0xA8;
> +
> +	return spi_write(spi, &val, 1);

Even a one byte spi_write() requires a DMA safe buffer.
Feel free to use spi_write_then_read() with zero size read as that doesn't
need dma safety.

> +}

> +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> +{
> +	const u32 start = 1;

Only used in the check so why the local variable? Maybe this makes sense in
later patches in which case fine to leave it here.

> +
> +	if (!in_range(freq, start, AD4052_MAX_RATE(st->chip->grade)))
> +		return -EINVAL;
> +
> +	st->pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	return pwm_apply_might_sleep(st->cnv_pwm, &st->pwm_st);
> +}


> +static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			const bool *ref_sel)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	int ret;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	u8 val = FIELD_PREP(AD4052_REG_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |

Declarations still at the top (unless doing cleanup.h stuff)

> +		 FIELD_PREP(AD4052_REG_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> +				 AD4052_REG_GP_CONF_MODE_MSK_1 | AD4052_REG_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_REG_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_REG_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));

Excess brackets. Also why set this here to write it many lines later?

> +
> +	if (st->chip->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_REG_TIMER_CONFIG_FS_MASK,
> +					      AD4052_REG_TIMER_CONFIG_300KSPS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_ADC_MODES,
> +				 AD4052_REG_ADC_CONFIG_REF_EN_MSK,
> +				 FIELD_PREP(AD4052_REG_ADC_CONFIG_REF_EN_MSK,
> +					    *ref_sel));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			   AD4052_REG_DEVICE_STATUS_DEVICE_RESET);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> +				  AD4052_REG_INTR_CONF_EN_MSK_0 | AD4052_REG_INTR_CONF_EN_MSK_1,
> +				  val);
> +}



> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> +{
> +	struct spi_device *spi = st->spi;
> +	struct spi_transfer t_cnv = {};
> +	int ret;
> +
> +	reinit_completion(&st->completion);
> +
> +	if (st->cnv_gp) {
> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> +	} else {
> +		ret = spi_sync_transfer(spi, &t_cnv, 1);

Add a comment for this.   I can't immediately spot documentation on what
a content free transfer actually does.  I assume pulses the chip select?
is that true for all SPI controllers?

> +		if (ret)
> +			return ret;
> +	}
> +	/*
> +	 * Single sample read should be used only for oversampling and
> +	 * sampling frequency pairs that take less than 1 sec.
> +	 */
> +	if (st->gp1_irq) {
> +		ret = wait_for_completion_timeout(&st->completion,
> +						  msecs_to_jiffies(1000));
> +		if (!ret)
> +			return -ETIMEDOUT;
> +	}
> +
> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (st->xfer.len == 2)
> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
> +	else
> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
> +
> +	return ret;
> +}

> +
> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	int ret;
> +
> +	if (info ==  IIO_CHAN_INFO_SAMP_FREQ)
> +		return ad4052_get_samp_freq(indio_dev, chan, val, val2);
> +	else if (info == IIO_CHAN_INFO_SCALE)

if (info == IIO_CHAN_INFO_SCALE) as you returned in the if above.


Or just make it a switch statement with default: covering the rest
of the cases.

> +		return ad4052_get_chan_scale(indio_dev, chan, val, val2);
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = ad4052_read_raw_dispatch(indio_dev, chan, val, val2, info);
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;
> +}

> +
> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +				     unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))

For these guards in the debugfs callback, please add a comment on why they
are needed.   We've had a lot of questions about these recently and I'd
like it to be clear to people when they should cut and paste these and when
not.

> +		return -EBUSY;
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}

> +static int __ad4052_validate_trigger_sources(struct of_phandle_args *trigger_sources)
> +{
> +	switch (trigger_sources->args[1]) {
> +	case AD4052_TRIGGER_PIN_GP1:
> +		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_DATA_READY ?
> +		       0 : -EINVAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4052_validate_trigger_sources(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct of_phandle_args trigger_sources;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = st->spi->dev.of_node;
> +	ret = of_parse_phandle_with_args(np, "trigger-sources",
> +					 "#trigger-source-cells", 0,
> +					 &trigger_sources);

Can we use fwnode_property_get_reference_args() here?
Ideally we don't use of specific code in drivers.

TWith that use dev_fnwode() to get the more generic representation.

> +	if (ret)
> +		return ret;
> +
> +	ret = __ad4052_validate_trigger_sources(&trigger_sources);
> +	of_node_put(trigger_sources.np);
> +	return ret;
> +}
> +
> +static int ad4052_regulators_get(struct ad4052_state *st, bool *ref_sel)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int uv;
> +
> +	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> +	if (st->vio_uv < 0)
> +		return dev_err_probe(dev, st->vio_uv,
> +				     "Failed to enable and read vio voltage\n");
> +
> +	uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (uv < 0)
> +		return dev_err_probe(dev, uv,
> +				     "Failed to enable vdd regulator\n");
> +
> +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	*ref_sel = st->vref_uv == -ENODEV;
> +	if (st->vref_uv == -ENODEV)
> +		st->vref_uv = uv;

We probably don't care but we could support only enabling vdd if we aren't
using it's voltage.  That would allow a fake regulator if a board didn't
supply it (as always on).  Bit of an odd corner case though to rely on a stub
for that one but require the other regulators. Hence this is more of a comment
than a reason to change anything!

> +	else if (st->vref_uv < 0)
> +		return dev_err_probe(dev, st->vref_uv,
> +				     "Failed to enable and read ref voltage\n");
> +	return 0;
> +}


> +
> +static int ad4052_runtime_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_clear_bits(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +				AD4052_REG_DEVICE_CONFIG_POWER_MODE_MSK);
> +
> +	fsleep(4000);
I doesn't hugely matter as we don't expect an error, but normal approx
assumption of a regmap call failing is that we have lost comms with the chip
and in that case we just error out fast.  

	if (ret)
		return ret;

	fsleep(4000);

	return 0;

would reflect that handling.

> +	return ret;
> +}

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[David Lechner]

On 6/14/25 5:08 AM, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:37 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
>> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
>> approximation register (SAR) analog-to-digital converter (ADC) that
>> enables low-power, high-density data acquisition solutions without
>> sacrificing precision. This ADC offers a unique balance of performance
>> and power efficiency, plus innovative features for seamlessly switching
>> between high-resolution and low-power modes tailored to the immediate
>> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
>> battery-powered, compact data acquisition and edge sensing applications.
>>

...

>> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
>> +				   struct iio_chan_spec const *chan)
>> +{
>> +	struct ad4052_state *st = iio_priv(indio_dev);
>> +	const struct iio_scan_type *scan_type;
>> +	struct spi_transfer *xfer = &st->xfer;
>> +
>> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
>> +	if (IS_ERR(scan_type))
>> +		return PTR_ERR(scan_type);
>> +
>> +	xfer->rx_buf = st->raw;
>> +	xfer->bits_per_word = scan_type->realbits;
>> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;
> 
> This is a little odd. I'm not sure what happens with len not dividing
> into a whole number of bits per word chunks.
> Maybe a comment?

Even better, there is now spi_bpw_to_bytes() for this.

> 
>> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
>> +
>> +	return 0;
>> +}
> 
> 

...

> 
>> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
>> +{
>> +	struct spi_device *spi = st->spi;
>> +	struct spi_transfer t_cnv = {};
>> +	int ret;
>> +
>> +	reinit_completion(&st->completion);
>> +
>> +	if (st->cnv_gp) {
>> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
>> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
>> +	} else {
>> +		ret = spi_sync_transfer(spi, &t_cnv, 1);
> 
> Add a comment for this.   I can't immediately spot documentation on what
> a content free transfer actually does.  I assume pulses the chip select?
> is that true for all SPI controllers?

Should be. Setting .delay in the xfer would also make it more
clear that this is doing.

> 
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	/*
>> +	 * Single sample read should be used only for oversampling and
>> +	 * sampling frequency pairs that take less than 1 sec.
>> +	 */
>> +	if (st->gp1_irq) {
>> +		ret = wait_for_completion_timeout(&st->completion,
>> +						  msecs_to_jiffies(1000));
>> +		if (!ret)
>> +			return -ETIMEDOUT;
>> +	}
>> +
>> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (st->xfer.len == 2)
>> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
>> +	else
>> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
>> +
>> +	return ret;
>> +}
> 

...

>> +
>> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>> +				     unsigned int writeval, unsigned int *readval)
>> +{
>> +	struct ad4052_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	if (!iio_device_claim_direct(indio_dev))
> 
> For these guards in the debugfs callback, please add a comment on why they
> are needed.   We've had a lot of questions about these recently and I'd
> like it to be clear to people when they should cut and paste these and when
> not.

The reason I started doing this is that running the iio_info command attemps
to read register 0x00 via the debug attribute of every single iio device. So
if you run iio_info during a buffered read, and 0x00 is a valid register, it
would break things without this check.

Ideally, general purpose commands wouldn't be poking debug registers, but
that isn't the case. But I suppose we could "fix" iio_info instead.

> 
>> +		return -EBUSY;
>> +
>> +	if (readval)
>> +		ret = regmap_read(st->regmap, reg, readval);
>> +	else
>> +		ret = regmap_write(st->regmap, reg, writeval);
>> +	iio_device_release_direct(indio_dev);
>> +	return ret;
>> +}
> 

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Jonathan Cameron]

On Mon, 16 Jun 2025 09:54:52 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/14/25 5:08 AM, Jonathan Cameron wrote:
> > On Tue, 10 Jun 2025 09:34:37 +0200
> > Jorge Marques <jorge.marques@analog.com> wrote:
> >   
> >> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
> >> approximation register (SAR) analog-to-digital converter (ADC) that
> >> enables low-power, high-density data acquisition solutions without
> >> sacrificing precision. This ADC offers a unique balance of performance
> >> and power efficiency, plus innovative features for seamlessly switching
> >> between high-resolution and low-power modes tailored to the immediate
> >> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
> >> battery-powered, compact data acquisition and edge sensing applications.
> >>  
> 
> ...
> 
> >> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> >> +				   struct iio_chan_spec const *chan)
> >> +{
> >> +	struct ad4052_state *st = iio_priv(indio_dev);
> >> +	const struct iio_scan_type *scan_type;
> >> +	struct spi_transfer *xfer = &st->xfer;
> >> +
> >> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> >> +	if (IS_ERR(scan_type))
> >> +		return PTR_ERR(scan_type);
> >> +
> >> +	xfer->rx_buf = st->raw;
> >> +	xfer->bits_per_word = scan_type->realbits;
> >> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;  
> > 
> > This is a little odd. I'm not sure what happens with len not dividing
> > into a whole number of bits per word chunks.
> > Maybe a comment?  
> 
> Even better, there is now spi_bpw_to_bytes() for this.
> 
> >   
> >> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> >> +
> >> +	return 0;
> >> +}  
> > 
> >   
> 
> ...
> 
> >   
> >> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> >> +{
> >> +	struct spi_device *spi = st->spi;
> >> +	struct spi_transfer t_cnv = {};
> >> +	int ret;
> >> +
> >> +	reinit_completion(&st->completion);
> >> +
> >> +	if (st->cnv_gp) {
> >> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> >> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> >> +	} else {
> >> +		ret = spi_sync_transfer(spi, &t_cnv, 1);  
> > 
> > Add a comment for this.   I can't immediately spot documentation on what
> > a content free transfer actually does.  I assume pulses the chip select?
> > is that true for all SPI controllers?  
> 
> Should be. Setting .delay in the xfer would also make it more
> clear that this is doing.
> 
> >   
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +	/*
> >> +	 * Single sample read should be used only for oversampling and
> >> +	 * sampling frequency pairs that take less than 1 sec.
> >> +	 */
> >> +	if (st->gp1_irq) {
> >> +		ret = wait_for_completion_timeout(&st->completion,
> >> +						  msecs_to_jiffies(1000));
> >> +		if (!ret)
> >> +			return -ETIMEDOUT;
> >> +	}
> >> +
> >> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (st->xfer.len == 2)
> >> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
> >> +	else
> >> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
> >> +
> >> +	return ret;
> >> +}  
> >   
> 
> ...
> 
> >> +
> >> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> >> +				     unsigned int writeval, unsigned int *readval)
> >> +{
> >> +	struct ad4052_state *st = iio_priv(indio_dev);
> >> +	int ret;
> >> +
> >> +	if (!iio_device_claim_direct(indio_dev))  
> > 
> > For these guards in the debugfs callback, please add a comment on why they
> > are needed.   We've had a lot of questions about these recently and I'd
> > like it to be clear to people when they should cut and paste these and when
> > not.  
> 
> The reason I started doing this is that running the iio_info command attemps
> to read register 0x00 via the debug attribute of every single iio device. So
> if you run iio_info during a buffered read, and 0x00 is a valid register, it
> would break things without this check.
> 
> Ideally, general purpose commands wouldn't be poking debug registers, but
> that isn't the case. But I suppose we could "fix" iio_info instead.
> 

Please do fix iio_info.  It absolutely should not be poking the debug interfaces
except on specific debug calls.  The user has to know they may be shooting themselves
in the foot.

I'm not sure why a read of that register would break buffered capture though.
Is it a volatile register or is there a sequencing problem with multiple
accesses in this driver?  If it is multiple accesses then that should be
prevented via a local lock, not whether we are in buffer mode or not.

So I'm fine with this defense where it is necessary for all register
accesses, but I would like to see comments on why it is necessary.

Jonathan

> >   
> >> +		return -EBUSY;
> >> +
> >> +	if (readval)
> >> +		ret = regmap_read(st->regmap, reg, readval);
> >> +	else
> >> +		ret = regmap_write(st->regmap, reg, writeval);
> >> +	iio_device_release_direct(indio_dev);
> >> +	return ret;
> >> +}  
> >   

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[David Lechner]

On 6/21/25 11:08 AM, Jonathan Cameron wrote:
> On Mon, 16 Jun 2025 09:54:52 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 6/14/25 5:08 AM, Jonathan Cameron wrote:
>>> On Tue, 10 Jun 2025 09:34:37 +0200
>>> Jorge Marques <jorge.marques@analog.com> wrote:
>>>   
>>>> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
>>>> approximation register (SAR) analog-to-digital converter (ADC) that
>>>> enables low-power, high-density data acquisition solutions without
>>>> sacrificing precision. This ADC offers a unique balance of performance
>>>> and power efficiency, plus innovative features for seamlessly switching
>>>> between high-resolution and low-power modes tailored to the immediate
>>>> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
>>>> battery-powered, compact data acquisition and edge sensing applications.
>>>>  
>>
>> ...
>>
>>>> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
>>>> +				   struct iio_chan_spec const *chan)
>>>> +{
>>>> +	struct ad4052_state *st = iio_priv(indio_dev);
>>>> +	const struct iio_scan_type *scan_type;
>>>> +	struct spi_transfer *xfer = &st->xfer;
>>>> +
>>>> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
>>>> +	if (IS_ERR(scan_type))
>>>> +		return PTR_ERR(scan_type);
>>>> +
>>>> +	xfer->rx_buf = st->raw;
>>>> +	xfer->bits_per_word = scan_type->realbits;
>>>> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;  
>>>
>>> This is a little odd. I'm not sure what happens with len not dividing
>>> into a whole number of bits per word chunks.
>>> Maybe a comment?  
>>
>> Even better, there is now spi_bpw_to_bytes() for this.
>>
>>>   
>>>> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
>>>> +
>>>> +	return 0;
>>>> +}  
>>>
>>>   
>>
>> ...
>>
>>>   
>>>> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
>>>> +{
>>>> +	struct spi_device *spi = st->spi;
>>>> +	struct spi_transfer t_cnv = {};
>>>> +	int ret;
>>>> +
>>>> +	reinit_completion(&st->completion);
>>>> +
>>>> +	if (st->cnv_gp) {
>>>> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
>>>> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
>>>> +	} else {
>>>> +		ret = spi_sync_transfer(spi, &t_cnv, 1);  
>>>
>>> Add a comment for this.   I can't immediately spot documentation on what
>>> a content free transfer actually does.  I assume pulses the chip select?
>>> is that true for all SPI controllers?  
>>
>> Should be. Setting .delay in the xfer would also make it more
>> clear that this is doing.
>>
>>>   
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +	/*
>>>> +	 * Single sample read should be used only for oversampling and
>>>> +	 * sampling frequency pairs that take less than 1 sec.
>>>> +	 */
>>>> +	if (st->gp1_irq) {
>>>> +		ret = wait_for_completion_timeout(&st->completion,
>>>> +						  msecs_to_jiffies(1000));
>>>> +		if (!ret)
>>>> +			return -ETIMEDOUT;
>>>> +	}
>>>> +
>>>> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (st->xfer.len == 2)
>>>> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
>>>> +	else
>>>> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
>>>> +
>>>> +	return ret;
>>>> +}  
>>>   
>>
>> ...
>>
>>>> +
>>>> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>>>> +				     unsigned int writeval, unsigned int *readval)
>>>> +{
>>>> +	struct ad4052_state *st = iio_priv(indio_dev);
>>>> +	int ret;
>>>> +
>>>> +	if (!iio_device_claim_direct(indio_dev))  
>>>
>>> For these guards in the debugfs callback, please add a comment on why they
>>> are needed.   We've had a lot of questions about these recently and I'd
>>> like it to be clear to people when they should cut and paste these and when
>>> not.  
>>
>> The reason I started doing this is that running the iio_info command attemps
>> to read register 0x00 via the debug attribute of every single iio device. So
>> if you run iio_info during a buffered read, and 0x00 is a valid register, it
>> would break things without this check.
>>
>> Ideally, general purpose commands wouldn't be poking debug registers, but
>> that isn't the case. But I suppose we could "fix" iio_info instead.
>>
> 
> Please do fix iio_info.  It absolutely should not be poking the debug interfaces
> except on specific debug calls.  The user has to know they may be shooting themselves
> in the foot.
> 
> I'm not sure why a read of that register would break buffered capture though.
> Is it a volatile register or is there a sequencing problem with multiple
> accesses in this driver?  If it is multiple accesses then that should be
> prevented via a local lock, not whether we are in buffer mode or not.

IIRC, this was particularly a problem on chips that have a separate data
capture mode and reading a register exits data capture mode.

> 
> So I'm fine with this defense where it is necessary for all register
> accesses, but I would like to see comments on why it is necessary.
> 
> Jonathan
> 
>>>   
>>>> +		return -EBUSY;
>>>> +
>>>> +	if (readval)
>>>> +		ret = regmap_read(st->regmap, reg, readval);
>>>> +	else
>>>> +		ret = regmap_write(st->regmap, reg, writeval);
>>>> +	iio_device_release_direct(indio_dev);
>>>> +	return ret;
>>>> +}  
>>>   
> 

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Jonathan Cameron]

On Sat, 21 Jun 2025 11:13:58 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/21/25 11:08 AM, Jonathan Cameron wrote:
> > On Mon, 16 Jun 2025 09:54:52 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 6/14/25 5:08 AM, Jonathan Cameron wrote:  
> >>> On Tue, 10 Jun 2025 09:34:37 +0200
> >>> Jorge Marques <jorge.marques@analog.com> wrote:
> >>>     
> >>>> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
> >>>> approximation register (SAR) analog-to-digital converter (ADC) that
> >>>> enables low-power, high-density data acquisition solutions without
> >>>> sacrificing precision. This ADC offers a unique balance of performance
> >>>> and power efficiency, plus innovative features for seamlessly switching
> >>>> between high-resolution and low-power modes tailored to the immediate
> >>>> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
> >>>> battery-powered, compact data acquisition and edge sensing applications.
> >>>>    
> >>
> >> ...
> >>  
> >>>> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> >>>> +				   struct iio_chan_spec const *chan)
> >>>> +{
> >>>> +	struct ad4052_state *st = iio_priv(indio_dev);
> >>>> +	const struct iio_scan_type *scan_type;
> >>>> +	struct spi_transfer *xfer = &st->xfer;
> >>>> +
> >>>> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> >>>> +	if (IS_ERR(scan_type))
> >>>> +		return PTR_ERR(scan_type);
> >>>> +
> >>>> +	xfer->rx_buf = st->raw;
> >>>> +	xfer->bits_per_word = scan_type->realbits;
> >>>> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;    
> >>>
> >>> This is a little odd. I'm not sure what happens with len not dividing
> >>> into a whole number of bits per word chunks.
> >>> Maybe a comment?    
> >>
> >> Even better, there is now spi_bpw_to_bytes() for this.
> >>  
> >>>     
> >>>> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> >>>> +
> >>>> +	return 0;
> >>>> +}    
> >>>
> >>>     
> >>
> >> ...
> >>  
> >>>     
> >>>> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> >>>> +{
> >>>> +	struct spi_device *spi = st->spi;
> >>>> +	struct spi_transfer t_cnv = {};
> >>>> +	int ret;
> >>>> +
> >>>> +	reinit_completion(&st->completion);
> >>>> +
> >>>> +	if (st->cnv_gp) {
> >>>> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> >>>> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> >>>> +	} else {
> >>>> +		ret = spi_sync_transfer(spi, &t_cnv, 1);    
> >>>
> >>> Add a comment for this.   I can't immediately spot documentation on what
> >>> a content free transfer actually does.  I assume pulses the chip select?
> >>> is that true for all SPI controllers?    
> >>
> >> Should be. Setting .delay in the xfer would also make it more
> >> clear that this is doing.
> >>  
> >>>     
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +	/*
> >>>> +	 * Single sample read should be used only for oversampling and
> >>>> +	 * sampling frequency pairs that take less than 1 sec.
> >>>> +	 */
> >>>> +	if (st->gp1_irq) {
> >>>> +		ret = wait_for_completion_timeout(&st->completion,
> >>>> +						  msecs_to_jiffies(1000));
> >>>> +		if (!ret)
> >>>> +			return -ETIMEDOUT;
> >>>> +	}
> >>>> +
> >>>> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	if (st->xfer.len == 2)
> >>>> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
> >>>> +	else
> >>>> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
> >>>> +
> >>>> +	return ret;
> >>>> +}    
> >>>     
> >>
> >> ...
> >>  
> >>>> +
> >>>> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> >>>> +				     unsigned int writeval, unsigned int *readval)
> >>>> +{
> >>>> +	struct ad4052_state *st = iio_priv(indio_dev);
> >>>> +	int ret;
> >>>> +
> >>>> +	if (!iio_device_claim_direct(indio_dev))    
> >>>
> >>> For these guards in the debugfs callback, please add a comment on why they
> >>> are needed.   We've had a lot of questions about these recently and I'd
> >>> like it to be clear to people when they should cut and paste these and when
> >>> not.    
> >>
> >> The reason I started doing this is that running the iio_info command attemps
> >> to read register 0x00 via the debug attribute of every single iio device. So
> >> if you run iio_info during a buffered read, and 0x00 is a valid register, it
> >> would break things without this check.
> >>
> >> Ideally, general purpose commands wouldn't be poking debug registers, but
> >> that isn't the case. But I suppose we could "fix" iio_info instead.
> >>  
> > 
> > Please do fix iio_info.  It absolutely should not be poking the debug interfaces
> > except on specific debug calls.  The user has to know they may be shooting themselves
> > in the foot.
> > 
> > I'm not sure why a read of that register would break buffered capture though.
> > Is it a volatile register or is there a sequencing problem with multiple
> > accesses in this driver?  If it is multiple accesses then that should be
> > prevented via a local lock, not whether we are in buffer mode or not.  
> 
> IIRC, this was particularly a problem on chips that have a separate data
> capture mode and reading a register exits data capture mode.

Those ones I'm fine with just having a comment that hopefully means it
doesn't get cut and paste somewhere inappropriate!

Jonathan

> 
> > 
> > So I'm fine with this defense where it is necessary for all register
> > accesses, but I would like to see comments on why it is necessary.
> > 
> > Jonathan
> >   
> >>>     
> >>>> +		return -EBUSY;
> >>>> +
> >>>> +	if (readval)
> >>>> +		ret = regmap_read(st->regmap, reg, readval);
> >>>> +	else
> >>>> +		ret = regmap_write(st->regmap, reg, writeval);
> >>>> +	iio_device_release_direct(indio_dev);
> >>>> +	return ret;
> >>>> +}    
> >>>     
> >   
> 

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Uwe Kleine-König]

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

On Tue, Jun 10, 2025 at 09:34:37AM +0200, Jorge Marques wrote:
> +static int ad4052_get_samp_freq(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val,
> +				int *val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +
> +	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
> +	return IIO_VAL_INT;

st->pwm_st.period is the period that was requested before. If you want
the real period that is currently emitted, check pwm_get_state_hw().

> +}

Best regards
Uwe

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

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Jorge Marques]

On Tue, Jun 17, 2025 at 04:59:48PM +0200, Uwe Kleine-König wrote:
> On Tue, Jun 10, 2025 at 09:34:37AM +0200, Jorge Marques wrote:
> > +static int ad4052_get_samp_freq(struct iio_dev *indio_dev,
> > +				struct iio_chan_spec const *chan,
> > +				int *val,
> > +				int *val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +
> > +	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
> > +	return IIO_VAL_INT;
> 
> st->pwm_st.period is the period that was requested before. If you want
> the real period that is currently emitted, check pwm_get_state_hw().
> 
Hi Uwe,

I believe only ad4695.c uses this method and the reason for that is if
the pwm is disabled we still want to obtain the requested value.

Reverting slightly to v2, the semantic to allow fetching from hw when
enabled, and using the managed state when disabled, would be:

	struct pwm_state pwm_st;
	int ret

	ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
	if (ret)
		goto out_release;

	if (!pwm_st.enabled)
		pwm_st = st->pwm_st;

	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);

Is this ok?

Best regards,
Jorge
> > +}
> 
> Best regards
> Uwe

Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Uwe Kleine-König]

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

Hello,

On Tue, Jun 17, 2025 at 05:34:56PM +0200, Jorge Marques wrote:
> On Tue, Jun 17, 2025 at 04:59:48PM +0200, Uwe Kleine-König wrote:
> > On Tue, Jun 10, 2025 at 09:34:37AM +0200, Jorge Marques wrote:
> > > +static int ad4052_get_samp_freq(struct iio_dev *indio_dev,
> > > +				struct iio_chan_spec const *chan,
> > > +				int *val,
> > > +				int *val2)
> > > +{
> > > +	struct ad4052_state *st = iio_priv(indio_dev);
> > > +
> > > +	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
> > > +	return IIO_VAL_INT;
> > 
> > st->pwm_st.period is the period that was requested before. If you want
> > the real period that is currently emitted, check pwm_get_state_hw().
> 
> I believe only ad4695.c uses this method and the reason for that is if
> the pwm is disabled we still want to obtain the requested value.
> 
> Reverting slightly to v2, the semantic to allow fetching from hw when
> enabled, and using the managed state when disabled, would be:
> 
> 	struct pwm_state pwm_st;
> 	int ret
> 
> 	ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> 	if (ret)
> 		goto out_release;
> 
> 	if (!pwm_st.enabled)
> 		pwm_st = st->pwm_st;
> 
> 	*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> 
> Is this ok?

Looks fine to me. I didn't object the original suggested code, just
wanted to highlight the semantics.

I would expect that the compiler optimizes out the unnecessary
assignments done in pwm_st = st->pwm_st, as only pwm_st.period is used
later on.

Best regards
Uwe

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

[PATCH v3 5/8] docs: iio: ad4052: Add offload support documentation

[Jorge Marques]

Explain the AD4052 support for the SPI Engine Offload.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 Documentation/iio/ad4052.rst | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
index 25e55eb72e167bd2b2195ba789b45ce402869b0f..2ad1c7712766118b03c48929d1f2b7670d5f8d41 100644
--- a/Documentation/iio/ad4052.rst
+++ b/Documentation/iio/ad4052.rst
@@ -49,6 +49,12 @@ The ADC contains only one channel with following attributes:
    * - ``in_voltage_oversampling_ratio_available``
      - List of available oversampling values. Value 1 disable the burst
        averaging mode.
+   * - ``in_voltage_sampling_frequency``
+     - Set the sampling frequency, only present if SPI Offload is being used.
+
+If `gp1` interrupt is not set, the driver will not wait for the data ready
+assertion, and will result in reading a sample before it's ready, particularly
+in oversampling mode.
 
 Also contain the following device attributes:
 
@@ -69,3 +75,21 @@ Low-power mode
 The device enters low-power mode on idle to save power. Enabling an event puts
 the device out of the low-power since the ADC autonomously samples to assert
 the event condition.
+
+SPI offload support
+===================
+
+To be able to achieve the maximum sample rate, the driver can be used with the
+`AXI SPI Engine`_ to provide SPI offload support.
+
+.. _AXI SPI Engine: http://analogdevicesinc.github.io/hdl/projects/ad4052_ardz/index.html
+
+When SPI offload is being used, additional attributes are present:
+
+.. list-table:: Additional attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``in_voltage_sampling_frequency``
+     - Set the sampling frequency.

-- 
2.49.0

[PATCH v3 6/8] iio: adc: Add offload support for ad4052

[Jorge Marques]

Support SPI offload with appropriate FPGA firmware. Since the SPI-Engine
offload module always sends 32-bit data to the DMA engine, the
scantype.storagebytes is set to 32-bit and the SPI transfer length is
based on the scantype.realbits. This combination allows to optimize the
SPI to transfer only 2 or 3 bytes (depending on the granularity and
mode), while the number of samples are computed correctly by tools on
top of the iio scantype.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 drivers/iio/adc/ad4052.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 242 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
index 842f5972a1c58701addf5243e7b87da9c26c773f..7d32dc4701ddb0204b5505a650ce7caafc2cb5ed 100644
--- a/drivers/iio/adc/ad4052.c
+++ b/drivers/iio/adc/ad4052.c
@@ -11,6 +11,8 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/buffer-dmaengine.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/interrupt.h>
@@ -23,6 +25,8 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/spi/offload/consumer.h>
+#include <linux/spi/offload/provider.h>
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/units.h>
@@ -111,6 +115,7 @@ enum ad4052_interrupt_en {
 
 struct ad4052_chip_info {
 	const struct iio_chan_spec channels[1];
+	const struct iio_chan_spec offload_channels[1];
 	const char *name;
 	u16 prod_id;
 	u8 max_avg;
@@ -156,7 +161,11 @@ struct ad4052_state {
 	const struct ad4052_bus_ops *ops;
 	const struct ad4052_chip_info *chip;
 	enum ad4052_operation_mode mode;
+	struct spi_offload *offload;
+	struct spi_offload_trigger *offload_trigger;
 	struct spi_device *spi;
+	struct spi_transfer offload_xfer;
+	struct spi_message offload_msg;
 	struct pwm_device *cnv_pwm;
 	struct pwm_state pwm_st;
 	struct spi_transfer xfer;
@@ -344,6 +353,7 @@ AD4052_EXT_INFO(AD4052_500KSPS);
 static const struct ad4052_chip_info ad4050_chip_info = {
 	.name = "ad4050",
 	.channels = { AD4052_CHAN(12, AD4052_2MSPS) },
+	.offload_channels = { AD4052_OFFLOAD_CHAN(12, AD4052_2MSPS) },
 	.prod_id = 0x70,
 	.max_avg = AD4050_MAX_AVG,
 	.grade = AD4052_2MSPS,
@@ -352,6 +362,7 @@ static const struct ad4052_chip_info ad4050_chip_info = {
 static const struct ad4052_chip_info ad4052_chip_info = {
 	.name = "ad4052",
 	.channels = { AD4052_CHAN(16, AD4052_2MSPS) },
+	.offload_channels = { AD4052_OFFLOAD_CHAN(16, AD4052_2MSPS) },
 	.prod_id = 0x72,
 	.max_avg = AD4052_MAX_AVG,
 	.grade = AD4052_2MSPS,
@@ -360,6 +371,7 @@ static const struct ad4052_chip_info ad4052_chip_info = {
 static const struct ad4052_chip_info ad4056_chip_info = {
 	.name = "ad4056",
 	.channels = { AD4052_CHAN(12, AD4052_500KSPS) },
+	.offload_channels = { AD4052_OFFLOAD_CHAN(12, AD4052_500KSPS) },
 	.prod_id = 0x76,
 	.max_avg = AD4050_MAX_AVG,
 	.grade = AD4052_500KSPS,
@@ -368,6 +380,7 @@ static const struct ad4052_chip_info ad4056_chip_info = {
 static const struct ad4052_chip_info ad4058_chip_info = {
 	.name = "ad4058",
 	.channels = { AD4052_CHAN(16, AD4052_500KSPS) },
+	.offload_channels = { AD4052_OFFLOAD_CHAN(16, AD4052_500KSPS) },
 	.prod_id = 0x78,
 	.max_avg = AD4052_MAX_AVG,
 	.grade = AD4052_500KSPS,
@@ -392,6 +405,28 @@ static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+	struct spi_transfer *xfer = &st->offload_xfer;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	xfer->bits_per_word = scan_type->realbits;
+	xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+	xfer->len = scan_type->realbits == 24 ? 4 : 2;
+	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
+
+	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
+	st->offload_msg.offload = st->offload;
+
+	return 0;
+}
+
 static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
 					 const struct iio_chan_spec *chan,
 					 unsigned int val)
@@ -838,6 +873,87 @@ static int ad4052_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int ad4052_offload_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct spi_offload_trigger_config config = {
+		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
+	};
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&st->spi->dev);
+	if (ret)
+		return ret;
+
+	ret = ad4052_set_operation_mode(st, st->mode);
+	if (ret)
+		goto out_mode_error;
+
+	ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
+	if (ret)
+		goto out_xfer_error;
+
+	ret = spi_optimize_message(st->spi, &st->offload_msg);
+	if (ret)
+		goto out_xfer_error;
+
+	/* SPI Offload handles the data ready irq */
+	if (st->gp1_irq)
+		disable_irq(st->gp1_irq);
+
+	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
+					 &config);
+	if (ret)
+		goto out_offload_error;
+
+	st->pwm_st.enabled = true;
+	ret = pwm_apply_might_sleep(st->cnv_pwm, &st->pwm_st);
+	if (ret)
+		goto out_pwm_error;
+
+	return 0;
+
+out_pwm_error:
+	spi_offload_trigger_disable(st->offload, st->offload_trigger);
+out_offload_error:
+	if (st->gp1_irq)
+		enable_irq(st->gp1_irq);
+	spi_unoptimize_message(&st->offload_msg);
+out_xfer_error:
+	ad4052_exit_command(st);
+out_mode_error:
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+
+	return ret;
+}
+
+static int ad4052_offload_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	st->pwm_st.enabled = false;
+	pwm_apply_might_sleep(st->cnv_pwm, &st->pwm_st);
+
+	spi_offload_trigger_disable(st->offload, st->offload_trigger);
+	spi_unoptimize_message(&st->offload_msg);
+
+	ret = ad4052_exit_command(st);
+
+	if (st->gp1_irq)
+		enable_irq(st->gp1_irq);
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops ad4052_buffer_offload_setup_ops = {
+	.postenable = &ad4052_offload_buffer_postenable,
+	.predisable = &ad4052_offload_buffer_predisable,
+};
+
 static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 				     unsigned int writeval, unsigned int *readval)
 {
@@ -884,9 +1000,82 @@ static const struct regmap_config ad4052_regmap_config = {
 	.can_sleep = true,
 };
 
+static const struct spi_offload_config ad4052_offload_config = {
+	.capability_flags = SPI_OFFLOAD_CAP_TRIGGER |
+			    SPI_OFFLOAD_CAP_RX_STREAM_DMA,
+};
+
+static void ad4052_pwm_disable(void *pwm)
+{
+	pwm_disable(pwm);
+}
+
+static bool ad4052_offload_trigger_match(struct spi_offload_trigger *trigger,
+					 enum spi_offload_trigger_type type,
+					 u64 *args, u32 nargs)
+{
+	return type == SPI_OFFLOAD_TRIGGER_DATA_READY;
+}
+
+static const struct spi_offload_trigger_ops ad4052_offload_trigger_ops = {
+	.match = ad4052_offload_trigger_match,
+};
+
+static int ad4052_request_offload(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	struct dma_chan *rx_dma;
+	struct spi_offload_trigger_info trigger_info = {
+		.fwnode = dev_fwnode(dev),
+		.ops = &ad4052_offload_trigger_ops,
+		.priv = st,
+	};
+	int ret;
+
+	indio_dev->setup_ops = &ad4052_buffer_offload_setup_ops;
+
+	ret = devm_spi_offload_trigger_register(dev, &trigger_info);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register offload trigger\n");
+
+	st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload,
+							   SPI_OFFLOAD_TRIGGER_DATA_READY);
+	if (IS_ERR(st->offload_trigger))
+		return PTR_ERR(st->offload_trigger);
+
+	st->cnv_pwm = devm_pwm_get(dev, NULL);
+	if (IS_ERR(st->cnv_pwm))
+		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm), "failed to get CNV PWM\n");
+
+	pwm_init_state(st->cnv_pwm, &st->pwm_st);
+
+	st->pwm_st.enabled = false;
+	st->pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
+	st->pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, AD4052_MAX_RATE(st->chip->grade));
+
+	ret = pwm_apply_might_sleep(st->cnv_pwm, &st->pwm_st);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to apply CNV PWM\n");
+
+	ret = devm_add_action_or_reset(dev, ad4052_pwm_disable, st->cnv_pwm);
+	if (ret)
+		return ret;
+
+	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload);
+	if (IS_ERR(rx_dma))
+		return PTR_ERR(rx_dma);
+
+	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
+							   IIO_BUFFER_DIRECTION_IN);
+}
+
 static int __ad4052_validate_trigger_sources(struct of_phandle_args *trigger_sources)
 {
 	switch (trigger_sources->args[1]) {
+	case AD4052_TRIGGER_PIN_GP0:
+		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_EITHER_THRESH ?
+		       0 : -EINVAL;
 	case AD4052_TRIGGER_PIN_GP1:
 		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_DATA_READY ?
 		       0 : -EINVAL;
@@ -903,14 +1092,45 @@ static int ad4052_validate_trigger_sources(struct iio_dev *indio_dev)
 	int ret;
 
 	np = st->spi->dev.of_node;
+	for (u8 i = 0; i < 2; i++) {
+		ret = of_parse_phandle_with_args(np, "trigger-sources",
+						 "#trigger-source-cells", i,
+						 &trigger_sources);
+		if (ret)
+			return ret;
+
+		ret = __ad4052_validate_trigger_sources(&trigger_sources);
+		of_node_put(trigger_sources.np);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
+static int ad4052_validate_parent_trigger_sources(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct of_phandle_args trigger_sources;
+	struct device_node *np;
+	int ret;
+
+	np = of_get_parent(st->spi->dev.of_node);
+	if (!np)
+		return -ENODEV;
+
 	ret = of_parse_phandle_with_args(np, "trigger-sources",
 					 "#trigger-source-cells", 0,
 					 &trigger_sources);
 	if (ret)
-		return ret;
+		goto out_error;
 
-	ret = __ad4052_validate_trigger_sources(&trigger_sources);
+	if (trigger_sources.args[0] != AD4052_TRIGGER_EVENT_DATA_READY ||
+	    trigger_sources.args[1] != AD4052_TRIGGER_PIN_GP1)
+		ret = -EINVAL;
 	of_node_put(trigger_sources.np);
+out_error:
+	of_node_put(np);
 	return ret;
 }
 
@@ -986,6 +1206,26 @@ static int ad4052_probe(struct spi_device *spi)
 	indio_dev->info = &ad4052_info;
 	indio_dev->name = chip->name;
 
+	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
+	ret = PTR_ERR_OR_ZERO(st->offload);
+
+	if (ret == -ENODEV) {
+		st->offload_trigger = NULL;
+		indio_dev->channels = chip->channels;
+	} else if (!ret) {
+		indio_dev->channels = chip->offload_channels;
+		ret = ad4052_request_offload(indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to configure offload\n");
+		ret = ad4052_validate_parent_trigger_sources(indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to validate parent trigger sources\n");
+	} else {
+		return dev_err_probe(dev, ret, "Failed to get offload\n");
+	}
+
 	ret = ad4052_validate_trigger_sources(indio_dev);
 	if (ret)
 		return dev_err_probe(dev, ret,

-- 
2.49.0

Re: [PATCH v3 6/8] iio: adc: Add offload support for ad4052

[Jonathan Cameron]

On Tue, 10 Jun 2025 09:34:39 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> Support SPI offload with appropriate FPGA firmware. Since the SPI-Engine
> offload module always sends 32-bit data to the DMA engine, the
> scantype.storagebytes is set to 32-bit and the SPI transfer length is
> based on the scantype.realbits. This combination allows to optimize the
> SPI to transfer only 2 or 3 bytes (depending on the granularity and
> mode), while the number of samples are computed correctly by tools on
> top of the iio scantype.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Minor comments inline.  I think they are all follow up from comments on
earlier patches that apply here as well.

> ---
>  drivers/iio/adc/ad4052.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 242 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> index 842f5972a1c58701addf5243e7b87da9c26c773f..7d32dc4701ddb0204b5505a650ce7caafc2cb5ed 100644
> --- a/drivers/iio/adc/ad4052.c
> +++ b/drivers/iio/adc/ad4052.c
> @@ -11,6 +11,8 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/buffer-dmaengine.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/interrupt.h>
> @@ -23,6 +25,8 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> +#include <linux/spi/offload/consumer.h>
> +#include <linux/spi/offload/provider.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/units.h>
> @@ -111,6 +115,7 @@ enum ad4052_interrupt_en {
>  
>  struct ad4052_chip_info {
>  	const struct iio_chan_spec channels[1];
> +	const struct iio_chan_spec offload_channels[1];

If there is only ever one of these drop the array.


>  
> +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	struct spi_transfer *xfer = &st->offload_xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;

Same question on length vs bits_per_word applies here as in the earlier
patch.

> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> +
> +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +	st->offload_msg.offload = st->offload;
> +
> +	return 0;
> +}
> +
>  static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
>  					 const struct iio_chan_spec *chan,
>  					 unsigned int val)
> @@ -838,6 +873,87 @@ static int ad4052_write_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }

>  static int __ad4052_validate_trigger_sources(struct of_phandle_args *trigger_sources)
>  {
>  	switch (trigger_sources->args[1]) {
> +	case AD4052_TRIGGER_PIN_GP0:
> +		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_EITHER_THRESH ?
> +		       0 : -EINVAL;
>  	case AD4052_TRIGGER_PIN_GP1:
>  		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_DATA_READY ?
>  		       0 : -EINVAL;
> @@ -903,14 +1092,45 @@ static int ad4052_validate_trigger_sources(struct iio_dev *indio_dev)
>  	int ret;
>  
>  	np = st->spi->dev.of_node;
> +	for (u8 i = 0; i < 2; i++) {
> +		ret = of_parse_phandle_with_args(np, "trigger-sources",
> +						 "#trigger-source-cells", i,
> +						 &trigger_sources);
> +		if (ret)
> +			return ret;
> +
> +		ret = __ad4052_validate_trigger_sources(&trigger_sources);
> +		of_node_put(trigger_sources.np);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return ret;

I think this is always 0. So return 0; preferred to make that explicit.

> +}
> 

Re: [PATCH v3 6/8] iio: adc: Add offload support for ad4052

[Jorge Marques]

On Sat, Jun 14, 2025 at 11:20:22AM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:39 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
> > Support SPI offload with appropriate FPGA firmware. Since the SPI-Engine
> > offload module always sends 32-bit data to the DMA engine, the
> > scantype.storagebytes is set to 32-bit and the SPI transfer length is
> > based on the scantype.realbits. This combination allows to optimize the
> > SPI to transfer only 2 or 3 bytes (depending on the granularity and
> > mode), while the number of samples are computed correctly by tools on
> > top of the iio scantype.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> Minor comments inline.  I think they are all follow up from comments on
> earlier patches that apply here as well.
> 
> > ---
> >  drivers/iio/adc/ad4052.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 242 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> > index 842f5972a1c58701addf5243e7b87da9c26c773f..7d32dc4701ddb0204b5505a650ce7caafc2cb5ed 100644
> > --- a/drivers/iio/adc/ad4052.c
> > +++ b/drivers/iio/adc/ad4052.c
> > @@ -11,6 +11,8 @@
> >  #include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer-dmaengine.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/interrupt.h>
> > @@ -23,6 +25,8 @@
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> > +#include <linux/spi/offload/consumer.h>
> > +#include <linux/spi/offload/provider.h>
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> >  #include <linux/units.h>
> > @@ -111,6 +115,7 @@ enum ad4052_interrupt_en {
> >  
> >  struct ad4052_chip_info {
> >  	const struct iio_chan_spec channels[1];
> > +	const struct iio_chan_spec offload_channels[1];
> 
> If there is only ever one of these drop the array.
> 
Hi Jonathan,

It is hard to predict if no other similar device will have only two
channels. But I would say most drivers end-up having more channels.
> 
> >  
> > +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> > +				      struct iio_chan_spec const *chan)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +	struct spi_transfer *xfer = &st->offload_xfer;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> > +	xfer->len = scan_type->realbits == 24 ? 4 : 2;
> 
> Same question on length vs bits_per_word applies here as in the earlier
> patch.
> 
To be able to optimize the SPI message, len must be a multiple of 16
bits. To achieve maximum throughput, no extra bits (and therefore SCLK
clock cycles) must be transferred during the SPI transfer. This is set
by bits_per_word, 24-bits means 24 SCLK.

Finally, storagebits is the number of bits actually used to store the
reading, and for the offload channel is the DMA width, always 32-bits.
An abstraction to obtain the DMA width should be created, so the 32-bits
value is not hard-coded into the driver, still, for this series, it is.

> > +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> > +
> > +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> > +	st->offload_msg.offload = st->offload;
> > +
> > +	return 0;
> > +}
> > +
> >  static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
> >  					 const struct iio_chan_spec *chan,
> >  					 unsigned int val)
> > @@ -838,6 +873,87 @@ static int ad4052_write_raw(struct iio_dev *indio_dev,
> >  	return ret;
> >  }
> 
> >  static int __ad4052_validate_trigger_sources(struct of_phandle_args *trigger_sources)
> >  {
> >  	switch (trigger_sources->args[1]) {
> > +	case AD4052_TRIGGER_PIN_GP0:
> > +		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_EITHER_THRESH ?
> > +		       0 : -EINVAL;
> >  	case AD4052_TRIGGER_PIN_GP1:
> >  		return trigger_sources->args[0] == AD4052_TRIGGER_EVENT_DATA_READY ?
> >  		       0 : -EINVAL;
> > @@ -903,14 +1092,45 @@ static int ad4052_validate_trigger_sources(struct iio_dev *indio_dev)
> >  	int ret;
> >  
> >  	np = st->spi->dev.of_node;
> > +	for (u8 i = 0; i < 2; i++) {
> > +		ret = of_parse_phandle_with_args(np, "trigger-sources",
> > +						 "#trigger-source-cells", i,
> > +						 &trigger_sources);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = __ad4052_validate_trigger_sources(&trigger_sources);
> > +		of_node_put(trigger_sources.np);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return ret;
> 
> I think this is always 0. So return 0; preferred to make that explicit.
> 
Well, this whole method is deleted for v4 due to the trigger-sources
discussion. Per following David suggestion, gp0 is assumed drdy and gp1
threshold events, unless the parent (spi offload) trigger-sources says
otherwise (gp1).

Best regards,
Jorge
> > +}
> > 
> 

Re: [PATCH v3 6/8] iio: adc: Add offload support for ad4052

[Jonathan Cameron]

On Fri, 20 Jun 2025 20:52:10 +0200
Jorge Marques <gastmaier@gmail.com> wrote:

> On Sat, Jun 14, 2025 at 11:20:22AM +0100, Jonathan Cameron wrote:
> > On Tue, 10 Jun 2025 09:34:39 +0200
> > Jorge Marques <jorge.marques@analog.com> wrote:
> >   
> > > Support SPI offload with appropriate FPGA firmware. Since the SPI-Engine
> > > offload module always sends 32-bit data to the DMA engine, the
> > > scantype.storagebytes is set to 32-bit and the SPI transfer length is
> > > based on the scantype.realbits. This combination allows to optimize the
> > > SPI to transfer only 2 or 3 bytes (depending on the granularity and
> > > mode), while the number of samples are computed correctly by tools on
> > > top of the iio scantype.
> > > 
> > > Signed-off-by: Jorge Marques <jorge.marques@analog.com>  
> > Minor comments inline.  I think they are all follow up from comments on
> > earlier patches that apply here as well.
> >   
> > > ---
> > >  drivers/iio/adc/ad4052.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 242 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> > > index 842f5972a1c58701addf5243e7b87da9c26c773f..7d32dc4701ddb0204b5505a650ce7caafc2cb5ed 100644
> > > --- a/drivers/iio/adc/ad4052.c
> > > +++ b/drivers/iio/adc/ad4052.c
> > > @@ -11,6 +11,8 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/err.h>
> > >  #include <linux/gpio/consumer.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/buffer-dmaengine.h>
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > >  #include <linux/interrupt.h>
> > > @@ -23,6 +25,8 @@
> > >  #include <linux/regmap.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/spi/spi.h>
> > > +#include <linux/spi/offload/consumer.h>
> > > +#include <linux/spi/offload/provider.h>
> > >  #include <linux/string.h>
> > >  #include <linux/types.h>
> > >  #include <linux/units.h>
> > > @@ -111,6 +115,7 @@ enum ad4052_interrupt_en {
> > >  
> > >  struct ad4052_chip_info {
> > >  	const struct iio_chan_spec channels[1];
> > > +	const struct iio_chan_spec offload_channels[1];  
> > 
> > If there is only ever one of these drop the array.
> >   
> Hi Jonathan,
> 
> It is hard to predict if no other similar device will have only two
> channels. But I would say most drivers end-up having more channels.

Ok. I don't mind that much, but it does feel like planning for a future
that might or might not come.   Easy enough to refactor later.

> >   
> > >  
> > > +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> > > +				      struct iio_chan_spec const *chan)
> > > +{
> > > +	struct ad4052_state *st = iio_priv(indio_dev);
> > > +	const struct iio_scan_type *scan_type;
> > > +	struct spi_transfer *xfer = &st->offload_xfer;
> > > +
> > > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > > +	if (IS_ERR(scan_type))
> > > +		return PTR_ERR(scan_type);
> > > +
> > > +	xfer->bits_per_word = scan_type->realbits;
> > > +	xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> > > +	xfer->len = scan_type->realbits == 24 ? 4 : 2;  
> > 
> > Same question on length vs bits_per_word applies here as in the earlier
> > patch.
> >   
> To be able to optimize the SPI message, len must be a multiple of 16
> bits. To achieve maximum throughput, no extra bits (and therefore SCLK
> clock cycles) must be transferred during the SPI transfer. This is set
> by bits_per_word, 24-bits means 24 SCLK.

I got that intention, what I wasn't sure on was what the spi subsystem
would do with this case.

I checked the docs and this case is called out though only in the
spi_device docs for bits_per_word (not mentioned in the spi_transfer
docs) so fair enough.  Just seemed strange!
 
> 
> Finally, storagebits is the number of bits actually used to store the
> reading, and for the offload channel is the DMA width, always 32-bits.
> An abstraction to obtain the DMA width should be created, so the 32-bits
> value is not hard-coded into the driver, still, for this series, it is.
> 
Thanks

Jonathan

[PATCH v3 7/8] docs: iio: ad4052: Add event documentation

[Jorge Marques]

Explain the AD4052 monitoring support by exposing as an IIO event.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 Documentation/iio/ad4052.rst | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
index 2ad1c7712766118b03c48929d1f2b7670d5f8d41..da9c0481dfcd3b4eecd91280b7f259aa9118467c 100644
--- a/Documentation/iio/ad4052.rst
+++ b/Documentation/iio/ad4052.rst
@@ -93,3 +93,41 @@ When SPI offload is being used, additional attributes are present:
      - Description
    * - ``in_voltage_sampling_frequency``
      - Set the sampling frequency.
+
+Threshold events
+================
+
+The ADC supports a monitoring mode to raise threshold events. The driver
+supports a single interrupt for both rising and falling readings.
+
+The feature is enabled/disabled by setting ``thresh_either_en``. During monitor
+mode, the device continuously operates in autonomous mode until put back in
+configuration mode, due to this, the device returns busy until the feature is
+disabled.
+
+The following event attributes are available:
+
+.. list-table:: Event attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``sampling_frequency``
+     - Frequency used in the monitoring mode, sets the device internal sample
+       rate when the mode is activated.
+   * - ``sampling_frequency_available``
+     - List of available sample rates.
+   * - ``thresh_either_en``
+     - Enable monitoring mode.
+   * - ``thresh_falling_hysteresis``
+     - Set the hysteresis value for the minimum threshold.
+   * - ``thresh_falling_value``
+     - Set the minimum threshold value.
+   * - ``thresh_rising_hysteresis``
+     - Set the hysteresis value for the maximum threshold.
+   * - ``thresh_rising_value``
+     - Set the maximum threshold value.
+
+If the interrupt ``gp0`` is not provided, the IIO events attributes are still
+present, but no IIO Event will ever be triggered. This allows to use ``gp0``
+output to trigger other resource.

-- 
2.49.0

[PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jorge Marques]

The AD4052 family supports autonomous monitoring readings for threshold
crossings. Add support for catching the GPIO interrupt and expose as an IIO
event. The device allows to set either, rising and falling directions. Only
either threshold crossing is implemented.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 drivers/iio/adc/ad4052.c | 369 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 369 insertions(+)

diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
index 7d32dc4701ddb0204b5505a650ce7caafc2cb5ed..ff52ff002bfe0ee413ae352b0c1854798b8e89f8 100644
--- a/drivers/iio/adc/ad4052.c
+++ b/drivers/iio/adc/ad4052.c
@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer-dmaengine.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/interrupt.h>
@@ -69,6 +70,8 @@
 #define AD4052_REG_MON_VAL				0x2F
 #define AD4052_REG_FUSE_CRC				0x40
 #define AD4052_REG_DEVICE_STATUS			0x41
+#define     AD4052_REG_DEVICE_STATUS_MIN_FLAG		BIT(2)
+#define     AD4052_REG_DEVICE_STATUS_MAX_FLAG		BIT(3)
 #define     AD4052_REG_DEVICE_STATUS_DEVICE_RDY		BIT(7)
 #define     AD4052_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
 #define AD4052_REG_MIN_SAMPLE				0x45
@@ -173,6 +176,8 @@ struct ad4052_state {
 	struct completion completion;
 	struct regmap *regmap;
 	u16 oversampling_frequency;
+	u16 events_frequency;
+	bool wait_event;
 	int gp1_irq;
 	int vio_uv;
 	int vref_uv;
@@ -259,6 +264,26 @@ static const struct regmap_access_table ad4052_regmap_wr_table = {
 	.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_wr_ranges),
 };
 
+static const struct iio_event_spec ad4052_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+				      BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+				      BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+};
+
 static const char *const ad4052_conversion_freqs[] = {
 	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
 	"33300", "10000", "3000", "500",		/*  4 -  7 */
@@ -328,6 +353,8 @@ AD4052_EXT_INFO(AD4052_500KSPS);
 	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 	.indexed = 1,									\
 	.channel = 0,									\
+	.event_spec = ad4052_events,							\
+	.num_event_specs = ARRAY_SIZE(ad4052_events),					\
 	.has_ext_scan_type = 1,								\
 	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
 	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
@@ -344,6 +371,8 @@ AD4052_EXT_INFO(AD4052_500KSPS);
 	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 	.indexed = 1,									\
 	.channel = 0,									\
+	.event_spec = ad4052_events,							\
+	.num_event_specs = ARRAY_SIZE(ad4052_events),					\
 	.has_ext_scan_type = 1,								\
 	.ext_scan_type = ad4052_scan_type_##bits##_s,					\
 	.num_ext_scan_type = ARRAY_SIZE(ad4052_scan_type_##bits##_s),			\
@@ -386,6 +415,74 @@ static const struct ad4052_chip_info ad4058_chip_info = {
 	.grade = AD4052_500KSPS,
 };
 
+static ssize_t ad4052_events_frequency_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
+
+	return sysfs_emit(buf, "%s\n", ad4052_conversion_freqs[st->events_frequency]);
+}
+
+static ssize_t ad4052_events_frequency_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf,
+					     size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
+				   AD4052_FS_LEN(st->chip->grade), buf);
+	if (ret < 0)
+		goto out_release;
+
+	st->events_frequency = ret;
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(sampling_frequency, 0644,
+		       ad4052_events_frequency_show,
+		       ad4052_events_frequency_store, 0);
+
+static ssize_t sampling_frequency_available_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
+	int ret = 0;
+
+	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
+	     i < AD4052_FS_LEN(st->chip->grade); i++)
+		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
+
+	ret += sysfs_emit_at(buf, ret, "\n");
+	return ret;
+}
+
+static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
+
+static struct attribute *ad4052_event_attributes[] = {
+	&iio_dev_attr_sampling_frequency.dev_attr.attr,
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ad4052_event_attribute_group = {
+	.attrs = ad4052_event_attributes,
+};
+
 static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
 				   struct iio_chan_spec const *chan)
 {
@@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
 				  val);
 }
 
+static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+
+	iio_push_event(indio_dev,
+		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
+					    IIO_EV_TYPE_THRESH,
+					    IIO_EV_DIR_EITHER),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
 {
 	struct ad4052_state *st = private;
@@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
 	struct device *dev = &st->spi->dev;
 	int ret;
 
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
+	if (ret > 0) {
+		ret = devm_request_threaded_irq(dev, ret, NULL,
+						ad4052_irq_handler_thresh,
+						IRQF_ONESHOT, indio_dev->name,
+						indio_dev);
+		if (ret)
+			return ret;
+	} else if (ret == -EPROBE_DEFER) {
+		return ret;
+	}
+
 	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
 	if (ret > 0) {
 		ret = devm_request_threaded_irq(dev, ret, NULL,
@@ -822,6 +944,7 @@ static int ad4052_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long info)
 {
+	struct ad4052_state *st = iio_priv(indio_dev);
 	int ret;
 
 	if (info ==  IIO_CHAN_INFO_SAMP_FREQ)
@@ -831,8 +954,14 @@ static int ad4052_read_raw(struct iio_dev *indio_dev,
 
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
 
 	ret = ad4052_read_raw_dispatch(indio_dev, chan, val, val2, info);
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret ? ret : IIO_VAL_INT;
 }
@@ -867,8 +996,231 @@ static int ad4052_write_raw(struct iio_dev *indio_dev,
 
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
 
 	ret = ad4052_write_raw_dispatch(indio_dev, chan, val, val2, info);
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4052_monitor_mode_enable(struct ad4052_state *st)
+{
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&st->spi->dev);
+	if (ret)
+		return ret;
+
+	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
+	if (ret)
+		goto out_error;
+
+	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
+	if (ret)
+		goto out_error;
+
+	return ret;
+out_error:
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+	return ret;
+}
+
+static int ad4052_monitor_mode_disable(struct ad4052_state *st)
+{
+	int ret;
+
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+
+	ret = ad4052_exit_command(st);
+	if (ret)
+		return ret;
+	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
+			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
+			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
+}
+
+static int ad4052_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+
+	return st->wait_event;
+}
+
+static int ad4052_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     bool state)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event == state) {
+		ret = 0;
+		goto out_release;
+	}
+
+	if (state)
+		ret = ad4052_monitor_mode_enable(st);
+	else
+		ret = ad4052_monitor_mode_disable(st);
+
+	if (!ret)
+		st->wait_event = state;
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int __ad4052_read_event_info_value(struct ad4052_state *st,
+					   enum iio_event_direction dir, int *val)
+{
+	int ret;
+	u8 reg;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_LIMIT;
+	else
+		reg = AD4052_REG_MIN_LIMIT;
+
+	ret = regmap_bulk_read(st->regmap, reg, &st->raw, 2);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(get_unaligned_be16(st->raw), 11);
+
+	return 0;
+}
+
+static int __ad4052_read_event_info_hysteresis(struct ad4052_state *st,
+						enum iio_event_direction dir, int *val)
+{
+	u8 reg;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_HYST;
+	else
+		reg = AD4052_REG_MIN_HYST;
+	return regmap_read(st->regmap, reg, val);
+}
+
+static int ad4052_read_event_value(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info, int *val,
+				   int *val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = __ad4052_read_event_info_value(st, dir, val);
+		break;
+	case IIO_EV_INFO_HYSTERESIS:
+		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int __ad4052_write_event_info_value(struct ad4052_state *st,
+					   enum iio_event_direction dir, int val)
+{
+	u8 reg;
+
+	if (val > 2047 || val < -2048)
+		return -EINVAL;
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_LIMIT;
+	else
+		reg = AD4052_REG_MIN_LIMIT;
+	put_unaligned_be16(val, &st->raw);
+
+	return regmap_bulk_write(st->regmap, reg, &st->raw, 2);
+}
+
+static int __ad4052_write_event_info_hysteresis(struct ad4052_state *st,
+						enum iio_event_direction dir, int val)
+{
+	u8 reg;
+
+	if (val >= BIT(7))
+		return -EINVAL;
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4052_REG_MAX_HYST;
+	else
+		reg = AD4052_REG_MIN_HYST;
+
+	return regmap_write(st->regmap, reg, val);
+}
+
+static int ad4052_write_event_value(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info, int val,
+				    int val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = __ad4052_write_event_info_value(st, dir, val);
+			break;
+		case IIO_EV_INFO_HYSTERESIS:
+			ret = __ad4052_write_event_info_hysteresis(st, dir, val);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret;
 }
@@ -881,6 +1233,9 @@ static int ad4052_offload_buffer_postenable(struct iio_dev *indio_dev)
 	};
 	int ret;
 
+	if (st->wait_event)
+		return -EBUSY;
+
 	ret = pm_runtime_resume_and_get(&st->spi->dev);
 	if (ret)
 		return ret;
@@ -963,10 +1318,17 @@ static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
 
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
 	if (readval)
 		ret = regmap_read(st->regmap, reg, readval);
 	else
 		ret = regmap_write(st->regmap, reg, writeval);
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret;
 }
@@ -985,6 +1347,11 @@ static const struct iio_info ad4052_info = {
 	.read_raw = ad4052_read_raw,
 	.write_raw = ad4052_write_raw,
 	.read_avail = ad4052_read_avail,
+	.read_event_config = &ad4052_read_event_config,
+	.write_event_config = &ad4052_write_event_config,
+	.read_event_value = &ad4052_read_event_value,
+	.write_event_value = &ad4052_write_event_value,
+	.event_attrs = &ad4052_event_attribute_group,
 	.get_current_scan_type = &ad4052_get_current_scan_type,
 	.debugfs_reg_access = &ad4052_debugfs_reg_access,
 };
@@ -1193,8 +1560,10 @@ static int ad4052_probe(struct spi_device *spi)
 				     "Failed to initialize regmap\n");
 
 	st->mode = AD4052_SAMPLE_MODE;
+	st->wait_event = false;
 	st->chip = chip;
 	st->oversampling_frequency = AD4052_FS_OFFSET(st->chip->grade);
+	st->events_frequency = AD4052_FS_OFFSET(st->chip->grade);
 
 	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
 	if (IS_ERR(st->cnv_gp))

-- 
2.49.0

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[David Lechner]

On 6/10/25 2:34 AM, Jorge Marques wrote:
> The AD4052 family supports autonomous monitoring readings for threshold
> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> event. The device allows to set either, rising and falling directions. Only
> either threshold crossing is implemented.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---

...

> +
> +static ssize_t ad4052_events_frequency_store(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf,
> +					     size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}

I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
so that we don't have to implement this manually everywhere. If monitor mode was
claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.

> +
> +	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
> +				   AD4052_FS_LEN(st->chip->grade), buf);
> +	if (ret < 0)
> +		goto out_release;
> +
> +	st->events_frequency = ret;
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : len;
> +}
> +
> +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> +		       ad4052_events_frequency_show,
> +		       ad4052_events_frequency_store, 0);
> +
> +static ssize_t sampling_frequency_available_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int ret = 0;
> +
> +	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
> +	     i < AD4052_FS_LEN(st->chip->grade); i++)
> +		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
> +
> +	ret += sysfs_emit_at(buf, ret, "\n");
> +	return ret;
> +}
> +
> +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> +
> +static struct attribute *ad4052_event_attributes[] = {
> +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ad4052_event_attribute_group = {
> +	.attrs = ad4052_event_attributes,
> +};
> +
>  static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
>  				   struct iio_chan_spec const *chan)
>  {
> @@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
>  				  val);
>  }
>  
> +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +

Can we not read the status register here to find out what the exact
event was? I guess that would require taking it out of monitor mode.

> +	iio_push_event(indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> +					    IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
>  {
>  	struct ad4052_state *st = private;
> @@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
>  	struct device *dev = &st->spi->dev;
>  	int ret;
>  
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret > 0) {
> +		ret = devm_request_threaded_irq(dev, ret, NULL,
> +						ad4052_irq_handler_thresh,
> +						IRQF_ONESHOT, indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return ret;
> +	} else if (ret == -EPROBE_DEFER) {
> +		return ret;
> +	}

By swapping the order, we can avoid the else. Also, do we really want to
ignore all other errors? It seems like there would just be ENODEV or ENOENT
that means the interrupt is not there and we would want to pass on other
errors.

> +
>  	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
>  	if (ret > 0) {
>  		ret = devm_request_threaded_irq(dev, ret, NULL,


...

> +
> +static int ad4052_monitor_mode_enable(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> +	if (ret)
> +		goto out_error;
> +
> +	return ret;
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +	return ret;
> +}
> +
> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	ret = ad4052_exit_command(st);
> +	if (ret)
> +		return ret;
> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> +}
> +

It seems like we need to make sure monitor mode is disabled when the
driver is removed. Otherwise we could end up with unbalanced calls to
the pm_runtime stuff and leave the chip running.


> +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val,
> +				   int *val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = __ad4052_read_event_info_value(st, dir, val);
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> +		break;

These functions don't need __ prefix. There is no name clash.

> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;
> +}
> +

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jorge Marques]

Hi David,
On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:
> On 6/10/25 2:34 AM, Jorge Marques wrote:
> > The AD4052 family supports autonomous monitoring readings for threshold
> > crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > event. The device allows to set either, rising and falling directions. Only
> > either threshold crossing is implemented.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> 
> ...
> 
> > +
> > +static ssize_t ad4052_events_frequency_store(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     const char *buf,
> > +					     size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> 
> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> so that we don't have to implement this manually everywhere. If monitor mode was
> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> 
I don't think this would scale with other vendors and devices, it is a
limitation of ADI:ADC:SPI requiring to enter configuration mode to read
registers. A deep dive into the other drivers that use IIO Events is
needed.
> > +
> > +	ret = __sysfs_match_string(AD4052_FS(st->chip->grade),
> > +				   AD4052_FS_LEN(st->chip->grade), buf);
> > +	if (ret < 0)
> > +		goto out_release;
> > +
> > +	st->events_frequency = ret;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> > +		       ad4052_events_frequency_show,
> > +		       ad4052_events_frequency_store, 0);
> > +
> > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > +						 struct device_attribute *attr,
> > +						 char *buf)
> > +{
> > +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	int ret = 0;
> > +
> > +	for (u8 i = AD4052_FS_OFFSET(st->chip->grade);
> > +	     i < AD4052_FS_LEN(st->chip->grade); i++)
> > +		ret += sysfs_emit_at(buf, ret, "%s ", ad4052_conversion_freqs[i]);
> > +
> > +	ret += sysfs_emit_at(buf, ret, "\n");
> > +	return ret;
> > +}
> > +
> > +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > +
> > +static struct attribute *ad4052_event_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group ad4052_event_attribute_group = {
> > +	.attrs = ad4052_event_attributes,
> > +};
> > +
> >  static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> >  				   struct iio_chan_spec const *chan)
> >  {
> > @@ -602,6 +699,19 @@ static int ad4052_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
> >  				  val);
> >  }
> >  
> > +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +
> 
> Can we not read the status register here to find out what the exact
> event was? I guess that would require taking it out of monitor mode.
> 
It requires entering configuration mode and results in a monitoring
downtime. Earlier versions of this driver would do that, but the
conclusion was that it was better to have the user disabling events and
reading registers, so he is explicitly aware of the monitoring downtime.
> > +	iio_push_event(indio_dev,
> > +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> > +					    IIO_EV_TYPE_THRESH,
> > +					    IIO_EV_DIR_EITHER),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
> >  {
> >  	struct ad4052_state *st = private;
> > @@ -616,6 +726,18 @@ static int ad4052_request_irq(struct iio_dev *indio_dev)
> >  	struct device *dev = &st->spi->dev;
> >  	int ret;
> >  
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> > +	if (ret > 0) {
> > +		ret = devm_request_threaded_irq(dev, ret, NULL,
> > +						ad4052_irq_handler_thresh,
> > +						IRQF_ONESHOT, indio_dev->name,
> > +						indio_dev);
> > +		if (ret)
> > +			return ret;
> > +	} else if (ret == -EPROBE_DEFER) {
> > +		return ret;
> > +	}
> 
> By swapping the order, we can avoid the else. Also, do we really want to
> ignore all other errors? It seems like there would just be ENODEV or ENOENT
> that means the interrupt is not there and we would want to pass on other
> errors.
> 
Ack on the swap order.

If not set on the devicetree, including improper devicetree cases, it
should continue without. If the driver that manages the irq is not
probed, defer probe.

I tested different devicetrees and got:

* any property is missing: -EINVAL
* wrong interrupt-names: -ENODATA
* inconsistent array length between properties: -EOVERFLOW

EPROTO and ENXIO errors are also expected according the method comment,
the latter seems to be when the system doesn't support dts at all? And
EPROTO just another user-set dts issue.
I'm okay with ignoring them silently, or logging if gp0/1 found or not,
but not micromanage every error.

> > +
> >  	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> >  	if (ret > 0) {
> >  		ret = devm_request_threaded_irq(dev, ret, NULL,
> 
> 
> ...
> 
> > +
> > +static int ad4052_monitor_mode_enable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_conversion_frequency_set(st, st->events_frequency);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	return ret;
> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +	return ret;
> > +}
> > +
> > +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	ret = ad4052_exit_command(st);
> > +	if (ret)
> > +		return ret;
> > +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> > +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> > +}
> > +
> 
> It seems like we need to make sure monitor mode is disabled when the
> driver is removed. Otherwise we could end up with unbalanced calls to
> the pm_runtime stuff and leave the chip running.
> 
> 
When monitor mode is enabled, pm is already disabled (won't enter low
power). I expect the pm to handle the clean-up properly since devm is
used.
The .remove() I suggest is reg access to:

* Put in configuration mode, if not.
* Put on low power mode, if not.

> > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info, int *val,
> > +				   int *val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > +		break;
> 
> These functions don't need __ prefix. There is no name clash.
> 
Ack.

Best regards,
Jorge
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> > +}
> > +

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[David Lechner]

On 6/13/25 5:02 AM, Jorge Marques wrote:
> Hi David,
> On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:
>> On 6/10/25 2:34 AM, Jorge Marques wrote:
>>> The AD4052 family supports autonomous monitoring readings for threshold
>>> crossings. Add support for catching the GPIO interrupt and expose as an IIO
>>> event. The device allows to set either, rising and falling directions. Only
>>> either threshold crossing is implemented.
>>>
>>> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
>>> ---
>>
>> ...
>>
>>> +
>>> +static ssize_t ad4052_events_frequency_store(struct device *dev,
>>> +					     struct device_attribute *attr,
>>> +					     const char *buf,
>>> +					     size_t len)
>>> +{
>>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +	struct ad4052_state *st = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	if (!iio_device_claim_direct(indio_dev))
>>> +		return -EBUSY;
>>> +	if (st->wait_event) {
>>> +		ret = -EBUSY;
>>> +		goto out_release;
>>> +	}
>>
>> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
>> so that we don't have to implement this manually everywhere. If monitor mode was
>> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
>> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
>> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
>>
> I don't think this would scale with other vendors and devices, it is a

Why not? I've seen lots of devices that have some sort of monitor mode
where they are internally continuously comparing measurements to something
and only signal an interrupt when some condition is met.

> limitation of ADI:ADC:SPI requiring to enter configuration mode to read

I don't see how it could be a limitiation exclusive to this combination of
vendor, sensor type and bus type.

> registers. A deep dive into the other drivers that use IIO Events is
> needed.
>>> +

...

>>> +
>>> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
>>> +{
>>> +	int ret;
>>> +
>>> +	pm_runtime_mark_last_busy(&st->spi->dev);
>>> +	pm_runtime_put_autosuspend(&st->spi->dev);
>>> +
>>> +	ret = ad4052_exit_command(st);
>>> +	if (ret)
>>> +		return ret;
>>> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
>>> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
>>> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
>>> +}
>>> +
>>
>> It seems like we need to make sure monitor mode is disabled when the
>> driver is removed. Otherwise we could end up with unbalanced calls to
>> the pm_runtime stuff and leave the chip running.
>>
>>
> When monitor mode is enabled, pm is already disabled (won't enter low
> power). I expect the pm to handle the clean-up properly since devm is
> used.
> The .remove() I suggest is reg access to:
> 
> * Put in configuration mode, if not.
> * Put on low power mode, if not.
> 
I was just thinking something like:

	if (st->wait_event)
		ad4052_monitor_mode_disable(st);

Also might need to use devm_add_action_or_reset() instead of .remove
to get correct ordering.

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jonathan Cameron]

On Fri, 13 Jun 2025 11:03:24 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/13/25 5:02 AM, Jorge Marques wrote:
> > Hi David,
> > On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:  
> >> On 6/10/25 2:34 AM, Jorge Marques wrote:  
> >>> The AD4052 family supports autonomous monitoring readings for threshold
> >>> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> >>> event. The device allows to set either, rising and falling directions. Only
> >>> either threshold crossing is implemented.
> >>>
> >>> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> >>> ---  
> >>
> >> ...
> >>  
> >>> +
> >>> +static ssize_t ad4052_events_frequency_store(struct device *dev,
> >>> +					     struct device_attribute *attr,
> >>> +					     const char *buf,
> >>> +					     size_t len)
> >>> +{
> >>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> +	struct ad4052_state *st = iio_priv(indio_dev);
> >>> +	int ret;
> >>> +
> >>> +	if (!iio_device_claim_direct(indio_dev))
> >>> +		return -EBUSY;
> >>> +	if (st->wait_event) {
> >>> +		ret = -EBUSY;
> >>> +		goto out_release;
> >>> +	}  
> >>
> >> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> >> so that we don't have to implement this manually everywhere. If monitor mode was
> >> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> >> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> >> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> >>  
> > I don't think this would scale with other vendors and devices, it is a  
> 
> Why not? I've seen lots of devices that have some sort of monitor mode
> where they are internally continuously comparing measurements to something
> and only signal an interrupt when some condition is met.

There are lots that support such a monitor, but I think far fewer were direct
accesses don't work at the same time.  The max1363 comes to mind but in that
case it is possible to do both monitor and direct reads it is just that the
data format changes and I think we never bothered implementing the handling
for that combination.

I wouldn't mind such helpers if there are at least a couple of users.

> 
> > limitation of ADI:ADC:SPI requiring to enter configuration mode to read  
> 
> I don't see how it could be a limitiation exclusive to this combination of
> vendor, sensor type and bus type.
> 
> > registers. A deep dive into the other drivers that use IIO Events is
> > needed.  
> >>> +  
> 
> ...
> 
> >>> +
> >>> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	pm_runtime_mark_last_busy(&st->spi->dev);
> >>> +	pm_runtime_put_autosuspend(&st->spi->dev);
> >>> +
> >>> +	ret = ad4052_exit_command(st);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> >>> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> >>> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> >>> +}
> >>> +  
> >>
> >> It seems like we need to make sure monitor mode is disabled when the
> >> driver is removed. Otherwise we could end up with unbalanced calls to
> >> the pm_runtime stuff and leave the chip running.
> >>
> >>  
> > When monitor mode is enabled, pm is already disabled (won't enter low
> > power). I expect the pm to handle the clean-up properly since devm is
> > used.
> > The .remove() I suggest is reg access to:
> > 
> > * Put in configuration mode, if not.
> > * Put on low power mode, if not.
> >   
> I was just thinking something like:
> 
> 	if (st->wait_event)
> 		ad4052_monitor_mode_disable(st);
> 
> Also might need to use devm_add_action_or_reset() instead of .remove
> to get correct ordering.

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jonathan Cameron]

On Sat, 14 Jun 2025 11:25:44 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 13 Jun 2025 11:03:24 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 6/13/25 5:02 AM, Jorge Marques wrote:  
> > > Hi David,
> > > On Thu, Jun 12, 2025 at 02:38:45PM -0500, David Lechner wrote:    
> > >> On 6/10/25 2:34 AM, Jorge Marques wrote:    
> > >>> The AD4052 family supports autonomous monitoring readings for threshold
> > >>> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > >>> event. The device allows to set either, rising and falling directions. Only
> > >>> either threshold crossing is implemented.
> > >>>
> > >>> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > >>> ---    
> > >>
> > >> ...
> > >>    
> > >>> +
> > >>> +static ssize_t ad4052_events_frequency_store(struct device *dev,
> > >>> +					     struct device_attribute *attr,
> > >>> +					     const char *buf,
> > >>> +					     size_t len)
> > >>> +{
> > >>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > >>> +	struct ad4052_state *st = iio_priv(indio_dev);
> > >>> +	int ret;
> > >>> +
> > >>> +	if (!iio_device_claim_direct(indio_dev))
> > >>> +		return -EBUSY;
> > >>> +	if (st->wait_event) {
> > >>> +		ret = -EBUSY;
> > >>> +		goto out_release;
> > >>> +	}    
> > >>
> > >> I'm wondering if we should instead have some kind of iio_device_claim_monitor_mode()
> > >> so that we don't have to implement this manually everywhere. If monitor mode was
> > >> claimed, then iio_device_claim_direct() and iio_device_claim_buffer_mode() would
> > >> both return -EBUSY. If buffer mode was claimed, iio_device_claim_monitor_mode()
> > >> would fail. If direct mode was claimed, iio_device_claim_monitor_mode() would wait.
> > >>    
> > > I don't think this would scale with other vendors and devices, it is a    
> > 
> > Why not? I've seen lots of devices that have some sort of monitor mode
> > where they are internally continuously comparing measurements to something
> > and only signal an interrupt when some condition is met.  
> 
> There are lots that support such a monitor, but I think far fewer were direct
> accesses don't work at the same time.  The max1363 comes to mind but in that
> case it is possible to do both monitor and direct reads it is just that the
> data format changes and I think we never bothered implementing the handling
> for that combination.
> 
> I wouldn't mind such helpers if there are at least a couple of users.
> 
I got this wrong.  Key here is not direct access and monitor, but rather
monitor and buffering (which is the odd format case on the max1363 etc).

Anyhow, conclusion that helpers are fine is the same.

I would try to minimise what doesn't work when monitor mode is enabled though
(as commented in review of this patch).

We also have to cover the internal cases where buffer mode is claimed but
there isn't (IIRC) a call to that particular claim function as we don't
want to end up holding the lock - same will be true for monitor mode - there
is a difference between temporary fixing of state where locks are fine
and the mode running for a long period in which we don't hold the lock.

> >   
> > > limitation of ADI:ADC:SPI requiring to enter configuration mode to read    
> > 
> > I don't see how it could be a limitiation exclusive to this combination of
> > vendor, sensor type and bus type.
> >   
> > > registers. A deep dive into the other drivers that use IIO Events is
> > > needed.    
> > >>> +    
> > 
> > ...
> >   
> > >>> +
> > >>> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> > >>> +{
> > >>> +	int ret;
> > >>> +
> > >>> +	pm_runtime_mark_last_busy(&st->spi->dev);
> > >>> +	pm_runtime_put_autosuspend(&st->spi->dev);
> > >>> +
> > >>> +	ret = ad4052_exit_command(st);
> > >>> +	if (ret)
> > >>> +		return ret;
> > >>> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > >>> +			    AD4052_REG_DEVICE_STATUS_MAX_FLAG |
> > >>> +			    AD4052_REG_DEVICE_STATUS_MIN_FLAG);
> > >>> +}
> > >>> +    
> > >>
> > >> It seems like we need to make sure monitor mode is disabled when the
> > >> driver is removed. Otherwise we could end up with unbalanced calls to
> > >> the pm_runtime stuff and leave the chip running.
> > >>
> > >>    
> > > When monitor mode is enabled, pm is already disabled (won't enter low
> > > power). I expect the pm to handle the clean-up properly since devm is
> > > used.
> > > The .remove() I suggest is reg access to:
> > > 
> > > * Put in configuration mode, if not.
> > > * Put on low power mode, if not.
> > >     
> > I was just thinking something like:
> > 
> > 	if (st->wait_event)
> > 		ad4052_monitor_mode_disable(st);
> > 
> > Also might need to use devm_add_action_or_reset() instead of .remove
> > to get correct ordering.  
> 
> 

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jonathan Cameron]

On Tue, 10 Jun 2025 09:34:41 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> The AD4052 family supports autonomous monitoring readings for threshold
> crossings. Add support for catching the GPIO interrupt and expose as an IIO
> event. The device allows to set either, rising and falling directions. Only
> either threshold crossing is implemented.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jorge,

A few comments inline.

Jonathan

>
> +
> +static int ad4052_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event == state) {
> +		ret = 0;

Feels like a case where init ret at declaration would be reasonable.

> +		goto out_release;
> +	}
> +
> +	if (state)
> +		ret = ad4052_monitor_mode_enable(st);
> +	else
> +		ret = ad4052_monitor_mode_disable(st);
> +
> +	if (!ret)
> +		st->wait_event = state;
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}

> +
> +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val,
> +				   int *val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;

Not being able to read event parameters whilst monitoring them seems
very restrictive.  Can't we cache the values?  Either play games to ensure
we get them from the regmap cache or just cache these few values in st.

Checking what you are monitoring for feels like the sort of thing
userspace might well do.

Even blocking changing the monitoring parameters is unusually strict.
Why not just drop out of monitor mode, update them and go back in?


> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = __ad4052_read_event_info_value(st, dir, val);
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;
> +}

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jorge Marques]

On Sat, Jun 14, 2025 at 11:36:16AM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:41 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
> > The AD4052 family supports autonomous monitoring readings for threshold
> > crossings. Add support for catching the GPIO interrupt and expose as an IIO
> > event. The device allows to set either, rising and falling directions. Only
> > either threshold crossing is implemented.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jonathan,
> Hi Jorge,
> 
> A few comments inline.
> 
> Jonathan
> 
> >
> > +
> > +static int ad4052_write_event_config(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     enum iio_event_type type,
> > +				     enum iio_event_direction dir,
> > +				     bool state)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event == state) {
> > +		ret = 0;
> 
> Feels like a case where init ret at declaration would be reasonable.
> 
Ack.
> > +		goto out_release;
> > +	}
> > +
> > +	if (state)
> > +		ret = ad4052_monitor_mode_enable(st);
> > +	else
> > +		ret = ad4052_monitor_mode_disable(st);
> > +
> > +	if (!ret)
> > +		st->wait_event = state;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> 
> > +
> > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info, int *val,
> > +				   int *val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> 

Below are two distinct options with different implications.
> Not being able to read event parameters whilst monitoring them seems
> very restrictive.  Can't we cache the values?  Either play games to ensure
> we get them from the regmap cache or just cache these few values in st.
> 
> Checking what you are monitoring for feels like the sort of thing
> userspace might well do.

(1)
I agree, I can investigate regcache_cache_only and the other cache
options to achieve this. If I come to the conclusion it is not possible,
storing into st will achieve the same.

> 
> Even blocking changing the monitoring parameters is unusually strict.
> Why not just drop out of monitor mode, update them and go back in?
> 
(2)
The core point of the blocking behaviour is to not have hidden downtimes
in the monitoring for the user. An early driver used to do what you
describe and it was a design decision.

Since a custom regmap_bus was necessary to restrict the regmap access
speed (ADC access is faster), bringing back this by behavior embedding
it in the custom regmap now seems plausible, with proper explanation in
the rst page. This should fully dismiss the st->wait_event -> -EBUSY.

Considering (1) and (2), what is the preferred approach?

Regards,
Jorge
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> > +}

Re: [PATCH v3 8/8] iio: adc: Add events support to ad4052

[Jonathan Cameron]

> > > +
> > > +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> > > +				   const struct iio_chan_spec *chan,
> > > +				   enum iio_event_type type,
> > > +				   enum iio_event_direction dir,
> > > +				   enum iio_event_info info, int *val,
> > > +				   int *val2)
> > > +{
> > > +	struct ad4052_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	if (!iio_device_claim_direct(indio_dev))
> > > +		return -EBUSY;
> > > +
> > > +	if (st->wait_event) {
> > > +		ret = -EBUSY;
> > > +		goto out_release;  
> >   
> 
> Below are two distinct options with different implications.
> > Not being able to read event parameters whilst monitoring them seems
> > very restrictive.  Can't we cache the values?  Either play games to ensure
> > we get them from the regmap cache or just cache these few values in st.
> > 
> > Checking what you are monitoring for feels like the sort of thing
> > userspace might well do.  
> 
> (1)
> I agree, I can investigate regcache_cache_only and the other cache
> options to achieve this. If I come to the conclusion it is not possible,
> storing into st will achieve the same.
> 
> > 
> > Even blocking changing the monitoring parameters is unusually strict.
> > Why not just drop out of monitor mode, update them and go back in?
> >   
> (2)
> The core point of the blocking behaviour is to not have hidden downtimes
> in the monitoring for the user. An early driver used to do what you
> describe and it was a design decision.
> 
> Since a custom regmap_bus was necessary to restrict the regmap access
> speed (ADC access is faster), bringing back this by behavior embedding
> it in the custom regmap now seems plausible, with proper explanation in
> the rst page. This should fully dismiss the st->wait_event -> -EBUSY.
> 
> Considering (1) and (2), what is the preferred approach?

Key here is that the user made the choice to change the parameters.
Most of the time they won't choose to do that, but if they do then
that's what they want to do. Why make them turn the monitoring off,
change value and turn it on again if we can support it reasonably
cleanly.  In many devices there is no interruption to monitoring so
we may well have userspace code written against assumption it
can just update this stuff without that dance.  So prefer (2)
but (1) is better than nothing if (2) proves too complex.

J
> 
> Regards,
> Jorge
> > > +	}
> > > +
> > > +	switch (info) {
> > > +	case IIO_EV_INFO_VALUE:
> > > +		ret = __ad4052_read_event_info_value(st, dir, val);
> > > +		break;
> > > +	case IIO_EV_INFO_HYSTERESIS:
> > > +		ret = __ad4052_read_event_info_hysteresis(st, dir, val);
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		break;
> > > +	}
> > > +
> > > +out_release:
> > > +	iio_device_release_direct(indio_dev);
> > > +	return ret ? ret : IIO_VAL_INT;
> > > +}