[PATCH 0/4] Add support for AD4052 device family

[PATCH 0/4] 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>
---
Jorge Marques (4):
      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

 .../devicetree/bindings/iio/adc/adi,ad4052.yaml    |   80 ++
 Documentation/iio/ad4052.rst                       |   93 ++
 MAINTAINERS                                        |    8 +
 drivers/iio/adc/Kconfig                            |   14 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/ad4052.c                           | 1289 ++++++++++++++++++++
 drivers/iio/industrialio-core.c                    |    2 +-
 include/linux/iio/iio.h                            |    2 +-
 8 files changed, 1487 insertions(+), 2 deletions(-)
---
base-commit: aac287ec80d71a7ab7e44c936a434625417c3e30
change-id: 20250306-iio-driver-ad4052-a4acc3bb11b3

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

[PATCH 1/4] 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 b9f4113ae5fc3ee1ef76be6808cc437286690dae..a727278752676fcc9e2c5a9b5358092b44c6fff0 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -211,7 +211,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 07a0e8132e88278c82be740e12ca2d9078710206..33612df92779d8ac380a66340e0e76eeb7e2cd5d 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.48.1

[PATCH 2/4] 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.
Contain selectable oversampling and sample rate, the latter for both
oversampling and monitor mode.
The monitor capability is exposed as an IIO threshold either direction
event.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad4052.yaml    | 80 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 ++
 2 files changed, 86 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..4602f1f0184d58f33883852ff6d76933758825f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
@@ -0,0 +1,80 @@
+# 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.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4050
+      - adi,ad4052
+      - adi,ad4056
+      - adi,ad4058
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description:
+      Reference clock
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: threshold events.
+      - description: device ready and data ready.
+
+  cnv-gpios:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 62500000
+
+  vdd-supply: true
+  vdd_1_8-supply: true
+  vio-supply: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+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>;
+            spi-max-frequency = <25000000>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
+                         <0 1 IRQ_TYPE_EDGE_RISING>;
+            cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 06f122cb8bbd15a0076c229dfc89be0b5126f237..fef8adaee888d59e1aa3b3592dda5a8bea0b7677 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1317,6 +1317,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.48.1

[PATCH 3/4] 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 | 93 ++++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS                  |  1 +
 2 files changed, 94 insertions(+)

diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
new file mode 100644
index 0000000000000000000000000000000000000000..cf0cbd60d0a48ea52f74ea02fde659fcdba61aa1
--- /dev/null
+++ b/Documentation/iio/ad4052.rst
@@ -0,0 +1,93 @@
+.. 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.
+   * - ``sample_rate``
+     - Device internal sample rate used in the burst averaging mode.
+   * - ``sample_rate_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.
+
+During monitor mode, the device is busy since other transactions
+require to put the device in configuration mode first.
+
+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 fef8adaee888d59e1aa3b3592dda5a8bea0b7677..312b2cf94b8f06298b1cbe5975ee32e2cf9a74d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1322,6 +1322,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.48.1

[PATCH 4/4] 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 | 1289 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1305 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 312b2cf94b8f06298b1cbe5975ee32e2cf9a74d8..275e2bace4731d37a3ef8eab173d11514e482e72 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1323,6 +1323,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 27413516216cb3f83cf1d995b9ffc22bf01776a4..f518dadbdd3a6b0543d0b78206fcbc203898454c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -62,6 +62,20 @@ config AD4130
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4130.
 
+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 AD4695
 	tristate "Analog Device AD4695 ADC Driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9f26d5eca8225e28f308b3db437e25eb45b41a3c..412e35dfeb9be244c47e384d3abb67cef943a0f0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4030) += ad4030.o
 obj-$(CONFIG_AD4130) += ad4130.o
+obj-$(CONFIG_AD4052) += ad4052.o
 obj-$(CONFIG_AD4695) += ad4695.o
 obj-$(CONFIG_AD4851) += ad4851.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
new file mode 100644
index 0000000000000000000000000000000000000000..29452963fb15ab1b11e3a2fc59c34a2579f25910
--- /dev/null
+++ b/drivers/iio/adc/ad4052.c
@@ -0,0 +1,1289 @@
+// 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/interrupt.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/offload/consumer.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_CONFIG		0x24
+#define AD4052_REG_INTR_CONFIG		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
+/* GP_CONFIG */
+#define AD4052_GP_MODE_MSK(x)		(GENMASK(2, 0) << (x) * 4)
+/* INTR_CONFIG */
+#define AD4052_INTR_EN_MSK(x)		(GENMASK(1, 0) << (x) * 4)
+/* ADC_MODES */
+#define AD4052_DATA_FORMAT		BIT(7)
+/* DEVICE_CONFIG */
+#define AD4052_POWER_MODE_MSK		GENMASK(1, 0)
+#define AD4052_LOW_POWER_MODE		3
+/* DEVICE_STATUS */
+#define AD4052_DEVICE_RESET		BIT(6)
+#define AD4052_THRESH_OVERRUN		BIT(4)
+#define AD4052_MAX_FLAG			BIT(3)
+#define AD4052_MIN_FLAG			BIT(2)
+#define AD4052_EVENT_CLEAR		(AD4052_THRESH_OVERRUN | AD4052_MAX_FLAG | AD4052_MIN_FLAG)
+/* TIMER_CONFIG */
+#define AD4052_FS_MASK			GENMASK(7, 4)
+#define AD4052_300KSPS			0x2
+
+#define AD4052_SPI_VENDOR		0x0456
+
+#define AD4050_MAX_AVG			0x7
+#define AD4052_MAX_AVG			0xB
+#define AD4052_CHECK_OVERSAMPLING(x, y)	({typeof(y) y_ = (y); \
+					  ((y_) < 0 || (y_) > BIT((x) + 1)); })
+#define AD4052_MAX_RATE(x)		((x) == AD4052_500KSPS ? 500000 : 2000000)
+#define AD4052_CHECK_RATE(x, y)		({typeof(y) y_ = (y);				\
+					  ((y_) > AD4052_MAX_RATE(x) || (y_) <= 0); })
+#define AD4052_FS_OFFSET(g)		((g) == AD4052_500KSPS ? 2 : 0)
+#define AD4052_FS(g)			(&ad4052_sample_rates[AD4052_FS_OFFSET(g)])
+#define AD4052_FS_LEN(g)		(ARRAY_SIZE(ad4052_sample_rates) - (AD4052_FS_OFFSET(g)))
+
+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;
+	u8 grade;
+};
+
+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;
+	unsigned long offload_trigger_hz;
+	struct spi_device *spi;
+	struct spi_transfer offload_xfer;
+	struct spi_message offload_msg;
+	struct spi_transfer xfer;
+	struct spi_message msg;
+	struct gpio_desc *cnv_gp;
+	struct completion completion;
+	struct regmap *regmap;
+	bool wait_event;
+	int gp1_irq;
+	u8 data_format;
+	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 int ad4052_sample_rate_avail[] = {
+	2000000, 1000000, 300000, 100000, 33300,
+	10000, 3000, 500, 333, 250, 200,
+	166, 140, 125, 111
+};
+
+static const char *const ad4052_sample_rates[] = {
+	"2000000", "1000000", "300000", "100000", "33300",
+	"10000", "3000", "500", "333", "250", "200",
+	"166", "140", "124", "111",
+};
+
+static int ad4052_iio_device_claim_direct(struct iio_dev *indio_dev,
+					  struct ad4052_state *st)
+{
+	if (!iio_device_claim_direct(indio_dev))
+		return false;
+
+	/**
+	 * If the device is in monitor mode, no register access is allowed,
+	 * since it would put the device back in configuration mode.
+	 */
+	if (st->wait_event) {
+		iio_device_release_direct(indio_dev);
+		return false;
+	}
+	return true;
+}
+
+static int ad4052_sample_rate_get(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret, val;
+
+	if (!ad4052_iio_device_claim_direct(indio_dev, st))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap, AD4052_REG_TIMER_CONFIG, &val);
+	val = FIELD_GET(AD4052_FS_MASK, val);
+
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : val - AD4052_FS_OFFSET(st->chip->grade);
+}
+
+static int ad4052_sample_rate_set(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 (!ad4052_iio_device_claim_direct(indio_dev, st))
+		return -EBUSY;
+
+	val += AD4052_FS_OFFSET(st->chip->grade);
+	val = FIELD_PREP(AD4052_FS_MASK, val);
+	ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG, val);
+
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static const struct iio_enum AD4052_500KSPS_sample_rate_enum = {
+	.items = AD4052_FS(AD4052_500KSPS),
+	.num_items = AD4052_FS_LEN(AD4052_500KSPS),
+	.set = ad4052_sample_rate_set,
+	.get = ad4052_sample_rate_get,
+};
+
+static const struct iio_enum AD4052_2MSPS_sample_rate_enum = {
+	.items = AD4052_FS(AD4052_2MSPS),
+	.num_items = AD4052_FS_LEN(AD4052_2MSPS),
+	.set = ad4052_sample_rate_set,
+	.get = ad4052_sample_rate_get,
+};
+
+#define AD4052_EXT_INFO(grade)								\
+static struct iio_chan_spec_ext_info grade##_ext_info[] = {				\
+	IIO_ENUM("sample_rate", IIO_SHARED_BY_ALL, &grade##_sample_rate_enum),		\
+	IIO_ENUM_AVAILABLE("sample_rate", IIO_SHARED_BY_ALL, &grade##_sample_rate_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,							\
+}
+
+const struct ad4052_chip_info ad4050_chip_info = {
+	.name = "ad4050",
+	.channels = { AD4052_CHAN(12, AD4052_2MSPS) },
+	.offload_channels = { AD4052_OFFLOAD_CHAN(12, AD4052_2MSPS) },
+	.prod_id = 0x70,
+	.max_avg = AD4050_MAX_AVG,
+	.grade = AD4052_2MSPS,
+};
+
+const struct ad4052_chip_info ad4052_chip_info = {
+	.name = "ad4052",
+	.channels = { AD4052_CHAN(16, AD4052_2MSPS) },
+	.offload_channels = { AD4052_OFFLOAD_CHAN(16, AD4052_2MSPS) },
+	.prod_id = 0x72,
+	.max_avg = AD4052_MAX_AVG,
+	.grade = AD4052_2MSPS,
+};
+
+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 = 0x70,
+	.max_avg = AD4050_MAX_AVG,
+	.grade = AD4052_500KSPS,
+};
+
+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 = 0x72,
+	.max_avg = AD4052_MAX_AVG,
+	.grade = AD4052_500KSPS,
+};
+
+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);
+
+	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);
+
+	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 (AD4052_CHECK_OVERSAMPLING(st->chip->max_avg, val))
+		return -EINVAL;
+
+	/* 0 or 1 disables oversampling */
+	if (val == 0 || 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 = 0;
+		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_assert(struct ad4052_state *st)
+{
+	int ret;
+	u16 val;
+
+	ret = regmap_bulk_read(st->regmap, AD4052_REG_PROD_ID_1, &st->d16, 2);
+	if (ret)
+		return ret;
+
+	val = be16_to_cpu(st->d16);
+	if (val != st->chip->prod_id)
+		return -ENODEV;
+
+	ret = regmap_bulk_read(st->regmap, AD4052_REG_VENDOR_H, &st->d16, 2);
+	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)
+{
+	u8 val = st->data_format | mode;
+	int ret;
+
+	ret = regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
+	if (ret)
+		return ret;
+
+	val = BIT(0);
+	return regmap_write(st->regmap, AD4052_REG_MODE_SET, val);
+}
+
+static int __ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
+{
+	struct spi_offload_trigger_config config = {
+		.type = SPI_OFFLOAD_TRIGGER_PERIODIC,
+		.periodic = {
+			.frequency_hz = freq,
+		},
+	};
+	int ret;
+
+	ret = spi_offload_trigger_validate(st->offload_trigger, &config);
+	if (ret)
+		return ret;
+
+	st->offload_trigger_hz = config.periodic.frequency_hz;
+
+	return 0;
+}
+
+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_set_non_defaults(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);
+
+	u8 val = FIELD_PREP(AD4052_GP_MODE_MSK(0), AD4052_GP_INTR) |
+		 FIELD_PREP(AD4052_GP_MODE_MSK(1), AD4052_GP_DRDY);
+	int ret;
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONFIG,
+				 AD4052_GP_MODE_MSK(1) | AD4052_GP_MODE_MSK(0),
+				 val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4052_INTR_EN_MSK(0), (AD4052_INTR_EN_EITHER)) |
+	      FIELD_PREP(AD4052_INTR_EN_MSK(1), (AD4052_INTR_EN_NEITHER));
+
+	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONFIG,
+				 AD4052_INTR_EN_MSK(0) | AD4052_INTR_EN_MSK(1),
+				 val);
+	if (ret)
+		return ret;
+
+	val = 0;
+	if (scan_type->sign == 's')
+		val |= AD4052_DATA_FORMAT;
+
+	st->data_format = val;
+
+	if (st->chip->grade == AD4052_500KSPS) {
+		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
+				   FIELD_PREP(AD4052_FS_MASK, AD4052_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(dev_fwnode(&st->spi->dev), 0);
+	if (ret <= 0)
+		return ret ? ret : -EINVAL;
+
+	ret = devm_request_threaded_irq(dev,
+					ret, NULL, ad4052_irq_handler_thresh,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = fwnode_irq_get(dev_fwnode(&st->spi->dev), 1);
+	if (ret <= 0)
+		return ret ? ret : -EINVAL;
+
+	st->gp1_irq = ret;
+	ret = devm_request_threaded_irq(dev,
+					ret, NULL, ad4052_irq_handler_drdy,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					indio_dev->name, st);
+	return ret;
+}
+
+static const int ad4052_oversampling_avail[] = {
+	0, 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 ssize_t ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int val)
+{
+	int ret;
+
+	if (AD4052_CHECK_RATE(st->chip->grade, val))
+		return -EINVAL;
+
+	ret = __ad4052_set_sampling_freq(st, val);
+
+	return ret;
+}
+
+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 = st->d16;
+		if (st->data_format & AD4052_DATA_FORMAT)
+			*val = sign_extend32(*val, 15);
+	} else {
+		*val = st->d32;
+		if (st->data_format & AD4052_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);
+	int ret;
+
+	if (!ad4052_iio_device_claim_direct(indio_dev, st))
+		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:
+		*val = st->offload_trigger_hz;
+		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 (!ad4052_iio_device_claim_direct(indio_dev, st))
+		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_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);
+	int ret, state;
+
+	if (!ad4052_iio_device_claim_direct(indio_dev, st))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap, AD4052_REG_GP_CONFIG, &state);
+
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : state & AD4052_GP_MODE_MSK(0);
+}
+
+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)
+		goto out_release;
+
+	if (state) {
+		ret = pm_runtime_resume_and_get(&st->spi->dev);
+		if (ret)
+			goto out_release;
+
+		ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
+		if (ret)
+			goto out_err_suspend;
+	} else {
+		pm_runtime_mark_last_busy(&st->spi->dev);
+		pm_runtime_put_autosuspend(&st->spi->dev);
+
+		ret = ad4052_exit_command(st);
+	}
+	st->wait_event = state;
+	iio_device_release_direct(indio_dev);
+	return ret;
+
+out_err_suspend:
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4052_read_event_value(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info, int *val,
+				   int *val2)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	u8 reg, size = 1;
+	int ret;
+
+	if (!ad4052_iio_device_claim_direct(indio_dev, st))
+		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++;
+		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:
+		iio_device_release_direct(indio_dev);
+		return -EINVAL;
+	}
+
+	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
+	if (ret) {
+		iio_device_release_direct(indio_dev);
+		return ret;
+	}
+
+	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
+		*val = be16_to_cpu(st->d16);
+		if (st->data_format & AD4052_DATA_FORMAT)
+			*val = sign_extend32(*val, 11);
+	} else {
+		*val = st->d32;
+	}
+
+	iio_device_release_direct(indio_dev);
+	return 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 (!ad4052_iio_device_claim_direct(indio_dev, st))
+		return -EBUSY;
+
+	st->d16 = cpu_to_be16(val);
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			if (st->data_format & AD4052_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++;
+			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 >>= 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_preenable(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	struct spi_offload_trigger_config config = {
+		.type = SPI_OFFLOAD_TRIGGER_PERIODIC,
+		.periodic = {
+			.frequency_hz = st->offload_trigger_hz,
+		},
+	};
+	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;
+
+	disable_irq(st->gp1_irq);
+
+	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
+					 &config);
+	if (ret)
+		goto out_offload_error;
+
+	return 0;
+
+out_offload_error:
+	enable_irq(st->gp1_irq);
+out_error:
+	pm_runtime_mark_last_busy(&st->spi->dev);
+	pm_runtime_put_autosuspend(&st->spi->dev);
+
+	return ret;
+}
+
+static int ad4052_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad4052_state *st = iio_priv(indio_dev);
+	int ret;
+
+	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 = {
+	.preenable = &ad4052_buffer_preenable,
+	.postdisable = &ad4052_buffer_postdisable,
+};
+
+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 (!ad4052_iio_device_claim_direct(indio_dev, st))
+		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);
+
+	/*
+	 * REVISIT: the supported offload has a fixed length of 32-bits
+	 * to fit the 24-bits oversampled case, requiring the additional
+	 * offload scan types.
+	 */
+	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,
+	.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,
+	.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 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;
+	int ret;
+
+	indio_dev->setup_ops = &ad4052_buffer_setup_ops;
+
+	st->offload_xfer.offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+	st->offload_trigger = devm_spi_offload_trigger_get(dev,
+							   st->offload,
+							   SPI_OFFLOAD_TRIGGER_PERIODIC);
+
+	if (IS_ERR(st->offload_trigger))
+		return PTR_ERR(st->offload_trigger);
+
+	ret = __ad4052_set_sampling_freq(st, AD4052_MAX_RATE(st->chip->grade));
+	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;
+	u8 buf;
+
+	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(&spi->dev,  PTR_ERR(st->regmap),
+				     "Failed to initialize regmap\n");
+
+	st->mode = AD4052_SAMPLE_MODE;
+	st->wait_event = false;
+	st->chip = chip;
+
+	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);
+	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;
+
+	ret = ad4052_soft_reset(st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "AD4052 failed to soft reset\n");
+
+	ret = ad4052_assert(st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "AD4052 fields assertions failed\n");
+
+	ret = ad4052_set_non_defaults(indio_dev, indio_dev->channels);
+	if (ret)
+		return ret;
+
+	buf = AD4052_DEVICE_RESET;
+	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS, buf);
+	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_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	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");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int ad4052_runtime_suspend(struct device *dev)
+{
+	u8 val = FIELD_PREP(AD4052_POWER_MODE_MSK, AD4052_LOW_POWER_MODE);
+	struct ad4052_state *st = dev_get_drvdata(dev);
+
+	return regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG, val);
+}
+
+static int ad4052_runtime_resume(struct device *dev)
+{
+	struct ad4052_state *st = dev_get_drvdata(dev);
+	u8 val = FIELD_PREP(AD4052_POWER_MODE_MSK, 0);
+	int ret;
+
+	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG, val);
+	if (ret)
+		return ret;
+
+	fsleep(2000);
+	return 0;
+}
+
+static const struct dev_pm_ops ad4052_pm_ops = {
+	RUNTIME_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.48.1

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

[Conor Dooley]

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

On Thu, Mar 06, 2025 at 03:03:15PM +0100, Jorge Marques wrote:
> Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
> low-power with monitor capabilities SAR ADCs.
> Contain selectable oversampling and sample rate, the latter for both
> oversampling and monitor mode.
> The monitor capability is exposed as an IIO threshold either direction
> event.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4052.yaml    | 80 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 86 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..4602f1f0184d58f33883852ff6d76933758825f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> @@ -0,0 +1,80 @@
> +# 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.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4050
> +      - adi,ad4052
> +      - adi,ad4056
> +      - adi,ad4058

Can you mention in your commit message what differs between these
devices that makes picking one as the "base"/fallback compatible
unsuitable please?

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      Reference clock
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: threshold events.
> +      - description: device ready and data ready.
> +
> +  cnv-gpios:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 62500000
> +
> +  vdd-supply: true
> +  vdd_1_8-supply: true

You're allowed to use . in property names, and the _s should be -s.
That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in
that case?

> +  vio-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +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>;
> +            spi-max-frequency = <25000000>;
> +
> +            interrupt-parent = <&gpio>;
> +            interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> +                         <0 1 IRQ_TYPE_EDGE_RISING>;
> +            cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 06f122cb8bbd15a0076c229dfc89be0b5126f237..fef8adaee888d59e1aa3b3592dda5a8bea0b7677 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1317,6 +1317,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.48.1
> 

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

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

[David Lechner]

On Thu, Mar 6, 2025 at 3:04 PM Jorge Marques <jorge.marques@analog.com> wrote:
>
> Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
> low-power with monitor capabilities SAR ADCs.

> Contain selectable oversampling and sample rate, the latter for both
> oversampling and monitor mode.
> The monitor capability is exposed as an IIO threshold either direction
> event.

These sounds like they are describing the driver so aren't appropriate
for this commit message. Here we should only be talking about the
bindings.

>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4052.yaml    | 80 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 86 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..4602f1f0184d58f33883852ff6d76933758825f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> @@ -0,0 +1,80 @@
> +# 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.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf

The links above don't work for me. Instead...

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
> +
> +  clocks:
> +    description:
> +      Reference clock
> +    maxItems: 1

I don't see any pins in the datasheet about a "reference clock" input.
Is this for the CNV pin? If this is for the internal clock, then we
don't need a property for it.

> +
> +  interrupts:
> +    items:
> +      - description: threshold events.
> +      - description: device ready and data ready.
> +

Since there are multiple interrupts, we should also have an
interrupt-names property. Also, the interrupts should be named after
the pin they are connected to, not the function. So the interrupt
names should be "rdy", "gp0", and "gp1".

> +  cnv-gpios:
> +    maxItems: 1

Not necessary, but I would not mind having a description that says
that the CNV pin may also be connected to the CS line of the SPI
controller if it is not connected to a GPIO.

> +
> +  spi-max-frequency:
> +    maximum: 62500000
> +
> +  vdd-supply: true

> +  vdd_1_8-supply: true

This one seems redundant and should be dropped.

But there is also a possible separate reference voltage supply, so we
should have a ref-supply property.

> +  vio-supply: true

These chips also have GPIO pins, so we can add the gpio-controller and
#gpio-cells properties to the bindings even if we don't implement this
in the driver.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts

The chip won't work without vcc-supply and vio-supply so they should
be required. ref-supply is clearly optional though.



> +
> +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>;
> +            spi-max-frequency = <25000000>;
> +
> +            interrupt-parent = <&gpio>;
> +            interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> +                         <0 1 IRQ_TYPE_EDGE_RISING>;
> +            cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 06f122cb8bbd15a0076c229dfc89be0b5126f237..fef8adaee888d59e1aa3b3592dda5a8bea0b7677 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1317,6 +1317,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.48.1
>

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

[David Lechner]

On Thu, Mar 6, 2025 at 3:04 PM Jorge Marques <jorge.marques@analog.com> 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 | 93 ++++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                  |  1 +
>  2 files changed, 94 insertions(+)
>
> diff --git a/Documentation/iio/ad4052.rst b/Documentation/iio/ad4052.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..cf0cbd60d0a48ea52f74ea02fde659fcdba61aa1
> --- /dev/null
> +++ b/Documentation/iio/ad4052.rst
> @@ -0,0 +1,93 @@
> +.. 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.
> +   * - ``sample_rate``
> +     - Device internal sample rate used in the burst averaging mode.
> +   * - ``sample_rate_available``
> +     - List of available sample rates.

Why not using the standard sampling_frequency[_available] attributes?

> +
> +Threshold events
> +================
> +
> +The ADC supports a monitoring mode to raise threshold events.
> +The driver supports a single interrupt for both rising and falling
> +readings.
> +
> +During monitor mode, the device is busy since other transactions
> +require to put the device in configuration mode first.

This isn't so clear to me. Is this saying that events do not work
while doing a buffered read? Do you need to do need to read the
in_voltage0_raw input to trigger an event?

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

This diagram show a PWM connected to the CNV pin on the ADC, but I
didn't see a pwms property in the DT bindings to describe this.

> +
> +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 fef8adaee888d59e1aa3b3592dda5a8bea0b7677..312b2cf94b8f06298b1cbe5975ee32e2cf9a74d8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1322,6 +1322,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.48.1
>

I didn't have time to read the full datasheet or look at the driver
code yet, but can do that next week.

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

[kernel test robot]

Hi Jorge,

kernel test robot noticed the following build warnings:

[auto build test WARNING on aac287ec80d71a7ab7e44c936a434625417c3e30]

url:    https://github.com/intel-lab-lkp/linux/commits/Jorge-Marques/iio-code-mark-iio_dev-as-const-in-iio_buffer_enabled/20250306-220719
base:   aac287ec80d71a7ab7e44c936a434625417c3e30
patch link:    https://lore.kernel.org/r/20250306-iio-driver-ad4052-v1-4-2badad30116c%40analog.com
patch subject: [PATCH 4/4] iio: adc: add support for ad4052
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250307/202503071916.STHJTSlp-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250307/202503071916.STHJTSlp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503071916.STHJTSlp-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/ad4052.c:239:18: warning: 'ad4052_sample_rate_avail' defined but not used [-Wunused-const-variable=]
     239 | static const int ad4052_sample_rate_avail[] = {
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/ad4052.c:214:41: warning: 'ad4052_regmap_wr_table' defined but not used [-Wunused-const-variable=]
     214 | static const struct regmap_access_table ad4052_regmap_wr_table = {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/ad4052.c:201:41: warning: 'ad4052_regmap_rd_table' defined but not used [-Wunused-const-variable=]
     201 | static const struct regmap_access_table ad4052_regmap_rd_table = {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~


vim +/ad4052_sample_rate_avail +239 drivers/iio/adc/ad4052.c

   200	
 > 201	static const struct regmap_access_table ad4052_regmap_rd_table = {
   202		.yes_ranges = ad4052_regmap_rd_ranges,
   203		.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_rd_ranges),
   204	};
   205	
   206	static const struct regmap_range ad4052_regmap_wr_ranges[] = {
   207		regmap_reg_range(AD4052_REG_INTERFACE_CONFIG_A, AD4052_REG_DEVICE_CONFIG),
   208		regmap_reg_range(AD4052_REG_SCRATCH_PAD, AD4052_REG_SCRATCH_PAD),
   209		regmap_reg_range(AD4052_REG_STREAM_MODE, AD4052_REG_INTERFACE_STATUS),
   210		regmap_reg_range(AD4052_REG_MODE_SET, AD4052_REG_MON_VAL),
   211		regmap_reg_range(AD4052_REG_FUSE_CRC, AD4052_REG_DEVICE_STATUS),
   212	};
   213	
 > 214	static const struct regmap_access_table ad4052_regmap_wr_table = {
   215		.yes_ranges = ad4052_regmap_wr_ranges,
   216		.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_wr_ranges),
   217	};
   218	
   219	static const struct iio_event_spec ad4052_events[] = {
   220		{
   221			.type = IIO_EV_TYPE_THRESH,
   222			.dir = IIO_EV_DIR_EITHER,
   223			.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE)
   224		},
   225		{
   226			.type = IIO_EV_TYPE_THRESH,
   227			.dir = IIO_EV_DIR_RISING,
   228			.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
   229					      BIT(IIO_EV_INFO_HYSTERESIS)
   230		},
   231		{
   232			.type = IIO_EV_TYPE_THRESH,
   233			.dir = IIO_EV_DIR_FALLING,
   234			.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
   235					      BIT(IIO_EV_INFO_HYSTERESIS)
   236		}
   237	};
   238	
 > 239	static const int ad4052_sample_rate_avail[] = {
   240		2000000, 1000000, 300000, 100000, 33300,
   241		10000, 3000, 500, 333, 250, 200,
   242		166, 140, 125, 111
   243	};
   244	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Jorge,

kernel test robot noticed the following build warnings:

[auto build test WARNING on aac287ec80d71a7ab7e44c936a434625417c3e30]

url:    https://github.com/intel-lab-lkp/linux/commits/Jorge-Marques/iio-code-mark-iio_dev-as-const-in-iio_buffer_enabled/20250306-220719
base:   aac287ec80d71a7ab7e44c936a434625417c3e30
patch link:    https://lore.kernel.org/r/20250306-iio-driver-ad4052-v1-4-2badad30116c%40analog.com
patch subject: [PATCH 4/4] iio: adc: add support for ad4052
config: powerpc-randconfig-r132-20250307 (https://download.01.org/0day-ci/archive/20250307/202503072008.ysqhEBaX-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250307/202503072008.ysqhEBaX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503072008.ysqhEBaX-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/adc/ad4052.c:357:31: sparse: sparse: symbol 'ad4050_chip_info' was not declared. Should it be static?
>> drivers/iio/adc/ad4052.c:366:31: sparse: sparse: symbol 'ad4052_chip_info' was not declared. Should it be static?
>> drivers/iio/adc/ad4052.c:375:31: sparse: sparse: symbol 'ad4056_chip_info' was not declared. Should it be static?
>> drivers/iio/adc/ad4052.c:384:31: sparse: sparse: symbol 'ad4058_chip_info' was not declared. Should it be static?
>> drivers/iio/adc/ad4052.c:711:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected int @@     got restricted __be16 [usertype] d16 @@
   drivers/iio/adc/ad4052.c:711:22: sparse:     expected int
   drivers/iio/adc/ad4052.c:711:22: sparse:     got restricted __be16 [usertype] d16
>> drivers/iio/adc/ad4052.c:715:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected int @@     got restricted __be32 [usertype] d32 @@
   drivers/iio/adc/ad4052.c:715:22: sparse:     expected int
   drivers/iio/adc/ad4052.c:715:22: sparse:     got restricted __be32 [usertype] d32
   drivers/iio/adc/ad4052.c:912:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected int @@     got restricted __be32 [usertype] d32 @@
   drivers/iio/adc/ad4052.c:912:22: sparse:     expected int
   drivers/iio/adc/ad4052.c:912:22: sparse:     got restricted __be32 [usertype] d32
>> drivers/iio/adc/ad4052.c:958:33: sparse: sparse: bad assignment (>>=) to restricted __be16
   drivers/iio/adc/ad4052.c:251:12: sparse: sparse: context imbalance in 'ad4052_iio_device_claim_direct' - different lock contexts for basic block
   drivers/iio/adc/ad4052.c:277:9: sparse: sparse: context imbalance in 'ad4052_sample_rate_get' - unexpected unlock
   drivers/iio/adc/ad4052.c:294:9: sparse: sparse: context imbalance in 'ad4052_sample_rate_set' - unexpected unlock
   drivers/iio/adc/ad4052.c:780:34: sparse: sparse: context imbalance in 'ad4052_read_raw' - unexpected unlock
   drivers/iio/adc/ad4052.c:805:34: sparse: sparse: context imbalance in 'ad4052_write_raw' - unexpected unlock
   drivers/iio/adc/ad4052.c:820:9: sparse: sparse: context imbalance in 'ad4052_read_event_config' - unexpected unlock
   drivers/iio/adc/ad4052.c:903:42: sparse: sparse: context imbalance in 'ad4052_read_event_value' - unexpected unlock
   drivers/iio/adc/ad4052.c:971:34: sparse: sparse: context imbalance in 'ad4052_write_event_value' - unexpected unlock
   drivers/iio/adc/ad4052.c:1055:34: sparse: sparse: context imbalance in 'ad4052_debugfs_reg_access' - unexpected unlock

vim +/ad4050_chip_info +357 drivers/iio/adc/ad4052.c

   356	
 > 357	const struct ad4052_chip_info ad4050_chip_info = {
   358		.name = "ad4050",
   359		.channels = { AD4052_CHAN(12, AD4052_2MSPS) },
   360		.offload_channels = { AD4052_OFFLOAD_CHAN(12, AD4052_2MSPS) },
   361		.prod_id = 0x70,
   362		.max_avg = AD4050_MAX_AVG,
   363		.grade = AD4052_2MSPS,
   364	};
   365	
 > 366	const struct ad4052_chip_info ad4052_chip_info = {
   367		.name = "ad4052",
   368		.channels = { AD4052_CHAN(16, AD4052_2MSPS) },
   369		.offload_channels = { AD4052_OFFLOAD_CHAN(16, AD4052_2MSPS) },
   370		.prod_id = 0x72,
   371		.max_avg = AD4052_MAX_AVG,
   372		.grade = AD4052_2MSPS,
   373	};
   374	
 > 375	const struct ad4052_chip_info ad4056_chip_info = {
   376		.name = "ad4056",
   377		.channels = { AD4052_CHAN(12, AD4052_500KSPS) },
   378		.offload_channels = { AD4052_OFFLOAD_CHAN(12, AD4052_500KSPS) },
   379		.prod_id = 0x70,
   380		.max_avg = AD4050_MAX_AVG,
   381		.grade = AD4052_500KSPS,
   382	};
   383	
 > 384	const struct ad4052_chip_info ad4058_chip_info = {
   385		.name = "ad4058",
   386		.channels = { AD4052_CHAN(16, AD4052_500KSPS) },
   387		.offload_channels = { AD4052_OFFLOAD_CHAN(16, AD4052_500KSPS) },
   388		.prod_id = 0x72,
   389		.max_avg = AD4052_MAX_AVG,
   390		.grade = AD4052_500KSPS,
   391	};
   392	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Jorge,

kernel test robot noticed the following build warnings:

[auto build test WARNING on aac287ec80d71a7ab7e44c936a434625417c3e30]

url:    https://github.com/intel-lab-lkp/linux/commits/Jorge-Marques/iio-code-mark-iio_dev-as-const-in-iio_buffer_enabled/20250306-220719
base:   aac287ec80d71a7ab7e44c936a434625417c3e30
patch link:    https://lore.kernel.org/r/20250306-iio-driver-ad4052-v1-4-2badad30116c%40analog.com
patch subject: [PATCH 4/4] iio: adc: add support for ad4052
config: um-randconfig-r073-20250307 (https://download.01.org/0day-ci/archive/20250308/202503080031.APfLdyiz-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250308/202503080031.APfLdyiz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503080031.APfLdyiz-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/iio/adc/ad4052.c:14:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:549:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     549 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:567:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     567 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/iio/adc/ad4052.c:14:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:585:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/iio/adc/ad4052.c:14:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:601:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     601 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:616:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     616 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:631:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     631 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:724:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     724 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:737:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     737 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:750:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     750 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:764:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     764 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:778:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     778 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:792:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     792 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> drivers/iio/adc/ad4052.c:201:41: warning: unused variable 'ad4052_regmap_rd_table' [-Wunused-const-variable]
     201 | static const struct regmap_access_table ad4052_regmap_rd_table = {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/ad4052.c:214:41: warning: unused variable 'ad4052_regmap_wr_table' [-Wunused-const-variable]
     214 | static const struct regmap_access_table ad4052_regmap_wr_table = {
         |                                         ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/ad4052.c:239:18: warning: unused variable 'ad4052_sample_rate_avail' [-Wunused-const-variable]
     239 | static const int ad4052_sample_rate_avail[] = {
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~
   15 warnings generated.


vim +/ad4052_regmap_rd_table +201 drivers/iio/adc/ad4052.c

   200	
 > 201	static const struct regmap_access_table ad4052_regmap_rd_table = {
   202		.yes_ranges = ad4052_regmap_rd_ranges,
   203		.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_rd_ranges),
   204	};
   205	
   206	static const struct regmap_range ad4052_regmap_wr_ranges[] = {
   207		regmap_reg_range(AD4052_REG_INTERFACE_CONFIG_A, AD4052_REG_DEVICE_CONFIG),
   208		regmap_reg_range(AD4052_REG_SCRATCH_PAD, AD4052_REG_SCRATCH_PAD),
   209		regmap_reg_range(AD4052_REG_STREAM_MODE, AD4052_REG_INTERFACE_STATUS),
   210		regmap_reg_range(AD4052_REG_MODE_SET, AD4052_REG_MON_VAL),
   211		regmap_reg_range(AD4052_REG_FUSE_CRC, AD4052_REG_DEVICE_STATUS),
   212	};
   213	
 > 214	static const struct regmap_access_table ad4052_regmap_wr_table = {
   215		.yes_ranges = ad4052_regmap_wr_ranges,
   216		.n_yes_ranges = ARRAY_SIZE(ad4052_regmap_wr_ranges),
   217	};
   218	
   219	static const struct iio_event_spec ad4052_events[] = {
   220		{
   221			.type = IIO_EV_TYPE_THRESH,
   222			.dir = IIO_EV_DIR_EITHER,
   223			.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE)
   224		},
   225		{
   226			.type = IIO_EV_TYPE_THRESH,
   227			.dir = IIO_EV_DIR_RISING,
   228			.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
   229					      BIT(IIO_EV_INFO_HYSTERESIS)
   230		},
   231		{
   232			.type = IIO_EV_TYPE_THRESH,
   233			.dir = IIO_EV_DIR_FALLING,
   234			.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
   235					      BIT(IIO_EV_INFO_HYSTERESIS)
   236		}
   237	};
   238	
 > 239	static const int ad4052_sample_rate_avail[] = {
   240		2000000, 1000000, 300000, 100000, 33300,
   241		10000, 3000, 500, 333, 250, 200,
   242		166, 140, 125, 111
   243	};
   244	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Jonathan Cameron]

> > +  vdd-supply: true
> > +  vdd_1_8-supply: true  
> 
> You're allowed to use . in property names, and the _s should be -s.
> That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in
> that case?
I got curious and opened datasheet.  Only seeing two supplies (vdd and vio)

So checked driver and that doesn't enable any of them.
> 
> > +  vio-supply: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
Supplies that are not actually optional (i.e. device can run without them)
should be in required list even though the regulator core will provide
stubs if they aren't in your dts.  The aim is to reflect what should 
be provided, not what Linux does with it.

Jonathan

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

[Jonathan Cameron]

On Thu, 6 Mar 2025 15:03:15 +0100
Jorge Marques <jorge.marques@analog.com> wrote:

> Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
> low-power with monitor capabilities SAR ADCs.
> Contain selectable oversampling and sample rate, the latter for both
> oversampling and monitor mode.
> The monitor capability is exposed as an IIO threshold either direction
> event.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4052.yaml    | 80 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 86 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..4602f1f0184d58f33883852ff6d76933758825f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> @@ -0,0 +1,80 @@
> +# 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.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf
It's this data sheet that I opened.  Seems it describes things a bit
different that you have here.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4050
> +      - adi,ad4052
> +      - adi,ad4056
> +      - adi,ad4058
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      Reference clock
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: threshold events.
> +      - description: device ready and data ready.

People have a nasty habit of wiring just one. So use
interrupt-names and let them come in any order.  The driver can require
both or a specific one if it likes, but in future we may need to make it
more flexible and the dt-binding should allow that.

They seem to be GP0 and GP1 on datasheet and don't have fixed roles
like this implies.

> +
> +  cnv-gpios:

Not the most self explanatory of names. I'd suggest a bit of help
text for this one.

> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 62500000
> +
> +  vdd-supply: true
> +  vdd_1_8-supply: true
As per other thread, supplies like this normally required and this
one at least doesn't seem to exist in the datasheet I randomly picked.

> +  vio-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +

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

[Jonathan Cameron]

On Thu, 6 Mar 2025 15:03:17 +0100
Jorge Marques <jorge.marques@analog.com> wrote:

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

Various fairly minor comments inline.

Jonathan

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 27413516216cb3f83cf1d995b9ffc22bf01776a4..f518dadbdd3a6b0543d0b78206fcbc203898454c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -62,6 +62,20 @@ config AD4130
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad4130.
>  
> +config AD4052
Aim for alphanumeric order so this should at least be before AD4130

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

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

...

> +#define AD4052_REG_FUSE_CRC		0x40
> +#define AD4052_REG_DEVICE_STATUS	0x41
> +#define AD4052_REG_MIN_SAMPLE		0x45
> +#define AD4052_MAX_REG			0x45
> +/* GP_CONFIG */
Where possible it makes for easier to follow code if the field defines
include what register they are in rather than relying on comments. 
e.g.
#define AD4052_GP_CONFIG_MODE_MASK(x) etc

> +#define AD4052_GP_MODE_MSK(x)		(GENMASK(2, 0) << (x) * 4)
Macro is a bit too 'clever'.  I think it would easier to just have
AD4052_GP_CONFIG_GP0_MODE_MSK	GENMMSK(2, 0)
AD4052_GP_CONFIG_GP1_MODE_MSK	GENMASK(6, 4)

> +/* INTR_CONFIG */
> +#define AD4052_INTR_EN_MSK(x)		(GENMASK(1, 0) << (x) * 4)
Similar for this one.
> +/* ADC_MODES */
> +#define AD4052_DATA_FORMAT		BIT(7)
> +/* DEVICE_CONFIG */
> +#define AD4052_POWER_MODE_MSK		GENMASK(1, 0)
> +#define AD4052_LOW_POWER_MODE		3
> +/* DEVICE_STATUS */
> +#define AD4052_DEVICE_RESET		BIT(6)
> +#define AD4052_THRESH_OVERRUN		BIT(4)
> +#define AD4052_MAX_FLAG			BIT(3)
> +#define AD4052_MIN_FLAG			BIT(2)
> +#define AD4052_EVENT_CLEAR		(AD4052_THRESH_OVERRUN | AD4052_MAX_FLAG | AD4052_MIN_FLAG)
Wrap the define with \ to break the line.

> +/* TIMER_CONFIG */
> +#define AD4052_FS_MASK			GENMASK(7, 4)
> +#define AD4052_300KSPS			0x2
> +
> +#define AD4052_SPI_VENDOR		0x0456
> +
> +#define AD4050_MAX_AVG			0x7
> +#define AD4052_MAX_AVG			0xB
> +#define AD4052_CHECK_OVERSAMPLING(x, y)	({typeof(y) y_ = (y); \
> +					  ((y_) < 0 || (y_) > BIT((x) + 1)); })

Don't have single use macros like these.  Better to have the code inline
where we can see what it is doing.

> +#define AD4052_MAX_RATE(x)		((x) == AD4052_500KSPS ? 500000 : 2000000)
> +#define AD4052_CHECK_RATE(x, y)		({typeof(y) y_ = (y);				\
> +					  ((y_) > AD4052_MAX_RATE(x) || (y_) <= 0); })
> +#define AD4052_FS_OFFSET(g)		((g) == AD4052_500KSPS ? 2 : 0)
> +#define AD4052_FS(g)			(&ad4052_sample_rates[AD4052_FS_OFFSET(g)])
> +#define AD4052_FS_LEN(g)		(ARRAY_SIZE(ad4052_sample_rates) - (AD4052_FS_OFFSET(g)))

...

> +static const int ad4052_sample_rate_avail[] = {
> +	2000000, 1000000, 300000, 100000, 33300,
> +	10000, 3000, 500, 333, 250, 200,
> +	166, 140, 125, 111

trailing comma missing

> +};
> +
> +static const char *const ad4052_sample_rates[] = {
> +	"2000000", "1000000", "300000", "100000", "33300",
> +	"10000", "3000", "500", "333", "250", "200",
> +	"166", "140", "124", "111",
Not sure why this can't be done with read_avail and the values above.
> +};
> +
> +static int ad4052_iio_device_claim_direct(struct iio_dev *indio_dev,
> +					  struct ad4052_state *st)
> +{
> +	if (!iio_device_claim_direct(indio_dev))
> +		return false;

This might stretch sparses ability to keep track or __acquire / __release.
Make sure to check that with a C=1 build. If the cond_acquires
stuff is merged into sparse, this may need a revisit for markings.

> +
> +	/**

Not kernel-doc, so /*

> +	 * If the device is in monitor mode, no register access is allowed,
> +	 * since it would put the device back in configuration mode.
> +	 */
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static int ad4052_sample_rate_get(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret, val;
> +
> +	if (!ad4052_iio_device_claim_direct(indio_dev, st))
> +		return -EBUSY;
> +
> +	ret = regmap_read(st->regmap, AD4052_REG_TIMER_CONFIG, &val);
> +	val = FIELD_GET(AD4052_FS_MASK, val);

I don't really like the double use of the val variable as it loses
meaning we could otherwise provide in the variable naming.

> +
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : val - AD4052_FS_OFFSET(st->chip->grade);
> +}
> +
> +static int ad4052_sample_rate_set(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 (!ad4052_iio_device_claim_direct(indio_dev, st))
> +		return -EBUSY;
> +
> +	val += AD4052_FS_OFFSET(st->chip->grade);
> +	val = FIELD_PREP(AD4052_FS_MASK, val);

Using val for two different things here. I'd avoid that
by just having this last line merged with the next one.

> +	ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG, val);
> +
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}

> +
> +#define AD4052_EXT_INFO(grade)								\
> +static struct iio_chan_spec_ext_info grade##_ext_info[] = {				\
> +	IIO_ENUM("sample_rate", IIO_SHARED_BY_ALL, &grade##_sample_rate_enum),		\
> +	IIO_ENUM_AVAILABLE("sample_rate", IIO_SHARED_BY_ALL, &grade##_sample_rate_enum),\
> +	{}										\
{ }
preferred slightly.

> +}


> +static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
> +					 unsigned int *val)
> +{
> +	int ret;
> +
> +	if (st->mode == AD4052_SAMPLE_MODE) {
> +		*val = 0;

Probably = 1 to reflect no oversampling.
IIRC the attribute allows either but to me at least a default of 1
is more logical.

> +		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_assert(struct ad4052_state *st)
Slighly odd name.  check_ids or something like that.

> +{
> +	int ret;
> +	u16 val;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4052_REG_PROD_ID_1, &st->d16, 2);

sizeof(st->d16) here and in similar places.

> +	if (ret)
> +		return ret;
> +
> +	val = be16_to_cpu(st->d16);
> +	if (val != st->chip->prod_id)
> +		return -ENODEV;

Should not be treated as a failure as that breaks the future use of
fallback compatible values in DT (support new hardware on old kernel)
Instead just print a message saying it didn't match and carry on as if it did.

> +
> +	ret = regmap_bulk_read(st->regmap, AD4052_REG_VENDOR_H, &st->d16, 2);
> +	if (ret)
> +		return ret;
> +
> +	val = be16_to_cpu(st->d16);
> +	if (val != AD4052_SPI_VENDOR)
> +		return -ENODEV;
> +
> +	return 0;
> +}

> +
> +static int ad4052_set_operation_mode(struct ad4052_state *st, enum ad4052_operation_mode mode)
> +{
> +	u8 val = st->data_format | mode;

Maybe regmap_update_bits so we don't have to store st->data_format if
that bit has already been written.

> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> +	if (ret)
> +		return ret;
> +
> +	val = BIT(0);

This should have some sort of define and then use that inline
in the regmap_write() call.


> +	return regmap_write(st->regmap, AD4052_REG_MODE_SET, val);
> +}

> +
> +static int ad4052_set_non_defaults(struct iio_dev *indio_dev,
i kind of get where you are coming from with the 'non defaults'
but we are setting software driven defaults here.

Maybe just rename as ad4052_setup() or something similarly vague.

> +				   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);
> +
> +	u8 val = FIELD_PREP(AD4052_GP_MODE_MSK(0), AD4052_GP_INTR) |
> +		 FIELD_PREP(AD4052_GP_MODE_MSK(1), AD4052_GP_DRDY);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONFIG,
> +				 AD4052_GP_MODE_MSK(1) | AD4052_GP_MODE_MSK(0),
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_INTR_EN_MSK(0), (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_INTR_EN_MSK(1), (AD4052_INTR_EN_NEITHER));
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONFIG,
> +				 AD4052_INTR_EN_MSK(0) | AD4052_INTR_EN_MSK(1),
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = 0;
> +	if (scan_type->sign == 's')
> +		val |= AD4052_DATA_FORMAT;
> +
> +	st->data_format = val;
> +
> +	if (st->chip->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_FS_MASK, AD4052_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;
> +
> +	ret = fwnode_irq_get(dev_fwnode(&st->spi->dev), 0);

As per the binding review, use named variant as we should
not be controlling the order, but rather specifying which
is which in the dt-binding.

> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	ret = devm_request_threaded_irq(dev,
> +					ret, NULL, ad4052_irq_handler_thresh,

odd wrap.  Take each line up to 80 chars before wrapping to next one.

> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,

Direction should come from firmware, not be specified here.
There might be an inverter in the path.  That used to be a common cheap
way of doing level conversion for interrupt lines and it is handled by
flipping the sense of the interrupt in the dts.

> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_irq_get(dev_fwnode(&st->spi->dev), 1);
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	st->gp1_irq = ret;
> +	ret = devm_request_threaded_irq(dev,
> +					ret, NULL, ad4052_irq_handler_drdy,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					indio_dev->name, st);
return devm_request_thread_irq.

> +	return ret;
> +}
> +
> +static const int ad4052_oversampling_avail[] = {
> +	0, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096

Always a trailing comma unless it is some form of terminator.

Oversampling ratio of 0 is a bit strange. That should probably be 1
to reflect 1 sample per reading output (or no oversampling).

> +};

> +
> +static ssize_t ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int val)
> +{
> +	int ret;
> +
> +	if (AD4052_CHECK_RATE(st->chip->grade, val))
> +		return -EINVAL;
> +
> +	ret = __ad4052_set_sampling_freq(st, val);
> +
> +	return ret;
	return __ad4052_set_sampling_freq(st, val);

> +}



> +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)
> +		goto out_release;
> +
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(&st->spi->dev);
> +		if (ret)
> +			goto out_release;
> +
> +		ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> +		if (ret)
> +			goto out_err_suspend;
Given the error handling is different in the two paths, I'd
split this into two helpers - one each for enable and disable.
Probably take the direct claim around where they are called.

> +	} else {
> +		pm_runtime_mark_last_busy(&st->spi->dev);
> +		pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +		ret = ad4052_exit_command(st);
> +	}
> +	st->wait_event = state;
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +
> +out_err_suspend:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}
> +
> +static int ad4052_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val,
> +				   int *val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	u8 reg, size = 1;
> +	int ret;
> +
> +	if (!ad4052_iio_device_claim_direct(indio_dev, st))
> +		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++;

As below.  Seems to me better to just set size to 2 here.

> +		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:
> +		iio_device_release_direct(indio_dev);
> +		return -EINVAL;
Maybe use an error block and goto.  You could factor
out the stuff under the direct claim as an alternative
path to simpler code.

> +	}
> +
> +	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
> +	if (ret) {
> +		iio_device_release_direct(indio_dev);
> +		return ret;
> +	}
> +
> +	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_DATA_FORMAT)
> +			*val = sign_extend32(*val, 11);
> +	} else {
> +		*val = st->d32;
> +	}
> +
> +	iio_device_release_direct(indio_dev);
> +	return 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 (!ad4052_iio_device_claim_direct(indio_dev, st))
> +		return -EBUSY;
> +
> +	st->d16 = cpu_to_be16(val);
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			if (st->data_format & AD4052_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++;
Set size directly to 2 perhaps. I'm not really understanding why
the increment scheme makes more sense.

> +			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 >>= 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_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct spi_offload_trigger_config config = {
> +		.type = SPI_OFFLOAD_TRIGGER_PERIODIC,
> +		.periodic = {
> +			.frequency_hz = st->offload_trigger_hz,
> +		},
> +	};
> +	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;
> +
> +	disable_irq(st->gp1_irq);

Add a comment on why.

> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
> +	if (ret)
> +		goto out_offload_error;
> +
> +	return 0;
> +
> +out_offload_error:
> +	enable_irq(st->gp1_irq);
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->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);
> +
> +	/*
> +	 * REVISIT: the supported offload has a fixed length of 32-bits
> +	 * to fit the 24-bits oversampled case, requiring the additional
> +	 * offload scan types.
> +	 */

That's an additional feature I think. We don't need to have a comment
about things we haven't done in the driver.

> +	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 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;
> +	u8 buf;
> +
> +	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(&spi->dev,  PTR_ERR(st->regmap),

Use dev instead of spi->dev

> +				     "Failed to initialize regmap\n");
> +
> +	st->mode = AD4052_SAMPLE_MODE;
> +	st->wait_event = false;
> +	st->chip = chip;
> +
> +	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv",
> +					     GPIOD_OUT_LOW);

wrap to 80 chars - so don't wrap the above.

> +	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);
> +	ret = PTR_ERR_OR_ZERO(st->offload);

Use IS_ERR() to detect error and PTR_ERR() to get it if needed (will
end up calling PTR_ERR() twice but similar anyway.

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

No need to wrap as fairly sure that's under 80 chars anyway.

> +
> +	ret = ad4052_assert(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "AD4052 fields assertions failed\n");
> +
> +	ret = ad4052_set_non_defaults(indio_dev, indio_dev->channels);
> +	if (ret)
> +		return ret;
> +
> +	buf = AD4052_DEVICE_RESET;

Pass directly into regmap_write()

> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS, buf);
> +	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_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);

These autosuspend things are normally done after enabling runtime pm.
If nothing else that keeps the devm cleanup as being in opposite
order of what is set up here.  
https://elixir.bootlin.com/linux/v6.13.5/source/drivers/base/power/runtime.c#L1548

> +	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");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int ad4052_runtime_suspend(struct device *dev)
> +{
> +	u8 val = FIELD_PREP(AD4052_POWER_MODE_MSK, AD4052_LOW_POWER_MODE);

Put that inline and no need for local variable val.

> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +
> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG, val);
> +}
> +
> +static int ad4052_runtime_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	u8 val = FIELD_PREP(AD4052_POWER_MODE_MSK, 0);
Put that inline - no real point in the local variable.
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG, val);
	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
			   FIELD_PREP(AD4052_POWER_MODE_MSK, 0));

> +	if (ret)
> +		return ret;
> +
> +	fsleep(2000);

Sleeps like this should ideally have a spec reference as a comment to
justify why that value is chosen.

> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ad4052_pm_ops = {
> +	RUNTIME_PM_OPS(ad4052_runtime_suspend, ad4052_runtime_resume, NULL)
Can you allow this to be used for suspend and resume as well?  e.g.
DEFINE_RUNTIME_DEV_PM_OPS()

It is a rare case where that isn't safe to do even if there might be
deeper sleep states available that would be even better.

> +};
> +
> +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 },
> +	{}
Trivial but I'm slowly trying to standardize formatting of these tables
in IIO and I randomly decided to go with 
	{ }
Please use that for terminating entries in this new driver.

> +};
> +MODULE_DEVICE_TABLE(of, ad4052_of_match);

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

[Christophe JAILLET]

Le 06/03/2025 à 15:03, Jorge Marques a écrit :
> 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.
...

> +#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),	\

Nitpick: Unneeded extra space before BIT

> +	.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),	\

Nitpick: Unneeded extra space before BIT

> +	.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 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;
> +	u8 buf;
> +
> +	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(&spi->dev,  PTR_ERR(st->regmap),

Nitpick: Unneeded extra space before PTR_ERR

> +				     "Failed to initialize regmap\n");
> +
> +	st->mode = AD4052_SAMPLE_MODE;
> +	st->wait_event = false;
> +	st->chip = chip;

...

CJ

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

[Jorge Marques]

> > +  compatible:
> > +    enum:
> > +      - adi,ad4050
> > +      - adi,ad4052
> > +      - adi,ad4056
> > +      - adi,ad4058
> 
> Can you mention in your commit message what differs between these
> devices that makes picking one as the "base"/fallback compatible
> unsuitable please?
Sure, to be added:

 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 two required interrupts (gp0, gp1) and one optional
 gpio (cnv).

> > +
> > +  vdd-supply: true
> > +  vdd_1_8-supply: true
> 
> You're allowed to use . in property names, and the _s should be -s.
> That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in
> that case?
I overlooked the supplies, the correct are vdd, vio as mandatory,
and vref is optional.

Jorge

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

[Jorge Marques]

> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf
> 
> The links above don't work for me. Instead...
> 
> 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
Thanks!

> > +  clocks:
> > +    description:
> > +      Reference clock
> > +    maxItems: 1
> 
> I don't see any pins in the datasheet about a "reference clock" input.
> Is this for the CNV pin? If this is for the internal clock, then we
> don't need a property for it.
Indeed, this will be removed.

> 
> > +
> > +  interrupts:
> > +    items:
> > +      - description: threshold events.
> > +      - description: device ready and data ready.
> > +
> 
> Since there are multiple interrupts, we should also have an
> interrupt-names property. Also, the interrupts should be named after
> the pin they are connected to, not the function. So the interrupt
> names should be "rdy", "gp0", and "gp1".
Agreed.

> 
> > +  cnv-gpios:
> > +    maxItems: 1
> 
> Not necessary, but I would not mind having a description that says
> that the CNV pin may also be connected to the CS line of the SPI
> controller if it is not connected to a GPIO.
Included.

> 
> > +
> > +  spi-max-frequency:
> > +    maximum: 62500000
> > +
> > +  vdd-supply: true
> 
> > +  vdd_1_8-supply: true
> 
> This one seems redundant and should be dropped.
> 
> But there is also a possible separate reference voltage supply, so we
> should have a ref-supply property.
> 
> > +  vio-supply: true
Yes, I overlooked the supplies, vio, vdd are mandatory, and vref is optional.
Also noted Jonathan's comment that the aim is to reflect what should 
be provided, not what Linux does with it.

> 
> These chips also have GPIO pins, so we can add the gpio-controller and
> #gpio-cells properties to the bindings even if we don't implement this
> in the driver.
Good to know, added as suggested.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> 
> The chip won't work without vcc-supply and vio-supply so they should
> be required. ref-supply is clearly optional though.
Aggreed.

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

[Jorge Marques]

> > +description: |
> > +  Analog Devices AD4052 Single Channel Precision SAR ADC family
> > +
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf
> It's this data sheet that I opened.  Seems it describes things a bit
> different that you have here.
The power supplies were wrong, the correct ones are vdd, vio and vref
(optional).

> People have a nasty habit of wiring just one. So use
> interrupt-names and let them come in any order.  The driver can require
> both or a specific one if it likes, but in future we may need to make it
> more flexible and the dt-binding should allow that.
Agreed.

> They seem to be GP0 and GP1 on datasheet and don't have fixed roles
> like this implies.

GP0 and GP1 can output data ready, chop (not implemented), device enabled,
interrupt (threshold) or a constant logic state.
In the driver implementation, one takes the role of threshold interrupt and the
other of data ready.

> 
> > +
> > +  cnv-gpios:
> 
> Not the most self explanatory of names. I'd suggest a bit of help
> text for this one.
Noted.

> > +  vdd-supply: true
> > +  vdd_1_8-supply: true
> As per other thread, supplies like this normally required and this
> one at least doesn't seem to exist in the datasheet I randomly picked.
Fixed.

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

[Jorge Marques]

> > +.. 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.
> > +   * - ``sample_rate``
> > +     - Device internal sample rate used in the burst averaging mode.
> > +   * - ``sample_rate_available``
> > +     - List of available sample rates.
> 
> Why not using the standard sampling_frequency[_available] attributes?
Because sampling_frequency is the sampling frequency for the pwm trigger
during buffer readings.
sample_rate is the internal device clock used during monitor and burst
averaging modes.

> > +
> > +Threshold events
> > +================
> > +
> > +The ADC supports a monitoring mode to raise threshold events.
> > +The driver supports a single interrupt for both rising and falling
> > +readings.
> > +
> > +During monitor mode, the device is busy since other transactions
> > +require to put the device in configuration mode first.
> 
> This isn't so clear to me. Is this saying that events do not work
> while doing a buffered read? Do you need to do need to read the
> in_voltage0_raw input to trigger an event?
> 
No, the device monitor mode and trigger mode autonomously samples using the
internal clock set with the sample rate property.
I rephrased that to:

 The feature is enabled/disabled by setting ``thresh_either_en``.
 During monitor mode, the device continuously operate in autonomous mode until
 put back in configuration mode, due to this, the device returns busy until the
 feature is disabled.

The reasoning is that during configuration mode no ADC
conversion is done, including if the previous mode was autonomous.
If instead of return busy the driver hided this and resumed monitor mode
after the access, a hidden (to the user) monitoring down-time would and
thresholds crossings could be lost, undermining the feature.

> > +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
> 
> This diagram show a PWM connected to the CNV pin on the ADC, but I
> didn't see a pwms property in the DT bindings to describe this.
> 
It is not clear to me where the pwm property should be in the DT
bindings, since the PWM node now belongs to the offload-capable SPI controller.

> I didn't have time to read the full datasheet or look at the driver
> code yet, but can do that next week.
Ok, thank you for the review

Jorge

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

[Jorge Marques]

Hi Jonathan thank you for the review, comments excluded in the reply are
agreed and applied.

> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called ad4130.
> >  
> > +config AD4052
> Aim for alphanumeric order so this should at least be before AD4130
Ups, I got tricked during cherry-pick.
 
> > +#define AD4052_MIN_FLAG			BIT(2)
> > +#define AD4052_EVENT_CLEAR		(AD4052_THRESH_OVERRUN | AD4052_MAX_FLAG | AD4052_MIN_FLAG)
> Wrap the define with \ to break the line.
Deadcode... removed.

> > +};
> > +
> > +static const char *const ad4052_sample_rates[] = {
> > +	"2000000", "1000000", "300000", "100000", "33300",
> > +	"10000", "3000", "500", "333", "250", "200",
> > +	"166", "140", "124", "111",
> Not sure why this can't be done with read_avail and the values above.
Since it is the internal device sample rate, it is an extended
device attribute.
The channel IIO_SAMPLING_FREQUENCY is used for the sampling frequency during
buffer readings.
The macro IIO_ENUM_AVAILABLE is used to reduce redundancy.

The previous integer declaration was unused since the char array index is
used as the register r/w value.

> > +};
> > +
> > +static int ad4052_iio_device_claim_direct(struct iio_dev *indio_dev,
> > +					  struct ad4052_state *st)
> > +{
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return false;
> 
> This might stretch sparses ability to keep track or __acquire / __release.
> Make sure to check that with a C=1 build. If the cond_acquires
> stuff is merged into sparse, this may need a revisit for markings.

You are right!
I also did further fixes caught by sparse.

> > +{
> > +	int ret;
> > +
> > +	if (st->mode == AD4052_SAMPLE_MODE) {
> > +		*val = 0;
> 
> Probably = 1 to reflect no oversampling.
> IIRC the attribute allows either but to me at least a default of 1
> is more logical.
  
Agreed, 1 is now the only no oversampling value.

> > +}
> > +
> > +static int ad4052_assert(struct ad4052_state *st)
> Slighly odd name.  check_ids or something like that.
> 
Went with 'check_ids'.

> > +
> > +	val = be16_to_cpu(st->d16);
> > +	if (val != st->chip->prod_id)
> > +		return -ENODEV;
> 
> Should not be treated as a failure as that breaks the future use of
> fallback compatible values in DT (support new hardware on old kernel)
> Instead just print a message saying it didn't match and carry on as if it did.

Noted, added:

  "Production ID x%x does not match known values", val);

> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->spi->dev;
> > +	int ret = 0;
> > +
> > +	ret = fwnode_irq_get(dev_fwnode(&st->spi->dev), 0);
> 
> As per the binding review, use named variant as we should
> not be controlling the order, but rather specifying which
> is which in the dt-binding.

Makes more sense indeed.

> > +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> 
> Direction should come from firmware, not be specified here.
> There might be an inverter in the path.  That used to be a common cheap
> way of doing level conversion for interrupt lines and it is handled by
> flipping the sense of the interrupt in the dts.
> 
Agreed, defined the level flags as 0, and kept IRQF_ONESHOT the irq
flag, to dismiss the threaded IRQ with NULL handler needs to be oneshot.


> > +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)
> > +		goto out_release;
> > +
> > +	if (state) {
> > +		ret = pm_runtime_resume_and_get(&st->spi->dev);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> > +		if (ret)
> > +			goto out_err_suspend;
> Given the error handling is different in the two paths, I'd
> split this into two helpers - one each for enable and disable.
> Probably take the direct claim around where they are called.

Yes, that will make it easier to follow.

> > +
> > +	ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	disable_irq(st->gp1_irq);
> 
> Add a comment on why.

Added 

   /* SPI Offload handles the data ready irq */

> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +
> > +	/*
> > +	 * REVISIT: the supported offload has a fixed length of 32-bits
> > +	 * to fit the 24-bits oversampled case, requiring the additional
> > +	 * offload scan types.
> > +	 */
> 
> That's an additional feature I think. We don't need to have a comment
> about things we haven't done in the driver.

Removed comment.

> > +	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);
> > +	ret = PTR_ERR_OR_ZERO(st->offload);
> 
> Use IS_ERR() to detect error and PTR_ERR() to get it if needed (will
> end up calling PTR_ERR() twice but similar anyway.

Ok.

> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS, buf);
> > +	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_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> 
> These autosuspend things are normally done after enabling runtime pm.
> If nothing else that keeps the devm cleanup as being in opposite
> order of what is set up here.  
> https://elixir.bootlin.com/linux/v6.13.5/source/drivers/base/power/runtime.c#L1548

Makes sense.

> > +	if (ret)
> > +		return ret;
> > +
> > +	fsleep(2000);
> 
> Sleeps like this should ideally have a spec reference as a comment to
> justify why that value is chosen.
> 

This sleep is not needed since there is no timing specification in the
datasheet, removed.

> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops ad4052_pm_ops = {
> > +	RUNTIME_PM_OPS(ad4052_runtime_suspend, ad4052_runtime_resume, NULL)
> Can you allow this to be used for suspend and resume as well?  e.g.
> DEFINE_RUNTIME_DEV_PM_OPS()
> 
> It is a rare case where that isn't safe to do even if there might be
> deeper sleep states available that would be even better.

Yes.

> > +	{}
> Trivial but I'm slowly trying to standardize formatting of these tables
> in IIO and I randomly decided to go with 
> 	{ }
> Please use that for terminating entries in this new driver.

Done on all instances.

I will wait further reviews before resubmitting the patch
If useful for other reviewers, my current head is at
https://github.com/analogdevicesinc/linux/tree/staging/ad4052

Jorge

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

[Jorge Marques]

Applied suggestions, thanks!

Jorge

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

[David Lechner]

On 3/9/25 3:49 PM, Jorge Marques wrote:
>>> +.. 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.
>>> +   * - ``sample_rate``
>>> +     - Device internal sample rate used in the burst averaging mode.
>>> +   * - ``sample_rate_available``
>>> +     - List of available sample rates.
>>
>> Why not using the standard sampling_frequency[_available] attributes?
> Because sampling_frequency is the sampling frequency for the pwm trigger
> during buffer readings.
> sample_rate is the internal device clock used during monitor and burst
> averaging modes.

I haven't done a chips with a monitor mode yet where we aren't reading
the samples, so hopefully Jonathan will chime in here on the usual way
to handle that.

For the burst averaging mode, I understand the need for a separate attribute
now. I would suggest to call this the conversion_frequency rather than
sampling_rate since IIO already defines "sampling" to be the data read
from the chip to Linux even if it is an averaged value, it still counts
as one sample.

> 
>>> +
>>> +Threshold events
>>> +================
>>> +
>>> +The ADC supports a monitoring mode to raise threshold events.
>>> +The driver supports a single interrupt for both rising and falling
>>> +readings.
>>> +
>>> +During monitor mode, the device is busy since other transactions
>>> +require to put the device in configuration mode first.
>>
>> This isn't so clear to me. Is this saying that events do not work
>> while doing a buffered read? Do you need to do need to read the
>> in_voltage0_raw input to trigger an event?
>>
> No, the device monitor mode and trigger mode autonomously samples using the
> internal clock set with the sample rate property.
> I rephrased that to:
> 
>  The feature is enabled/disabled by setting ``thresh_either_en``.
>  During monitor mode, the device continuously operate in autonomous mode until
>  put back in configuration mode, due to this, the device returns busy until the
>  feature is disabled.

This is better, thanks.

> 
> The reasoning is that during configuration mode no ADC
> conversion is done, including if the previous mode was autonomous.
> If instead of return busy the driver hided this and resumed monitor mode
> after the access, a hidden (to the user) monitoring down-time would and
> thresholds crossings could be lost, undermining the feature.
> 
>>> +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
>>
>> This diagram show a PWM connected to the CNV pin on the ADC, but I
>> didn't see a pwms property in the DT bindings to describe this.
>>
> It is not clear to me where the pwm property should be in the DT
> bindings, since the PWM node now belongs to the offload-capable SPI controller.

If the PWM output is connected to the CNV pin of the ADC, then the PWM
consumer property belongs in the ADC devicetree node.

If the PWM output is connected directly to the SPI offload trigger input
then then it should use the trigger-sources bindings in the SPI controller.

From the diagram in the link you gave, it seems clear to me that the
PWM is connected to the CNV pin on the ADC and GP1 is connected to the
SPI offload trigger input on the SPI controller.

So the ADC node gets pwms and #trigger-source-cells properties and the
SPI controller gets a trigger-sources property that is a phandle to the
ADC node.

> 
>> I didn't have time to read the full datasheet or look at the driver
>> code yet, but can do that next week.
> Ok, thank you for the review
> 
> Jorge

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

[Jonathan Cameron]

On Sun, 9 Mar 2025 20:43:55 +0100
Jorge Marques <gastmaier@gmail.com> wrote:

> > > +  compatible:
> > > +    enum:
> > > +      - adi,ad4050
> > > +      - adi,ad4052
> > > +      - adi,ad4056
> > > +      - adi,ad4058  
> > 
> > Can you mention in your commit message what differs between these
> > devices that makes picking one as the "base"/fallback compatible
> > unsuitable please?  
> Sure, to be added:
> 
>  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 two required interrupts (gp0, gp1) and one optional
>  gpio (cnv).

Explain why the interrupts are required.  That is unusual.

Note the driver can be stricter than the binding, so it may make sense
to require them in the driver, but leave it flexible in the binding.
If someone has a board without them wired, then they can look at adding
polling or timing logic to avoid the need for the interrupt lines or
at reducing functionality of the driver.

> 
> > > +
> > > +  vdd-supply: true
> > > +  vdd_1_8-supply: true  
> > 
> > You're allowed to use . in property names, and the _s should be -s.
> > That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in
> > that case?  
> I overlooked the supplies, the correct are vdd, vio as mandatory,
> and vref is optional.
> 
> Jorge

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

[Jonathan Cameron]

On Sun, 9 Mar 2025 21:49:24 +0100
Jorge Marques <gastmaier@gmail.com> wrote:

> > > +.. 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.
> > > +   * - ``sample_rate``
> > > +     - Device internal sample rate used in the burst averaging mode.
> > > +   * - ``sample_rate_available``
> > > +     - List of available sample rates.  
> > 
> > Why not using the standard sampling_frequency[_available] attributes?  
> Because sampling_frequency is the sampling frequency for the pwm trigger
> during buffer readings.
> sample_rate is the internal device clock used during monitor and burst
> averaging modes.

For an ABI that is very vague and the two use cases seem to be logically
quite different.

Seems that for each trigger we have an oversampling ratio controlled number
of samples at this rate. It is unusual to be able to control oversampling
rate separately from the trigger clock, hence the lack of ABI.  If
we add something new for this it should something relating to oversampling.
oversampling_frequency perhaps.

For monitor mode, it is tied to the sampling frequency for most devices.
But there are exceptions.  E.g. the max1363. Trick is to make it an event
ABI property and hence under events/ rather than in the root directory.

In this case you'll have to store two values and write the appropriate
one into the register to suit a given operating mode.


> 
> > > +
> > > +Threshold events
> > > +================
> > > +
> > > +The ADC supports a monitoring mode to raise threshold events.
> > > +The driver supports a single interrupt for both rising and falling
> > > +readings.
> > > +
> > > +During monitor mode, the device is busy since other transactions
> > > +require to put the device in configuration mode first.  
> > 
> > This isn't so clear to me. Is this saying that events do not work
> > while doing a buffered read? Do you need to do need to read the
> > in_voltage0_raw input to trigger an event?
> >   
> No, the device monitor mode and trigger mode autonomously samples using the
> internal clock set with the sample rate property.
> I rephrased that to:
> 
>  The feature is enabled/disabled by setting ``thresh_either_en``.
>  During monitor mode, the device continuously operate in autonomous mode until
>  put back in configuration mode, due to this, the device returns busy until the
>  feature is disabled.
> 
> The reasoning is that during configuration mode no ADC
> conversion is done, including if the previous mode was autonomous.
> If instead of return busy the driver hided this and resumed monitor mode
> after the access, a hidden (to the user) monitoring down-time would and
> thresholds crossings could be lost, undermining the feature.

hmm. This is a trade off between usability and precise matching of expectations.
From your description does monitor mode only trigger if the threshold is
crossed or is it a level comparison?  If it's level I'd consider the
option of brief disabling.  Unlikely to be a problem interrupting things
in vast majority of usecases. Documentation can then express this issue.

> 
> > > +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  
> > 
> > This diagram show a PWM connected to the CNV pin on the ADC, but I
> > didn't see a pwms property in the DT bindings to describe this.
> >   
> It is not clear to me where the pwm property should be in the DT
> bindings, since the PWM node now belongs to the offload-capable SPI controller.
> 
> > I didn't have time to read the full datasheet or look at the driver
> > code yet, but can do that next week.  
> Ok, thank you for the review
> 
> Jorge

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

[Jonathan Cameron]

On Mon, 10 Mar 2025 09:31:45 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 3/9/25 3:49 PM, Jorge Marques wrote:
> >>> +.. 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.
> >>> +   * - ``sample_rate``
> >>> +     - Device internal sample rate used in the burst averaging mode.
> >>> +   * - ``sample_rate_available``
> >>> +     - List of available sample rates.  
> >>
> >> Why not using the standard sampling_frequency[_available] attributes?  
> > Because sampling_frequency is the sampling frequency for the pwm trigger
> > during buffer readings.
> > sample_rate is the internal device clock used during monitor and burst
> > averaging modes.  
> 
> I haven't done a chips with a monitor mode yet where we aren't reading
> the samples, so hopefully Jonathan will chime in here on the usual way
> to handle that.
> 
> For the burst averaging mode, I understand the need for a separate attribute
> now. I would suggest to call this the conversion_frequency rather than
> sampling_rate since IIO already defines "sampling" to be the data read
> from the chip to Linux even if it is an averaged value, it still counts
> as one sample.

I should have read on.  I'd like this more closely associated with oversampling.
As per other reply we use sampling_frequency in the events directory for
the monitoring frequency case.  One of our very first drivers did this
(max1363) so it's been in the ABI a long time!

Jonathan

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

[Jorge Marques]

On Mon, Mar 10, 2025 at 07:56:29PM +0000, Jonathan Cameron wrote:
> On Mon, 10 Mar 2025 09:31:45 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 3/9/25 3:49 PM, Jorge Marques wrote:
> > >>> +   * - ``sample_rate``
> > >>> +     - Device internal sample rate used in the burst averaging mode.
> > >>> +   * - ``sample_rate_available``
> > >>> +     - List of available sample rates.  
> > >>
> > >> Why not using the standard sampling_frequency[_available] attributes?  
> > > Because sampling_frequency is the sampling frequency for the pwm trigger
> > > during buffer readings.
> > > sample_rate is the internal device clock used during monitor and burst
> > > averaging modes.  
> > 
> > I haven't done a chips with a monitor mode yet where we aren't reading
> > the samples, so hopefully Jonathan will chime in here on the usual way
> > to handle that.
> > 
> > For the burst averaging mode, I understand the need for a separate attribute
> > now. I would suggest to call this the conversion_frequency rather than
> > sampling_rate since IIO already defines "sampling" to be the data read
> > from the chip to Linux even if it is an averaged value, it still counts
> > as one sample.
> 
> I should have read on.  I'd like this more closely associated with oversampling.
> As per other reply we use sampling_frequency in the events directory for
> the monitoring frequency case.  One of our very first drivers did this
> (max1363) so it's been in the ABI a long time!
> 

I get the idea but maybe the datasheet sample rate as conversion_frequency
and stored as a channel attribute (iio_chan_spec.ext_info) is clear enough.

The datasheet sample rate affects both the burst averaging mode (oversampling) and
monitor mode (threshold events).

The max1363 stores as an event attribute (iio_info.event_attr) and requires iio/sysfs.h include.
A last option is to store as a general purpose device attribute (iio_info.attrs).
As a channel attribute, the driver logic is slightly simpler by using the macros.

Jorge

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

[Jorge Marques]

On Mon, Mar 10, 2025 at 07:54:16PM +0000, Jonathan Cameron wrote:
> On Sun, 9 Mar 2025 21:49:24 +0100
> Jorge Marques <gastmaier@gmail.com> wrote:
> 
> > > > +.. 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.
> > > > +   * - ``sample_rate``
> > > > +     - Device internal sample rate used in the burst averaging mode.
> > > > +   * - ``sample_rate_available``
> > > > +     - List of available sample rates.  
> > > 
> > > Why not using the standard sampling_frequency[_available] attributes?  
> > Because sampling_frequency is the sampling frequency for the pwm trigger
> > during buffer readings.
> > sample_rate is the internal device clock used during monitor and burst
> > averaging modes.
> 
> For an ABI that is very vague and the two use cases seem to be logically
> quite different.
> 
> Seems that for each trigger we have an oversampling ratio controlled number
> of samples at this rate. It is unusual to be able to control oversampling
> rate separately from the trigger clock, hence the lack of ABI.  If
> we add something new for this it should something relating to oversampling.
> oversampling_frequency perhaps.
> 
> For monitor mode, it is tied to the sampling frequency for most devices.
> But there are exceptions.  E.g. the max1363. Trick is to make it an event
> ABI property and hence under events/ rather than in the root directory.
> 
> In this case you'll have to store two values and write the appropriate
> one into the register to suit a given operating mode.
> 

If doing buffer captures with oversampling enabled, both sampling
frequencies have an impact:

e.g.,
oversampling: 4
sample_rate: 2MHz
PWM sampling frequency: 500KHz

PWM trigger out (CNV)   |       |       |       |       |
ADC conversion          ++++    ++++    ++++    ++++    ++++
ADC data ready  (GP)       *       *       *       *       *

For monitor mode, it will constantly be doing conversion to check for
threshold crossings, at the defined sample_rate.

I like the idea of having the device's sample_rate as
conversion_frequency.

> > 
> > > > +
> > > > +Threshold events
> > > > +================
> > > > +
> > > > +The ADC supports a monitoring mode to raise threshold events.
> > > > +The driver supports a single interrupt for both rising and falling
> > > > +readings.
> > > > +
> > > > +During monitor mode, the device is busy since other transactions
> > > > +require to put the device in configuration mode first.  
> > > 
> > > This isn't so clear to me. Is this saying that events do not work
> > > while doing a buffered read? Do you need to do need to read the
> > > in_voltage0_raw input to trigger an event?
> > >   
> > No, the device monitor mode and trigger mode autonomously samples using the
> > internal clock set with the sample rate property.
> > I rephrased that to:
> > 
> >  The feature is enabled/disabled by setting ``thresh_either_en``.
> >  During monitor mode, the device continuously operate in autonomous mode until
> >  put back in configuration mode, due to this, the device returns busy until the
> >  feature is disabled.
> > 
> > The reasoning is that during configuration mode no ADC
> > conversion is done, including if the previous mode was autonomous.
> > If instead of return busy the driver hided this and resumed monitor mode
> > after the access, a hidden (to the user) monitoring down-time would and
> > thresholds crossings could be lost, undermining the feature.
> 
> hmm. This is a trade off between usability and precise matching of expectations.
> From your description does monitor mode only trigger if the threshold is
> crossed or is it a level comparison?  If it's level I'd consider the
> option of brief disabling.  Unlikely to be a problem interrupting things
> in vast majority of usecases. Documentation can then express this issue.
> 

The gpio asserts when the threshold is crossed, and desserts when it is
back in bounds.
The interrupt controller should be configured to detecting rising
edges, to not call multiple irq_handlers for the same crossing.
If the interrupt controller is set to trigger on level,
multiple irq handler calls will occur before being able to access
the device register to disable the GPIO.

Jorge

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

[David Lechner]

On 3/14/25 1:13 PM, Jorge Marques wrote:
> On Mon, Mar 10, 2025 at 07:54:16PM +0000, Jonathan Cameron wrote:
>> On Sun, 9 Mar 2025 21:49:24 +0100
>> Jorge Marques <gastmaier@gmail.com> wrote:
>>
>>>>> +.. 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.
>>>>> +   * - ``sample_rate``
>>>>> +     - Device internal sample rate used in the burst averaging mode.
>>>>> +   * - ``sample_rate_available``
>>>>> +     - List of available sample rates.  
>>>>
>>>> Why not using the standard sampling_frequency[_available] attributes?  
>>> Because sampling_frequency is the sampling frequency for the pwm trigger
>>> during buffer readings.
>>> sample_rate is the internal device clock used during monitor and burst
>>> averaging modes.
>>
>> For an ABI that is very vague and the two use cases seem to be logically
>> quite different.
>>
>> Seems that for each trigger we have an oversampling ratio controlled number
>> of samples at this rate. It is unusual to be able to control oversampling
>> rate separately from the trigger clock, hence the lack of ABI.  If
>> we add something new for this it should something relating to oversampling.
>> oversampling_frequency perhaps.
>>
>> For monitor mode, it is tied to the sampling frequency for most devices.
>> But there are exceptions.  E.g. the max1363. Trick is to make it an event
>> ABI property and hence under events/ rather than in the root directory.
>>
>> In this case you'll have to store two values and write the appropriate
>> one into the register to suit a given operating mode.
>>
> 
> If doing buffer captures with oversampling enabled, both sampling
> frequencies have an impact:
> 
> e.g.,
> oversampling: 4
> sample_rate: 2MHz
> PWM sampling frequency: 500KHz
> 
> PWM trigger out (CNV)   |       |       |       |       |
> ADC conversion          ++++    ++++    ++++    ++++    ++++
> ADC data ready  (GP)       *       *       *       *       *
> 
> For monitor mode, it will constantly be doing conversion to check for
> threshold crossings, at the defined sample_rate.
> 
> I like the idea of having the device's sample_rate as
> conversion_frequency.

In addition to what makes sense for this chip, we should also consider what
makes sense other chips with similar features. For example, I am working on
ad7606c which has control for the oversampling burst frequency (frequency of
"+" in the diagram above). So it would make sense to have a standard attribute
that would work for both chips.

On ad4052, just because we have a single register that controls two different
functions doesn't mean we have to be limited to a single attribute that controls
that register.

So I would create the events/sampling_frequency{,_available} attributes like
Jonathan suggested for controlling the sampling frequency in monitor mode and
introduce new oversampling_burst_frequency{,_available} attributes for
controlling the conversion frequency when oversampling. When an attribute is
written, we can cache the requested value in the state struct instead of
writing it directly to the register on the ADC if we want the attributes to be
independent. Then only write the register when we enable monitor mode or when
we start reading samples with oversampling enabled.

Sure, it is more work to implement it in the driver this way, but that shouldn't
be an an excuse to do things in a way that isn't compatible with other ADCs.

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

[Jonathan Cameron]

On Fri, 14 Mar 2025 18:34:46 +0100
Jorge Marques <gastmaier@gmail.com> wrote:

> On Mon, Mar 10, 2025 at 07:56:29PM +0000, Jonathan Cameron wrote:
> > On Mon, 10 Mar 2025 09:31:45 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> > > On 3/9/25 3:49 PM, Jorge Marques wrote:  
> > > >>> +   * - ``sample_rate``
> > > >>> +     - Device internal sample rate used in the burst averaging mode.
> > > >>> +   * - ``sample_rate_available``
> > > >>> +     - List of available sample rates.    
> > > >>
> > > >> Why not using the standard sampling_frequency[_available] attributes?    
> > > > Because sampling_frequency is the sampling frequency for the pwm trigger
> > > > during buffer readings.
> > > > sample_rate is the internal device clock used during monitor and burst
> > > > averaging modes.    
> > > 
> > > I haven't done a chips with a monitor mode yet where we aren't reading
> > > the samples, so hopefully Jonathan will chime in here on the usual way
> > > to handle that.
> > > 
> > > For the burst averaging mode, I understand the need for a separate attribute
> > > now. I would suggest to call this the conversion_frequency rather than
> > > sampling_rate since IIO already defines "sampling" to be the data read
> > > from the chip to Linux even if it is an averaged value, it still counts
> > > as one sample.  
> > 
> > I should have read on.  I'd like this more closely associated with oversampling.
> > As per other reply we use sampling_frequency in the events directory for
> > the monitoring frequency case.  One of our very first drivers did this
> > (max1363) so it's been in the ABI a long time!
> >   
> 
> I get the idea but maybe the datasheet sample rate as conversion_frequency
> and stored as a channel attribute (iio_chan_spec.ext_info) is clear enough.

It's not standard ABI. So no standard userspace code will be aware of it.
we can add to standard ABI, but only if we can't support a feature with
what is already defined. Event then we need to keep it consistent with
existing ABI.

Note that the first step on any ABI is to write documentation for it in 
Documentation/ABI/testing/sysfs-bus-iio-*
That can help people understand what is being proposed and allows discussion
of how to generalize any new ABI to be useful across many drivers.
> 
> The datasheet sample rate affects both the burst averaging mode (oversampling) and
> monitor mode (threshold events).

True, but are both occurring at the same time?  My reading of the situation was
that they weren't but I could be wrong.  If they aren't then just rewrite the register
to a cached value when you change mode.

> 
> The max1363 stores as an event attribute (iio_info.event_attr) and requires iio/sysfs.h include.
> A last option is to store as a general purpose device attribute (iio_info.attrs).
> As a channel attribute, the driver logic is slightly simpler by using the macros.

Agreed that non standard event attributes are a bit trickier to deal with.  They
have never been common enough for us to fix that.  However the ABI exists
and is documented, so that's almost certainly the way to go.

Jonathan
> 
> Jorge

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

[Jorge Marques]

On Fri, Mar 14, 2025 at 01:56:32PM -0500, David Lechner wrote:
> On 3/14/25 1:13 PM, Jorge Marques wrote:
> > On Mon, Mar 10, 2025 at 07:54:16PM +0000, Jonathan Cameron wrote:
> >> On Sun, 9 Mar 2025 21:49:24 +0100
> >> Jorge Marques <gastmaier@gmail.com> wrote:
> >>
> >>>>> +.. 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.
> >>>>> +   * - ``sample_rate``
> >>>>> +     - Device internal sample rate used in the burst averaging mode.
> >>>>> +   * - ``sample_rate_available``
> >>>>> +     - List of available sample rates.  
> >>>>
> >>>> Why not using the standard sampling_frequency[_available] attributes?  
> >>> Because sampling_frequency is the sampling frequency for the pwm trigger
> >>> during buffer readings.
> >>> sample_rate is the internal device clock used during monitor and burst
> >>> averaging modes.
> >>
> >> For an ABI that is very vague and the two use cases seem to be logically
> >> quite different.
> >>
> >> Seems that for each trigger we have an oversampling ratio controlled number
> >> of samples at this rate. It is unusual to be able to control oversampling
> >> rate separately from the trigger clock, hence the lack of ABI.  If
> >> we add something new for this it should something relating to oversampling.
> >> oversampling_frequency perhaps.
> >>
> >> For monitor mode, it is tied to the sampling frequency for most devices.
> >> But there are exceptions.  E.g. the max1363. Trick is to make it an event
> >> ABI property and hence under events/ rather than in the root directory.
> >>
> >> In this case you'll have to store two values and write the appropriate
> >> one into the register to suit a given operating mode.
> >>
> > 
> > If doing buffer captures with oversampling enabled, both sampling
> > frequencies have an impact:
> > 
> > e.g.,
> > oversampling: 4
> > sample_rate: 2MHz
> > PWM sampling frequency: 500KHz
> > 
> > PWM trigger out (CNV)   |       |       |       |       |
> > ADC conversion          ++++    ++++    ++++    ++++    ++++
> > ADC data ready  (GP)       *       *       *       *       *
> > 
> > For monitor mode, it will constantly be doing conversion to check for
> > threshold crossings, at the defined sample_rate.
> > 
> > I like the idea of having the device's sample_rate as
> > conversion_frequency.
> 
> In addition to what makes sense for this chip, we should also consider what
> makes sense other chips with similar features. For example, I am working on
> ad7606c which has control for the oversampling burst frequency (frequency of
> "+" in the diagram above). So it would make sense to have a standard attribute
> that would work for both chips.
> 
> On ad4052, just because we have a single register that controls two different
> functions doesn't mean we have to be limited to a single attribute that controls
> that register.
> 

I looked into the ad7606c driver and summarized below to organize our
ideas:

  PADDING OVERSAMPLING
  --------------------
  Delay between conversions:

  OS_CLOCK(Hz) = 1 / (1+OS_PAD/16)
  
  OS_CLOCK: internal clock, reg

  0x08 OVERSAMPLING
    OS_PAD[7:4]: Extends the internal oversampling period allowing
                 evenly spaced sampling between CONVST rising edges,
                 from 0 to 15
    OS_RATIO[3:0]: from off(1) to 256
    
  Therefore, OS_CLOCK range is therefore 1Hz .. 0.516Hz
  (1) from previous discussion, iio oversampling 1 equals off.

  EXTERNAL OVERSAMPLING CLOCK
  ---------------------------
  Use CONVST as the external trigger for
  each conversion

On AD4052 family:

  BURST AVERAGING MODE
  --------------------

  Delay between conversions

  Total latency:
  (AVG_WIN_LEN-1)/FS_BURST_AUTO + t_CONV

  0x23 AVG_CONFIG
    AVG_WIN_LEN[3:0]: Averaging ratio/number of samples
  0x27 TIMER_CONFIG
    FS_BURST_AUTO[7:4]: from 111Hz to 2 MHz, internal sample rate

  AVERAGING MODE
  --------------
  Use CONVST as the external trigger for
  each conversion

So, we can say that
PADDING OVERSAMPLING == BURST AVERAGING MODE, and
EXTERNAL OVERSAMPLING CLOCK == AVERAGING MODE

> So I would create the events/sampling_frequency{,_available} attributes like
> Jonathan suggested for controlling the sampling frequency in monitor mode and
> introduce new oversampling_burst_frequency{,_available} attributes for
> controlling the conversion frequency when oversampling. When an attribute is
> written, we can cache the requested value in the state struct instead of
> writing it directly to the register on the ADC if we want the attributes to be
> independent. Then only write the register when we enable monitor mode or when
> we start reading samples with oversampling enabled.
> 
> Sure, it is more work to implement it in the driver this way, but that shouldn't
> be an an excuse to do things in a way that isn't compatible with other ADCs.
> 

I am alright with that and will follow the suggestion of having the
values independent through cache.

So, two new attributes will be implemented:

* oversampling_[burst_]frequency{,_available} (new ABI required)
* events/sampling_frequency{,_available}

And I will drop conversion_frequency (early sample_rate) attribute.