[PATCH v2 0/5] Add support for AD4052 device family

[PATCH v2 0/5] 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 series starts with marking iio_dev as const in iio_buffer_enabled,
to not discard the qualifier when calling from get_current_can_type.
This is required since the size of storage bytes varies if the offload
buffer is used or not.

The scan_type also 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.
SPI messages related to exiting the ADC mode, and reading raw values are
never optimized.

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.
An auxiliary method ad4052_iio_device_claim_direct manages the IIO claim
direct as well as the required wait_event boolean.
The device has an internal sampling rate for the autonomous modes,
exposed as the sample_rate attribute.

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.
* Monitor mode: Similar to trigger mode used in the monitoring mode, but
  doesn't exit to configuration mode on event, being awkward to expose
  to user space.

Signed-off-by: Jorge Marques <jorge.marques@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 (5):
      Documentation: ABI: add oversampling frequency in sysfs-bus-iio
      iio: code: mark iio_dev as const in iio_buffer_enabled
      dt-bindings: iio: adc: Add adi,ad4052
      docs: iio: new docs for ad4052 driver
      iio: adc: add support for ad4052

 Documentation/ABI/testing/sysfs-bus-iio            |   17 +
 .../devicetree/bindings/iio/adc/adi,ad4052.yaml    |   98 ++
 Documentation/iio/ad4052.rst                       |   95 ++
 MAINTAINERS                                        |    8 +
 drivers/iio/adc/Kconfig                            |   14 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/ad4052.c                           | 1425 ++++++++++++++++++++
 drivers/iio/industrialio-core.c                    |    2 +-
 include/linux/iio/iio.h                            |    2 +-
 9 files changed, 1660 insertions(+), 2 deletions(-)
---
base-commit: 905d6b57b18fa932b3f05578e82625d22a4dd17f
change-id: 20250306-iio-driver-ad4052-a4acc3bb11b3

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

[PATCH v2 1/5] 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.

Devices with this feature are max1363, ad7606, ad799x, and ad4052.
The max1363 driver included the events/sampling_frequency in
commit 168c9d95a940 ("iio:adc:max1363 move from staging.")
and ad799x in
commit ba1d79613df3 ("staging:iio:ad799x: Use event spec for threshold
hysteresis")
but went undocumented so far.

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 33c09c4ac60a4feec82308461643134f5ba84b66..129061befb21b82a51142a01a94d96fcf1b60072 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.15
+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.15
+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

[PATCH v2 2/5] iio: code: mark iio_dev as const in iio_buffer_enabled

[Jorge Marques]

The iio_dev struct is never modified inside the method, mark it as
const.
This allows to be called from get_current_scan_type, and is useful
when the scan_type depends on the buffer state.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 drivers/iio/industrialio-core.c | 2 +-
 include/linux/iio/iio.h         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 178e99b111debc59a247fcc3a6037e429db3bebf..bc6a2ac6415eccf201e148ea98c0b5982787eb6d 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -212,7 +212,7 @@ EXPORT_SYMBOL_GPL(iio_device_id);
  *
  * Returns: True, if the buffer is enabled.
  */
-bool iio_buffer_enabled(struct iio_dev *indio_dev)
+bool iio_buffer_enabled(const struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..88569e1a888bde4d2bfb5b9f030096af1c15d68d 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -629,7 +629,7 @@ struct iio_dev {
 
 int iio_device_id(struct iio_dev *indio_dev);
 int iio_device_get_current_mode(struct iio_dev *indio_dev);
-bool iio_buffer_enabled(struct iio_dev *indio_dev);
+bool iio_buffer_enabled(const struct iio_dev *indio_dev);
 
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si);

-- 
2.49.0

[PATCH v2 3/5] 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).

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad4052.yaml    | 98 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 ++
 2 files changed, 104 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..a0510d485f130ceec15b887e8f8deeb2cf6562c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
@@ -0,0 +1,98 @@
+# 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 (threshold).
+      - description: Signal coming from the GP1 pin (data ready).
+
+  interrupt-names:
+    items:
+      - const: gp0
+      - const: gp1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+    description: |
+      The first cell is the GPn number: 0 to 1.
+      The second cell takes standard GPIO flags.
+
+  cnv-gpios:
+    description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 62500000
+
+  vdd-supply:
+    description: Analog power supply.
+
+  vio-supply:
+    description: Digital interface logic power supply.
+
+  vref-supply:
+    description: Reference voltage to set the ADC full-scale range.
+
+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>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad4052";
+            reg = <0>;
+            vdd-supply = <&adc_vdd>;
+            vio-supply = <&adc_vio>;
+            spi-max-frequency = <25000000>;
+
+            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 01079a189c93697c1db6b0ca4e54212d25589974..81fbe7176475c48eae03ab04115d4ef5b6299fac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1329,6 +1329,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 AD4130 DRIVER
 M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
 L:	linux-iio@vger.kernel.org

-- 
2.49.0

[PATCH v2 4/5] 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 | 95 ++++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS                  |  1 +
 2 files changed, 96 insertions(+)

diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
new file mode 100644
index 0000000000000000000000000000000000000000..410aaa437ed5fea6a2924d374fa5f816f5754e22
--- /dev/null
+++ b/Documentation/iio/ad4052.rst
@@ -0,0 +1,95 @@
+.. 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 a 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 contain only one channels, and the following attributes:
+
+.. list-table:: Driver attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``in_voltage0_raw``
+     - Raw ADC voltage value
+   * - ``in_voltage0_oversampling_ratio``
+     - Enable the device's burst averaging mode to over sample using
+       the internal sample rate.
+   * - ``in_voltage0_oversampling_ratio_available``
+     - List of available oversampling values. Value 0 disable the burst
+       averaging mode.
+   * - ``conversion_frequency``
+     - Device internal sample rate used in the burst averaging mode.
+   * - ``conversion_frequency_available``
+     - List of available sample rates.
+
+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.
+
+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_voltage0_sampling_frequency``
+     - Set the sampling frequency.
+   * - ``in_voltage0_sampling_frequency_available``
+     - Get the sampling frequency range.
+
+The scan type is different when the buffer with offload support is enabled.
diff --git a/MAINTAINERS b/MAINTAINERS
index 81fbe7176475c48eae03ab04115d4ef5b6299fac..04aa8db44bee418382a2e74cb6b1d03a810bd781 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1334,6 +1334,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 AD4130 DRIVER
 M:	Cosmin Tanislav <cosmin.tanislav@analog.com>

-- 
2.49.0

[PATCH v2 5/5] 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  |   14 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1441 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 04aa8db44bee418382a2e74cb6b1d03a810bd781..56adb540fa172d5729735acbeaee6c576132cddd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1335,6 +1335,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 AD4130 DRIVER
 M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ad06cf5567851ee71f1211ec69d59fd5c1857ee5..68f8ce91320ea6db9377b3442408598a863ab428 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -55,6 +55,20 @@ 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
+	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 AD4130
 	tristate "Analog Device AD4130 ADC Driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 07d4b832c42e6352f2a4c956295308f007af0f45..edc0d1809f0cf9fcd65ee2cf72218d0dc3ecacbf 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_AD4130) += ad4130.o
 obj-$(CONFIG_AD4695) += ad4695.o
 obj-$(CONFIG_AD4851) += ad4851.o
diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
new file mode 100644
index 0000000000000000000000000000000000000000..f412f0884bab4f500091f6c7ca761967c8f6e3b6
--- /dev/null
+++ b/drivers/iio/adc/ad4052.c
@@ -0,0 +1,1425 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AD4052 SPI ADC driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#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>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/offload/consumer.h>
+#include <linux/spi/offload/provider.h>
+
+#define AD4052_REG_INTERFACE_CONFIG_A		0x00
+#define AD4052_REG_DEVICE_CONFIG		0x02
+#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_MODE_SET			0x20
+#define AD4052_REG_ADC_MODES			0x21
+#define AD4052_REG_AVG_CONFIG			0x23
+#define AD4052_REG_GP_CONF			0x24
+#define AD4052_REG_INTR_CONF			0x25
+#define AD4052_REG_TIMER_CONFIG			0x27
+#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_MIN_SAMPLE			0x45
+#define AD4052_MAX_REG				0x45
+#define AD4052_ADC_MODES_MODE_MSK		GENMASK(1, 0)
+#define AD4052_GP_CONF_MODE_MSK_0		GENMASK(2, 0)
+#define AD4052_GP_CONF_MODE_MSK_1		GENMASK(6, 4)
+#define AD4052_INTR_CONF_EN_MSK_0		GENMASK(1, 0)
+#define AD4052_INTR_CONF_EN_MSK_1		GENMASK(5, 4)
+#define AD4052_MODE_SET_ENTER_ADC		BIT(0)
+#define AD4052_ADC_MODES_DATA_FORMAT		BIT(7)
+#define AD4052_DEVICE_CONFIG_POWER_MODE_MSK	GENMASK(1, 0)
+#define AD4052_DEVICE_CONFIG_LOW_POWER_MODE	3
+#define AD4052_DEVICE_STATUS_DEVICE_RESET	BIT(6)
+#define AD4052_TIMER_CONFIG_FS_MASK		GENMASK(7, 4)
+#define AD4052_TIMER_CONFIG_300KSPS		0x2
+
+#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_T_CNVH_NS	10
+
+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 struct iio_chan_spec offload_channels[1];
+	const char *name;
+	u16 prod_id;
+	u8 max_avg;
+};
+
+enum {
+	AD4052_SCAN_TYPE_SAMPLE,
+	AD4052_SCAN_TYPE_BURST_AVG,
+	AD4052_SCAN_TYPE_OFFLOAD_SAMPLE,
+	AD4052_SCAN_TYPE_OFFLOAD_BURST_AVG,
+};
+
+static const struct iio_scan_type ad4052_scan_type_12_s[] = {
+	[AD4052_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 16,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 16,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_OFFLOAD_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_OFFLOAD_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 = 16,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_OFFLOAD_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+	[AD4052_SCAN_TYPE_OFFLOAD_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_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 spi_transfer xfer;
+	struct spi_message msg;
+	struct gpio_desc *cnv_gp;
+	struct completion completion;
+	struct regmap *regmap;
+	u16 oversampling_frequency;
+	u16 events_frequency;
+	bool wait_event;
+	int gp1_irq;
+	u8 data_format;
+	u8 grade;
+	union {
+		__be16 d16;
+		__be32 d32;
+	} __aligned(IIO_DMA_MINALIGN);
+	u8 buf_reset_pattern[18];
+};
+
+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 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", "33300",
+	"10000", "3000", "500", "333", "250", "200",
+	"166", "140", "124", "111",
+};
+
+static int ad4052_conversion_frequency_set(struct ad4052_state *st, u8 val)
+{
+	val += AD4052_FS_OFFSET(st->grade);
+	return regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
+			    FIELD_PREP(AD4052_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->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);
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		iio_device_release_direct(indio_dev);
+		return -EBUSY;
+	}
+
+	val += AD4052_FS_OFFSET(st->grade);
+	st->oversampling_frequency = val;
+
+	iio_device_release_direct(indio_dev);
+	return 0;
+}
+
+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 sprintf(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) {
+		iio_device_release_direct(indio_dev);
+		return -EBUSY;
+	}
+
+	ret = __sysfs_match_string(AD4052_FS(st->grade),
+				   AD4052_FS_LEN(st->grade), buf);
+	if (ret >= 0)
+		st->events_frequency = ret;
+
+	iio_device_release_direct(indio_dev);
+
+	return ret < 0 ? ret : len;
+}
+
+static ssize_t ad4052_events_frequency_available_show(struct device *dev,
+						      struct device_attribute *attr,
+						      char *buf)
+{
+	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
+	int len = 0;
+
+	for (u8 i = AD4052_FS_OFFSET(st->grade);
+	     i < AD4052_FS_LEN(st->grade); i++)
+		len += sprintf(buf + len, "%s ", ad4052_conversion_freqs[i]);
+
+	return sprintf(buf + len, "\n") + len;
+}
+
+static IIO_DEVICE_ATTR(sampling_frequency, 0644,
+		       ad4052_events_frequency_show,
+		       ad4052_events_frequency_store, 0);
+
+static IIO_DEVICE_ATTR(sampling_frequency_available, 0444,
+		       ad4052_events_frequency_available_show,
+		       NULL, 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 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_OVERSAMPLING_RATIO),		\
+	.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),			\
+	.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_OVERSAMPLING_RATIO) |		\
+				    BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.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),			\
+	.ext_info = grade##_ext_info,							\
+}
+
+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,
+};
+
+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,
+};
+
+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,
+};
+
+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,
+};
+
+static void 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;
+
+	xfer->bits_per_word = scan_type->realbits;
+	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
+}
+
+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->xfer;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	xfer = &st->offload_xfer;
+	xfer->bits_per_word = scan_type->realbits;
+	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
+
+	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
+	st->offload_msg.offload = st->offload;
+
+	return spi_optimize_message(st->spi, &st->offload_msg);
+}
+
+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;
+	}
+
+	ad4052_update_xfer_raw(indio_dev, chan);
+
+	return 0;
+}
+
+static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
+					 unsigned int *val)
+{
+	int ret;
+
+	if (st->mode == AD4052_SAMPLE_MODE) {
+		*val = 1;
+		return 0;
+	}
+
+	ret = regmap_read(st->regmap, AD4052_REG_AVG_CONFIG, val);
+	if (ret)
+		return ret;
+
+	*val = BIT(*val + 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->d16,
+			       sizeof(st->d16));
+	if (ret)
+		return ret;
+
+	val = be16_to_cpu(st->d16);
+	if (val != st->chip->prod_id)
+		dev_info(&st->spi->dev,
+			 "Production ID x%x does not match known values", val);
+
+	ret = regmap_bulk_read(st->regmap, AD4052_REG_VENDOR_H, &st->d16,
+			       sizeof(st->d16));
+	if (ret)
+		return ret;
+
+	val = be16_to_cpu(st->d16);
+	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_ADC_MODES_MODE_MSK, mode);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4052_REG_MODE_SET,
+			    AD4052_MODE_SET_ENTER_ADC);
+}
+
+static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
+{
+	struct pwm_state pwm_st;
+
+	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
+		return -EINVAL;
+
+	pwm_get_state(st->cnv_pwm, &pwm_st);
+	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
+	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
+}
+
+static int ad4052_soft_reset(struct ad4052_state *st)
+{
+	int ret;
+
+	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
+	for (int i = 0; i < 3; i++)
+		st->buf_reset_pattern[6 * (i + 1) - 1] = 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)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+
+	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_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
+		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
+	int ret;
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
+				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
+				 val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
+	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
+				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
+				 val);
+	if (ret)
+		return ret;
+
+	val = 0;
+	if (scan_type->sign == 's')
+		val |= AD4052_ADC_MODES_DATA_FORMAT;
+
+	st->data_format = val;
+
+	if (st->grade == AD4052_500KSPS) {
+		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
+				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
+					      AD4052_TIMER_CONFIG_300KSPS));
+		if (ret)
+			return ret;
+	}
+
+	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, 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;
+
+	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 = 0;
+
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
+	if (ret <= 0)
+		return ret ? ret : -EINVAL;
+
+	ret = devm_request_threaded_irq(dev, ret, NULL,
+					ad4052_irq_handler_thresh,
+					IRQF_ONESHOT, indio_dev->name,
+					indio_dev);
+	if (ret)
+		return ret;
+
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
+	if (ret <= 0)
+		return ret ? ret : -EINVAL;
+
+	st->gp1_irq = ret;
+	return devm_request_threaded_irq(dev, ret, NULL,
+					 ad4052_irq_handler_drdy,
+					 IRQF_ONESHOT, indio_dev->name,
+					 st);
+}
+
+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_read_chan_raw(struct ad4052_state *st, int *val)
+{
+	struct spi_device *spi = st->spi;
+	int ret;
+	struct spi_transfer t_cnv = {
+		.len = 0
+	};
+
+	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.
+	 */
+	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 = be16_to_cpu(st->d16);
+		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
+			*val = sign_extend32(*val, 15);
+	} else {
+		*val = be32_to_cpu(st->d32);
+		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
+			*val = sign_extend32(*val, 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(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct pwm_state pwm_st;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		iio_device_release_direct(indio_dev);
+		return -EBUSY;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ad4052_read_chan_raw(indio_dev, val);
+		if (ret)
+			goto out_release;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = ad4052_get_oversampling_ratio(st, val);
+		if (ret)
+			goto out_release;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
+		if (ret)
+			goto out_release;
+
+		if (!pwm_st.enabled)
+			pwm_get_state(st->cnv_pwm, &pwm_st);
+
+		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
+
+		ret = IIO_VAL_INT;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+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;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		iio_device_release_direct(indio_dev);
+		return -EBUSY;
+	}
+
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = ad4052_set_oversampling_ratio(indio_dev, chan, val);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = ad4052_set_sampling_freq(st, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	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)
+		pm_runtime_resume_and_get(&st->spi->dev);
+
+	return ret;
+}
+
+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 = -EBUSY;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event == state) {
+		iio_device_release_direct(indio_dev);
+		return 0;
+	}
+
+	if (state)
+		ret = ad4052_monitor_mode_enable(st);
+	else
+		ret = ad4052_monitor_mode_disable(st);
+
+	if (!ret)
+		st->wait_event = state;
+
+	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);
+	u8 reg, size = 1;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		iio_device_release_direct(indio_dev);
+		return -EBUSY;
+	}
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (dir == IIO_EV_DIR_RISING)
+			reg = AD4052_REG_MAX_LIMIT;
+		else
+			reg = AD4052_REG_MIN_LIMIT;
+		size = 2;
+		break;
+	case IIO_EV_INFO_HYSTERESIS:
+		if (dir == IIO_EV_DIR_RISING)
+			reg = AD4052_REG_MAX_HYST;
+		else
+			reg = AD4052_REG_MIN_HYST;
+		break;
+	default:
+		ret = -EINVAL;
+		goto out_release;
+	}
+
+	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
+	if (ret)
+		goto out_release;
+
+	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
+		*val = be16_to_cpu(st->d16);
+		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
+			*val = sign_extend32(*val, 11);
+	} else {
+		*val = be32_to_cpu(st->d32);
+	}
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+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 = -EINVAL;
+	u8 reg, size = 1;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	if (st->wait_event) {
+		iio_device_release_direct(indio_dev);
+		return -EBUSY;
+	}
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT) {
+				if (val > 2047 || val < -2048)
+					goto out_release;
+			} else if (val > 4095 || val < 0) {
+				goto out_release;
+			}
+			if (dir == IIO_EV_DIR_RISING)
+				reg = AD4052_REG_MAX_LIMIT;
+			else
+				reg = AD4052_REG_MIN_LIMIT;
+			size = 2;
+			st->d16 = cpu_to_be16(val);
+			break;
+		case IIO_EV_INFO_HYSTERESIS:
+			if (val & BIT(7))
+				goto out_release;
+			if (dir == IIO_EV_DIR_RISING)
+				reg = AD4052_REG_MAX_HYST;
+			else
+				reg = AD4052_REG_MIN_HYST;
+			st->d16 = cpu_to_be16(val >> 8);
+			break;
+		default:
+			goto out_release;
+		}
+		break;
+	default:
+		goto out_release;
+	}
+
+	ret = regmap_bulk_write(st->regmap, reg, &st->d16, size);
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4052_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;
+
+	if (st->wait_event)
+		return -EBUSY;
+
+	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_update_xfer_offload(indio_dev, indio_dev->channels);
+	if (ret)
+		goto out_error;
+
+	/* SPI Offload handles the data ready irq */
+	disable_irq(st->gp1_irq);
+
+	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
+					 &config);
+	if (ret)
+		goto out_offload_error;
+
+	ret = pwm_enable(st->cnv_pwm);
+	if (ret)
+		goto out_pwm_error;
+
+	return 0;
+
+out_pwm_error:
+	spi_offload_trigger_disable(st->offload, st->offload_trigger);
+out_offload_error:
+	enable_irq(st->gp1_irq);
+	spi_unoptimize_message(&st->offload_msg);
+	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_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	pwm_disable(st->cnv_pwm);
+
+	spi_offload_trigger_disable(st->offload, st->offload_trigger);
+	spi_unoptimize_message(&st->offload_msg);
+	enable_irq(st->gp1_irq);
+
+	ret = ad4052_exit_command(st);
+
+	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_setup_ops = {
+	.postenable = &ad4052_buffer_postenable,
+	.predisable = &ad4052_buffer_predisable,
+};
+
+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 (st->wait_event) {
+		iio_device_release_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);
+
+	if (iio_buffer_enabled(indio_dev))
+		return st->mode == AD4052_BURST_AVERAGING_MODE ?
+				   AD4052_SCAN_TYPE_OFFLOAD_BURST_AVG :
+				   AD4052_SCAN_TYPE_OFFLOAD_SAMPLE;
+
+	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,
+	.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,
+};
+
+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 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,
+	};
+	struct pwm_state pwm_st;
+	int ret;
+
+	indio_dev->setup_ops = &ad4052_buffer_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, &pwm_st);
+
+	pwm_st.enabled = false;
+	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
+	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
+					 AD4052_MAX_RATE(st->grade));
+
+	ret = pwm_apply_might_sleep(st->cnv_pwm, &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_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;
+	int ret = 0;
+
+	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);
+
+	st->regmap = devm_regmap_init_spi(spi, &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->wait_event = false;
+	st->chip = chip;
+	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
+	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
+	st->events_frequency = AD4052_FS_OFFSET(st->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_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ad4052_info;
+	indio_dev->name = chip->name;
+
+	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
+	if (IS_ERR(st->offload))
+		return PTR_ERR(st->offload);
+
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get offload\n");
+
+	if (ret == -ENODEV) {
+		st->offload_trigger = NULL;
+		indio_dev->channels = chip->channels;
+	} else {
+		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");
+	}
+
+	st->xfer.rx_buf = &st->d32;
+
+	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);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
+			   AD4052_DEVICE_STATUS_DEVICE_RESET);
+	if (ret)
+		return ret;
+
+	ret = ad4052_request_irq(indio_dev);
+	if (ret)
+		return ret;
+
+	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
+
+	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_DEVICE_CONFIG_POWER_MODE_MSK,
+				       AD4052_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_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
+			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
+	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 v2 1/5] Documentation: ABI: add oversampling frequency in sysfs-bus-iio

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 01:34:46PM +0200, Jorge Marques wrote:
> 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.
> 
> Devices with this feature are max1363, ad7606, ad799x, and ad4052.
> The max1363 driver included the events/sampling_frequency in
> commit 168c9d95a940 ("iio:adc:max1363 move from staging.")
> and ad799x in
> commit ba1d79613df3 ("staging:iio:ad799x: Use event spec for threshold
> hysteresis")
> but went undocumented so far.

So, it was no documentation for the nodes this change describes, right?

...

> +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency
> +KernelVersion:	6.15

Then why don't you put the real version of the first release that has it?

> +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.15

Ditto.

> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Hardware dependent values supported by the oversampling
> +		frequency.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] iio: code: mark iio_dev as const in iio_buffer_enabled

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 01:34:47PM +0200, Jorge Marques wrote:
> The iio_dev struct is never modified inside the method, mark it as
> const.
> This allows to be called from get_current_scan_type, and is useful
> when the scan_type depends on the buffer state.

Assuming it compiles and works,
Reviewed-by: Andy Shevchenko <andy@kernel.org>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques 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.

...

+ array_size.h

> +#include <linux/bitfield.h>

+ bitops.h
+ completion.h

> +#include <linux/delay.h>

+ dev_printk.h
+ err.h

> +#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>

+ jiffies.h
+ math.h

> +#include <linux/pm_runtime.h>

+ property.h

> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/offload/consumer.h>
> +#include <linux/spi/offload/provider.h>

+ string.h
+ types.h

+ asm/byteorder.h

...

> +#define AD4052_FS(g)		((&ad4052_conversion_freqs[AD4052_FS_OFFSET(g)]))

Why double parentheses? What does this mean?

...

> +#define AD4052_FS_LEN(g)	(ARRAY_SIZE(ad4052_conversion_freqs) - (AD4052_FS_OFFSET(g)))

Too many parentheses.

...

> +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)

Leave trailing comma.

> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> +				      BIT(IIO_EV_INFO_HYSTERESIS)

Ditto.

> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> +				      BIT(IIO_EV_INFO_HYSTERESIS)

Ditto.

> +	}
> +};

...

> +static const char *const ad4052_conversion_freqs[] = {
> +	"2000000", "1000000", "300000", "100000", "33300",
> +	"10000", "3000", "500", "333", "250", "200",
> +	"166", "140", "124", "111",

Better to format with equal amount of members per line (usually power-of-two)
with a comment.

	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
	"33300", "10000", "3000", "500",		/*  4 -  7 */
	"333", "250", "200", "166",			/*  8 - 11 */
	"140", "124", "111",				/* 12 - 15 */

And why these are string literals?

> +};

...

> +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 sprintf(buf, "%s\n", ad4052_conversion_freqs[st->events_frequency]);

You should use sysfs_emit() from sysfs.h.

> +}

...

> +static ssize_t ad4052_events_frequency_available_show(struct device *dev,
> +						      struct device_attribute *attr,
> +						      char *buf)
> +{
> +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int len = 0;
> +
> +	for (u8 i = AD4052_FS_OFFSET(st->grade);
> +	     i < AD4052_FS_LEN(st->grade); i++)
> +		len += sprintf(buf + len, "%s ", ad4052_conversion_freqs[i]);
> +
> +	return sprintf(buf + len, "\n") + len;

sysfs_emit_at(). Use of sprintf() is quite wrong here even if don't care about
sysfs_emit*() APIs.

> +}

> +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> +		       ad4052_events_frequency_show,
> +		       ad4052_events_frequency_store, 0);
> +
> +static IIO_DEVICE_ATTR(sampling_frequency_available, 0444,
> +		       ad4052_events_frequency_available_show,
> +		       NULL, 0);

Please, move each of them closer to the callback. Also, why not
IIO_DEVICE_ATTR_RO() to begin with?

...

> +static struct attribute *ad4052_event_attributes[] = {
> +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,

No comma in terminator entry.

> +};

...

> +static void 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);

> +

Unneeded blank line.

> +	if (IS_ERR(scan_type))
> +		return;
> +
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> +}
> +
> +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->xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +

Ditto.

> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	xfer = &st->offload_xfer;
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> +
> +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +	st->offload_msg.offload = st->offload;
> +
> +	return spi_optimize_message(st->spi, &st->offload_msg);
> +}

...

> +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))

Too many parentheses.

> +		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;
> +	}
> +
> +	ad4052_update_xfer_raw(indio_dev, chan);
> +
> +	return 0;
> +}

...

> +static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
> +					 unsigned int *val)
> +{
> +	int ret;
> +
> +	if (st->mode == AD4052_SAMPLE_MODE) {
> +		*val = 1;
> +		return 0;
> +	}
> +
> +	ret = regmap_read(st->regmap, AD4052_REG_AVG_CONFIG, val);
> +	if (ret)
> +		return ret;
> +
> +	*val = BIT(*val + 1);

Please, introduce a local variable and use it. This one looks bad because it
will write into output knowing when it's an error case.

> +	return 0;
> +}

...

> +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> +{
> +	struct pwm_state pwm_st;
> +
> +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))

in_range() from minmax.h?

> +		return -EINVAL;
> +
> +	pwm_get_state(st->cnv_pwm, &pwm_st);
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> +}

...

> +static int ad4052_soft_reset(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> +	for (int i = 0; i < 3; i++)
> +		st->buf_reset_pattern[6 * (i + 1) - 1] = 0xFE;

Only three times and simple oneliner, can we unroll the loop and show
the indices explicitly? It will help a lot in understanding what the actual
pattern is.

> +	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)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);

> +

Unneeded blank line.

> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	u8 val = FIELD_PREP(AD4052_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> +				 val);
> +	if (ret)
> +		return ret;

> +	val = 0;
> +	if (scan_type->sign == 's')
> +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> +
> +	st->data_format = val;

Why not simply:

	if (scan_type->sign == 's')
		st->data_format = val | AD4052_ADC_MODES_DATA_FORMAT;
	else
		st->data_format = 0;

?

> +	if (st->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> +					      AD4052_TIMER_CONFIG_300KSPS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> +}

...

> +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 = 0;

Why an assignment?

> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	ret = devm_request_threaded_irq(dev, ret, NULL,
> +					ad4052_irq_handler_thresh,
> +					IRQF_ONESHOT, indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> +	if (ret <= 0)

Ahy comparison to 0?

> +		return ret ? ret : -EINVAL;

This is not needed in such a form. Please, read the above API's doc and act
accordingly.

> +	st->gp1_irq = ret;
> +	return devm_request_threaded_irq(dev, ret, NULL,
> +					 ad4052_irq_handler_drdy,
> +					 IRQF_ONESHOT, indio_dev->name,
> +					 st);
> +}

...

> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> +{
> +	struct spi_device *spi = st->spi;
> +	int ret;

> +	struct spi_transfer t_cnv = {
> +		.len = 0

Missing comma, but...

> +	};

This all is not needed, just make it

	struct spi_transfer t_cnv = {};

> +	reinit_completion(&st->completion);
> +
> +	if (st->cnv_gp) {
> +		gpiod_set_value_cansleep(st->cnv_gp, 1);

No delay?

> +		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.
> +	 */
> +	ret = wait_for_completion_timeout(&st->completion,
> +					  msecs_to_jiffies(1000));

Where msec_to_jiffies() is defined?

> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (st->xfer.len == 2) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 15);

Where sign_extend32() is defined?

> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 23);
> +	}
> +
> +	return ret;
> +}

...

> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {

> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;

Inconsistent approach, use the same goto.

> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;

You may have a great deal of reducing or at least making this readable if split
to two, one is claim_direct wrapped.

> +}
> +
> +static int ad4052_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long info)

Ditto for the above function. At least try and see the result. I believe that
it might even shrink number of LoCs.

...

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

> +

Unneeded blank line.

> +	if (ret)
> +		pm_runtime_resume_and_get(&st->spi->dev);
> +
> +	return ret;
> +}

...

> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	u8 reg, size = 1;

Make size assignment explicit in each case, it will help a lot for both:
maintaining in long term and reading the code.

> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = AD4052_REG_MAX_LIMIT;
> +		else
> +			reg = AD4052_REG_MIN_LIMIT;
> +		size = 2;
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = AD4052_REG_MAX_HYST;
> +		else
> +			reg = AD4052_REG_MIN_HYST;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto out_release;
> +	}
> +
> +	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
> +	if (ret)
> +		goto out_release;
> +
> +	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 11);

Better pattern is to use local variable and assign if and only if the function
returns success.

> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;

Again, try with a wrapper.

> +}

...

> +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)

Same comments as per previous one.

> +static int ad4052_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	pwm_disable(st->cnv_pwm);
> +
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +	spi_unoptimize_message(&st->offload_msg);
> +	enable_irq(st->gp1_irq);
> +
> +	ret = ad4052_exit_command(st);

You leave IRQ enabled even in error case, is it on purpose?

> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	return ret;
> +}

...

> +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,
> +	};
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	indio_dev->setup_ops = &ad4052_buffer_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");

One line?

> +	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");

Can be one line (you already have above 91 character long line).

> +	pwm_init_state(st->cnv_pwm, &pwm_st);
> +
> +	pwm_st.enabled = false;
> +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;

> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> +					 AD4052_MAX_RATE(st->grade));

It can be one line (83 characters).


> +	ret = pwm_apply_might_sleep(st->cnv_pwm, &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_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;

> +	int ret = 0;

What is this for?

> +	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);
> +
> +	st->regmap = devm_regmap_init_spi(spi, &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->wait_event = false;
> +	st->chip = chip;
> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> +	st->events_frequency = AD4052_FS_OFFSET(st->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_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad4052_info;
> +	indio_dev->name = chip->name;
> +
> +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
> +	if (IS_ERR(st->offload))
> +		return PTR_ERR(st->offload);

> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get offload\n");

Huh?! Leftover?

> +	if (ret == -ENODEV) {
> +		st->offload_trigger = NULL;
> +		indio_dev->channels = chip->channels;

How this is not a dead code, please?

> +	} else {
> +		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");
> +	}
> +
> +	st->xfer.rx_buf = &st->d32;
> +
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			   AD4052_DEVICE_STATUS_DEVICE_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_request_irq(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> +
> +	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_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> +	return ret;

Redundant local variable.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[Rob Herring (Arm)]

On Tue, 22 Apr 2025 13:34:48 +0200, Jorge Marques 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 device contains one input (cnv) and two outputs (gp0, gp1).
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4052.yaml    | 98 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 104 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

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

[David Lechner]

On 4/22/25 6:34 AM, Jorge Marques wrote:

...

> Devices with this feature are max1363, ad7606, ad799x, and ad4052.
> The max1363 driver included the events/sampling_frequency in
> commit 168c9d95a940 ("iio:adc:max1363 move from staging.")
> and ad799x in
> commit ba1d79613df3 ("staging:iio:ad799x: Use event spec for threshold
> hysteresis")
> but went undocumented so far.

It looks like this part was copied from a different commit and isn't related
to this one.

> 
> 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 33c09c4ac60a4feec82308461643134f5ba84b66..129061befb21b82a51142a01a94d96fcf1b60072 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.15
> +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.

Don't need a newline after every period.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency_available
> +KernelVersion:	6.15
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Hardware dependent values supported by the oversampling
> +		frequency.

		oversampling_frequency attribute.

> +
>  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 v2 4/5] docs: iio: new docs for ad4052 driver

[David Lechner]

On 4/22/25 6:34 AM, Jorge Marques wrote:
> 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 | 95 ++++++++++++++++++++++++++++++++++++++++++++

Also need to update the table of contents in Documentation/iio/index.rst,
otherwise this page won't be build (and will cause a build error).

You can run `make htmldocs SPHINXDIRS=iio` to speed things up and only build
the iio directory to make sure you have it right.

More info: https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html

>  MAINTAINERS                  |  1 +
>  2 files changed, 96 insertions(+)
> 
> diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..410aaa437ed5fea6a2924d374fa5f816f5754e22
> --- /dev/null
> +++ b/Documentation/iio/ad4052.rst
> @@ -0,0 +1,95 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=============
> +AD4052 driver
> +=============
> +
> +ADC driver for Analog Devices Inc. AD4052 and similar devices.

Please don't put newline after every period. Here and throughout the document.
It makes it harder to read.

> +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 a 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 contain only one channels, and the following attributes:
> +
> +.. list-table:: Driver attributes
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Description
> +   * - ``in_voltage0_raw``
> +     - Raw ADC voltage value

No scale attribute? How do we convert raw to millivolts?

> +   * - ``in_voltage0_oversampling_ratio``
> +     - Enable the device's burst averaging mode to over sample using
> +       the internal sample rate.
> +   * - ``in_voltage0_oversampling_ratio_available``
> +     - List of available oversampling values. Value 0 disable the burst
> +       averaging mode.

Typically 1 means no oversampling, not zero. (It is a ratio, divide by 1 is the
same as doing nothing, but divide by 0 is undefined.)

> +   * - ``conversion_frequency``

Needs to be updated to ``oversampling_frequency``.

> +     - Device internal sample rate used in the burst averaging mode.
> +   * - ``conversion_frequency_available``
> +     - List of available sample rates.
> +
> +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.

Probably worth mentioning the ``events/sampling_frequency`` and
``sampling_frequency_available`` attributes since we've mentioned all of the
other attributes.

> +
> +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_voltage0_sampling_frequency``
> +     - Set the sampling frequency.
> +   * - ``in_voltage0_sampling_frequency_available``
> +     - Get the sampling frequency range.

In the driver, this is currently info_mask_shared_by_type, so would be
``in_voltage_sampling_frequency``. And there currently isn't 
``in_voltage_sampling_frequency_available`` in the driver, so it needs to be
added in the driver (or removed here).

> +
> +The scan type is different when the buffer with offload support is enabled.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 81fbe7176475c48eae03ab04115d4ef5b6299fac..04aa8db44bee418382a2e74cb6b1d03a810bd781 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1334,6 +1334,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 AD4130 DRIVER
>  M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
> 

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

On 4/22/25 6:34 AM, Jorge Marques 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 device contains one input (cnv) and two outputs (gp0, gp1).

Don't need line breaks after every period.

> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---

...

> +  interrupts:
> +    items:
> +      - description: Signal coming from the GP0 pin (threshold).
> +      - description: Signal coming from the GP1 pin (data ready).
> +
> +  interrupt-names:
> +    items:
> +      - const: gp0
> +      - const: gp1
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +    description: |
> +      The first cell is the GPn number: 0 to 1.
> +      The second cell takes standard GPIO flags.
> +
> +  cnv-gpios:
> +    description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
> +    maxItems: 1
> +

Assuming the diagram at [1] is correct, for SPI offload use, we are missing:

  #trigger-source-cells:
    const: 2
    description: |
      Output pins used as trigger source.

      Cell 0 defines which pin:
      * 0 = GP0
      * 1 = GP1

      Cell 1 defines the event:
      * 0 = Data ready
      * 1 = Min threshold
      * 2 = Max threshold
      * 3 = Either threshold
      * 4 = Device ready
      * 5 = Device enable
      * 6 = Chop control

Bonus points for adding a header with macros for the arbitrary event values.

And we are missing:

  pwms:
    maxItems: 1
    description: PWM connected to the CNV pin.

[1]: https://analogdevicesinc.github.io/hdl/projects/ad4052_ardz/index.html

> +  spi-max-frequency:
> +    maximum: 62500000

Datasheet Table 5. SPI Timing—ADC Modes, VIO ≥ 3.0 V says period can be 12 ns.

So that would make max frequency 83333333.

...

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "adi,ad4052";
> +            reg = <0>;
> +            vdd-supply = <&adc_vdd>;
> +            vio-supply = <&adc_vio>;
> +            spi-max-frequency = <25000000>;
> +
> +            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>;
> +        };
> +    };

Could be nice to have a 2nd example showing SPI offload usage.

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[David Lechner]

On 4/22/25 6:34 AM, Jorge Marques 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>
> ---
>  MAINTAINERS              |    1 +
>  drivers/iio/adc/Kconfig  |   14 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++

This patch is way too big, so I didn't review most of it yet. But time to call
it quits for today. In the future, it would be a lot easier for reviewers if
you can split things into multiple patches instead of implementing all of the
features at once. E.g. start with just a basic driver, then a patch to add
oversampling support, then another patch to add SPI offload support. 500 lines
is a more manageable size for review.

...

> +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->xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	xfer = &st->offload_xfer;
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);

This doesn't work for oversampling. realbits may be 16 while storagebits is 32.
But the SPI controller needs to know how many realbits-sized words to read.

So this should be 

	xfer->len = BITS_TO_BYTES(scan_type->realbits);



> +
> +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +	st->offload_msg.offload = st->offload;
> +
> +	return spi_optimize_message(st->spi, &st->offload_msg);

I know it is like this in a few other drivers already, but I don't like having
spi_optimize_message() in this funtion because it makes it really easy to
forget to do have balanced calls to spi_unoptimize_message().

> +}
> +

...

> +static const struct iio_buffer_setup_ops ad4052_buffer_setup_ops = {
> +	.postenable = &ad4052_buffer_postenable,
> +	.predisable = &ad4052_buffer_predisable,
> +};

Would be nice to add "offload" to the name of this struct and the callbacks
to make it clear that these are only for the SPI offload use case.

...

> +
> +static bool ad4052_offload_trigger_match(struct spi_offload_trigger *trigger,
> +					 enum spi_offload_trigger_type type,
> +					 u64 *args, u32 nargs)
> +{

We should be checking the args here according to what I suggested in my reply
to the devicetree bindings patch. Right now it is assuming that we are only
using this for SPI offload and that the pin used is GP1 and the event is data
read. We should at least verify that the args match those assumptions.

For bonus points, we could implement allowing GPO as well.

> +	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,
> +	};
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	indio_dev->setup_ops = &ad4052_buffer_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");

Strictly speaking, the trigger-source provider is indendant of using it for
SPI offload. I guess this is fine here for now though.

> +
> +	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, &pwm_st);
> +
> +	pwm_st.enabled = false;
> +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> +					 AD4052_MAX_RATE(st->grade));
> +
> +	ret = pwm_apply_might_sleep(st->cnv_pwm, &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_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;
> +	int ret = 0;
> +
> +	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);
> +
> +	st->regmap = devm_regmap_init_spi(spi, &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->wait_event = false;
> +	st->chip = chip;
> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> +	st->events_frequency = AD4052_FS_OFFSET(st->grade);

Somewhere around here, we should be turning on the power supplies. Also, it
looks like we need some special handling to get the reference volage. If there
is a supply connected to REF, use that, if not, use VDD which requires writing
to a register to let the chip know.

> +
> +	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_BUFFER_HARDWARE | INDIO_DIRECT_MODE;

INDIO_BUFFER_HARDWARE should not be set here. If using SPI offload,
devm_iio_dmaengine_buffer_setup_with_handle() will add it automatically.
For non-SPI-offload operation, it should not be set.

> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad4052_info;
> +	indio_dev->name = chip->name;
> +
> +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);

This

> +	if (IS_ERR(st->offload))
> +		return PTR_ERR(st->offload);

should be

	ret = PTR_ERR_OR_ZERO(st->offload);

> +
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get offload\n");
> +
> +	if (ret == -ENODEV) {
> +		st->offload_trigger = NULL;
> +		indio_dev->channels = chip->channels;
> +	} else {
> +		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");
> +	}
> +
> +	st->xfer.rx_buf = &st->d32;

I don't think we want this set globally. I.e. it doesn't make sense for SPI
offload xfers.

> +
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			   AD4052_DEVICE_STATUS_DEVICE_RESET);

Why not include this in ad4052_setup() or even ad4052_soft_reset()?

> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_request_irq(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> +
> +	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_DEVICE_CONFIG_POWER_MODE_MSK,
> +				       AD4052_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_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));

regmap_clear_bits() would be shorter if there isn't going to be a macro to
explain the meaning of 0.

> +	return ret;
> +}
> +

Re: [PATCH v2 2/5] iio: code: mark iio_dev as const in iio_buffer_enabled

[Jonathan Cameron]

On Tue, 22 Apr 2025 13:34:47 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> The iio_dev struct is never modified inside the method, mark it as
> const.
> This allows to be called from get_current_scan_type, and is useful
> when the scan_type depends on the buffer state.
Now I'm confused.   scan type is only relevant when the buffer is enabled
so how can it change as a result of that action?

Maybe all will become clear in later patches!

Jonathan

> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 2 +-
>  include/linux/iio/iio.h         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 178e99b111debc59a247fcc3a6037e429db3bebf..bc6a2ac6415eccf201e148ea98c0b5982787eb6d 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -212,7 +212,7 @@ EXPORT_SYMBOL_GPL(iio_device_id);
>   *
>   * Returns: True, if the buffer is enabled.
>   */
> -bool iio_buffer_enabled(struct iio_dev *indio_dev)
> +bool iio_buffer_enabled(const struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..88569e1a888bde4d2bfb5b9f030096af1c15d68d 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -629,7 +629,7 @@ struct iio_dev {
>  
>  int iio_device_id(struct iio_dev *indio_dev);
>  int iio_device_get_current_mode(struct iio_dev *indio_dev);
> -bool iio_buffer_enabled(struct iio_dev *indio_dev);
> +bool iio_buffer_enabled(const struct iio_dev *indio_dev);
>  
>  const struct iio_chan_spec
>  *iio_find_channel_from_si(struct iio_dev *indio_dev, int si);
> 

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Jonathan Cameron]

On Tue, 22 Apr 2025 13:34:50 +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,

A few additional comments from me.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f412f0884bab4f500091f6c7ca761967c8f6e3b6
> --- /dev/null
> +++ b/drivers/iio/adc/ad4052.c
> @@ -0,0 +1,1425 @@


> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}


> +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 = -EINVAL;
> +	u8 reg, size = 1;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;

Sometimes it is worth a n internal function factored out in cases
like this to allow direct returns in error cases with the release
always happening before we check if the inner function failed.

That suggestion applies in other places in this code.
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT) {
> +				if (val > 2047 || val < -2048)
> +					goto out_release;
> +			} else if (val > 4095 || val < 0) {
> +				goto out_release;
> +			}
> +			if (dir == IIO_EV_DIR_RISING)
> +				reg = AD4052_REG_MAX_LIMIT;
> +			else
> +				reg = AD4052_REG_MIN_LIMIT;
> +			size = 2;
> +			st->d16 = cpu_to_be16(val);
> +			break;
> +		case IIO_EV_INFO_HYSTERESIS:
> +			if (val & BIT(7))
> +				goto out_release;
> +			if (dir == IIO_EV_DIR_RISING)
> +				reg = AD4052_REG_MAX_HYST;
> +			else
> +				reg = AD4052_REG_MIN_HYST;
> +			st->d16 = cpu_to_be16(val >> 8);
> +			break;
> +		default:
> +			goto out_release;
> +		}
> +		break;
> +	default:
> +		goto out_release;
> +	}
> +
> +	ret = regmap_bulk_write(st->regmap, reg, &st->d16, size);
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}
> +
> +static int ad4052_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;
> +
> +	if (st->wait_event)
> +		return -EBUSY;
> +
> +	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_update_xfer_offload(indio_dev, indio_dev->channels);
> +	if (ret)
> +		goto out_error;
> +
> +	/* SPI Offload handles the data ready irq */
> +	disable_irq(st->gp1_irq);
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
> +	if (ret)
> +		goto out_offload_error;
> +
> +	ret = pwm_enable(st->cnv_pwm);
> +	if (ret)
> +		goto out_pwm_error;
> +
> +	return 0;
> +
> +out_pwm_error:
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +out_offload_error:
> +	enable_irq(st->gp1_irq);
> +	spi_unoptimize_message(&st->offload_msg);
> +	ad4052_exit_command(st);

What is this matching to?  Feels like it would be set_operation_mode()
but I may be wrong on that.  If it is then you need another label
to call only this update_xfer_offload fails.

> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->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 (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
Probably use a goto to release this in only one place.

> +		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);
> +
> +	if (iio_buffer_enabled(indio_dev))

This is the bit I'm not really following. Why is the enabling or not of
the buffer related to whether offload is going on?


> +		return st->mode == AD4052_BURST_AVERAGING_MODE ?
> +				   AD4052_SCAN_TYPE_OFFLOAD_BURST_AVG :
> +				   AD4052_SCAN_TYPE_OFFLOAD_SAMPLE;
> +
> +	return st->mode == AD4052_BURST_AVERAGING_MODE ?
> +			   AD4052_SCAN_TYPE_BURST_AVG :
> +			   AD4052_SCAN_TYPE_SAMPLE;
> +}


> +static int ad4052_probe(struct spi_device *spi)
> +{
...

> +
> +	st->mode = AD4052_SAMPLE_MODE;
> +	st->wait_event = false;
> +	st->chip = chip;
> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;

That feels like it should be encoded directly in chip.  Basing it
on prod_id seems liable to bite us at somepoint in the future.




> +
> +static int ad4052_runtime_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> +	return ret;

	return regmap_write();
and no need for the local variable ret.

> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(ad4052_pm_ops, ad4052_runtime_suspend,
> +				 ad4052_runtime_resume, NULL);

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

[Jorge Marques]

Hi Andy,

I agree with your suggestion, and in this case the appropriate kernel
version is 3.10.

On Tue, Apr 22, 2025 at 06:50:19PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 22, 2025 at 01:34:46PM +0200, Jorge Marques wrote:
> > 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.
> > 
> > Devices with this feature are max1363, ad7606, ad799x, and ad4052.
> > The max1363 driver included the events/sampling_frequency in
> > commit 168c9d95a940 ("iio:adc:max1363 move from staging.")
> > and ad799x in
> > commit ba1d79613df3 ("staging:iio:ad799x: Use event spec for threshold
> > hysteresis")
> > but went undocumented so far.
> 
> So, it was no documentation for the nodes this change describes, right?
> 
> ...
> 
> > +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency
> > +KernelVersion:	6.15
> 
> Then why don't you put the real version of the first release that has it?
> 
> > +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.15
> 
> Ditto.
> 
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Hardware dependent values supported by the oversampling
> > +		frequency.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

Best regards,
Jorge

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

[Jorge Marques]

Hi David,

On Fri, Apr 25, 2025 at 04:16:20PM -0500, David Lechner wrote:
> On 4/22/25 6:34 AM, Jorge Marques wrote:
> 
> ...
> 
> > Devices with this feature are max1363, ad7606, ad799x, and ad4052.
> > The max1363 driver included the events/sampling_frequency in
> > commit 168c9d95a940 ("iio:adc:max1363 move from staging.")
> > and ad799x in
> > commit ba1d79613df3 ("staging:iio:ad799x: Use event spec for threshold
> > hysteresis")
> > but went undocumented so far.
> 
> It looks like this part was copied from a different commit and isn't related
> to this one.
> 

You are right, this is from the other already applied patch, I will remove.

> > 
> > 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 33c09c4ac60a4feec82308461643134f5ba84b66..129061befb21b82a51142a01a94d96fcf1b60072 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.15
> > +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.
> 
> Don't need a newline after every period.

Ack.

> 
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency_available
> > +KernelVersion:	6.15
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Hardware dependent values supported by the oversampling
> > +		frequency.
> 
> 		oversampling_frequency attribute.
> 

Ack.

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

Regards,
Jorge

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

Hi David, 

I didn't went through your's and Jonathan's ad4052.c review yet,
but for the trigger-source-cells I need to dig deeper and make
considerable changes to the driver, as well as hardware tests.
My idea was to have a less customizable driver, but I get that it is
more interesting to make it user-definable.

On Fri, Apr 25, 2025 at 05:50:58PM -0500, David Lechner wrote:
> On 4/22/25 6:34 AM, Jorge Marques 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 device contains one input (cnv) and two outputs (gp0, gp1).
> 
> Don't need line breaks after every period.

Ack.

> 
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> 
> ...
> 
> > +  interrupts:
> > +    items:
> > +      - description: Signal coming from the GP0 pin (threshold).
> > +      - description: Signal coming from the GP1 pin (data ready).
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: gp0
> > +      - const: gp1
> > +
> > +  gpio-controller: true
> > +
> > +  "#gpio-cells":
> > +    const: 2
> > +    description: |
> > +      The first cell is the GPn number: 0 to 1.
> > +      The second cell takes standard GPIO flags.
> > +
> > +  cnv-gpios:
> > +    description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
> > +    maxItems: 1
> > +
> 
> Assuming the diagram at [1] is correct, for SPI offload use, we are missing:
> 
>   #trigger-source-cells:
>     const: 2
>     description: |
>       Output pins used as trigger source.
> 
>       Cell 0 defines which pin:
>       * 0 = GP0
>       * 1 = GP1
> 
>       Cell 1 defines the event:
>       * 0 = Data ready
>       * 1 = Min threshold
>       * 2 = Max threshold
>       * 3 = Either threshold
>       * 4 = Device ready
>       * 5 = Device enable
>       * 6 = Chop control
> 
> Bonus points for adding a header with macros for the arbitrary event values.

In the sense of describing the device and not what the driver does, I
believe the proper mapping would be:

  Cell 1 defines the event:
  * 0 = Disabled
  * 1 = Data ready
  * 2 = Min threshold
  * 3 = Max threshold
  * 4 = Either threshold
  * 5 = CHOP control
  * 6 = Device enable
  * 7 = Device ready (only GP1)

I will investigate further this.

> 
> And we are missing:
> 
>   pwms:
>     maxItems: 1
>     description: PWM connected to the CNV pin.
> 
> [1]: https://analogdevicesinc.github.io/hdl/projects/ad4052_ardz/index.html
> 
> > +  spi-max-frequency:
> > +    maximum: 62500000
> 
> Datasheet Table 5. SPI Timing—ADC Modes, VIO ≥ 3.0 V says period can be 12 ns.
> 
> So that would make max frequency 83333333.

Ack.

> 
> ...
> 
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@0 {
> > +            compatible = "adi,ad4052";
> > +            reg = <0>;
> > +            vdd-supply = <&adc_vdd>;
> > +            vio-supply = <&adc_vio>;
> > +            spi-max-frequency = <25000000>;
> > +
> > +            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>;
> > +        };
> > +    };
> 
> Could be nice to have a 2nd example showing SPI offload usage.
> .

Will add.


Regards,
Jorge

Re: [PATCH v2 4/5] docs: iio: new docs for ad4052 driver

[Jorge Marques]

On Fri, Apr 25, 2025 at 04:44:20PM -0500, David Lechner wrote:
> On 4/22/25 6:34 AM, Jorge Marques wrote:
> > 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 | 95 ++++++++++++++++++++++++++++++++++++++++++++
> 
> Also need to update the table of contents in Documentation/iio/index.rst,
> otherwise this page won't be build (and will cause a build error).
> 
> You can run `make htmldocs SPHINXDIRS=iio` to speed things up and only build
> the iio directory to make sure you have it right.
> 
> More info: https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html
> 

Yes, sorry.

> >  MAINTAINERS                  |  1 +
> >  2 files changed, 96 insertions(+)
> > 
> > diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..410aaa437ed5fea6a2924d374fa5f816f5754e22
> > --- /dev/null
> > +++ b/Documentation/iio/ad4052.rst
> > @@ -0,0 +1,95 @@
> > +.. SPDX-License-Identifier: GPL-2.0-only
> > +
> > +=============
> > +AD4052 driver
> > +=============
> > +
> > +ADC driver for Analog Devices Inc. AD4052 and similar devices.
> 
> Please don't put newline after every period. Here and throughout the document.
> It makes it harder to read.
> 

Ack, that was a personal touch that at the time was easier for me to
read ahah. Luckly, vim's `gqq` can flawlessly convert.

> > +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 a 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 contain only one channels, and the following attributes:
> > +
> > +.. list-table:: Driver attributes
> > +   :header-rows: 1
> > +
> > +   * - Attribute
> > +     - Description
> > +   * - ``in_voltage0_raw``
> > +     - Raw ADC voltage value
> 
> No scale attribute? How do we convert raw to millivolts?
> 

I will add here and to the driver.

> > +   * - ``in_voltage0_oversampling_ratio``
> > +     - Enable the device's burst averaging mode to over sample using
> > +       the internal sample rate.
> > +   * - ``in_voltage0_oversampling_ratio_available``
> > +     - List of available oversampling values. Value 0 disable the burst
> > +       averaging mode.
> 
> Typically 1 means no oversampling, not zero. (It is a ratio, divide by 1 is the
> same as doing nothing, but divide by 0 is undefined.)
> 

Outdated doc, fixed.

> > +   * - ``conversion_frequency``
> 
> Needs to be updated to ``oversampling_frequency``.
> 

Ack.

> > +     - Device internal sample rate used in the burst averaging mode.
> > +   * - ``conversion_frequency_available``
> > +     - List of available sample rates.
> > +
> > +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.
> 
> Probably worth mentioning the ``events/sampling_frequency`` and
> ``sampling_frequency_available`` attributes since we've mentioned all of the
> other attributes.
> 

Ack.

> > +
> > +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_voltage0_sampling_frequency``
> > +     - Set the sampling frequency.
> > +   * - ``in_voltage0_sampling_frequency_available``
> > +     - Get the sampling frequency range.
> 
> In the driver, this is currently info_mask_shared_by_type, so would be
> ``in_voltage_sampling_frequency``. And there currently isn't 
> ``in_voltage_sampling_frequency_available`` in the driver, so it needs to be
> added in the driver (or removed here).

Removed here, it is constrained by the PWM trigger.

> 
> > +
> > +The scan type is different when the buffer with offload support is enabled.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 81fbe7176475c48eae03ab04115d4ef5b6299fac..04aa8db44bee418382a2e74cb6b1d03a810bd781 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1334,6 +1334,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 AD4130 DRIVER
> >  M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
> > 
> 

Sorry about this submission, this file was indeed not updated between
version.

Regards,
Jorge

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Jorge Marques]

Hi Andy, thank you for the review.
I just have comments on
The ad4052_conversion_freqs string literal [#f0].
iio_device_claim_direct_scoped (that could be
used to organize some parts of the code, but can't because the macro was
removed) [#f1].
And why there is no delay [#f2].

On Tue, Apr 22, 2025 at 07:33:07PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques 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.
> 
> ...
> 
> + array_size.h
> 
> > +#include <linux/bitfield.h>
> 
> + bitops.h
> + completion.h
> 
> > +#include <linux/delay.h>
> 
> + dev_printk.h
> + err.h
> 
> > +#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>
> 
> + jiffies.h
> + math.h
> 
> > +#include <linux/pm_runtime.h>
> 
> + property.h
> 
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/offload/consumer.h>
> > +#include <linux/spi/offload/provider.h>
> 
> + string.h
> + types.h
> 
> + asm/byteorder.h

Ack.

> 
> ...
> 
> > +#define AD4052_FS(g)		((&ad4052_conversion_freqs[AD4052_FS_OFFSET(g)]))
> 
> Why double parentheses? What does this mean?

Dropped.

> 
> ...
> 
> > +#define AD4052_FS_LEN(g)	(ARRAY_SIZE(ad4052_conversion_freqs) - (AD4052_FS_OFFSET(g)))
> 
> Too many parentheses.
> 

Dropped.

> ...
> 
> > +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)
> 
> Leave trailing comma.

Ack.
> 
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> > +				      BIT(IIO_EV_INFO_HYSTERESIS)
> 
> Ditto.
Ack.
> 
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> > +				      BIT(IIO_EV_INFO_HYSTERESIS)
> 
> Ditto.
Ack.
> 
> > +	}
> > +};
> 
> ...
> 
> > +static const char *const ad4052_conversion_freqs[] = {
> > +	"2000000", "1000000", "300000", "100000", "33300",
> > +	"10000", "3000", "500", "333", "250", "200",
> > +	"166", "140", "124", "111",
> 
> Better to format with equal amount of members per line (usually power-of-two)
> with a comment.
> 
> 	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
> 	"33300", "10000", "3000", "500",		/*  4 -  7 */
> 	"333", "250", "200", "166",			/*  8 - 11 */
> 	"140", "124", "111",				/* 12 - 15 */
> 
> And why these are string literals?
> 

Ack.
The string literals are because this array is also used in the
iio_enum struct that takes string literals.
[#f0]

> > +};
> 
> ...
> 
> > +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 sprintf(buf, "%s\n", ad4052_conversion_freqs[st->events_frequency]);
> 
> You should use sysfs_emit() from sysfs.h.
> 

Ack.

> > +}
> 
> ...
> 
> > +static ssize_t ad4052_events_frequency_available_show(struct device *dev,
> > +						      struct device_attribute *attr,
> > +						      char *buf)
> > +{
> > +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	int len = 0;
> > +
> > +	for (u8 i = AD4052_FS_OFFSET(st->grade);
> > +	     i < AD4052_FS_LEN(st->grade); i++)
> > +		len += sprintf(buf + len, "%s ", ad4052_conversion_freqs[i]);
> > +
> > +	return sprintf(buf + len, "\n") + len;
> 
> sysfs_emit_at(). Use of sprintf() is quite wrong here even if don't care about
> sysfs_emit*() APIs.
> 

Ack.

> > +}
> 
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> > +		       ad4052_events_frequency_show,
> > +		       ad4052_events_frequency_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(sampling_frequency_available, 0444,
> > +		       ad4052_events_frequency_available_show,
> > +		       NULL, 0);
> 
> Please, move each of them closer to the callback. Also, why not
> IIO_DEVICE_ATTR_RO() to begin with?
> 

Moved closer to the callback.
Yes, IIO_DEVICE_ATTR_RO should have been used here.

> ...
> 
> > +static struct attribute *ad4052_event_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL,
> 
> No comma in terminator entry.
> 
Ack.
> > +};
> 
> ...
> 
> > +static void 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);
> 
> > +
> 
> Unneeded blank line.
> 

Ack.
> > +	if (IS_ERR(scan_type))
> > +		return;
> > +
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> > +}
> > +
> > +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->xfer;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > +
> 
> Ditto.
> 
Ack.
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	xfer = &st->offload_xfer;
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> > +
> > +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> > +	st->offload_msg.offload = st->offload;
> > +
> > +	return spi_optimize_message(st->spi, &st->offload_msg);
> > +}
> 
> ...
> 
> > +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))
> 
> Too many parentheses.
> 
Ack.

> > +		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;
> > +	}
> > +
> > +	ad4052_update_xfer_raw(indio_dev, chan);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
> > +					 unsigned int *val)
> > +{
> > +	int ret;
> > +
> > +	if (st->mode == AD4052_SAMPLE_MODE) {
> > +		*val = 1;
> > +		return 0;
> > +	}
> > +
> > +	ret = regmap_read(st->regmap, AD4052_REG_AVG_CONFIG, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*val = BIT(*val + 1);
> 
> Please, introduce a local variable and use it. This one looks bad because it
> will write into output knowing when it's an error case.
> 
Makes sense.
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> > +{
> > +	struct pwm_state pwm_st;
> > +
> > +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> 
> in_range() from minmax.h?
> 

Done.

> > +		return -EINVAL;
> > +
> > +	pwm_get_state(st->cnv_pwm, &pwm_st);
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> > +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> > +}
> 
> ...
> 
> > +static int ad4052_soft_reset(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> > +	for (int i = 0; i < 3; i++)
> > +		st->buf_reset_pattern[6 * (i + 1) - 1] = 0xFE;
> 
> Only three times and simple oneliner, can we unroll the loop and show
> the indices explicitly? It will help a lot in understanding what the actual
> pattern is.
> 
Of course, done.
> > +	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)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> 
> > +
> 
> Unneeded blank line.
> 
Ok.
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	u8 val = FIELD_PREP(AD4052_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> > +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> > +	int ret;
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> > +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> > +				 val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> > +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> > +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> > +				 val);
> > +	if (ret)
> > +		return ret;
> 
> > +	val = 0;
> > +	if (scan_type->sign == 's')
> > +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> > +
> > +	st->data_format = val;
> 
> Why not simply:
> 
> 	if (scan_type->sign == 's')
> 		st->data_format = val | AD4052_ADC_MODES_DATA_FORMAT;
> 	else
> 		st->data_format = 0;
> 
> ?
> 

Changed to

	if (scan_type->sign == 's')
	        st->data_format = AD4052_ADC_MODES_DATA_FORMAT;
	else
	        st->data_format = 0;

> > +	if (st->grade == AD4052_500KSPS) {
> > +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> > +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> > +					      AD4052_TIMER_CONFIG_300KSPS));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> > +}
> 
> ...
> 
> > +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 = 0;
> 
> Why an assignment?
> 
Ack.

> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> > +	if (ret <= 0)
> > +		return ret ? ret : -EINVAL;
> > +
> > +	ret = devm_request_threaded_irq(dev, ret, NULL,
> > +					ad4052_irq_handler_thresh,
> > +					IRQF_ONESHOT, indio_dev->name,
> > +					indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> > +	if (ret <= 0)
> 
> Ahy comparison to 0?
> 
> > +		return ret ? ret : -EINVAL;
> 
> This is not needed in such a form. Please, read the above API's doc and act
> accordingly.
> 
You are right, I missed the fix [1], sorry.
https://lore.kernel.org/all/Y1gNDtE4dRC4WuP%2F@smile.fi.intel.com/
                             
> > +	st->gp1_irq = ret;   
> > +	return devm_request_threaded_irq(dev, ret, NULL,
> > +					 ad4052_irq_handler_drdy,
> > +					 IRQF_ONESHOT, indio_dev->name,
> > +					 st);
> > +}
> 
> ...
> 
> > +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> > +{
> > +	struct spi_device *spi = st->spi;
> > +	int ret;
> 
> > +	struct spi_transfer t_cnv = {
> > +		.len = 0
> 
> Missing comma, but...
> 
> > +	};
> 
> This all is not needed, just make it
> 
> 	struct spi_transfer t_cnv = {};
Fair, applied.
> 
> > +	reinit_completion(&st->completion);
> > +
> > +	if (st->cnv_gp) {
> > +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> 
> No delay?
> 
In practice it is not needed since tcnv high minimum is 10ns,
no system should switch gpio that fast through these methods.
One approach is to have gpiod_set_value around spi_sync_transfer
"making a sandwich", but since there is the fallback with spi
transfer to also trigger a conversion, this seemed better.
[#f2]
> > +		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.
> > +	 */
> > +	ret = wait_for_completion_timeout(&st->completion,
> > +					  msecs_to_jiffies(1000));
> 
> Where msec_to_jiffies() is defined?
> 
Ack.
> > +	if (!ret)
> > +		return -ETIMEDOUT;
> > +
> > +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->xfer.len == 2) {
> > +		*val = be16_to_cpu(st->d16);
> > +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> > +			*val = sign_extend32(*val, 15);
> 
> Where sign_extend32() is defined?
> 
Ack.
> > +	} else {
> > +		*val = be32_to_cpu(st->d32);
> > +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> > +			*val = sign_extend32(*val, 23);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static int ad4052_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> 
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> 
> Inconsistent approach, use the same goto.
> 
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ad4052_read_chan_raw(indio_dev, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		ret = ad4052_get_oversampling_ratio(st, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		if (!pwm_st.enabled)
> > +			pwm_get_state(st->cnv_pwm, &pwm_st);
> > +
> > +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> 
> You may have a great deal of reducing or at least making this readable if split
> to two, one is claim_direct wrapped.
> 
Done.

> > +}
> > +
> > +static int ad4052_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int val,
> > +			    int val2, long info)
> 
> Ditto for the above function. At least try and see the result. I believe that
> it might even shrink number of LoCs.
> 
> ...
Done.
> 
> > +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);
> 
> > +
> 
> Unneeded blank line.
Ack.
> 
> > +	if (ret)
> > +		pm_runtime_resume_and_get(&st->spi->dev);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	u8 reg, size = 1;
> 
> Make size assignment explicit in each case, it will help a lot for both:
> maintaining in long term and reading the code.
Agreed.
> 
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			reg = AD4052_REG_MAX_LIMIT;
> > +		else
> > +			reg = AD4052_REG_MIN_LIMIT;
> > +		size = 2;
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			reg = AD4052_REG_MAX_HYST;
> > +		else
> > +			reg = AD4052_REG_MIN_HYST;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		goto out_release;
> > +	}
> > +
> > +	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
> > +	if (ret)
> > +		goto out_release;
> > +
> > +	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
> > +		*val = be16_to_cpu(st->d16);
> > +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> > +			*val = sign_extend32(*val, 11);
> 
> Better pattern is to use local variable and assign if and only if the function
> returns success.
Agree.
> 
> > +	} else {
> > +		*val = be32_to_cpu(st->d32);
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> 
> Again, try with a wrapper.
> 
Here all paths require claiming the iio device, so in this case I don't
see a problem.
> > +}
> 
> ...
> 
> > +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)
> 
> Same comments as per previous one.
> 
Same as above.
Here would be useful to use the macro iio_device_claim_direct_scoped.
But it was removed because static analyzers didn't play nice with it.
4c571885898c5 (iio: Drop iio_device_claim_direct_scoped() and related infrastructure)
https://patch.msgid.link/20250209180624.701140-28-jic23@kernel.org
[#f1]

> > +static int ad4052_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	pwm_disable(st->cnv_pwm);
> > +
> > +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> > +	spi_unoptimize_message(&st->offload_msg);
> > +	enable_irq(st->gp1_irq);
> > +
> > +	ret = ad4052_exit_command(st);
> 
> You leave IRQ enabled even in error case, is it on purpose?
> 
Yes, there is not much to be done here.
The important part it to have it enabled after the trigger is disabled,
to not get an IRQ from the offload execution after the buffer is
disabled.
I will just move after the ad4052_exit_command with the pm calls.
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +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,
> > +	};
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	indio_dev->setup_ops = &ad4052_buffer_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");
> 
> One line?
> 
Ok
> > +	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");
> 
> Can be one line (you already have above 91 character long line).
> 
Ok
> > +	pwm_init_state(st->cnv_pwm, &pwm_st);
> > +
> > +	pwm_st.enabled = false;
> > +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
> 
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> > +					 AD4052_MAX_RATE(st->grade));
> 
> It can be one line (83 characters).
> 
Ok
> 
> > +	ret = pwm_apply_might_sleep(st->cnv_pwm, &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_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;
> 
> > +	int ret = 0;
> 
> What is this for?
> 
For Nothing
> > +	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);
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &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->wait_event = false;
> > +	st->chip = chip;
> > +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> > +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> > +	st->events_frequency = AD4052_FS_OFFSET(st->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_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
> > +	indio_dev->num_channels = 1;
> > +	indio_dev->info = &ad4052_info;
> > +	indio_dev->name = chip->name;
> > +
> > +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
> > +	if (IS_ERR(st->offload))
> > +		return PTR_ERR(st->offload);
> 
> > +	if (ret && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get offload\n");
> 
> Huh?! Leftover?
> 
Yes, leftover.
> > +	if (ret == -ENODEV) {
> > +		st->offload_trigger = NULL;
> > +		indio_dev->channels = chip->channels;
> 
> How this is not a dead code, please?
> 
It is dead code.
The important part here is that -ENODEV should be catch to set
offload_triffer NULL, since this feature is optional, both on the driver
as well as for the SPI controllers themself.
> > +	} else {
> > +		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");
> > +	}
> > +
> > +	st->xfer.rx_buf = &st->d32;
> > +
> > +	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);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			   AD4052_DEVICE_STATUS_DEVICE_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_request_irq(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> > +
> > +	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_resume(struct device *dev)
> > +{
> > +	struct ad4052_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> > +	return ret;
> 
> Redundant local variable.
Ack.
> 
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Regards,
Jorge

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

[David Lechner]

On 4/29/25 8:47 AM, Jorge Marques wrote:
> 
> Hi Andy,
> 
> I agree with your suggestion, and in this case the appropriate kernel
> version is 3.10.
> 
>>
>>> +What:		/sys/bus/iio/devices/iio:deviceX/oversampling_frequency
>>> +KernelVersion:	6.15
>>
>> Then why don't you put the real version of the first release that has it?
>>
>>> +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.15
>>
>> Ditto.
>>
>>> +Contact:	linux-iio@vger.kernel.org
>>> +Description:
>>> +		Hardware dependent values supported by the oversampling
>>> +		frequency.


I don't see oversampling_frequency used in any existing driver, so how could
it be introduced in kernel 3.10? I think you confuse it with
events/sampling_frequency.

oversampling_frequency is new and so 6.16 should be correct if Jonathan picks
this up in the next few weeks, otherwise it will be 6.17.

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

On 4/29/25 8:48 AM, Jorge Marques wrote:
> Hi David, 
> 
> I didn't went through your's and Jonathan's ad4052.c review yet,
> but for the trigger-source-cells I need to dig deeper and make
> considerable changes to the driver, as well as hardware tests.
> My idea was to have a less customizable driver, but I get that it is
> more interesting to make it user-definable.

We don't need to make the driver support all possibilities, but the devicetree
needs to be as complete as possible since it can't be as easily changed in the
future.

...

>>
>> Assuming the diagram at [1] is correct, for SPI offload use, we are missing:
>>
>>   #trigger-source-cells:
>>     const: 2
>>     description: |
>>       Output pins used as trigger source.
>>
>>       Cell 0 defines which pin:
>>       * 0 = GP0
>>       * 1 = GP1
>>
>>       Cell 1 defines the event:
>>       * 0 = Data ready
>>       * 1 = Min threshold
>>       * 2 = Max threshold
>>       * 3 = Either threshold
>>       * 4 = Device ready
>>       * 5 = Device enable
>>       * 6 = Chop control
>>
>> Bonus points for adding a header with macros for the arbitrary event values.
> 
> In the sense of describing the device and not what the driver does, I
> believe the proper mapping would be:
> 
>   Cell 1 defines the event:
>   * 0 = Disabled
>   * 1 = Data ready
>   * 2 = Min threshold
>   * 3 = Max threshold
>   * 4 = Either threshold
>   * 5 = CHOP control
>   * 6 = Device enable
>   * 7 = Device ready (only GP1)
> 
> I will investigate further this.
> 
>>

0 = Disabled doesn't make sense to me. One would just not wire up a
trigger-source in that case.

Re: [PATCH v2 4/5] docs: iio: new docs for ad4052 driver

[David Lechner]

On 4/29/25 8:49 AM, Jorge Marques wrote:
> On Fri, Apr 25, 2025 at 04:44:20PM -0500, David Lechner wrote:
>> On 4/22/25 6:34 AM, Jorge Marques wrote:
>>> This adds a new page to document how to use the ad4052 ADC driver.
>>>

...

>>
> 
> Sorry about this submission, this file was indeed not updated between
> version.

No worries. We all make mistakes from time to time. :-)

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

[Andy Shevchenko]

On Tue, Apr 29, 2025 at 6:40 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 4/29/25 8:47 AM, Jorge Marques wrote:
> >
> > Hi Andy,
> >
> > I agree with your suggestion, and in this case the appropriate kernel
> > version is 3.10.
> >
> >>
> >>> +What:              /sys/bus/iio/devices/iio:deviceX/oversampling_frequency
> >>> +KernelVersion:     6.15
> >>
> >> Then why don't you put the real version of the first release that has it?
> >>
> >>> +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.15
> >>
> >> Ditto.
> >>
> >>> +Contact:   linux-iio@vger.kernel.org
> >>> +Description:
> >>> +           Hardware dependent values supported by the oversampling
> >>> +           frequency.
>
>
> I don't see oversampling_frequency used in any existing driver, so how could
> it be introduced in kernel 3.10? I think you confuse it with
> events/sampling_frequency.
>
> oversampling_frequency is new and so 6.16 should be correct if Jonathan picks
> this up in the next few weeks, otherwise it will be 6.17.

If this is the case, the whole commit message should be revisited.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Wed, 30 Apr 2025 01:03:07 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 29, 2025 at 6:40 PM David Lechner <dlechner@baylibre.com> wrote:
> >
> > On 4/29/25 8:47 AM, Jorge Marques wrote:  
> > >
> > > Hi Andy,
> > >
> > > I agree with your suggestion, and in this case the appropriate kernel
> > > version is 3.10.
> > >  
> > >>  
> > >>> +What:              /sys/bus/iio/devices/iio:deviceX/oversampling_frequency
> > >>> +KernelVersion:     6.15  
> > >>
> > >> Then why don't you put the real version of the first release that has it?
> > >>  
> > >>> +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.15  
> > >>
> > >> Ditto.
> > >>  
> > >>> +Contact:   linux-iio@vger.kernel.org
> > >>> +Description:
> > >>> +           Hardware dependent values supported by the oversampling
> > >>> +           frequency.  
> >
> >
> > I don't see oversampling_frequency used in any existing driver, so how could
> > it be introduced in kernel 3.10? I think you confuse it with
> > events/sampling_frequency.
> >
> > oversampling_frequency is new and so 6.16 should be correct if Jonathan picks
> > this up in the next few weeks, otherwise it will be 6.17.  
> 
> If this is the case, the whole commit message should be revisited.
> 
Yeah. That last bit about the existing drivers is talking about unrelated
ABI.

Jonathan

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Uwe Kleine-König]

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

Hello,

On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> +{
> +	struct pwm_state pwm_st;
> +
> +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> +		return -EINVAL;
> +
> +	pwm_get_state(st->cnv_pwm, &pwm_st);
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);

Is it clear that pwm_st.duty_cycle isn't greater than
DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);

I'm not a big fan of pwm_get_state() because the semantic is a bit
strange. My preferred alternative would be to either use pwm_init_state
and initialize all fields, or maintain a struct pwm_state in struct
ad4052_state.

> +}
> +
> +static int ad4052_soft_reset(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> +	for (int i = 0; i < 3; i++)
> +		st->buf_reset_pattern[6 * (i + 1) - 1] = 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)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +
> +	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_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = 0;
> +	if (scan_type->sign == 's')
> +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> +
> +	st->data_format = val;
> +
> +	if (st->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> +					      AD4052_TIMER_CONFIG_300KSPS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, 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;
> +
> +	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 = 0;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	ret = devm_request_threaded_irq(dev, ret, NULL,
> +					ad4052_irq_handler_thresh,
> +					IRQF_ONESHOT, indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	st->gp1_irq = ret;
> +	return devm_request_threaded_irq(dev, ret, NULL,
> +					 ad4052_irq_handler_drdy,
> +					 IRQF_ONESHOT, indio_dev->name,
> +					 st);
> +}
> +
> +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_read_chan_raw(struct ad4052_state *st, int *val)
> +{
> +	struct spi_device *spi = st->spi;
> +	int ret;
> +	struct spi_transfer t_cnv = {
> +		.len = 0
> +	};
> +
> +	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.
> +	 */
> +	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 = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 15);
> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 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(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);

Is this the expected semantic? I.e. if the PWM isn't running report
sample freq assuming the last set period (or if the pwm wasn't set
before the configured period length set by the bootloader, or the value
specified in the device tree)?

> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> [...]
> +static int ad4052_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;
> +
> +	if (st->wait_event)
> +		return -EBUSY;
> +
> +	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_update_xfer_offload(indio_dev, indio_dev->channels);
> +	if (ret)
> +		goto out_error;
> +
> +	/* SPI Offload handles the data ready irq */
> +	disable_irq(st->gp1_irq);
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
> +	if (ret)
> +		goto out_offload_error;
> +
> +	ret = pwm_enable(st->cnv_pwm);
> +	if (ret)
> +		goto out_pwm_error;

pwm_enable() is another disguised pwm_get_state().

> +
> +	return 0;
> +
> +out_pwm_error:
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +out_offload_error:
> +	enable_irq(st->gp1_irq);
> +	spi_unoptimize_message(&st->offload_msg);
> +	ad4052_exit_command(st);
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	return ret;
> +}

Best regards
Uwe

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

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
> On 4/29/25 8:48 AM, Jorge Marques wrote:
> > Hi David, 
> > 
> > I didn't went through your's and Jonathan's ad4052.c review yet,
> > but for the trigger-source-cells I need to dig deeper and make
> > considerable changes to the driver, as well as hardware tests.
> > My idea was to have a less customizable driver, but I get that it is
> > more interesting to make it user-definable.
> 
> We don't need to make the driver support all possibilities, but the devicetree
> needs to be as complete as possible since it can't be as easily changed in the
> future.
> 

Ack.

I see that the node goes in the spi controller (the parent). To use the
same information in the driver I need to look-up the parent node, then
the node. I don't plan to do that in the version of the driver, just an
observation.

There is something else I want to discuss on the dt-bindings actually.
According to the schema, the spi-max-frequency is:

  > Maximum SPI clocking speed of the device in Hz.

The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
(higher, depends on VIO). The solution I came up, to not require a
custom regmap spi bus, is to have spi-max-frequency bound the
Configuration mode speed, and have ADC Mode set by VIO regulator
voltage, through spi_transfer.speed_hz. At the end of the day, both are
bounded by the spi controller maximum speed.

My concern is that having ADC mode speed higher than spi-max-frequency
may be counter-intuitive, still, it allows to achieve the max data sheet
speed considering VIO voltage with the lowest code boilerplate.

Let me know if I can proceed this way before submitting V3.

> ...
> 
> >>
> >> Assuming the diagram at [1] is correct, for SPI offload use, we are missing:
> >>
> >>   #trigger-source-cells:
> >>     const: 2
> >>     description: |
> >>       Output pins used as trigger source.
> >>
> >>       Cell 0 defines which pin:
> >>       * 0 = GP0
> >>       * 1 = GP1
> >>
> >>       Cell 1 defines the event:
> >>       * 0 = Data ready
> >>       * 1 = Min threshold
> >>       * 2 = Max threshold
> >>       * 3 = Either threshold
> >>       * 4 = Device ready
> >>       * 5 = Device enable
> >>       * 6 = Chop control
> >>
> >> Bonus points for adding a header with macros for the arbitrary event values.
> > 
> > In the sense of describing the device and not what the driver does, I
> > believe the proper mapping would be:
> > 
> >   Cell 1 defines the event:
> >   * 0 = Disabled
> >   * 1 = Data ready
> >   * 2 = Min threshold
> >   * 3 = Max threshold
> >   * 4 = Either threshold
> >   * 5 = CHOP control
> >   * 6 = Device enable
> >   * 7 = Device ready (only GP1)
> > 
> > I will investigate further this.
> > 
> >>
> 
> 0 = Disabled doesn't make sense to me. One would just not wire up a
> trigger-source in that case.

Ack.


Regards,
Jorge

Re: [PATCH v2 2/5] iio: code: mark iio_dev as const in iio_buffer_enabled

[Jorge Marques]

On Sat, Apr 26, 2025 at 04:45:24PM +0100, Jonathan Cameron wrote:
> On Tue, 22 Apr 2025 13:34:47 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
> 
> > The iio_dev struct is never modified inside the method, mark it as
> > const.
> > This allows to be called from get_current_scan_type, and is useful
> > when the scan_type depends on the buffer state.
> Now I'm confused.   scan type is only relevant when the buffer is enabled
> so how can it change as a result of that action?
> 
> Maybe all will become clear in later patches!
> 
> Jonathan

Hi Jonathan, you are right, this commit will be dropped in v3. The
driver scan type depends on oversampling value, so it has an
has_ext_scan_type, and is only relevant for buffer readings.

My mistake came to fruition from the fact the tool libiio at any context
but local does not support changes to /sys /dev, including scan_type
changes (it scans once at service start), so I kept getting odd
behaviour that led me to the wrong solution.

So, in summary for V3, the widths are set as follows:

* spi_transfer.bits_per_word = scan_type.realbits
* spi_transfer.len = scan_type.realbits == 24 ? 4 : 2
* scan_type.storagebits = 32: Used by tools, such as libiio, to compute
  number of samples.

This ensures the minimum number of bytes transferred in the SPI bus, to
optimize speed, while respecting SPI Engine Limitation of a fixed width
(generally 32-bits). Similar to commit
ce45446e520c85db022 (iio: adc: ad4000: Avoid potential double data word read)

Regards,
Jorge

> 
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> >  drivers/iio/industrialio-core.c | 2 +-
> >  include/linux/iio/iio.h         | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 178e99b111debc59a247fcc3a6037e429db3bebf..bc6a2ac6415eccf201e148ea98c0b5982787eb6d 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -212,7 +212,7 @@ EXPORT_SYMBOL_GPL(iio_device_id);
> >   *
> >   * Returns: True, if the buffer is enabled.
> >   */
> > -bool iio_buffer_enabled(struct iio_dev *indio_dev)
> > +bool iio_buffer_enabled(const struct iio_dev *indio_dev)
> >  {
> >  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >  
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..88569e1a888bde4d2bfb5b9f030096af1c15d68d 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -629,7 +629,7 @@ struct iio_dev {
> >  
> >  int iio_device_id(struct iio_dev *indio_dev);
> >  int iio_device_get_current_mode(struct iio_dev *indio_dev);
> > -bool iio_buffer_enabled(struct iio_dev *indio_dev);
> > +bool iio_buffer_enabled(const struct iio_dev *indio_dev);
> >  
> >  const struct iio_chan_spec
> >  *iio_find_channel_from_si(struct iio_dev *indio_dev, int si);
> > 
> 

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Jorge Marques]

Hi Uwe,

On Fri, May 16, 2025 at 12:11:56PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> > +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> > +{
> > +	struct pwm_state pwm_st;
> > +
> > +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> > +		return -EINVAL;
> > +
> > +	pwm_get_state(st->cnv_pwm, &pwm_st);
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> > +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> 
> Is it clear that pwm_st.duty_cycle isn't greater than
> DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> 
> I'm not a big fan of pwm_get_state() because the semantic is a bit
> strange. My preferred alternative would be to either use pwm_init_state
> and initialize all fields, or maintain a struct pwm_state in struct
> ad4052_state.

Ack. I will mantain pwm_state in ad4052_state.

> 
> > +static int ad4052_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ad4052_read_chan_raw(indio_dev, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		ret = ad4052_get_oversampling_ratio(st, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		if (!pwm_st.enabled)
> > +			pwm_get_state(st->cnv_pwm, &pwm_st);
> > +
> > +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> 
> Is this the expected semantic? I.e. if the PWM isn't running report
> sample freq assuming the last set period (or if the pwm wasn't set
> before the configured period length set by the bootloader, or the value
> specified in the device tree)?
> 

Yes, but I will just use the (new) managed pwm_state instead:

  *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
  return IIO_VAL_INT;

> > +
> > [...]
> > +
> > +	ret = pwm_enable(st->cnv_pwm);
> > +	if (ret)
> > +		goto out_pwm_error;
> 
> pwm_enable() is another disguised pwm_get_state().
> 

Ack.

> > +
> > +	return 0;
> > +
> > +out_pwm_error:
> > +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> > +out_offload_error:
> > +	enable_irq(st->gp1_irq);
> > +	spi_unoptimize_message(&st->offload_msg);
> > +	ad4052_exit_command(st);
> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	return ret;
> > +}
> 
> Best regards
> Uwe

Best regards,
Jorge

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Jorge Marques]

Hi Jonathan,

On Sat, Apr 26, 2025 at 05:03:02PM +0100, Jonathan Cameron wrote:
> On Tue, 22 Apr 2025 13:34:50 +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,
>
> A few additional comments from me.
>
> Thanks,
>
> Jonathan
>
> > diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f412f0884bab4f500091f6c7ca761967c8f6e3b6
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4052.c
> > @@ -0,0 +1,1425 @@
>
>
> > +static int ad4052_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ad4052_read_chan_raw(indio_dev, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		ret = ad4052_get_oversampling_ratio(st, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		if (!pwm_st.enabled)
> > +			pwm_get_state(st->cnv_pwm, &pwm_st);
> > +
> > +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
>
>
> > +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 = -EINVAL;
> > +	u8 reg, size = 1;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
>
> Sometimes it is worth a n internal function factored out in cases
> like this to allow direct returns in error cases with the release
> always happening before we check if the inner function failed.
>
> That suggestion applies in other places in this code.

Ack. I will refactor out the methods using auxiliary methods suffixed
with _dispatch.

> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (type) {
> > +	case IIO_EV_TYPE_THRESH:
> > +		switch (info) {
> > +		case IIO_EV_INFO_VALUE:
> > +			if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT) {
> > +				if (val > 2047 || val < -2048)
> > +					goto out_release;
> > +			} else if (val > 4095 || val < 0) {
> > +				goto out_release;
> > +			}
> > +			if (dir == IIO_EV_DIR_RISING)
> > +				reg = AD4052_REG_MAX_LIMIT;
> > +			else
> > +				reg = AD4052_REG_MIN_LIMIT;
> > +			size = 2;
> > +			st->d16 = cpu_to_be16(val);
> > +			break;
> > +		case IIO_EV_INFO_HYSTERESIS:
> > +			if (val & BIT(7))
> > +				goto out_release;
> > +			if (dir == IIO_EV_DIR_RISING)
> > +				reg = AD4052_REG_MAX_HYST;
> > +			else
> > +				reg = AD4052_REG_MIN_HYST;
> > +			st->d16 = cpu_to_be16(val >> 8);
> > +			break;
> > +		default:
> > +			goto out_release;
> > +		}
> > +		break;
> > +	default:
> > +		goto out_release;
> > +	}
> > +
> > +	ret = regmap_bulk_write(st->regmap, reg, &st->d16, size);
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> > +
> > +static int ad4052_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;
> > +
> > +	if (st->wait_event)
> > +		return -EBUSY;
> > +
> > +	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_update_xfer_offload(indio_dev, indio_dev->channels);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	/* SPI Offload handles the data ready irq */
> > +	disable_irq(st->gp1_irq);
> > +
> > +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> > +					 &config);
> > +	if (ret)
> > +		goto out_offload_error;
> > +
> > +	ret = pwm_enable(st->cnv_pwm);
> > +	if (ret)
> > +		goto out_pwm_error;
> > +
> > +	return 0;
> > +
> > +out_pwm_error:
> > +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> > +out_offload_error:
> > +	enable_irq(st->gp1_irq);
> > +	spi_unoptimize_message(&st->offload_msg);
> > +	ad4052_exit_command(st);
>
> What is this matching to?  Feels like it would be set_operation_mode()
> but I may be wrong on that.  If it is then you need another label
> to call only this update_xfer_offload fails.
>

Yep, here an additional label is needed, thanks!
Changing to:

	out_pwm_error:
		spi_offload_trigger_disable(st->offload, st->offload_trigger);
	out_offload_error:
		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;

> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->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 (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> Probably use a goto to release this in only one place.
>
Ack.

> > +		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);
> > +
> > +	if (iio_buffer_enabled(indio_dev))
>
> This is the bit I'm not really following. Why is the enabling or not of
> the buffer related to whether offload is going on?
>
Will be dropped and the scan_type only depends on the oversampling value.
My misunderstanding is explained on thread
[PATCH v2 2/5] iio: code: mark iio_dev as const in iio_buffer_enabled
> > +		return st->mode == AD4052_BURST_AVERAGING_MODE ?
> > +				   AD4052_SCAN_TYPE_OFFLOAD_BURST_AVG :
> > +				   AD4052_SCAN_TYPE_OFFLOAD_SAMPLE;
> > +
> > +	return st->mode == AD4052_BURST_AVERAGING_MODE ?
> > +			   AD4052_SCAN_TYPE_BURST_AVG :
> > +			   AD4052_SCAN_TYPE_SAMPLE;
> > +}
>
>
> > +static int ad4052_probe(struct spi_device *spi)
> > +{
> ...
>
> > +
> > +	st->mode = AD4052_SAMPLE_MODE;
> > +	st->wait_event = false;
> > +	st->chip = chip;
> > +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
>
> That feels like it should be encoded directly in chip.  Basing it
> on prod_id seems liable to bite us at somepoint in the future.
>
Ack. Moved to a const chip_info.grade.
> > +
> > +static int ad4052_runtime_resume(struct device *dev)
> > +{
> > +	struct ad4052_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> > +	return ret;
>
> 	return regmap_write();
> and no need for the local variable ret.
>

A sleep of 4ms will be added to ensure not triggering NOT_RDY_ERROR
flag, and therefore ret will be kept:

	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
	fsleep(4000);
	return ret;

> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(ad4052_pm_ops, ad4052_runtime_suspend,
> > +				 ad4052_runtime_resume, NULL);

Best regards,
Jorge

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[Jorge Marques]

Hi David,

On Fri, Apr 25, 2025 at 06:13:48PM -0500, David Lechner wrote:
> On 4/22/25 6:34 AM, Jorge Marques 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>
> > ---
> >  MAINTAINERS              |    1 +
> >  drivers/iio/adc/Kconfig  |   14 +
> >  drivers/iio/adc/Makefile |    1 +
> >  drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> This patch is way too big, so I didn't review most of it yet. But time to call
> it quits for today. In the future, it would be a lot easier for reviewers if
> you can split things into multiple patches instead of implementing all of the
> features at once. E.g. start with just a basic driver, then a patch to add
> oversampling support, then another patch to add SPI offload support. 500 lines
> is a more manageable size for review.
> 

Ack. I split v3 into three commits: base, offload and iio events.
My playground is here: https://github.com/gastmaier/adi-linux/pull/9

> ...
> 
> > +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->xfer;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > +
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	xfer = &st->offload_xfer;
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> 
> This doesn't work for oversampling. realbits may be 16 while storagebits is 32.
> But the SPI controller needs to know how many realbits-sized words to read.
> 
> So this should be 
> 
> 	xfer->len = BITS_TO_BYTES(scan_type->realbits);
> 

Agreed, but due to spi message optimization needs to be:

  	xfer->len = scan_type->realbits == 24 ? 4 : 2;

Because 3 bytes cannot be optimized.
> > +
> > +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> > +	st->offload_msg.offload = st->offload;
> > +
> > +	return spi_optimize_message(st->spi, &st->offload_msg);
> 
> I know it is like this in a few other drivers already, but I don't like having
> spi_optimize_message() in this funtion because it makes it really easy to
> forget to do have balanced calls to spi_unoptimize_message().
> 

Ack.

> > +}
> > +
> 
> ...
> 
> > +static const struct iio_buffer_setup_ops ad4052_buffer_setup_ops = {
> > +	.postenable = &ad4052_buffer_postenable,
> > +	.predisable = &ad4052_buffer_predisable,
> > +};
> 
> Would be nice to add "offload" to the name of this struct and the callbacks
> to make it clear that these are only for the SPI offload use case.
> 
Ack.
> ...
> 
> > +
> > +static bool ad4052_offload_trigger_match(struct spi_offload_trigger *trigger,
> > +					 enum spi_offload_trigger_type type,
> > +					 u64 *args, u32 nargs)
> > +{
> 
> We should be checking the args here according to what I suggested in my reply
> to the devicetree bindings patch. Right now it is assuming that we are only
> using this for SPI offload and that the pin used is GP1 and the event is data
> read. We should at least verify that the args match those assumptions.
> 
> For bonus points, we could implement allowing GPO as well.
> 

Yes, it is assuming as you mentioned.
I'm okay with "at least verifying".
but then I need to look-up at the parent node first, since it resides at
the spi-controller node, if that's ok.

> > +	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,
> > +	};
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	indio_dev->setup_ops = &ad4052_buffer_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");
> 
> Strictly speaking, the trigger-source provider is indendant of using it for
> SPI offload. I guess this is fine here for now though.
> 
Ok.
> > +
> > +	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, &pwm_st);
> > +
> > +	pwm_st.enabled = false;
> > +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> > +					 AD4052_MAX_RATE(st->grade));
> > +
> > +	ret = pwm_apply_might_sleep(st->cnv_pwm, &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_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;
> > +	int ret = 0;
> > +
> > +	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);
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &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->wait_event = false;
> > +	st->chip = chip;
> > +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> > +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> > +	st->events_frequency = AD4052_FS_OFFSET(st->grade);
> 
> Somewhere around here, we should be turning on the power supplies. Also, it
> looks like we need some special handling to get the reference volage. If there
> is a supply connected to REF, use that, if not, use VDD which requires writing
> to a register to let the chip know.
> 
Yes, v3 will add regulators.
Vref can be sourced from either REF (default) or VDD.
So the idea is, if REF node not provided, set VDD as REF?

> > +
> > +	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_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
> 
> INDIO_BUFFER_HARDWARE should not be set here. If using SPI offload,
> devm_iio_dmaengine_buffer_setup_with_handle() will add it automatically.
> For non-SPI-offload operation, it should not be set.
> 
Ack.
> > +	indio_dev->num_channels = 1;
> > +	indio_dev->info = &ad4052_info;
> > +	indio_dev->name = chip->name;
> > +
> > +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
> 
> This
> 
> > +	if (IS_ERR(st->offload))
> > +		return PTR_ERR(st->offload);
> 
> should be
> 
> 	ret = PTR_ERR_OR_ZERO(st->offload);
> 
Ack.

> > +
> > +	if (ret && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get offload\n");
> > +
> > +	if (ret == -ENODEV) {
> > +		st->offload_trigger = NULL;
> > +		indio_dev->channels = chip->channels;
> > +	} else {
> > +		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");
> > +	}
> > +
> > +	st->xfer.rx_buf = &st->d32;
> 
> I don't think we want this set globally. I.e. it doesn't make sense for SPI
> offload xfers.
> 
Ack.

> > +
> > +	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);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			   AD4052_DEVICE_STATUS_DEVICE_RESET);
> 
> Why not include this in ad4052_setup() or even ad4052_soft_reset()?
> 
Ack.
But on setup to not write registers before doing the sanity test.

> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_request_irq(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> > +
> > +	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_DEVICE_CONFIG_POWER_MODE_MSK,
> > +				       AD4052_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_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> 
> regmap_clear_bits() would be shorter if there isn't going to be a macro to
> explain the meaning of 0.
> 
Ack.
> > +	return ret;
> > +}
> > +

Regards,
Jorge

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

On 6/2/25 4:17 AM, Jorge Marques wrote:
> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
>> On 4/29/25 8:48 AM, Jorge Marques wrote:
>>> Hi David, 
>>>
>>> I didn't went through your's and Jonathan's ad4052.c review yet,
>>> but for the trigger-source-cells I need to dig deeper and make
>>> considerable changes to the driver, as well as hardware tests.
>>> My idea was to have a less customizable driver, but I get that it is
>>> more interesting to make it user-definable.
>>
>> We don't need to make the driver support all possibilities, but the devicetree
>> needs to be as complete as possible since it can't be as easily changed in the
>> future.
>>
> 
> Ack.
> 
> I see that the node goes in the spi controller (the parent). To use the
> same information in the driver I need to look-up the parent node, then
> the node. I don't plan to do that in the version of the driver, just an
> observation.
> 
> There is something else I want to discuss on the dt-bindings actually.
> According to the schema, the spi-max-frequency is:
> 
>   > Maximum SPI clocking speed of the device in Hz.
> 
> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
> (higher, depends on VIO). The solution I came up, to not require a
> custom regmap spi bus, is to have spi-max-frequency bound the
> Configuration mode speed,

The purpose of spi-max-frequency in the devicetree is that sometimes
the wiring of a complete system makes the effective max frequency
lower than what is allowed by the datasheet. So this really needs
to be the absolute highest frequency allowed.

> and have ADC Mode set by VIO regulator
> voltage, through spi_transfer.speed_hz. At the end of the day, both are
> bounded by the spi controller maximum speed.

If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
uses spi-max-frequency. So I don't think this would actually work.

> 
> My concern is that having ADC mode speed higher than spi-max-frequency
> may be counter-intuitive, still, it allows to achieve the max data sheet
> speed considering VIO voltage with the lowest code boilerplate.
> 
> Let me know if I can proceed this way before submitting V3.

Re: [PATCH v2 5/5] iio: adc: add support for ad4052

[David Lechner]

On 6/2/25 6:16 AM, Jorge Marques wrote:
> Hi David,
> 
> On Fri, Apr 25, 2025 at 06:13:48PM -0500, David Lechner wrote:
>> On 4/22/25 6:34 AM, Jorge Marques wrote:

...

>>> +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;
>>> +	int ret = 0;
>>> +
>>> +	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);
>>> +
>>> +	st->regmap = devm_regmap_init_spi(spi, &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->wait_event = false;
>>> +	st->chip = chip;
>>> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
>>> +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
>>> +	st->events_frequency = AD4052_FS_OFFSET(st->grade);
>>
>> Somewhere around here, we should be turning on the power supplies. Also, it
>> looks like we need some special handling to get the reference voltage. If there
>> is a supply connected to REF, use that, if not, use VDD which requires writing
>> to a register to let the chip know.
>>
> Yes, v3 will add regulators.
> Vref can be sourced from either REF (default) or VDD.
> So the idea is, if REF node not provided, set VDD as REF?
> 
Yes, you can do this with devm_regulator_get_enable_read_voltage() if it
returns -ENODEV, for the REF supply, then call it again on the VDD supply.

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

Hi David,

On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote:
> On 6/2/25 4:17 AM, Jorge Marques wrote:
> > On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
> >> On 4/29/25 8:48 AM, Jorge Marques wrote:
> >>> Hi David, 
> >>>
> >>> I didn't went through your's and Jonathan's ad4052.c review yet,
> >>> but for the trigger-source-cells I need to dig deeper and make
> >>> considerable changes to the driver, as well as hardware tests.
> >>> My idea was to have a less customizable driver, but I get that it is
> >>> more interesting to make it user-definable.
> >>
> >> We don't need to make the driver support all possibilities, but the devicetree
> >> needs to be as complete as possible since it can't be as easily changed in the
> >> future.
> >>
> > 
> > Ack.
> > 
> > I see that the node goes in the spi controller (the parent). To use the
> > same information in the driver I need to look-up the parent node, then
> > the node. I don't plan to do that in the version of the driver, just an
> > observation.
> > 
> > There is something else I want to discuss on the dt-bindings actually.
> > According to the schema, the spi-max-frequency is:
> > 
> >   > Maximum SPI clocking speed of the device in Hz.
> > 
> > The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
> > (higher, depends on VIO). The solution I came up, to not require a
> > custom regmap spi bus, is to have spi-max-frequency bound the
> > Configuration mode speed,
> 
> The purpose of spi-max-frequency in the devicetree is that sometimes
> the wiring of a complete system makes the effective max frequency
> lower than what is allowed by the datasheet. So this really needs
> to be the absolute highest frequency allowed.
> 
> > and have ADC Mode set by VIO regulator
> > voltage, through spi_transfer.speed_hz. At the end of the day, both are
> > bounded by the spi controller maximum speed.
> 
> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
> uses spi-max-frequency. So I don't think this would actually work.
> 
Ok, so that's something that may be worth some attention.

At spi/spi.c#2472
	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
		spi->max_speed_hz = value;

At spi/spi.c#4090
	if (!xfer->speed_hz)
		xfer->speed_hz = spi->max_speed_hz;

So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and
not bounded by it.

Then at spi-axi-spi-engine.c:

	static int spi_engine_precompile_message(struct spi_message *msg)
	{
  		clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
		xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
	}

Where max_hz is set only by the IP spi_clk. If at the driver I set
xfer.speed_hz, it won't be bounded by max-spi-frequency.

The only that seems to bound as described is the layer for flash memory
at spi-mem.c@spi_mem_adjust_op_freq.

For the adc driver, I will then consider your behavioral description and
create a custom regmap bus to limit set the reg access speed (fixed),
and keep adc mode speed set by VIO. And consider spi-max-frequency can
further reduce both speeds.
(or should instead be handled at the driver like spi-mem.c ?)

Thanks for the quick reply!
Jorge

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

On 6/2/25 11:32 AM, Jorge Marques wrote:
> Hi David,
> 
> On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote:
>> On 6/2/25 4:17 AM, Jorge Marques wrote:
>>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
>>>> On 4/29/25 8:48 AM, Jorge Marques wrote:
>>>>> Hi David, 
>>>>>
>>>>> I didn't went through your's and Jonathan's ad4052.c review yet,
>>>>> but for the trigger-source-cells I need to dig deeper and make
>>>>> considerable changes to the driver, as well as hardware tests.
>>>>> My idea was to have a less customizable driver, but I get that it is
>>>>> more interesting to make it user-definable.
>>>>
>>>> We don't need to make the driver support all possibilities, but the devicetree
>>>> needs to be as complete as possible since it can't be as easily changed in the
>>>> future.
>>>>
>>>
>>> Ack.
>>>
>>> I see that the node goes in the spi controller (the parent). To use the
>>> same information in the driver I need to look-up the parent node, then
>>> the node. I don't plan to do that in the version of the driver, just an
>>> observation.
>>>
>>> There is something else I want to discuss on the dt-bindings actually.
>>> According to the schema, the spi-max-frequency is:
>>>
>>>   > Maximum SPI clocking speed of the device in Hz.
>>>
>>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
>>> (higher, depends on VIO). The solution I came up, to not require a
>>> custom regmap spi bus, is to have spi-max-frequency bound the
>>> Configuration mode speed,
>>
>> The purpose of spi-max-frequency in the devicetree is that sometimes
>> the wiring of a complete system makes the effective max frequency
>> lower than what is allowed by the datasheet. So this really needs
>> to be the absolute highest frequency allowed.
>>
>>> and have ADC Mode set by VIO regulator
>>> voltage, through spi_transfer.speed_hz. At the end of the day, both are
>>> bounded by the spi controller maximum speed.
>>
>> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
>> uses spi-max-frequency. So I don't think this would actually work.
>>
> Ok, so that's something that may be worth some attention.
> 
> At spi/spi.c#2472
> 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> 		spi->max_speed_hz = value;
> 
> At spi/spi.c#4090
> 	if (!xfer->speed_hz)
> 		xfer->speed_hz = spi->max_speed_hz;
> 
> So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and
> not bounded by it.

Ah, OK, my memory was wrong. It is only bound by the controller max
speed, not the device max speed.

	if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
		xfer->speed_hz = ctlr->max_speed_hz;

It does seem odd that it would allow setting an individual xfer
speed higher than than the given device max speed. I suppose we
could submit a patch adding that check to the SPI core code and
see what Mark has to say.

> 
> Then at spi-axi-spi-engine.c:
> 
> 	static int spi_engine_precompile_message(struct spi_message *msg)
> 	{
>   		clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
> 		xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
> 	}
> 
> Where max_hz is set only by the IP spi_clk. If at the driver I set
> xfer.speed_hz, it won't be bounded by max-spi-frequency.
> 
> The only that seems to bound as described is the layer for flash memory
> at spi-mem.c@spi_mem_adjust_op_freq.
> 
> For the adc driver, I will then consider your behavioral description and
> create a custom regmap bus to limit set the reg access speed (fixed),
> and keep adc mode speed set by VIO. And consider spi-max-frequency can
> further reduce both speeds.
> (or should instead be handled at the driver like spi-mem.c ?)

It would be more work, but if it is common enough, we could generalize this
in the core code. For example add a spi-register-max-frequency binding (or
even a more general spi-max-freqency-map to map operations to max frequencies).
Then we could bake it into the regmap_spi code to handle this property
and not have to make a separate bus.

FWIW, there are also some SPI TFT displays that use a different frequency
for register access compared to framebuffer data that could potentially
use this too. Right now, these just have a hard-coded register access
frequency of e.g. 10 MHz.

> 
> Thanks for the quick reply!
> Jorge

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[Jorge Marques]

On Mon, Jun 02, 2025 at 12:23:40PM -0500, David Lechner wrote:
> On 6/2/25 11:32 AM, Jorge Marques wrote:
> > Hi David,
> > 
> > On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote:
> >> On 6/2/25 4:17 AM, Jorge Marques wrote:
> >>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
> >>>> On 4/29/25 8:48 AM, Jorge Marques wrote:
> >>>>> Hi David, 
> >>>>>
> >>>>> I didn't went through your's and Jonathan's ad4052.c review yet,
> >>>>> but for the trigger-source-cells I need to dig deeper and make
> >>>>> considerable changes to the driver, as well as hardware tests.
> >>>>> My idea was to have a less customizable driver, but I get that it is
> >>>>> more interesting to make it user-definable.
> >>>>
> >>>> We don't need to make the driver support all possibilities, but the devicetree
> >>>> needs to be as complete as possible since it can't be as easily changed in the
> >>>> future.
> >>>>
> >>>
> >>> Ack.
> >>>
> >>> I see that the node goes in the spi controller (the parent). To use the
> >>> same information in the driver I need to look-up the parent node, then
> >>> the node. I don't plan to do that in the version of the driver, just an
> >>> observation.
> >>>
> >>> There is something else I want to discuss on the dt-bindings actually.
> >>> According to the schema, the spi-max-frequency is:
> >>>
> >>>   > Maximum SPI clocking speed of the device in Hz.
> >>>
> >>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
> >>> (higher, depends on VIO). The solution I came up, to not require a
> >>> custom regmap spi bus, is to have spi-max-frequency bound the
> >>> Configuration mode speed,
> >>
> >> The purpose of spi-max-frequency in the devicetree is that sometimes
> >> the wiring of a complete system makes the effective max frequency
> >> lower than what is allowed by the datasheet. So this really needs
> >> to be the absolute highest frequency allowed.
> >>
> >>> and have ADC Mode set by VIO regulator
> >>> voltage, through spi_transfer.speed_hz. At the end of the day, both are
> >>> bounded by the spi controller maximum speed.
> >>
> >> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
> >> uses spi-max-frequency. So I don't think this would actually work.
> >>
> > Ok, so that's something that may be worth some attention.
> > 
> > At spi/spi.c#2472
> > 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> > 		spi->max_speed_hz = value;
> > 
> > At spi/spi.c#4090
> > 	if (!xfer->speed_hz)
> > 		xfer->speed_hz = spi->max_speed_hz;
> > 
> > So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and
> > not bounded by it.
> 
> Ah, OK, my memory was wrong. It is only bound by the controller max
> speed, not the device max speed.
> 
> 	if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
> 		xfer->speed_hz = ctlr->max_speed_hz;
> 
> It does seem odd that it would allow setting an individual xfer
> speed higher than than the given device max speed. I suppose we
> could submit a patch adding that check to the SPI core code and
> see what Mark has to say.
>

Agreed, the patch itself would be simple:

 	if (!xfer->speed_hz || xfer->speed_hz > spi->max_speed_hz)
 		xfer->speed_hz = spi->max_speed_hz;

But I wonder how many drivers rely on this behaviour
> > 
> > Then at spi-axi-spi-engine.c:
> > 
> > 	static int spi_engine_precompile_message(struct spi_message *msg)
> > 	{
> >   		clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
> > 		xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
> > 	}
> > 
> > Where max_hz is set only by the IP spi_clk. If at the driver I set
> > xfer.speed_hz, it won't be bounded by max-spi-frequency.
> > 
> > The only that seems to bound as described is the layer for flash memory
> > at spi-mem.c@spi_mem_adjust_op_freq.
> > 
> > For the adc driver, I will then consider your behavioral description and
> > create a custom regmap bus to limit set the reg access speed (fixed),
> > and keep adc mode speed set by VIO. And consider spi-max-frequency can
> > further reduce both speeds.
> > (or should instead be handled at the driver like spi-mem.c ?)
> 
> It would be more work, but if it is common enough, we could generalize this
> in the core code. For example add a spi-register-max-frequency binding (or
> even a more general spi-max-freqency-map to map operations to max frequencies).
> Then we could bake it into the regmap_spi code to handle this property
> and not have to make a separate bus.
> 
> FWIW, there are also some SPI TFT displays that use a different frequency
> for register access compared to framebuffer data that could potentially
> use this too. Right now, these just have a hard-coded register access
> frequency of e.g. 10 MHz.
> 

I implemented the custom regmap bus for this series.
With a `spi-max-frequency-map`, the regmap bus can be removed.
I don't want to include this regmap spi patch to this series.
As I see it, struct regmap_but first need to be extended to add
a max_speed, e.g.
  
   @max_speed: Max transfer speed that can be used on the bus.

regmap_spi.c would then look for the devicetree node to fill the value
and on regmap_write/read fill speed_hz.
In this case, it could be called "register-frequency" or
"regmap-frequency"
If instead it is up to spi.c to read the devicetree node, then a way to
differentiate "regular" transfers from "regmap" transfers would be
necessary.

About submitting v3, should I submit only up-to the base driver, or can
I submit also the add offload support and add event support commits?

Regards,
Jorge

Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner]

On 6/3/25 2:29 AM, Jorge Marques wrote:
> On Mon, Jun 02, 2025 at 12:23:40PM -0500, David Lechner wrote:
>> On 6/2/25 11:32 AM, Jorge Marques wrote:
>>> Hi David,
>>>
>>> On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote:
>>>> On 6/2/25 4:17 AM, Jorge Marques wrote:
>>>>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
>>>>>> On 4/29/25 8:48 AM, Jorge Marques wrote:
>>>>>>> Hi David, 
>>>>>>>
>>>>>>> I didn't went through your's and Jonathan's ad4052.c review yet,
>>>>>>> but for the trigger-source-cells I need to dig deeper and make
>>>>>>> considerable changes to the driver, as well as hardware tests.
>>>>>>> My idea was to have a less customizable driver, but I get that it is
>>>>>>> more interesting to make it user-definable.
>>>>>>
>>>>>> We don't need to make the driver support all possibilities, but the devicetree
>>>>>> needs to be as complete as possible since it can't be as easily changed in the
>>>>>> future.
>>>>>>
>>>>>
>>>>> Ack.
>>>>>
>>>>> I see that the node goes in the spi controller (the parent). To use the
>>>>> same information in the driver I need to look-up the parent node, then
>>>>> the node. I don't plan to do that in the version of the driver, just an
>>>>> observation.
>>>>>
>>>>> There is something else I want to discuss on the dt-bindings actually.
>>>>> According to the schema, the spi-max-frequency is:
>>>>>
>>>>>   > Maximum SPI clocking speed of the device in Hz.
>>>>>
>>>>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
>>>>> (higher, depends on VIO). The solution I came up, to not require a
>>>>> custom regmap spi bus, is to have spi-max-frequency bound the
>>>>> Configuration mode speed,
>>>>
>>>> The purpose of spi-max-frequency in the devicetree is that sometimes
>>>> the wiring of a complete system makes the effective max frequency
>>>> lower than what is allowed by the datasheet. So this really needs
>>>> to be the absolute highest frequency allowed.
>>>>
>>>>> and have ADC Mode set by VIO regulator
>>>>> voltage, through spi_transfer.speed_hz. At the end of the day, both are
>>>>> bounded by the spi controller maximum speed.
>>>>
>>>> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
>>>> uses spi-max-frequency. So I don't think this would actually work.
>>>>
>>> Ok, so that's something that may be worth some attention.
>>>
>>> At spi/spi.c#2472
>>> 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
>>> 		spi->max_speed_hz = value;
>>>
>>> At spi/spi.c#4090
>>> 	if (!xfer->speed_hz)
>>> 		xfer->speed_hz = spi->max_speed_hz;
>>>
>>> So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and
>>> not bounded by it.
>>
>> Ah, OK, my memory was wrong. It is only bound by the controller max
>> speed, not the device max speed.
>>
>> 	if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
>> 		xfer->speed_hz = ctlr->max_speed_hz;
>>
>> It does seem odd that it would allow setting an individual xfer
>> speed higher than than the given device max speed. I suppose we
>> could submit a patch adding that check to the SPI core code and
>> see what Mark has to say.
>>
> 
> Agreed, the patch itself would be simple:
> 
>  	if (!xfer->speed_hz || xfer->speed_hz > spi->max_speed_hz)
>  		xfer->speed_hz = spi->max_speed_hz;
> 
> But I wonder how many drivers rely on this behaviour

Only one way to find out. Try it. :-)

>>>
>>> Then at spi-axi-spi-engine.c:
>>>
>>> 	static int spi_engine_precompile_message(struct spi_message *msg)
>>> 	{
>>>   		clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
>>> 		xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
>>> 	}
>>>
>>> Where max_hz is set only by the IP spi_clk. If at the driver I set
>>> xfer.speed_hz, it won't be bounded by max-spi-frequency.
>>>
>>> The only that seems to bound as described is the layer for flash memory
>>> at spi-mem.c@spi_mem_adjust_op_freq.
>>>
>>> For the adc driver, I will then consider your behavioral description and
>>> create a custom regmap bus to limit set the reg access speed (fixed),
>>> and keep adc mode speed set by VIO. And consider spi-max-frequency can
>>> further reduce both speeds.
>>> (or should instead be handled at the driver like spi-mem.c ?)
>>
>> It would be more work, but if it is common enough, we could generalize this
>> in the core code. For example add a spi-register-max-frequency binding (or
>> even a more general spi-max-freqency-map to map operations to max frequencies).
>> Then we could bake it into the regmap_spi code to handle this property
>> and not have to make a separate bus.
>>
>> FWIW, there are also some SPI TFT displays that use a different frequency
>> for register access compared to framebuffer data that could potentially
>> use this too. Right now, these just have a hard-coded register access
>> frequency of e.g. 10 MHz.
>>
> 
> I implemented the custom regmap bus for this series.

Good plan.

> With a `spi-max-frequency-map`, the regmap bus can be removed.
> I don't want to include this regmap spi patch to this series.
> As I see it, struct regmap_but first need to be extended to add
> a max_speed, e.g.
>   
>    @max_speed: Max transfer speed that can be used on the bus.
> 
> regmap_spi.c would then look for the devicetree node to fill the value
> and on regmap_write/read fill speed_hz.
> In this case, it could be called "register-frequency" or
> "regmap-frequency"
> If instead it is up to spi.c to read the devicetree node, then a way to
> differentiate "regular" transfers from "regmap" transfers would be
> necessary.
> 
> About submitting v3, should I submit only up-to the base driver, or can
> I submit also the add offload support and add event support commits?

I wouldn't add anything new at this point. Being able to spread out
the review a bit will lead to better reviews.