[PATCH v2 0/9] Add support for AD4062 device family

[PATCH v2 0/9] Add support for AD4062 device family

[Jorge Marques]

The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
register (SAR) analog-to-digital converter (ADC).

The device uses a 2-wire I3C interface. The device simplifies acquisition
by providing 4-bytes in the register map, signal-extending the sample
reading accordingly.

The device has autonomous monitoring capabilities, that are exposed as
IIO events. Since register access requires leaving the monitoring state
and returning, any device access exits monitoring mode, disabling the
IIO event.

The device contains two optional outputs:

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

The devices utilizes PM to enter the low power mode.

The devices datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/ad4060.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/ad4062.pdf

The monitoring capabilities, I3C protocol, and multiple GPIOs were the
decision factor to have a standalone driver for this device family. The
device is expected to work with any I3C Bus. I tested the device with
with off-the-shelf I3C controllers STM32H7 (baremetal only) and the
open-source ADI I3C Controller (with Linux driver):
https://analogdevicesinc.github.io/hdl/library/i3c_controller/index.html
ADI I3C Controller lore:
https://lore.kernel.org/linux-i3c/175788312841.382502.16653824321627644225.b4-ty@bootlin.com/

The series is divided in 3 blocks, adding:
- The base driver.
- An software IIO trigger: captures samples continuously.
- IIO events support: exposes the device's threshold monitoring
  capability.

The device internal clock register is exposed twice, as
sampling_frequency and events/sampling_frequency, storing in distinct 
state variables, since the usage (burst averaging mode and monitor mode)
cannot be executed at the same time.

Non-implemented features:

- Averaging mode: Similar to burst averaging mode used in the
  oversampling, but requiring a sequence of CNV triggers for each
  conversion.
- Trigger mode: Similar to monitor mode used in the monitoring mode, but
  exits to configuration mode on event.

This device is almost identical to AD4052 family, but I decided to
submit the AD4062 before re-submitting AD4052 to better contextualize
the focus of the device family (high latency, medium-speed protocol,
low-power autonomous monitoring rather than high-throughput
acquisition).

Depending on the resolution of this driver, the AD4052 family may be
added to it, by splitting into ad4062_i3c.c, ad4062_spi.c,
ad4062_core.c, or as a standalone driver ad4052.c.

Depends on:
https://lore.kernel.org/linux-i3c/aRYLc%2F+KAD13g7T7@lizhi-Precision-Tower-5810/T/#t
(for devm ibi clean-up)

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
Changes in v2:
- dt-bindings:
  * add a short description of all mode that can be configured to during
    runtime.
  * add gpio-controller, to expose GPs not listed in interrupt-names as
    a GPO.
- sampling_frequency is the duration of a single sample (convert-start
  high edge until RDY falling edge) ((n_avg - 1) / fosc + tconv)
- Remove .grade from chip_info, since the supported devices have a
  single speed grade.
- Update state buffer to use dma-aligned union of __be32, __be16, u8 bytes[4].
- Use standard IIO_CHAN_INFO_SAMP_FREQ and _AVAIL
- Add defines to magic numbers.
- Ensure commits only contain code related to the particular commit.
- Use new ACQUIRE pm macros.
- Drop lock for debugfs, let user mess the state thorugh the debug
  interface.
- Restructure vio, vdd, ref voltages, only read if needed.
- Have error handling on top.
- Drop unnecessary check_ids error message.
- Use devm for IBI remove (requires patch on i3c subystem).
- Use heap buffers for all i3c_priv_xfer.
- Use CONV_READ if GP1 is routed (less overhead), use CONV_TRIGGER for
  IBI fallback.
- Drop usage pm_runtime_mark_last_busy, since it is now internal to pm_runtime_put_autosuspend
- Don't allow access if monitor mode is enabled, return -EBUSY.
- Implement gpio-controller to expose GPs not listed in interrupt-names
  as a GPO.
- Value in mv as ``raw * _scale`` (embed caliscale).
- Link to v1: https://lore.kernel.org/r/20251013-staging-ad4062-v1-0-0f8ce7fef50c@analog.com

---
Jorge Marques (9):
      dt-bindings: iio: adc: Add adi,ad4062
      docs: iio: New docs for ad4062 driver
      iio: adc: Add support for ad4062
      docs: iio: ad4062: Add IIO Trigger support
      iio: adc: ad4062: Add IIO Trigger support
      docs: iio: ad4062: Add IIO Events support
      iio: adc: ad4062: Add IIO Events support
      docs: iio: ad4062: Add GPIO Controller support
      iio: adc: ad4062: Add GPIO Controller support

 .../devicetree/bindings/iio/adc/adi,ad4062.yaml    |  123 ++
 Documentation/iio/ad4062.rst                       |  154 ++
 Documentation/iio/index.rst                        |    1 +
 MAINTAINERS                                        |    8 +
 drivers/iio/adc/Kconfig                            |   13 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/ad4062.c                           | 1531 ++++++++++++++++++++
 7 files changed, 1831 insertions(+)
---
base-commit: f9e05791642810a0cf6237d39fafd6fec5e0b4bb
change-id: 20251011-staging-ad4062-20d897d33ab6
prerequisite-change-id: 20251112-ibi-unsafe-48f343e178b8:v1
prerequisite-patch-id: 5f04cbbca0fcc3657c7a4d254656b03e289ad222

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

[PATCH v2 1/9] dt-bindings: iio: adc: Add adi,ad4062

[Jorge Marques]

Add dt-bindings for AD4062 family, devices AD4060/AD4062, low-power with
monitor capabilities SAR ADCs. Each variant of the family differs in
resolution. The device contains two outputs (gp0, gp1). The outputs can
be configured for range of options, such as threshold and data ready.
The device uses a 2-wire I3C interface.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad4062.yaml    | 123 +++++++++++++++++++++
 MAINTAINERS                                        |   6 +
 2 files changed, 129 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
new file mode 100644
index 0000000000000..a25af66dd64d2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2024 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4062.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4062 ADC family device driver
+
+maintainers:
+  - Jorge Marques <jorge.marques@analog.com>
+
+description: |
+  Analog Devices AD4062 Single Channel Precision SAR ADC family
+
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4060.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4062.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4060
+      - adi,ad4062
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description:
+      The interrupt pins are digital outputs that can be configured at runtime
+      as multiple interrupt signals. Each can be configured as GP_INTR, RDY,
+      DEV_EN, logic low, logic high and DEV_RDY (GP1 only). RDY is the
+      active-low data ready signal, indicates when new ADC data are ready to
+      read. DEV_EN synchronizes the enable and power-down states of signal
+      chain devices with the ADC sampling instant. DEV_RDY is an active-high
+      signal that indicates when the device is ready to accept serial interface
+      communications. In GP_INTR mode, the interrupt outputs one of the
+      threshold detection interrupt signals (MIN_INTR, MAX_INTR or either).
+    minItems: 1
+    items:
+      - description:
+          gp0, interrupt line for GP0 pin, cannot be configured as DEV_RDY.
+      - description:
+          gp1, interrupt line for GP1 pin, can be configured to any setting.
+
+  interrupt-names:
+    items:
+      - const: gp0
+      - const: gp1
+
+  gpio-controller:
+    description:
+      Marks the device node as a GPIO controller. GPs not listed in
+      interrupt-names are exposed as a GPO.
+
+  '#gpio-cells':
+    const: 2
+    description:
+      The first cell is the GPIO number and the second cell specifies
+      GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
+
+  vdd-supply:
+    description: Analog power supply.
+
+  vio-supply:
+    description: Digital interface logic power supply.
+
+  ref-supply:
+    description:
+      Reference voltage to set the ADC full-scale range. If not present,
+      vdd-supply is used as the reference voltage.
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vio-supply
+
+allOf:
+  - $ref: /schemas/i3c/i3c.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i3c {
+        #address-cells = <3>;
+        #size-cells = <0>;
+
+        adc@0,2ee007c0000 {
+            reg = <0x0 0x2ee 0x7c0000>;
+            vdd-supply = <&vdd>;
+            vio-supply = <&vio>;
+            ref-supply = <&ref>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
+                         <0 1 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-names = "gp0", "gp1";
+        };
+    };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i3c {
+        #address-cells = <3>;
+        #size-cells = <0>;
+
+        adc@0,2ee007c0000 {
+            reg = <0x0 0x2ee 0x7c0000>;
+            vdd-supply = <&vdd>;
+            vio-supply = <&vio>;
+            ref-supply = <&ref>;
+
+            gpio-controller;
+            #gpio-cells = <2>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 31d98efb1ad15..e22ba5ec8c849 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1432,6 +1432,12 @@ F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
 F:	Documentation/iio/ad4030.rst
 F:	drivers/iio/adc/ad4030.c
 
+ANALOG DEVICES INC AD4062 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,ad4062.yaml
+
 ANALOG DEVICES INC AD4080 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org

-- 
2.51.1

[PATCH v2 2/9] docs: iio: New docs for ad4062 driver

[Jorge Marques]

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

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

diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
new file mode 100644
index 0000000000000..e6bcca2bef24b
--- /dev/null
+++ b/Documentation/iio/ad4062.rst
@@ -0,0 +1,94 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=============
+AD4062 driver
+=============
+
+ADC driver for Analog Devices Inc. AD4060/AD4062 devices. The module name is
+``ad4062``.
+
+Supported devices
+=================
+
+The following chips are supported by this driver:
+
+* `AD4060 <https://www.analog.com/AD4060>`_
+* `AD4062 <https://www.analog.com/AD4062>`_
+
+Wiring modes
+============
+
+The ADC is interfaced through an I3C bus, and contains two programmable GPIOs.
+
+The ADC convert-start happens on the SDA rising edge of the I3C stop (P) bit
+at the end of the read command.
+
+The two programmable GPIOS are optional and have a role assigned if present in
+the devicetree ``interrupt-names`` property:
+
+- GP1: Is assigned the role of Data Ready signal.
+
+Device attributes
+=================
+
+The ADC contains only one channel with following attributes:
+
+.. list-table:: Channel attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``in_voltage_calibscale``
+     - Sets the gain scaling factor that the hardware applies to the sample,
+       to compensate for system gain error.
+   * - ``in_voltage_oversampling_ratio``
+     - Sets device's burst averaging mode to over sample using the
+       internal sample rate. Value 1 disable the burst averaging mode.
+   * - ``in_voltage_oversampling_ratio_available``
+     - List of available oversampling values.
+   * - ``in_voltage_raw``
+     - Returns the raw ADC voltage value.
+   * - ``in_voltage_scale``
+     - Returns the channel scale in reference to the reference voltage
+       ``ref-supply`` or ``vdd-supply`` if the former not present.
+
+Also contain the following device attributes:
+
+.. list-table:: Device attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``sampling_frequency``
+     - Sets the duration of a single scan, used in the burst averaging mode.
+       The duration is described by ``(n_avg - 1) / fosc + tconv``, where
+       ``n_avg`` is the oversampling ratio, ``fosc`` is the internal sample
+       rate and ``tconv`` is the ADC conversion time.
+   * - ``sampling_frequency_available``
+     - Lists the available sampling frequencies, computed on the current
+       oversampling ratio. If the ratio is 1, the frequency is ``1/tconv``.
+
+Interrupts
+==========
+
+The interrupts are mapped through the ``interrupt-names`` and ``interrupts``
+properties.
+
+The ``interrupt-names`` ``gp1`` entry sets the role of Data Ready signal.
+If it is not present, the driver fallback to enabling the same role as an
+I3C IBI.
+
+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.
+
+Unimplemented features
+======================
+
+- Monitor mode
+- Trigger mode
+- Averaging mode
+- General purpose output
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 315ae37d6fd4b..ba3e609c6a13c 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -22,6 +22,7 @@ Industrial I/O Kernel Drivers
    ad3552r
    ad4000
    ad4030
+   ad4062
    ad4695
    ad7191
    ad7380
diff --git a/MAINTAINERS b/MAINTAINERS
index e22ba5ec8c849..8fc28b789d639 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1437,6 +1437,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,ad4062.yaml
+F:	Documentation/iio/ad4062.rst
 
 ANALOG DEVICES INC AD4080 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>

-- 
2.51.1

[PATCH v2 3/9] iio: adc: Add support for ad4062

[Jorge Marques]

The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
register (SAR) analog-to-digital converter (ADC) with low-power and
threshold monitoring modes.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 8fc28b789d639..003f51cfb0d07 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1438,6 +1438,7 @@ S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
 F:	Documentation/iio/ad4062.rst
+F:	drivers/iio/adc/ad4062.c
 
 ANALOG DEVICES INC AD4080 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58da8255525e4..e506dbe83f488 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -70,6 +70,17 @@ config AD4030
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4030.
 
+config AD4062
+	tristate "Analog Devices AD4062 Driver"
+	depends on I3C
+	select REGMAP_I3C
+	help
+	  Say yes here to build support for Analog Devices AD4062 I3C analog
+	  to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4062.
+
 config AD4080
 	tristate "Analog Devices AD4080 high speed ADC"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7cc8f9a12f763..a897252eeed40 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4030) += ad4030.o
+obj-$(CONFIG_AD4062) += ad4062.o
 obj-$(CONFIG_AD4080) += ad4080.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD4170_4) += ad4170-4.o
diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
new file mode 100644
index 0000000000000..6866393ffef8d
--- /dev/null
+++ b/drivers/iio/adc/ad4062.c
@@ -0,0 +1,881 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AD4062 I3C ADC driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/units.h>
+#include <linux/unaligned.h>
+#include <linux/util_macros.h>
+
+#define AD4062_REG_INTERFACE_CONFIG_A			0x00
+#define AD4062_REG_DEVICE_CONFIG			0x02
+#define     AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK	GENMASK(1, 0)
+#define     AD4062_REG_DEVICE_CONFIG_LOW_POWER_MODE	3
+#define AD4062_REG_PROD_ID_1				0x05
+#define AD4062_REG_DEVICE_GRADE				0x06
+#define AD4062_REG_SCRATCH_PAD				0x0A
+#define AD4062_REG_VENDOR_H				0x0D
+#define AD4062_REG_STREAM_MODE				0x0E
+#define AD4062_REG_INTERFACE_STATUS			0x11
+#define AD4062_REG_MODE_SET				0x20
+#define     AD4062_REG_MODE_SET_ENTER_ADC		BIT(0)
+#define AD4062_REG_ADC_MODES				0x21
+#define     AD4062_REG_ADC_MODES_MODE_MSK		GENMASK(1, 0)
+#define AD4062_REG_ADC_CONFIG				0x22
+#define     AD4062_REG_ADC_CONFIG_REF_EN_MSK		BIT(5)
+#define     AD4062_REG_ADC_CONFIG_SCALE_EN_MSK		BIT(4)
+#define AD4062_REG_AVG_CONFIG				0x23
+#define AD4062_REG_GP_CONF				0x24
+#define     AD4062_REG_GP_CONF_MODE_MSK_1		GENMASK(6, 4)
+#define AD4062_REG_INTR_CONF				0x25
+#define     AD4062_REG_INTR_CONF_EN_MSK_1		GENMASK(5, 4)
+#define AD4062_REG_TIMER_CONFIG				0x27
+#define     AD4062_REG_TIMER_CONFIG_FS_MASK		GENMASK(7, 4)
+#define AD4062_REG_MON_VAL				0x2F
+#define AD4062_REG_ADC_IBI_EN				0x31
+#define AD4062_REG_ADC_IBI_EN_CONV_TRIGGER		BIT(2)
+#define AD4062_REG_FUSE_CRC				0x40
+#define AD4062_REG_DEVICE_STATUS			0x41
+#define     AD4062_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
+#define AD4062_REG_IBI_STATUS				0x48
+#define AD4062_REG_CONV_READ_LSB			0x50
+#define AD4062_REG_CONV_TRIGGER				0x59
+#define AD4062_REG_CONV_AUTO				0x61
+#define AD4062_MAX_REG					AD4062_REG_CONV_AUTO
+
+#define AD4062_I3C_VENDOR	0x0177
+
+#define AD4062_SOFT_RESET	0x81
+#define AD4060_MAX_AVG		0x7
+#define AD4062_MAX_AVG		0xB
+#define AD4062_MON_VAL_MAX_GAIN		1999970
+#define AD4062_MON_VAL_MIDDLE_POINT	0x8000
+#define AD4062_GP_DRDY		0x2
+#define AD4062_INTR_EN_NEITHER	0x0
+#define AD4062_TCONV_NS		270
+
+enum ad4062_operation_mode {
+	AD4062_SAMPLE_MODE = 0x0,
+	AD4062_BURST_AVERAGING_MODE = 0x1,
+	AD4062_MONITOR_MODE = 0x3,
+};
+
+struct ad4062_chip_info {
+	const struct iio_chan_spec channels[1];
+	const char *name;
+	u16 prod_id;
+	u8 max_avg;
+};
+
+enum {
+	AD4062_SCAN_TYPE_SAMPLE,
+	AD4062_SCAN_TYPE_BURST_AVG,
+};
+
+static const struct iio_scan_type ad4062_scan_type_12_s[] = {
+	[AD4062_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+	[AD4062_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static const struct iio_scan_type ad4062_scan_type_16_s[] = {
+	[AD4062_SCAN_TYPE_SAMPLE] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+	[AD4062_SCAN_TYPE_BURST_AVG] = {
+		.sign = 's',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static const int ad4062_conversion_freqs[] = {
+	2000000, 1000000, 300000, 100000,	/*  0 -  3 */
+	33300, 10000, 3000, 500,		/*  4 -  7 */
+	333, 250, 200, 166,			/*  8 - 11 */
+	140, 124, 111,				/* 12 - 15 */
+};
+
+struct ad4062_state {
+	const struct ad4062_chip_info *chip;
+	const struct ad4062_bus_ops *ops;
+	enum ad4062_operation_mode mode;
+	struct completion completion;
+	struct iio_trigger *trigger;
+	struct iio_dev *indio_dev;
+	struct i3c_device *i3cdev;
+	struct regmap *regmap;
+	u16 sampling_frequency;
+	int vref_uv;
+	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
+	u8 oversamp_ratio;
+	union {
+		__be32 be32;
+		__be16 be16;
+		u8 bytes[4];
+	} buf __aligned(IIO_DMA_MINALIGN);
+	u8 reg_addr_conv;
+};
+
+static const struct regmap_range ad4062_regmap_rd_ranges[] = {
+	regmap_reg_range(AD4062_REG_INTERFACE_CONFIG_A, AD4062_REG_DEVICE_GRADE),
+	regmap_reg_range(AD4062_REG_SCRATCH_PAD, AD4062_REG_INTERFACE_STATUS),
+	regmap_reg_range(AD4062_REG_MODE_SET, AD4062_REG_ADC_IBI_EN),
+	regmap_reg_range(AD4062_REG_FUSE_CRC, AD4062_REG_IBI_STATUS),
+	regmap_reg_range(AD4062_REG_CONV_READ_LSB, AD4062_REG_CONV_AUTO),
+};
+
+static const struct regmap_access_table ad4062_regmap_rd_table = {
+	.yes_ranges = ad4062_regmap_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4062_regmap_rd_ranges),
+};
+
+static const struct regmap_range ad4062_regmap_wr_ranges[] = {
+	regmap_reg_range(AD4062_REG_INTERFACE_CONFIG_A, AD4062_REG_DEVICE_CONFIG),
+	regmap_reg_range(AD4062_REG_SCRATCH_PAD, AD4062_REG_SCRATCH_PAD),
+	regmap_reg_range(AD4062_REG_STREAM_MODE, AD4062_REG_INTERFACE_STATUS),
+	regmap_reg_range(AD4062_REG_MODE_SET, AD4062_REG_ADC_IBI_EN),
+	regmap_reg_range(AD4062_REG_FUSE_CRC, AD4062_REG_DEVICE_STATUS),
+};
+
+static const struct regmap_access_table ad4062_regmap_wr_table = {
+	.yes_ranges = ad4062_regmap_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad4062_regmap_wr_ranges),
+};
+
+static int ad4062_conversion_frequency_set(struct ad4062_state *st, u8 val)
+{
+	return regmap_write(st->regmap, AD4062_REG_TIMER_CONFIG,
+			    FIELD_PREP(AD4062_REG_TIMER_CONFIG_FS_MASK, val));
+}
+
+#define AD4062_CHAN(bits) {							\
+	.type = IIO_VOLTAGE,								\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |				\
+				    BIT(IIO_CHAN_INFO_SCALE) |				\
+				    BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
+				    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.indexed = 1,									\
+	.channel = 0,									\
+	.has_ext_scan_type = 1,								\
+	.ext_scan_type = ad4062_scan_type_##bits##_s,					\
+	.num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s),			\
+}
+
+static const struct ad4062_chip_info ad4060_chip_info = {
+	.name = "ad4060",
+	.channels = { AD4062_CHAN(12) },
+	.prod_id = 0x7A,
+	.max_avg = AD4060_MAX_AVG,
+};
+
+static const struct ad4062_chip_info ad4062_chip_info = {
+	.name = "ad4062",
+	.channels = { AD4062_CHAN(16) },
+	.prod_id = 0x7C,
+	.max_avg = AD4062_MAX_AVG,
+};
+
+static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
+{
+	int ret;
+
+	if (val < 1 || val > BIT(st->chip->max_avg + 1))
+		return -EINVAL;
+
+	/* 1 disables oversampling */
+	val = ilog2(val);
+	if (val == 0) {
+		st->mode = AD4062_SAMPLE_MODE;
+	} else {
+		st->mode = AD4062_BURST_AVERAGING_MODE;
+		ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
+		if (ret)
+			return ret;
+	}
+	st->oversamp_ratio = BIT(val);
+
+	return 0;
+}
+
+static int ad4062_get_oversampling_ratio(struct ad4062_state *st,
+					 unsigned int *val)
+{
+	int ret, buf;
+
+	if (st->mode == AD4062_SAMPLE_MODE) {
+		*val = 1;
+		return 0;
+	}
+
+	ret = regmap_read(st->regmap, AD4062_REG_AVG_CONFIG, &buf);
+	return 0;
+}
+
+static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
+{
+	/* See datasheet page 31 */
+	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
+
+	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
+}
+
+static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
+{
+	for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
+		st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
+								   st->oversamp_ratio);
+	return 0;
+}
+
+static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
+{
+	*val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
+					      st->oversamp_ratio);
+	return 0;
+}
+
+static int ad4062_set_sampling_frequency(struct ad4062_state *st, int val)
+{
+	int ret;
+
+	ret = ad4062_populate_sampling_frequency(st);
+	if (ret)
+		return ret;
+
+	st->sampling_frequency = find_closest_descending(val, st->samp_freqs,
+							 ARRAY_SIZE(ad4062_conversion_freqs));
+	return 0;
+}
+
+static int ad4062_check_ids(struct ad4062_state *st)
+{
+	int ret;
+	u16 val;
+
+	ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1,
+			       &st->buf.be16, sizeof(st->buf.be16));
+	if (ret)
+		return ret;
+
+	val = get_unaligned_be16(st->buf.bytes);
+	if (val != st->chip->prod_id)
+		dev_warn(&st->i3cdev->dev,
+			 "Production ID x%x does not match known values", val);
+
+	ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H,
+			       &st->buf.be16, sizeof(st->buf.be16));
+	if (ret)
+		return ret;
+
+	val = get_unaligned_be16(st->buf.bytes);
+	if (val != AD4062_I3C_VENDOR) {
+		dev_err(&st->i3cdev->dev,
+			"Vendor ID x%x does not match expected value\n", val);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int ad4062_set_operation_mode(struct ad4062_state *st,
+				     enum ad4062_operation_mode mode)
+{
+	int ret;
+
+	if (mode == AD4062_BURST_AVERAGING_MODE) {
+		ret = ad4062_conversion_frequency_set(st, st->sampling_frequency);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
+				 AD4062_REG_ADC_MODES_MODE_MSK, mode);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4062_REG_MODE_SET,
+			    AD4062_REG_MODE_SET_ENTER_ADC);
+}
+
+static int ad4062_soft_reset(struct ad4062_state *st)
+{
+	u8 val = AD4062_SOFT_RESET;
+	int ret;
+
+	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
+	if (ret)
+		return ret;
+
+	/* Wait AD4062 treset time */
+	fsleep(5000);
+
+	return 0;
+}
+
+static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			const bool *ref_sel)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+	int ret;
+	u8 val;
+
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DRDY);
+	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
+				 AD4062_REG_GP_CONF_MODE_MSK_1, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
+				 AD4062_REG_ADC_CONFIG_REF_EN_MSK,
+				 FIELD_PREP(AD4062_REG_ADC_CONFIG_REF_EN_MSK,
+					    *ref_sel));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4062_REG_DEVICE_STATUS,
+			   AD4062_REG_DEVICE_STATUS_DEVICE_RESET);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, AD4062_INTR_EN_NEITHER);
+	ret = regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
+				 AD4062_REG_INTR_CONF_EN_MSK_1, val);
+	if (ret)
+		return ret;
+
+	put_unaligned_be16(AD4062_MON_VAL_MIDDLE_POINT, st->buf.bytes);
+	return regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
+				 &st->buf.be16, sizeof(st->buf.be16));
+}
+
+static irqreturn_t ad4062_irq_handler_drdy(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	complete(&st->completion);
+
+	return IRQ_HANDLED;
+}
+
+static void ad4062_ibi_handler(struct i3c_device *i3cdev,
+			       const struct i3c_ibi_payload *payload)
+{
+	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
+
+	complete(&st->completion);
+}
+
+static void ad4062_remove_ibi(void *data)
+{
+	struct i3c_device *i3cdev = data;
+
+	i3c_device_disable_ibi(i3cdev);
+	i3c_device_free_ibi(i3cdev);
+}
+
+static int ad4062_request_ibi(struct i3c_device *i3cdev)
+{
+	const struct i3c_ibi_setup ibireq = {
+		.max_payload_len = 1,
+		.num_slots = 1,
+		.handler = ad4062_ibi_handler,
+	};
+	int ret;
+
+	ret = i3c_device_request_ibi(i3cdev, &ibireq);
+	if (ret)
+		return ret;
+
+	ret = i3c_device_enable_ibi(i3cdev);
+	if (ret)
+		goto err_enable_ibi;
+
+	return devm_add_action_or_reset(&i3cdev->dev, ad4062_remove_ibi, i3cdev);
+
+err_enable_ibi:
+	i3c_device_free_ibi(i3cdev);
+	return ret;
+}
+
+static int ad4062_request_irq(struct iio_dev *indio_dev)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->i3cdev->dev;
+	int ret;
+
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
+	if (ret == -EPROBE_DEFER) {
+		return ret;
+	} else if (ret < 0) {
+		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
+					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
+					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
+	} else {
+		ret = devm_request_threaded_irq(dev, ret,
+						ad4062_irq_handler_drdy,
+						NULL, IRQF_ONESHOT, indio_dev->name,
+						indio_dev);
+	}
+
+	return ret;
+}
+
+static const int ad4062_oversampling_avail[] = {
+	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
+};
+
+static int ad4062_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, const int **vals,
+			     int *type, int *len, long mask)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = ad4062_oversampling_avail;
+		*len = ARRAY_SIZE(ad4062_oversampling_avail);
+		*type = IIO_VAL_INT;
+
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = ad4062_populate_sampling_frequency(st);
+		if (ret)
+			return ret;
+		*vals = st->samp_freqs;
+		*len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;
+		*type = IIO_VAL_INT;
+
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4062_get_chan_scale(struct iio_dev *indio_dev, int *val, int *val2)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+
+	scan_type = iio_get_current_scan_type(indio_dev, st->chip->channels);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	*val = (st->vref_uv * 2) / MILLI;
+
+	*val2 = scan_type->realbits - 1; /* signed */
+
+	return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
+{
+	u16 gain;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
+			       &st->buf.be16, sizeof(st->buf.be16));
+	if (ret)
+		return ret;
+
+	gain = get_unaligned_be16(st->buf.bytes);
+
+	/* From datasheet: code out = code in × mon_val/0x8000 */
+	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;
+	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
+				AD4062_MON_VAL_MIDDLE_POINT);
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
+{
+	u64 gain;
+	int ret;
+
+	if (gain_int < 0 || gain_frac < 0)
+		return -EINVAL;
+
+	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
+
+	if (gain > AD4062_MON_VAL_MAX_GAIN)
+		return -EINVAL;
+
+	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
+						 MICRO),
+			   st->buf.bytes);
+
+	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
+				&st->buf.be16, sizeof(st->buf.be16));
+	if (ret)
+		return ret;
+
+	/* Enable scale if gain is not one. */
+	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
+				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
+				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
+					     !(gain_int == 1 && gain_frac == 0)));
+}
+
+static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
+{
+	struct i3c_device *i3cdev = st->i3cdev;
+	struct i3c_priv_xfer t[2] = {
+		{
+			.data.out = &st->reg_addr_conv,
+			.len = sizeof(st->reg_addr_conv),
+			.rnw = false,
+		},
+		{
+			.data.in = &st->buf.be32,
+			.len = sizeof(st->buf.be32),
+			.rnw = true,
+		}
+	};
+	int ret;
+
+	reinit_completion(&st->completion);
+	/* Change address pointer to trigger conversion */
+	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 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 = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
+	if (ret)
+		return ret;
+	*val = get_unaligned_be32(st->buf.bytes);
+	return 0;
+}
+
+static int ad4062_read_chan_raw(struct ad4062_state *st, int *val)
+{
+	int ret;
+
+	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
+	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
+	if (ret)
+		return ret;
+
+	ret = ad4062_set_operation_mode(st, st->mode);
+	if (ret)
+		return ret;
+
+	return __ad4062_read_chan_raw(st, val);
+}
+
+static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
+				    long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		return ad4062_read_chan_raw(st, val);
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4062_get_chan_calibscale(st, val, val2);
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4062_get_oversampling_ratio(st, val);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4062_get_sampling_frequency(st, val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4062_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (info == IIO_CHAN_INFO_SCALE)
+		return ad4062_get_chan_scale(indio_dev, val, val2);
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = ad4062_read_raw_dispatch(st, val, val2, info);
+
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int ad4062_write_raw_dispatch(struct ad4062_state *st, int val, int val2,
+				     long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4062_set_oversampling_ratio(st, val);
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4062_set_chan_calibscale(st, val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4062_set_sampling_frequency(st, val);
+
+	default:
+		return -EINVAL;
+	}
+};
+
+static int ad4062_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long info)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = ad4062_write_raw_dispatch(st, val, val2, info);
+
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				     unsigned int writeval, unsigned int *readval)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (readval)
+		ret = regmap_read(st->regmap, reg, readval);
+	else
+		ret = regmap_write(st->regmap, reg, writeval);
+
+	return ret;
+}
+
+static int ad4062_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	return st->mode == AD4062_BURST_AVERAGING_MODE ?
+			   AD4062_SCAN_TYPE_BURST_AVG :
+			   AD4062_SCAN_TYPE_SAMPLE;
+}
+
+static const struct iio_info ad4062_info = {
+	.read_raw = ad4062_read_raw,
+	.write_raw = ad4062_write_raw,
+	.read_avail = ad4062_read_avail,
+	.get_current_scan_type = &ad4062_get_current_scan_type,
+	.debugfs_reg_access = &ad4062_debugfs_reg_access,
+};
+
+static const struct regmap_config ad4062_regmap_config = {
+	.name = "ad4062",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AD4062_MAX_REG,
+	.rd_table = &ad4062_regmap_rd_table,
+	.wr_table = &ad4062_regmap_wr_table,
+	.can_sleep = true,
+};
+
+static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
+{
+	struct device *dev = &st->i3cdev->dev;
+	int ret;
+
+	ret = devm_regulator_get_enable(dev, "vio");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable vio voltage\n");
+
+	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+	*ref_sel = st->vref_uv == -ENODEV;
+	if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {
+		return dev_err_probe(dev, st->vref_uv,
+				     "Failed to enable and read ref voltage\n");
+	} else if (st->vref_uv == -ENODEV) {
+		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
+		if (st->vref_uv < 0)
+			return dev_err_probe(dev, st->vref_uv,
+					     "Failed to enable and read vdd voltage\n");
+	} else {
+		ret = devm_regulator_get_enable(dev, "vdd");
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to enable vdd regulator\n");
+	}
+
+	return 0;
+}
+
+static const struct i3c_device_id ad4062_id_table[] = {
+	I3C_DEVICE(AD4062_I3C_VENDOR, ad4060_chip_info.prod_id, &ad4060_chip_info),
+	I3C_DEVICE(AD4062_I3C_VENDOR, ad4062_chip_info.prod_id, &ad4062_chip_info),
+	{ }
+};
+MODULE_DEVICE_TABLE(i3c, ad4062_id_table);
+
+static int ad4062_probe(struct i3c_device *i3cdev)
+{
+	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, ad4062_id_table);
+	const struct ad4062_chip_info *chip = id->data;
+	struct device *dev = &i3cdev->dev;
+	struct iio_dev *indio_dev;
+	struct ad4062_state *st;
+	bool ref_sel;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->i3cdev = i3cdev;
+	i3cdev_set_drvdata(i3cdev, st);
+	init_completion(&st->completion);
+
+	ret = ad4062_regulators_get(st, &ref_sel);
+	if (ret)
+		return ret;
+
+	st->regmap = devm_regmap_init_i3c(i3cdev, &ad4062_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to initialize regmap\n");
+
+	st->mode = AD4062_SAMPLE_MODE;
+	st->chip = chip;
+	st->sampling_frequency = 0;
+	st->oversamp_ratio = BIT(0);
+	st->indio_dev = indio_dev;
+	st->reg_addr_conv = AD4062_REG_CONV_TRIGGER;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ad4062_info;
+	indio_dev->name = chip->name;
+	indio_dev->channels = chip->channels;
+
+	ret = ad4062_soft_reset(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "AD4062 failed to soft reset\n");
+
+	ret = ad4062_check_ids(st);
+	if (ret)
+		return ret;
+
+	ret = ad4062_setup(indio_dev, indio_dev->channels, &ref_sel);
+	if (ret)
+		return ret;
+
+	ret = ad4062_request_irq(indio_dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_active(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n");
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = ad4062_request_ibi(i3cdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int ad4062_runtime_suspend(struct device *dev)
+{
+	struct ad4062_state *st = dev_get_drvdata(dev);
+
+	return regmap_write(st->regmap, AD4062_REG_DEVICE_CONFIG,
+			    FIELD_PREP(AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK,
+				       AD4062_REG_DEVICE_CONFIG_LOW_POWER_MODE));
+}
+
+static int ad4062_runtime_resume(struct device *dev)
+{
+	struct ad4062_state *st = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
+				AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
+	if (ret)
+		return ret;
+
+	fsleep(4000);
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops, ad4062_runtime_suspend,
+				 ad4062_runtime_resume, NULL);
+
+static struct i3c_driver ad4062_driver = {
+	.driver = {
+		.name = "ad4062",
+		.pm = pm_ptr(&ad4062_pm_ops),
+	},
+	.probe = ad4062_probe,
+	.id_table = ad4062_id_table,
+};
+module_i3c_driver(ad4062_driver);
+
+MODULE_AUTHOR("Jorge Marques <jorge.marques@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4062");
+MODULE_LICENSE("GPL");

-- 
2.51.1

[PATCH v2 4/9] docs: iio: ad4062: Add IIO Trigger support

[Jorge Marques]

Explains the IIO Trigger support and timings involved.

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

diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
index e6bcca2bef24b..9dda4eb782a02 100644
--- a/Documentation/iio/ad4062.rst
+++ b/Documentation/iio/ad4062.rst
@@ -85,6 +85,19 @@ 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.
 
+IIO trigger support
+===================
+
+An IIO trigger ``ad4062-devX`` is registered by the driver to be used by the
+same device, to capture samples to a software buffer. It is required to attach
+the trigger to the device by setting the ``current_trigger`` before enabling
+and reading the buffer.
+
+The acquisition is sequential and bounded by the protocol timings, software
+latency and internal timings, the sample rate is not configurable. The burst
+averaging mode does impact the effective sample rate, since it increases the
+internal timing to output a single sample.
+
 Unimplemented features
 ======================
 

-- 
2.51.1

[PATCH v2 5/9] iio: adc: ad4062: Add IIO Trigger support

[Jorge Marques]

Adds support for IIO Trigger. Optionally, gp1 is assigned as Data Ready
signal, if not present, fallback to an I3C IBI with the same role.
The software trigger is allocated by the device, but must be attached by
the user before enabling the buffer. The purpose is to not impede
removing the driver due to the increased reference count when
iio_trigger_set_immutable or iio_trigger_get is used.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 drivers/iio/adc/Kconfig  |   2 +
 drivers/iio/adc/ad4062.c | 164 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 161 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e506dbe83f488..ddb7820f0bdcc 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -74,6 +74,8 @@ config AD4062
 	tristate "Analog Devices AD4062 Driver"
 	depends on I3C
 	select REGMAP_I3C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Analog Devices AD4062 I3C analog
 	  to digital converters (ADC).
diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
index 6866393ffef8d..4e4be7358047f 100644
--- a/drivers/iio/adc/ad4062.c
+++ b/drivers/iio/adc/ad4062.c
@@ -12,8 +12,12 @@
 #include <linux/err.h>
 #include <linux/i3c/device.h>
 #include <linux/i3c/master.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>
 #include <linux/math.h>
@@ -60,6 +64,7 @@
 #define     AD4062_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
 #define AD4062_REG_IBI_STATUS				0x48
 #define AD4062_REG_CONV_READ_LSB			0x50
+#define AD4062_REG_CONV_READ				0x53
 #define AD4062_REG_CONV_TRIGGER				0x59
 #define AD4062_REG_CONV_AUTO				0x61
 #define AD4062_MAX_REG					AD4062_REG_CONV_AUTO
@@ -134,6 +139,7 @@ struct ad4062_state {
 	const struct ad4062_chip_info *chip;
 	const struct ad4062_bus_ops *ops;
 	enum ad4062_operation_mode mode;
+	struct work_struct trig_conv;
 	struct completion completion;
 	struct iio_trigger *trigger;
 	struct iio_dev *indio_dev;
@@ -143,6 +149,7 @@ struct ad4062_state {
 	int vref_uv;
 	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
 	u8 oversamp_ratio;
+	bool gpo_irq[2];
 	union {
 		__be32 be32;
 		__be16 be16;
@@ -396,7 +403,10 @@ static irqreturn_t ad4062_irq_handler_drdy(int irq, void *private)
 	struct iio_dev *indio_dev = private;
 	struct ad4062_state *st = iio_priv(indio_dev);
 
-	complete(&st->completion);
+	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))
+		iio_trigger_poll(st->trigger);
+	else
+		complete(&st->completion);
 
 	return IRQ_HANDLED;
 }
@@ -406,7 +416,56 @@ static void ad4062_ibi_handler(struct i3c_device *i3cdev,
 {
 	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
 
-	complete(&st->completion);
+	if (iio_buffer_enabled(st->indio_dev))
+		iio_trigger_poll_nested(st->trigger);
+	else
+		complete(&st->completion);
+}
+
+static void ad4062_trigger_work(struct work_struct *work)
+{
+	struct ad4062_state *st = container_of(work, struct ad4062_state,
+					       trig_conv);
+	int ret;
+
+	/* Read current conversion, if at reg CONV_READ, stop bit triggers
+	 * next sample and does not need writing the address.
+	 */
+	struct i3c_priv_xfer t[2] = {
+		{
+			.data.in = &st->buf.be32,
+			.len = sizeof(st->buf.be32),
+			.rnw = true,
+		},
+		{
+			.data.out = &st->reg_addr_conv,
+			.len = sizeof(st->reg_addr_conv),
+			.rnw = false,
+		},
+	};
+
+	ret = i3c_device_do_priv_xfers(st->i3cdev, &t[0], 1);
+	if (ret)
+		return;
+
+	iio_push_to_buffers_with_timestamp(st->indio_dev, &st->buf.be32,
+					   iio_get_time_ns(st->indio_dev));
+	if (st->gpo_irq[1])
+		return;
+
+	i3c_device_do_priv_xfers(st->i3cdev, &t[1], 1);
+}
+
+static irqreturn_t ad4062_poll_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	iio_trigger_notify_done(indio_dev->trig);
+	schedule_work(&st->trig_conv);
+
+	return IRQ_HANDLED;
 }
 
 static void ad4062_remove_ibi(void *data)
@@ -451,10 +510,14 @@ static int ad4062_request_irq(struct iio_dev *indio_dev)
 	if (ret == -EPROBE_DEFER) {
 		return ret;
 	} else if (ret < 0) {
+		st->gpo_irq[1] = false;
+		st->reg_addr_conv = AD4062_REG_CONV_TRIGGER;
 		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
 					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
 					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
 	} else {
+		st->gpo_irq[1] = true;
+		st->reg_addr_conv = AD4062_REG_CONV_READ;
 		ret = devm_request_threaded_irq(dev, ret,
 						ad4062_irq_handler_drdy,
 						NULL, IRQF_ONESHOT, indio_dev->name,
@@ -464,6 +527,34 @@ static int ad4062_request_irq(struct iio_dev *indio_dev)
 	return ret;
 }
 
+static const struct iio_trigger_ops ad4062_trigger_ops = {
+	.validate_device = &iio_trigger_validate_own_device,
+};
+
+static int ad4062_request_trigger(struct iio_dev *indio_dev)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->i3cdev->dev;
+	int ret;
+
+	st->trigger = devm_iio_trigger_alloc(dev, "%s-dev%d",
+					     indio_dev->name,
+					     iio_device_id(indio_dev));
+	if (!st->trigger)
+		return -ENOMEM;
+
+	st->trigger->ops = &ad4062_trigger_ops;
+	iio_trigger_set_drvdata(st->trigger, indio_dev);
+
+	ret = devm_iio_trigger_register(dev, st->trigger);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(st->trigger);
+
+	return 0;
+}
+
 static const int ad4062_oversampling_avail[] = {
 	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
 };
@@ -579,8 +670,8 @@ static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
 	int ret;
 
 	reinit_completion(&st->completion);
-	/* Change address pointer to trigger conversion */
-	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
+	/* Change address pointer (and read if CONV_READ) to trigger conversion. */
+	ret = i3c_device_do_priv_xfers(i3cdev, t, st->gpo_irq[1] ? 2 : 1);
 	if (ret)
 		return ret;
 	/*
@@ -689,6 +780,57 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
+	if (ret)
+		return ret;
+
+	ret = ad4062_set_operation_mode(st, st->mode);
+	if (ret)
+		goto out_mode_error;
+
+	/* CONV_READ requires read to trigger first sample. */
+	struct i3c_priv_xfer t[2] = {
+		{
+			.data.out = &st->reg_addr_conv,
+			.len = sizeof(st->reg_addr_conv),
+			.rnw = false,
+		},
+		{
+			.data.in = &st->buf.be32,
+			.len = sizeof(st->buf.be32),
+			.rnw = true,
+		}
+	};
+
+	ret = i3c_device_do_priv_xfers(st->i3cdev, t, st->gpo_irq[1] ? 2 : 1);
+	if (ret)
+		goto out_mode_error;
+	return 0;
+
+out_mode_error:
+	pm_runtime_put_autosuspend(&st->i3cdev->dev);
+
+	return ret;
+}
+
+static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad4062_state *st = iio_priv(indio_dev);
+
+	pm_runtime_put_autosuspend(&st->i3cdev->dev);
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ad4062_triggered_buffer_setup_ops = {
+	.postenable = &ad4062_triggered_buffer_postenable,
+	.predisable = &ad4062_triggered_buffer_predisable,
+};
+
 static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 				     unsigned int writeval, unsigned int *readval)
 {
@@ -801,7 +943,6 @@ static int ad4062_probe(struct i3c_device *i3cdev)
 	st->sampling_frequency = 0;
 	st->oversamp_ratio = BIT(0);
 	st->indio_dev = indio_dev;
-	st->reg_addr_conv = AD4062_REG_CONV_TRIGGER;
 
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->num_channels = 1;
@@ -825,6 +966,17 @@ static int ad4062_probe(struct i3c_device *i3cdev)
 	if (ret)
 		return ret;
 
+	ret = ad4062_request_trigger(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(&i3cdev->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ad4062_poll_handler,
+					      &ad4062_triggered_buffer_setup_ops);
+	if (ret)
+		return ret;
+
 	pm_runtime_set_active(dev);
 	ret = devm_pm_runtime_enable(dev);
 	if (ret)
@@ -837,6 +989,8 @@ static int ad4062_probe(struct i3c_device *i3cdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
 
+	INIT_WORK(&st->trig_conv, ad4062_trigger_work);
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 

-- 
2.51.1

[PATCH v2 6/9] docs: iio: ad4062: Add IIO Events support

[Jorge Marques]

Explains the IIO Events support.

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

diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
index 9dda4eb782a02..5afec4d8c2ddb 100644
--- a/Documentation/iio/ad4062.rst
+++ b/Documentation/iio/ad4062.rst
@@ -26,6 +26,7 @@ at the end of the read command.
 The two programmable GPIOS are optional and have a role assigned if present in
 the devicetree ``interrupt-names`` property:
 
+- GP0: Is assigned the role of Threshold Either signal.
 - GP1: Is assigned the role of Data Ready signal.
 
 Device attributes
@@ -74,8 +75,10 @@ Interrupts
 The interrupts are mapped through the ``interrupt-names`` and ``interrupts``
 properties.
 
-The ``interrupt-names`` ``gp1`` entry sets the role of Data Ready signal.
-If it is not present, the driver fallback to enabling the same role as an
+The ``interrupt-names`` ``gp0`` entry sets the role of Threshold signal, and
+entry ``gp1`` the role of Data Ready signal.
+
+If each is not present, the driver fallback to enabling the same role as an
 I3C IBI.
 
 Low-power mode
@@ -98,10 +101,43 @@ latency and internal timings, the sample rate is not configurable. The burst
 averaging mode does impact the effective sample rate, since it increases the
 internal timing to output a single 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.
+
+The feature is enabled/disabled by setting ``thresh_either_en``. During monitor
+mode, the device continuously operates in autonomous mode. Any register access
+puts the device back in configuration mode, due to this, any access disables
+monitor mode.
+
+The following event attributes are available:
+
+.. list-table:: Event attributes
+   :header-rows: 1
+
+   * - Attribute
+     - Description
+   * - ``sampling_frequency``
+     - Frequency used in the monitoring mode, sets the device internal sample
+       rate when the mode is activated.
+   * - ``sampling_frequency_available``
+     - List of available sample rates.
+   * - ``thresh_either_en``
+     - Enable monitoring mode.
+   * - ``thresh_falling_hysteresis``
+     - Set the hysteresis value for the minimum threshold.
+   * - ``thresh_falling_value``
+     - Set the minimum threshold value.
+   * - ``thresh_rising_hysteresis``
+     - Set the hysteresis value for the maximum threshold.
+   * - ``thresh_rising_value``
+     - Set the maximum threshold value.
+
 Unimplemented features
 ======================
 
-- Monitor mode
 - Trigger mode
 - Averaging mode
 - General purpose output

-- 
2.51.1

[PATCH v2 7/9] iio: adc: ad4062: Add IIO Events support

[Jorge Marques]

Adds support for IIO Events. Optionally, gp0 is assigned as Threshold
Either signal, if not present, fallback to an I3C IBI with the same
role.

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

diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
index 4e4be7358047f..3df7dbf29ae4a 100644
--- a/drivers/iio/adc/ad4062.c
+++ b/drivers/iio/adc/ad4062.c
@@ -13,6 +13,7 @@
 #include <linux/i3c/device.h>
 #include <linux/i3c/master.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger.h>
@@ -51,14 +52,22 @@
 #define     AD4062_REG_ADC_CONFIG_SCALE_EN_MSK		BIT(4)
 #define AD4062_REG_AVG_CONFIG				0x23
 #define AD4062_REG_GP_CONF				0x24
+#define     AD4062_REG_GP_CONF_MODE_MSK_0		GENMASK(2, 0)
 #define     AD4062_REG_GP_CONF_MODE_MSK_1		GENMASK(6, 4)
 #define AD4062_REG_INTR_CONF				0x25
+#define     AD4062_REG_INTR_CONF_EN_MSK_0		GENMASK(1, 0)
 #define     AD4062_REG_INTR_CONF_EN_MSK_1		GENMASK(5, 4)
 #define AD4062_REG_TIMER_CONFIG				0x27
 #define     AD4062_REG_TIMER_CONFIG_FS_MASK		GENMASK(7, 4)
+#define AD4062_REG_MAX_LIMIT				0x29
+#define AD4062_REG_MIN_LIMIT				0x2B
+#define AD4062_REG_MAX_HYST				0x2C
+#define AD4062_REG_MIN_HYST				0x2D
 #define AD4062_REG_MON_VAL				0x2F
 #define AD4062_REG_ADC_IBI_EN				0x31
 #define AD4062_REG_ADC_IBI_EN_CONV_TRIGGER		BIT(2)
+#define AD4062_REG_ADC_IBI_EN_MAX			BIT(1)
+#define AD4062_REG_ADC_IBI_EN_MIN			BIT(0)
 #define AD4062_REG_FUSE_CRC				0x40
 #define AD4062_REG_DEVICE_STATUS			0x41
 #define     AD4062_REG_DEVICE_STATUS_DEVICE_RESET	BIT(6)
@@ -76,8 +85,10 @@
 #define AD4062_MAX_AVG		0xB
 #define AD4062_MON_VAL_MAX_GAIN		1999970
 #define AD4062_MON_VAL_MIDDLE_POINT	0x8000
+#define AD4062_GP_INTR		0x1
 #define AD4062_GP_DRDY		0x2
 #define AD4062_INTR_EN_NEITHER	0x0
+#define AD4062_INTR_EN_EITHER	0x3
 #define AD4062_TCONV_NS		270
 
 enum ad4062_operation_mode {
@@ -146,6 +157,8 @@ struct ad4062_state {
 	struct i3c_device *i3cdev;
 	struct regmap *regmap;
 	u16 sampling_frequency;
+	u16 events_frequency;
+	bool wait_event;
 	int vref_uv;
 	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
 	u8 oversamp_ratio;
@@ -184,6 +197,26 @@ static const struct regmap_access_table ad4062_regmap_wr_table = {
 	.n_yes_ranges = ARRAY_SIZE(ad4062_regmap_wr_ranges),
 };
 
+static const struct iio_event_spec ad4062_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 int ad4062_conversion_frequency_set(struct ad4062_state *st, u8 val)
 {
 	return regmap_write(st->regmap, AD4062_REG_TIMER_CONFIG,
@@ -201,6 +234,8 @@ static int ad4062_conversion_frequency_set(struct ad4062_state *st, u8 val)
 	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
 	.indexed = 1,									\
 	.channel = 0,									\
+	.event_spec = ad4062_events,							\
+	.num_event_specs = ARRAY_SIZE(ad4062_events),					\
 	.has_ext_scan_type = 1,								\
 	.ext_scan_type = ad4062_scan_type_##bits##_s,					\
 	.num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s),			\
@@ -220,6 +255,70 @@ static const struct ad4062_chip_info ad4062_chip_info = {
 	.max_avg = AD4062_MAX_AVG,
 };
 
+static ssize_t ad4062_events_frequency_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct ad4062_state *st = iio_priv(dev_to_iio_dev(dev));
+
+	return sysfs_emit(buf, "%d\n", ad4062_conversion_freqs[st->events_frequency]);
+}
+
+static ssize_t ad4062_events_frequency_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad4062_state *st = iio_priv(indio_dev);
+	int val, ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret < 0)
+		goto out_release;
+
+	st->events_frequency = find_closest_descending(val, ad4062_conversion_freqs,
+						       ARRAY_SIZE(ad4062_conversion_freqs));
+	ret = 0;
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(sampling_frequency, 0644, ad4062_events_frequency_show,
+		       ad4062_events_frequency_store, 0);
+
+static ssize_t sampling_frequency_available_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	int ret = 0;
+
+	for (u8 i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
+		ret += sysfs_emit_at(buf, ret, "%d%s", ad4062_conversion_freqs[i],
+				     i != (ARRAY_SIZE(ad4062_conversion_freqs) - 1) ? " " : "\n");
+	return ret;
+}
+
+static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
+
+static struct attribute *ad4062_event_attributes[] = {
+	&iio_dev_attr_sampling_frequency.dev_attr.attr,
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ad4062_event_attribute_group = {
+	.attrs = ad4062_event_attributes,
+};
+
 static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
 {
 	int ret;
@@ -369,9 +468,12 @@ static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
 	if (IS_ERR(scan_type))
 		return PTR_ERR(scan_type);
 
-	val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DRDY);
+	val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_INTR) |
+	      FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DRDY);
+
 	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
-				 AD4062_REG_GP_CONF_MODE_MSK_1, val);
+				 AD4062_REG_GP_CONF_MODE_MSK_1 | AD4062_REG_GP_CONF_MODE_MSK_0,
+				 val);
 	if (ret)
 		return ret;
 
@@ -387,9 +489,11 @@ static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
 	if (ret)
 		return ret;
 
-	val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, AD4062_INTR_EN_NEITHER);
+	val = FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_0, AD4062_INTR_EN_EITHER) |
+	      FIELD_PREP(AD4062_REG_INTR_CONF_EN_MSK_1, AD4062_INTR_EN_NEITHER);
 	ret = regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
-				 AD4062_REG_INTR_CONF_EN_MSK_1, val);
+				 AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
+				 val);
 	if (ret)
 		return ret;
 
@@ -398,6 +502,19 @@ static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
 				 &st->buf.be16, sizeof(st->buf.be16));
 }
 
+static irqreturn_t ad4062_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 ad4062_irq_handler_drdy(int irq, void *private)
 {
 	struct iio_dev *indio_dev = private;
@@ -416,10 +533,18 @@ static void ad4062_ibi_handler(struct i3c_device *i3cdev,
 {
 	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
 
-	if (iio_buffer_enabled(st->indio_dev))
-		iio_trigger_poll_nested(st->trigger);
-	else
-		complete(&st->completion);
+	if (st->wait_event) {
+		iio_push_event(st->indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(st->indio_dev));
+	} else {
+		if (iio_buffer_enabled(st->indio_dev))
+			iio_trigger_poll_nested(st->trigger);
+		else
+			complete(&st->completion);
+	}
 }
 
 static void ad4062_trigger_work(struct work_struct *work)
@@ -506,6 +631,24 @@ static int ad4062_request_irq(struct iio_dev *indio_dev)
 	struct device *dev = &st->i3cdev->dev;
 	int ret;
 
+	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp0");
+	if (ret == -EPROBE_DEFER) {
+		return ret;
+	} else if (ret < 0) {
+		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
+					 AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN,
+					 AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN);
+		if (ret)
+			return ret;
+	} else {
+		ret = devm_request_threaded_irq(dev, ret, NULL,
+						ad4062_irq_handler_thresh,
+						IRQF_ONESHOT, indio_dev->name,
+						indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
 	if (ret == -EPROBE_DEFER) {
 		return ret;
@@ -739,9 +882,14 @@ static int ad4062_read_raw(struct iio_dev *indio_dev,
 
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
 
 	ret = ad4062_read_raw_dispatch(st, val, val2, info);
 
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret ? ret : IIO_VAL_INT;
 }
@@ -773,9 +921,215 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
 
 	if (!iio_device_claim_direct(indio_dev))
 		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
 
 	ret = ad4062_write_raw_dispatch(st, val, val2, info);
 
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
+{
+	int ret = 0;
+
+	if (!enable) {
+		pm_runtime_put_autosuspend(&st->i3cdev->dev);
+		return 0;
+	}
+
+	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
+	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
+	if (ret)
+		return ret;
+
+	ret = ad4062_conversion_frequency_set(st, st->events_frequency);
+	if (ret)
+		return ret;
+
+	ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
+	if (ret)
+		return ret;
+
+	pm_runtime_get_noresume(&st->i3cdev->dev);
+	return 0;
+}
+
+static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
+
+	return st->wait_event;
+}
+
+static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event == state) {
+		ret = 0;
+		goto out_release;
+	}
+
+	ret = ad4062_monitor_mode_enable(st, state);
+	if (!ret)
+		st->wait_event = state;
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret;
+}
+
+static int __ad4062_read_event_info_value(struct ad4062_state *st,
+					  enum iio_event_direction dir, int *val)
+{
+	int ret;
+	u8 reg;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4062_REG_MAX_LIMIT;
+	else
+		reg = AD4062_REG_MIN_LIMIT;
+
+	ret = regmap_bulk_read(st->regmap, reg, &st->buf.be16,
+			       sizeof(st->buf.be16));
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(get_unaligned_be16(st->buf.bytes), 11);
+
+	return 0;
+}
+
+static int __ad4062_read_event_info_hysteresis(struct ad4062_state *st,
+					       enum iio_event_direction dir, int *val)
+{
+	u8 reg;
+
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4062_REG_MAX_HYST;
+	else
+		reg = AD4062_REG_MIN_HYST;
+	return regmap_read(st->regmap, reg, val);
+}
+
+static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = __ad4062_read_event_info_value(st, dir, val);
+		break;
+	case IIO_EV_INFO_HYSTERESIS:
+		ret = __ad4062_read_event_info_hysteresis(st, dir, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_release:
+	iio_device_release_direct(indio_dev);
+	return ret ? ret : IIO_VAL_INT;
+}
+
+static int __ad4062_write_event_info_value(struct ad4062_state *st,
+					   enum iio_event_direction dir, int val)
+{
+	u8 reg;
+
+	if (val > 2047 || val < -2048)
+		return -EINVAL;
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4062_REG_MAX_LIMIT;
+	else
+		reg = AD4062_REG_MIN_LIMIT;
+	put_unaligned_be16(val, st->buf.bytes);
+
+	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
+				 sizeof(st->buf.be16));
+}
+
+static int __ad4062_write_event_info_hysteresis(struct ad4062_state *st,
+						enum iio_event_direction dir, int val)
+{
+	u8 reg;
+
+	if (val >= BIT(7))
+		return -EINVAL;
+	if (dir == IIO_EV_DIR_RISING)
+		reg = AD4062_REG_MAX_HYST;
+	else
+		reg = AD4062_REG_MIN_HYST;
+
+	return regmap_write(st->regmap, reg, val);
+}
+
+static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+	if (st->wait_event) {
+		ret = -EBUSY;
+		goto out_release;
+	}
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = __ad4062_write_event_info_value(st, dir, val);
+			break;
+		case IIO_EV_INFO_HYSTERESIS:
+			ret = __ad4062_write_event_info_hysteresis(st, dir, val);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out_release:
 	iio_device_release_direct(indio_dev);
 	return ret;
 }
@@ -785,13 +1139,17 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
 	struct ad4062_state *st = iio_priv(indio_dev);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
+	if (st->wait_event)
+		return -EBUSY;
+
+	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
+	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
 	if (ret)
 		return ret;
 
 	ret = ad4062_set_operation_mode(st, st->mode);
 	if (ret)
-		goto out_mode_error;
+		return ret;
 
 	/* CONV_READ requires read to trigger first sample. */
 	struct i3c_priv_xfer t[2] = {
@@ -809,13 +1167,10 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
 
 	ret = i3c_device_do_priv_xfers(st->i3cdev, t, st->gpo_irq[1] ? 2 : 1);
 	if (ret)
-		goto out_mode_error;
-	return 0;
-
-out_mode_error:
-	pm_runtime_put_autosuspend(&st->i3cdev->dev);
+		return ret;
 
-	return ret;
+	pm_runtime_get_noresume(&st->i3cdev->dev);
+	return 0;
 }
 
 static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
@@ -859,6 +1214,11 @@ static const struct iio_info ad4062_info = {
 	.read_raw = ad4062_read_raw,
 	.write_raw = ad4062_write_raw,
 	.read_avail = ad4062_read_avail,
+	.read_event_config = &ad4062_read_event_config,
+	.write_event_config = &ad4062_write_event_config,
+	.read_event_value = &ad4062_read_event_value,
+	.write_event_value = &ad4062_write_event_value,
+	.event_attrs = &ad4062_event_attribute_group,
 	.get_current_scan_type = &ad4062_get_current_scan_type,
 	.debugfs_reg_access = &ad4062_debugfs_reg_access,
 };
@@ -939,8 +1299,10 @@ static int ad4062_probe(struct i3c_device *i3cdev)
 				     "Failed to initialize regmap\n");
 
 	st->mode = AD4062_SAMPLE_MODE;
+	st->wait_event = false;
 	st->chip = chip;
 	st->sampling_frequency = 0;
+	st->events_frequency = 0;
 	st->oversamp_ratio = BIT(0);
 	st->indio_dev = indio_dev;
 

-- 
2.51.1

[PATCH v2 8/9] docs: iio: ad4062: Add GPIO Controller support

[Jorge Marques]

Explains the GPIO controller support with emphasis on the mask
depending on which GPs are exposed.

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

diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
index 5afec4d8c2ddb..78665755ebebc 100644
--- a/Documentation/iio/ad4062.rst
+++ b/Documentation/iio/ad4062.rst
@@ -29,6 +29,9 @@ the devicetree ``interrupt-names`` property:
 - GP0: Is assigned the role of Threshold Either signal.
 - GP1: Is assigned the role of Data Ready signal.
 
+If the property ``gpio-controller`` is present in the devicetree, then the GPO
+not present in the ``interrupt-names`` is exposed as a GPO.
+
 Device attributes
 =================
 
@@ -135,9 +138,17 @@ The following event attributes are available:
    * - ``thresh_rising_value``
      - Set the maximum threshold value.
 
+GPO controller support
+======================
+
+The device supports using GP0 and GP1 as GPOs. If the devicetree contains the
+node ``gpio-controller```, the device is marked as a GPIO controller and the
+GPs not listed in ``interrupt-names`` are exposed as a GPO. The GPIO index
+matches the pin name, so if GP0 is not exposed but GP1 is, index 0 is masked
+out and only index 1 can be set.
+
 Unimplemented features
 ======================
 
 - Trigger mode
 - Averaging mode
-- General purpose output

-- 
2.51.1

[PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Jorge Marques]

When gp0 or gp1 is not taken as an interrupt, expose them as gpo if
gpio-contoller is set in the devicetree.

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

diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
index 3df7dbf29ae4a..203b06276431f 100644
--- a/drivers/iio/adc/ad4062.c
+++ b/drivers/iio/adc/ad4062.c
@@ -10,6 +10,7 @@
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/gpio/driver.h>
 #include <linux/i3c/device.h>
 #include <linux/i3c/master.h>
 #include <linux/iio/buffer.h>
@@ -85,8 +86,11 @@
 #define AD4062_MAX_AVG		0xB
 #define AD4062_MON_VAL_MAX_GAIN		1999970
 #define AD4062_MON_VAL_MIDDLE_POINT	0x8000
+#define AD4062_GP_DISABLED	0x0
 #define AD4062_GP_INTR		0x1
 #define AD4062_GP_DRDY		0x2
+#define AD4062_GP_STATIC_LOW	0x5
+#define AD4062_GP_STATIC_HIGH	0x6
 #define AD4062_INTR_EN_NEITHER	0x0
 #define AD4062_INTR_EN_EITHER	0x3
 #define AD4062_TCONV_NS		270
@@ -635,12 +639,14 @@ static int ad4062_request_irq(struct iio_dev *indio_dev)
 	if (ret == -EPROBE_DEFER) {
 		return ret;
 	} else if (ret < 0) {
+		st->gpo_irq[0] = false;
 		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
 					 AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN,
 					 AD4062_REG_ADC_IBI_EN_MAX | AD4062_REG_ADC_IBI_EN_MIN);
 		if (ret)
 			return ret;
 	} else {
+		st->gpo_irq[0] = true;
 		ret = devm_request_threaded_irq(dev, ret, NULL,
 						ad4062_irq_handler_thresh,
 						IRQF_ONESHOT, indio_dev->name,
@@ -1263,6 +1269,130 @@ static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
 	return 0;
 }
 
+static int ad4062_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int ad4062_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct ad4062_state *st = gpiochip_get_data(gc);
+	unsigned int reg_val = value ? AD4062_GP_STATIC_HIGH : AD4062_GP_STATIC_LOW;
+
+	if (st->gpo_irq[offset])
+		return -ENODEV;
+
+	if (offset)
+		return regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
+					  AD4062_REG_GP_CONF_MODE_MSK_1,
+					  FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, reg_val));
+	else
+		return regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
+					  AD4062_REG_GP_CONF_MODE_MSK_0,
+					  FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, reg_val));
+}
+
+static int ad4062_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct ad4062_state *st = gpiochip_get_data(gc);
+	unsigned int reg_val;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4062_REG_GP_CONF, &reg_val);
+	if (ret)
+		return 0;
+
+	if (st->gpo_irq[offset])
+		return -ENODEV;
+
+	if (offset)
+		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_1, reg_val);
+	else
+		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_0, reg_val);
+
+	return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
+}
+
+static void ad4062_gpio_disable(void *data)
+{
+	struct ad4062_state *st = data;
+	u8 val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_DISABLED) |
+		 FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_DISABLED);
+
+	regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
+			   AD4062_REG_GP_CONF_MODE_MSK_1 | AD4062_REG_GP_CONF_MODE_MSK_0,
+			   val);
+}
+
+static int ad4062_gpio_init_valid_mask(struct gpio_chip *gc,
+				       unsigned long *valid_mask,
+				       unsigned int ngpios)
+{
+	struct ad4062_state *st = gpiochip_get_data(gc);
+
+	bitmap_zero(valid_mask, ngpios);
+
+	if (!st->gpo_irq[0])
+		set_bit(0, valid_mask);
+	if (!st->gpo_irq[1])
+		set_bit(1, valid_mask);
+
+	return 0;
+}
+
+static int ad4062_gpio_init(struct ad4062_state *st)
+{
+	struct device *dev = &st->i3cdev->dev;
+	struct gpio_chip *gc;
+	u8 val, mask;
+	int ret;
+
+	if ((st->gpo_irq[0] && st->gpo_irq[1]) ||
+	    !device_property_read_bool(dev, "gpio-controller"))
+		return 0;
+
+	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
+	if (!gc)
+		return -ENOMEM;
+
+	val = 0;
+	mask = 0;
+	if (!st->gpo_irq[0]) {
+		mask |= AD4062_REG_GP_CONF_MODE_MSK_0;
+		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_STATIC_LOW);
+	}
+	if (!st->gpo_irq[1]) {
+		mask |= AD4062_REG_GP_CONF_MODE_MSK_1;
+		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_STATIC_LOW);
+	}
+
+	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
+				 mask, val);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, ad4062_gpio_disable, st);
+	if (ret)
+		return ret;
+
+	gc->parent = dev;
+	gc->label = st->chip->name;
+	gc->owner = THIS_MODULE;
+	gc->base = -1;
+	gc->ngpio = 2;
+	gc->init_valid_mask = ad4062_gpio_init_valid_mask;
+	gc->get_direction = ad4062_gpio_get_direction;
+	gc->set = ad4062_gpio_set;
+	gc->get = ad4062_gpio_get;
+	gc->can_sleep = true;
+
+	ret = devm_gpiochip_add_data(dev, gc, st);
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to register GPIO chip\n");
+
+	return 0;
+}
+
 static const struct i3c_device_id ad4062_id_table[] = {
 	I3C_DEVICE(AD4062_I3C_VENDOR, ad4060_chip_info.prod_id, &ad4060_chip_info),
 	I3C_DEVICE(AD4062_I3C_VENDOR, ad4062_chip_info.prod_id, &ad4062_chip_info),
@@ -1351,6 +1481,10 @@ static int ad4062_probe(struct i3c_device *i3cdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
 
+	ret = ad4062_gpio_init(st);
+	if (ret)
+		return ret;
+
 	INIT_WORK(&st->trig_conv, ad4062_trigger_work);
 
 	return devm_iio_device_register(dev, indio_dev);

-- 
2.51.1

Re: [PATCH v2 5/9] iio: adc: ad4062: Add IIO Trigger support

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 10:18:04AM +0100, Jorge Marques wrote:
> Adds support for IIO Trigger. Optionally, gp1 is assigned as Data Ready
> signal, if not present, fallback to an I3C IBI with the same role.
> The software trigger is allocated by the device, but must be attached by
> the user before enabling the buffer. The purpose is to not impede
> removing the driver due to the increased reference count when
> iio_trigger_set_immutable or iio_trigger_get is used.

We refer to the functions as func(). Mind the parentheses.

...

> +	struct ad4062_state *st = container_of(work, struct ad4062_state,
> +					       trig_conv);

I think the

	struct ad4062_state *st =
		container_of(work, struct ad4062_state, trig_conv);

reads better.

> +	int ret;

...

> +	/* Read current conversion, if at reg CONV_READ, stop bit triggers
> +	 * next sample and does not need writing the address.
> +	 */

/*
 * The multi-line comment style is as in
 * this example. Please, check and update.
 */

> +static irqreturn_t ad4062_poll_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	schedule_work(&st->trig_conv);
> +
> +	return IRQ_HANDLED;
>  }

...

> +static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_mode_error;
> +
> +	/* CONV_READ requires read to trigger first sample. */
> +	struct i3c_priv_xfer t[2] = {
> +		{
> +			.data.out = &st->reg_addr_conv,
> +			.len = sizeof(st->reg_addr_conv),
> +			.rnw = false,
> +		},
> +		{
> +			.data.in = &st->buf.be32,
> +			.len = sizeof(st->buf.be32),
> +			.rnw = true,
> +		}
> +	};
> +
> +	ret = i3c_device_do_priv_xfers(st->i3cdev, t, st->gpo_irq[1] ? 2 : 1);
> +	if (ret)
> +		goto out_mode_error;
> +	return 0;
> +
> +out_mode_error:
> +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> +
> +	return ret;

I guess with ACQUIRE() this function will look better, because the explicit
reference count bumping (with an associated comment) is more descriptive on
what's going on here with PM. Same for other related functions.

> +}

...

>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
>  
> +	INIT_WORK(&st->trig_conv, ad4062_trigger_work);

This is mixture of devm_*() and non-devm_*() calls. How did you (stress) test
the removal and error paths here? Wouldn't devm-helpers.h APIs help here to
make / keep order correct?

>  	return devm_iio_device_register(dev, indio_dev);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Linus Walleij]

On Mon, Nov 24, 2025 at 10:19 AM Jorge Marques <jorge.marques@analog.com> wrote:

> When gp0 or gp1 is not taken as an interrupt, expose them as gpo if
> gpio-contoller is set in the devicetree.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>

It looks to me like it will work!

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> register (SAR) analog-to-digital converter (ADC) with low-power and
> threshold monitoring modes.

...

> +#define AD4062_SOFT_RESET	0x81

The grouping seems a bit strange. Haven't you forgotten a blank line here?
Ditto for other similar cases.

> +#define AD4060_MAX_AVG		0x7
> +#define AD4062_MAX_AVG		0xB

> +#define AD4062_MON_VAL_MAX_GAIN		1999970

This is decimal...

> +#define AD4062_MON_VAL_MIDDLE_POINT	0x8000

...and this is hexadecimal. Can you make these consistent?
Also, is there any explanation of the number above? To me
it looks like 2000000 - 30. Is it so? Or is this a fraction
number multiplied by 1000000 or so? In any case some elaboration
would be good to have.

> +#define AD4062_GP_DRDY		0x2
> +#define AD4062_INTR_EN_NEITHER	0x0
> +#define AD4062_TCONV_NS		270

...

> +struct ad4062_state {
> +	const struct ad4062_chip_info *chip;
> +	const struct ad4062_bus_ops *ops;
> +	enum ad4062_operation_mode mode;
> +	struct completion completion;
> +	struct iio_trigger *trigger;
> +	struct iio_dev *indio_dev;
> +	struct i3c_device *i3cdev;
> +	struct regmap *regmap;
> +	u16 sampling_frequency;
> +	int vref_uv;
> +	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> +	u8 oversamp_ratio;
> +	union {
> +		__be32 be32;
> +		__be16 be16;
> +		u8 bytes[4];
> +	} buf __aligned(IIO_DMA_MINALIGN);
> +	u8 reg_addr_conv;

Can't we group u8:s to save a few bytes of memory?

> +};

...

> +static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
> +{
> +	int ret;
> +
> +	if (val < 1 || val > BIT(st->chip->max_avg + 1))

in_range() ?

	in_range(val, 1, GENMASK(st->chip->max_avg, 0))

if I am not mistaken. Also note, the GENMASK() approach makes possible
to have all 32 bits set, however it's most unlikely to happen here anyway.

> +		return -EINVAL;
> +
> +	/* 1 disables oversampling */
> +	val = ilog2(val);
> +	if (val == 0) {
> +		st->mode = AD4062_SAMPLE_MODE;
> +	} else {
> +		st->mode = AD4062_BURST_AVERAGING_MODE;
> +		ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
> +		if (ret)
> +			return ret;
> +	}
> +	st->oversamp_ratio = BIT(val);
> +
> +	return 0;
> +}

...

> +static int ad4062_get_oversampling_ratio(struct ad4062_state *st,
> +					 unsigned int *val)
> +{
> +	int ret, buf;
> +
> +	if (st->mode == AD4062_SAMPLE_MODE) {
> +		*val = 1;
> +		return 0;
> +	}

> +	ret = regmap_read(st->regmap, AD4062_REG_AVG_CONFIG, &buf);
> +	return 0;

This is strange piece of code. Why do we have ret at all?
Please, try to compile kernel also with `make LLVM=1 W=1 ...`
assuming you have clang installed. It catches such issues quite
well.

> +}

...

> +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> +{
> +	/* See datasheet page 31 */
> +	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> +
> +	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);

Why u64?

The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
more than 4 billions?

> +}
> +
> +static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
> +{
> +	for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)

Why signed iterator?

> +		st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> +								   st->oversamp_ratio);

Perhaps

		st->samp_freqs[i] =
			ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
						       st->oversamp_ratio);

But I am not insisting on this case and similar.


> +	return 0;
> +}

> +static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
> +{
> +	*val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
> +					      st->oversamp_ratio);

Oh, temporary variable makes this better for readability.

> +	return 0;
> +}

...

> +static int ad4062_check_ids(struct ad4062_state *st)
> +{

	struct device *dev = &st->i3cdev->dev;

> +	int ret;
> +	u16 val;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1,
> +			       &st->buf.be16, sizeof(st->buf.be16));
> +	if (ret)
> +		return ret;
> +
> +	val = get_unaligned_be16(st->buf.bytes);
> +	if (val != st->chip->prod_id)
> +		dev_warn(&st->i3cdev->dev,
> +			 "Production ID x%x does not match known values", val);

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

> +	ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H,
> +			       &st->buf.be16, sizeof(st->buf.be16));
> +	if (ret)
> +		return ret;
> +
> +	val = get_unaligned_be16(st->buf.bytes);
> +	if (val != AD4062_I3C_VENDOR) {
> +		dev_err(&st->i3cdev->dev,
> +			"Vendor ID x%x does not match expected value\n", val);

		dev_err(dev, "Vendor ID x%x does not match expected value\n", val);

> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}

...

> +static int ad4062_soft_reset(struct ad4062_state *st)
> +{
> +	u8 val = AD4062_SOFT_RESET;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait AD4062 treset time */
> +	fsleep(5000);

5 * USEC_PER_MSEC

This gives a hint on the units without even a need to comment or look somewhere
else.

> +	return 0;
> +}

...

> +static int ad4062_request_irq(struct iio_dev *indio_dev)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->i3cdev->dev;
> +	int ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> +	if (ret == -EPROBE_DEFER) {
> +		return ret;

> +	} else if (ret < 0) {

Redundant 'else'

> +		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
> +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
> +	} else {
> +		ret = devm_request_threaded_irq(dev, ret,
> +						ad4062_irq_handler_drdy,
> +						NULL, IRQF_ONESHOT, indio_dev->name,
> +						indio_dev);
> +	}
> +
> +	return ret;
> +}

...

> +static const int ad4062_oversampling_avail[] = {
> +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,

It's not easy to count them at glance, please add a comment with indices.

> +};

...

> +static int ad4062_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, const int **vals,
> +			     int *type, int *len, long mask)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4062_oversampling_avail;
> +		*len = ARRAY_SIZE(ad4062_oversampling_avail);
> +		*type = IIO_VAL_INT;
> +
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = ad4062_populate_sampling_frequency(st);
> +		if (ret)
> +			return ret;
> +		*vals = st->samp_freqs;
> +		*len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;

Why not using positive conditional?

Funny trick that Elvis operator can be used in this case, but please don't,
it will make code harder to follow.

> +		*type = IIO_VAL_INT;
> +
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int ad4062_get_chan_scale(struct iio_dev *indio_dev, int *val, int *val2)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, st->chip->channels);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	*val = (st->vref_uv * 2) / MILLI;

It's most likely (MICRO / MILLI) instead of MILLI. Am I right?

> +	*val2 = scan_type->realbits - 1; /* signed */
> +
> +	return IIO_VAL_FRACTIONAL_LOG2;
> +}

...

> +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> +{
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> +			       &st->buf.be16, sizeof(st->buf.be16));
> +	if (ret)
> +		return ret;
> +
> +	gain = get_unaligned_be16(st->buf.bytes);
> +
> +	/* From datasheet: code out = code in × mon_val/0x8000 */
> +	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;

> +	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
> +				AD4062_MON_VAL_MIDDLE_POINT);

I don't see the need for 64-bit division. Can you elaborate what I miss here?

> +	return IIO_VAL_INT_PLUS_NANO;
> +}

...

> +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)

Forgot to wrap this line.

> +{
> +	u64 gain;
> +	int ret;
> +
> +	if (gain_int < 0 || gain_frac < 0)
> +		return -EINVAL;
> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;

> +

Redundant blank line.

> +	if (gain > AD4062_MON_VAL_MAX_GAIN)
> +		return -EINVAL;
> +
> +	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> +						 MICRO),
> +			   st->buf.bytes);

Also in doubt here about 64-bit division.

> +	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> +				&st->buf.be16, sizeof(st->buf.be16));
> +	if (ret)
> +		return ret;
> +
> +	/* Enable scale if gain is not one. */

"...is not equal to one."

Also be consistent with the style for one-line comments. Choose one and
use it everywhere. Usual cases:
- my one-line comment
- My one-line comment
- My one-line comment.


> +	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> +				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> +				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> +					     !(gain_int == 1 && gain_frac == 0)));
> +}

...

> +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)

Can be named without leading double underscore? Preference is to use
the suffix, like _no_pm (but you can find better one).

> +{
> +	struct i3c_device *i3cdev = st->i3cdev;
> +	struct i3c_priv_xfer t[2] = {
> +		{
> +			.data.out = &st->reg_addr_conv,
> +			.len = sizeof(st->reg_addr_conv),
> +			.rnw = false,
> +		},
> +		{
> +			.data.in = &st->buf.be32,
> +			.len = sizeof(st->buf.be32),
> +			.rnw = true,
> +		}
> +	};
> +	int ret;
> +
> +	reinit_completion(&st->completion);
> +	/* Change address pointer to trigger conversion */
> +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);

Why array? Just split them on per transfer and use separately. This gives a bit
odd feeling that the two goes together, but no. They are semi-related as we
have a special condition after the first one.

> +	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 = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
> +	if (ret)
> +		return ret;
> +	*val = get_unaligned_be32(st->buf.bytes);
> +	return 0;
> +}

...

> +static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
> +				    long info)

The parameters are split in a logical way here...

(however preference is

static int ad4062_read_raw_dispatch(struct ad4062_state *st,
				    int *val, int *val2, long info)

to fit 80 characters)

...

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

...but here. Why not

static int ad4062_read_raw(struct iio_dev *indio_dev,
			   struct iio_chan_spec const *chan,
			   int *val, int *val2, long info)

?

> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (info == IIO_CHAN_INFO_SCALE)
> +		return ad4062_get_chan_scale(indio_dev, val, val2);
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = ad4062_read_raw_dispatch(st, val, val2, info);
> +
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;

	return ret ?: IIO_VAL_INT;

> +}

...

> +static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +				     unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	return ret;

Do you expand this in the following patches? If not, ret is not needed.
Just return directly.

> +}

...

> +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> +{
> +	struct device *dev = &st->i3cdev->dev;
> +	int ret;
> +
> +	ret = devm_regulator_get_enable(dev, "vio");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable vio voltage\n");
> +
> +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	*ref_sel = st->vref_uv == -ENODEV;

_uV ?

> +	if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {

You already has the second part

	if (st->vref_uV < 0 && !*ref_sel) {

I believe this is better to understand as we check that ref_sel is not chosen.

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

> +	} else if (st->vref_uv == -ENODEV) {

Redundant 'else'

	if (*ref_sel) {

(also in similar way as above)

I don't know if the above was asked specifically, but if so, I ask
the requestor(s) to reconsider.

> +		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +		if (st->vref_uv < 0)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to enable and read vdd voltage\n");
> +	} else {
> +		ret = devm_regulator_get_enable(dev, "vdd");
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable vdd regulator\n");
> +	}
> +
> +	return 0;
> +}

...

> +static int ad4062_runtime_resume(struct device *dev)
> +{
> +	struct ad4062_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
> +				AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(4000);

4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.

> +	return 0;
> +}

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops, ad4062_runtime_suspend,
> +				 ad4062_runtime_resume, NULL);

I think the logical split is slightly better:

static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
				 ad4062_runtime_suspend, ad4062_runtime_resume, NULL);

OR

static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
				 ad4062_runtime_suspend,
				 ad4062_runtime_resume,
				 NULL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] iio: adc: ad4062: Add IIO Events support

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 10:18:06AM +0100, Jorge Marques wrote:
> Adds support for IIO Events. Optionally, gp0 is assigned as Threshold
> Either signal, if not present, fallback to an I3C IBI with the same
> role.

...

> +static ssize_t ad4062_events_frequency_store(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4062_state *st = iio_priv(indio_dev);
> +	int val, ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	ret = kstrtoint(buf, 10, &val);
> +	if (ret < 0)
> +		goto out_release;
> +
> +	st->events_frequency = find_closest_descending(val, ad4062_conversion_freqs,
> +						       ARRAY_SIZE(ad4062_conversion_freqs));
> +	ret = 0;
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : len;

	return ret ?: len;

> +}

...

> +static IIO_DEVICE_ATTR(sampling_frequency, 0644, ad4062_events_frequency_show,
> +		       ad4062_events_frequency_store, 0);

IIO_DEVICE_ATTR_RW()

...

>  {
>  	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
>  
> -	if (iio_buffer_enabled(st->indio_dev))
> -		iio_trigger_poll_nested(st->trigger);
> -	else
> -		complete(&st->completion);
> +	if (st->wait_event) {
> +		iio_push_event(st->indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(st->indio_dev));
> +	} else {
> +		if (iio_buffer_enabled(st->indio_dev))
> +			iio_trigger_poll_nested(st->trigger);
> +		else
> +			complete(&st->completion);
> +	}

Less ping-pong:ish if you simply add a new code.

	if (st->wait_event) {
		iio_push_event(st->indio_dev,
			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
						    IIO_EV_TYPE_THRESH,
						    IIO_EV_DIR_EITHER),
			       iio_get_time_ns(st->indio_dev));

		return;
	}

>  }

...

> +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> +{
> +	int ret = 0;

Unneeded assignment.

> +	if (!enable) {
> +		pm_runtime_put_autosuspend(&st->i3cdev->dev);
> +		return 0;
> +	}

Just split to two functions and drop parameter 'enable',

> +	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> +	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_get_noresume(&st->i3cdev->dev);
> +	return 0;
> +}

...

> +static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event == state) {
> +		ret = 0;
> +		goto out_release;
> +	}
> +
> +	ret = ad4062_monitor_mode_enable(st, state);
> +	if (!ret)
> +		st->wait_event = state;

Please use regular patter to check for errors first.

	if (st->wait_event == state)
		ret = 0;
	else
		ret = ad4062_monitor_mode_enable(st, state);
	if (ret)
		goto out_release;

	st->wait_event = state;

Always think about readability first and then about size of the source code.

> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}

...

> +static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	if (st->wait_event) {
> +		ret = -EBUSY;
> +		goto out_release;
> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = __ad4062_read_event_info_value(st, dir, val);
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		ret = __ad4062_read_event_info_hysteresis(st, dir, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;

	return ret ?: IIO_VAL_INT;

> +}

...

> +static int __ad4062_write_event_info_value(struct ad4062_state *st,
> +					   enum iio_event_direction dir, int val)
> +{
> +	u8 reg;
> +
> +	if (val > 2047 || val < -2048)
> +		return -EINVAL;

There was already magic '11', perhaps define it and use there and here?

#define x11	11 // needs a good name

	if (val > BIT(x11) || val < -BIT(x11))

> +	if (dir == IIO_EV_DIR_RISING)
> +		reg = AD4062_REG_MAX_LIMIT;
> +	else
> +		reg = AD4062_REG_MIN_LIMIT;
> +	put_unaligned_be16(val, st->buf.bytes);
> +
> +	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> +				 sizeof(st->buf.be16));
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:
> When gp0 or gp1 is not taken as an interrupt, expose them as gpo if

GPO

> gpio-contoller is set in the devicetree.

Why can't gpio-regmap be used?

...

> +static int ad4062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct ad4062_state *st = gpiochip_get_data(gc);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4062_REG_GP_CONF, &reg_val);
> +	if (ret)
> +		return 0;

> +	if (st->gpo_irq[offset])
> +		return -ENODEV;

Consider using valid_mask instead (.init_valid_mask() callback).
Hmm... And it seems it's in place. I didn't get what is here then and
why we need to do it after accessing the HW? If there are side-effects
they must be described.

> +	if (offset)
> +		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_1, reg_val);
> +	else
> +		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_0, reg_val);
> +
> +	return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;

	return !!(reg_val == AD4062_GP_STATIC_HIGH);

also will work.

> +}

> +static int ad4062_gpio_init_valid_mask(struct gpio_chip *gc,
> +				       unsigned long *valid_mask,
> +				       unsigned int ngpios)
> +{
> +	struct ad4062_state *st = gpiochip_get_data(gc);
> +
> +	bitmap_zero(valid_mask, ngpios);
> +
> +	if (!st->gpo_irq[0])
> +		set_bit(0, valid_mask);
> +	if (!st->gpo_irq[1])
> +		set_bit(1, valid_mask);

Why atomic bit set:s?


> +	return 0;
> +}
> +
> +static int ad4062_gpio_init(struct ad4062_state *st)
> +{
> +	struct device *dev = &st->i3cdev->dev;
> +	struct gpio_chip *gc;
> +	u8 val, mask;
> +	int ret;

> +	if ((st->gpo_irq[0] && st->gpo_irq[1]) ||
> +	    !device_property_read_bool(dev, "gpio-controller"))
> +		return 0;

Do you need this? valid_mask should take care of this.

> +	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> +	if (!gc)
> +		return -ENOMEM;
> +
> +	val = 0;
> +	mask = 0;
> +	if (!st->gpo_irq[0]) {
> +		mask |= AD4062_REG_GP_CONF_MODE_MSK_0;
> +		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_STATIC_LOW);
> +	}
> +	if (!st->gpo_irq[1]) {
> +		mask |= AD4062_REG_GP_CONF_MODE_MSK_1;
> +		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_STATIC_LOW);
> +	}
> +
> +	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
> +				 mask, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, ad4062_gpio_disable, st);
> +	if (ret)
> +		return ret;
> +
> +	gc->parent = dev;
> +	gc->label = st->chip->name;
> +	gc->owner = THIS_MODULE;
> +	gc->base = -1;
> +	gc->ngpio = 2;
> +	gc->init_valid_mask = ad4062_gpio_init_valid_mask;
> +	gc->get_direction = ad4062_gpio_get_direction;
> +	gc->set = ad4062_gpio_set;
> +	gc->get = ad4062_gpio_get;
> +	gc->can_sleep = true;
> +
> +	ret = devm_gpiochip_add_data(dev, gc, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to register GPIO chip\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] dt-bindings: iio: adc: Add adi,ad4062

[Krzysztof Kozlowski]

On Mon, Nov 24, 2025 at 10:18:00AM +0100, Jorge Marques wrote:
> Add dt-bindings for AD4062 family, devices AD4060/AD4062, low-power with
> monitor capabilities SAR ADCs. Each variant of the family differs in
> resolution. The device contains two outputs (gp0, gp1). The outputs can
> be configured for range of options, such as threshold and data ready.
> The device uses a 2-wire I3C interface.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4062.yaml    | 123 +++++++++++++++++++++
>  MAINTAINERS                                        |   6 +
>  2 files changed, 129 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
> new file mode 100644
> index 0000000000000..a25af66dd64d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2024 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4062.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4062 ADC family device driver
> +
> +maintainers:
> +  - Jorge Marques <jorge.marques@analog.com>
> +
> +description: |
> +  Analog Devices AD4062 Single Channel Precision SAR ADC family
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4060.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4062.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4060
> +      - adi,ad4062
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      The interrupt pins are digital outputs that can be configured at runtime
> +      as multiple interrupt signals. Each can be configured as GP_INTR, RDY,
> +      DEV_EN, logic low, logic high and DEV_RDY (GP1 only). RDY is the
> +      active-low data ready signal, indicates when new ADC data are ready to
> +      read. DEV_EN synchronizes the enable and power-down states of signal
> +      chain devices with the ADC sampling instant. DEV_RDY is an active-high
> +      signal that indicates when the device is ready to accept serial interface
> +      communications. In GP_INTR mode, the interrupt outputs one of the
> +      threshold detection interrupt signals (MIN_INTR, MAX_INTR or either).
> +    minItems: 1

So the wire/pin can be physically disconnected?

> +    items:
> +      - description:
> +          gp0, interrupt line for GP0 pin, cannot be configured as DEV_RDY.

Write concise statements - duplicating gp0 is not helping. "GP0 pin,
cannot be configured as DEV_RDY."

"GP1 pin, can be configured to any setting."


> +      - description:
> +          gp1, interrupt line for GP1 pin, can be configured to any setting.
> +
> +  interrupt-names:

Why this is not matching interrupts in number of items?

> +    items:
> +      - const: gp0
> +      - const: gp1
> +
> +  gpio-controller:
> +    description:
> +      Marks the device node as a GPIO controller. GPs not listed in
> +      interrupt-names are exposed as a GPO.

"not listed as interrupts are..."

> +
> +  '#gpio-cells':
> +    const: 2
> +    description:
> +      The first cell is the GPIO number and the second cell specifies
> +      GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.

Best regards,
Krzysztof

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Jorge Marques]

On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > register (SAR) analog-to-digital converter (ADC) with low-power and
> > threshold monitoring modes.
> 
> ...
> 
Hi Andy,
> > +#define AD4062_SOFT_RESET	0x81
> 
> The grouping seems a bit strange. Haven't you forgotten a blank line here?
> Ditto for other similar cases.
> 
Ack.
> > +#define AD4060_MAX_AVG		0x7
> > +#define AD4062_MAX_AVG		0xB
> 
> > +#define AD4062_MON_VAL_MAX_GAIN		1999970
> 
> This is decimal...
> 
> > +#define AD4062_MON_VAL_MIDDLE_POINT	0x8000
> 
> ...and this is hexadecimal. Can you make these consistent?
> Also, is there any explanation of the number above? To me
> it looks like 2000000 - 30. Is it so? Or is this a fraction
> number multiplied by 1000000 or so? In any case some elaboration
> would be good to have.
> 
Since this is not a magic number, I will use directly below.
It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000
> > +#define AD4062_GP_DRDY		0x2
> > +#define AD4062_INTR_EN_NEITHER	0x0
> > +#define AD4062_TCONV_NS		270
> 
> ...
> 
> > +struct ad4062_state {
> > +	const struct ad4062_chip_info *chip;
> > +	const struct ad4062_bus_ops *ops;
> > +	enum ad4062_operation_mode mode;
> > +	struct completion completion;
> > +	struct iio_trigger *trigger;
> > +	struct iio_dev *indio_dev;
> > +	struct i3c_device *i3cdev;
> > +	struct regmap *regmap;
> > +	u16 sampling_frequency;
> > +	int vref_uv;
> > +	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> > +	u8 oversamp_ratio;
> > +	union {
> > +		__be32 be32;
> > +		__be16 be16;
> > +		u8 bytes[4];
> > +	} buf __aligned(IIO_DMA_MINALIGN);
> > +	u8 reg_addr_conv;
> 
> Can't we group u8:s to save a few bytes of memory?
> 
Sure

  struct ad4062_state {
  	// ...
  	union {
  		__be32 be32;
  		__be16 be16;
  		u8 bytes[4];
  	} buf __aligned(IIO_DMA_MINALIGN);
  	u16 sampling_frequency;
  	u8 oversamp_ratio;
  	u8 reg_addr_conv;
  };

> > +};
> 
> ...
> 
> > +static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
> > +{
> > +	int ret;
> > +
> > +	if (val < 1 || val > BIT(st->chip->max_avg + 1))
> 
> in_range() ?
> 
> 	in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> 
> if I am not mistaken. Also note, the GENMASK() approach makes possible
> to have all 32 bits set, however it's most unlikely to happen here anyway.
> 
Sure, but requires locals to not trigger suspicious usage of sizeof.
  	// ...
  	const u32 _max = GENMASK(st->chip->max_avg, 0);
  	const u32 _min = 1;
  	int ret;
  
  	if (in_range(val, _min, _max))
> > +		return -EINVAL;
> > +
> > +	/* 1 disables oversampling */
> > +	val = ilog2(val);
> > +	if (val == 0) {
> > +		st->mode = AD4062_SAMPLE_MODE;
> > +	} else {
> > +		st->mode = AD4062_BURST_AVERAGING_MODE;
> > +		ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	st->oversamp_ratio = BIT(val);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_get_oversampling_ratio(struct ad4062_state *st,
> > +					 unsigned int *val)
> > +{
> > +	int ret, buf;
> > +
> > +	if (st->mode == AD4062_SAMPLE_MODE) {
> > +		*val = 1;
> > +		return 0;
> > +	}
> 
> > +	ret = regmap_read(st->regmap, AD4062_REG_AVG_CONFIG, &buf);
> > +	return 0;
> 
> This is strange piece of code. Why do we have ret at all?
> Please, try to compile kernel also with `make LLVM=1 W=1 ...`
> assuming you have clang installed. It catches such issues quite
> well.
> 
Can return directly yes.
> > +}
> 
> ...
> 
> > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > +{
> > +	/* See datasheet page 31 */
> > +	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > +
> > +	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> 
> Why u64?
> 
> The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> more than 4 billions?
> 
This is necessary since at fosc 111 Hz and avg 4096 it does take longer
than 4 seconds, even though I do timeout after 1 seconds in the raw
acquisition.
> > +}
> > +
> > +static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
> > +{
> > +	for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
> 
> Why signed iterator?
> 
Ack, can be u8.
> > +		st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> > +								   st->oversamp_ratio);
> 
> Perhaps
> 
> 		st->samp_freqs[i] =
> 			ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> 						       st->oversamp_ratio);
> 
> But I am not insisting on this case and similar.
> 
> 
Ack.
> > +	return 0;
> > +}
> 
> > +static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
> > +{
> > +	*val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
> > +					      st->oversamp_ratio);
> 
> Oh, temporary variable makes this better for readability.
> 
Ack.
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_check_ids(struct ad4062_state *st)
> > +{
> 
> 	struct device *dev = &st->i3cdev->dev;
> 
Ack.
> > +	int ret;
> > +	u16 val;
> > +
> > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1,
> > +			       &st->buf.be16, sizeof(st->buf.be16));
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = get_unaligned_be16(st->buf.bytes);
> > +	if (val != st->chip->prod_id)
> > +		dev_warn(&st->i3cdev->dev,
> > +			 "Production ID x%x does not match known values", val);
> 
> 		dev_warn(dev, "Production ID x%x does not match known values", val);
> 
Ack.
> > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H,
> > +			       &st->buf.be16, sizeof(st->buf.be16));
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = get_unaligned_be16(st->buf.bytes);
> > +	if (val != AD4062_I3C_VENDOR) {
> > +		dev_err(&st->i3cdev->dev,
> > +			"Vendor ID x%x does not match expected value\n", val);
> 
> 		dev_err(dev, "Vendor ID x%x does not match expected value\n", val);
> 
Ack.
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_soft_reset(struct ad4062_state *st)
> > +{
> > +	u8 val = AD4062_SOFT_RESET;
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Wait AD4062 treset time */
> > +	fsleep(5000);
> 
> 5 * USEC_PER_MSEC
> 
> This gives a hint on the units without even a need to comment or look somewhere
> else.
>
// TODO
Since the device functional blocks are powered when voltage is supplied,
here we can stick with the treset datasheet value 60ns (ndelay(60)).
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_request_irq(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->i3cdev->dev;
> > +	int ret;
> > +
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> > +	if (ret == -EPROBE_DEFER) {
> > +		return ret;
> 
> > +	} else if (ret < 0) {
> 
> Redundant 'else'
> 
Ack.
> > +		ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> > +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
> > +					 AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
> > +	} else {
> > +		ret = devm_request_threaded_irq(dev, ret,
> > +						ad4062_irq_handler_drdy,
> > +						NULL, IRQF_ONESHOT, indio_dev->name,
> > +						indio_dev);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static const int ad4062_oversampling_avail[] = {
> > +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> 
> It's not easy to count them at glance, please add a comment with indices.
> 
Ack, will use
  static const int ad4062_oversampling_avail[] = {
          BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
  	BIT(9), BIT(10), BIT(11), BIT(12),
  };
> > +};
> 
> ...
> 
> > +static int ad4062_read_avail(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan, const int **vals,
> > +			     int *type, int *len, long mask)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		*vals = ad4062_oversampling_avail;
> > +		*len = ARRAY_SIZE(ad4062_oversampling_avail);
> > +		*type = IIO_VAL_INT;
> > +
> > +		return IIO_AVAIL_LIST;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = ad4062_populate_sampling_frequency(st);
> > +		if (ret)
> > +			return ret;
> > +		*vals = st->samp_freqs;
> > +		*len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;
> 
> Why not using positive conditional?
> 
> Funny trick that Elvis operator can be used in this case, but please don't,
> it will make code harder to follow.
> 
Ack.
> > +		*type = IIO_VAL_INT;
> > +
> > +		return IIO_AVAIL_LIST;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> ...
> 
> > +static int ad4062_get_chan_scale(struct iio_dev *indio_dev, int *val, int *val2)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, st->chip->channels);
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	*val = (st->vref_uv * 2) / MILLI;
> 
> It's most likely (MICRO / MILLI) instead of MILLI. Am I right?
> 
Sure.
> > +	*val2 = scan_type->realbits - 1; /* signed */
> > +
> > +	return IIO_VAL_FRACTIONAL_LOG2;
> > +}
> 
> ...
> 
> > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > +{
> > +	u16 gain;
> > +	int ret;
> > +
> > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > +			       &st->buf.be16, sizeof(st->buf.be16));
> > +	if (ret)
> > +		return ret;
> > +
> > +	gain = get_unaligned_be16(st->buf.bytes);
> > +
> > +	/* From datasheet: code out = code in × mon_val/0x8000 */
> > +	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;
> 
> > +	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
> > +				AD4062_MON_VAL_MIDDLE_POINT);
> 
> I don't see the need for 64-bit division. Can you elaborate what I miss here?
> 
> > +	return IIO_VAL_INT_PLUS_NANO;
> > +}
> 
Can be improved to

  static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
  {
  	int ret;
  
  	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
  			       &st->buf.be16, sizeof(st->buf.be16));
  	if (ret)
  		return ret;
  
  	/* From datasheet: code out = code in × mon_val/0x8000 */
  	*val = get_unaligned_be16(st->buf.bytes) * 2;
  	*val2 = 16;
  
  	return IIO_VAL_FRACTIONAL_LOG2;
  }
> ...
> 
> > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> 
> Forgot to wrap this line.
> 
ack
> > +{
> > +	u64 gain;
> > +	int ret;
> > +
> > +	if (gain_int < 0 || gain_frac < 0)
> > +		return -EINVAL;
> > +
> > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> 
> > +
> 
> Redundant blank line.
> 
Ack.
> > +	if (gain > AD4062_MON_VAL_MAX_GAIN)
> > +		return -EINVAL;
> > +
> > +	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > +						 MICRO),
> > +			   st->buf.bytes);
> 
> Also in doubt here about 64-bit division.
> 
This can be slightly improved to

  static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
  				      int gain_frac)
  {
  	u32 gain;
  	int ret;
  
  	if (gain_int < 0 || gain_frac < 0)
  		return -EINVAL;
  
  	gain = gain_int * MICRO + gain_frac;
  	if (gain > 1999970)
  		return -EINVAL;
  
  	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
  						 MICRO),
  			   st->buf.bytes);
  
  	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
  				&st->buf.be16, sizeof(st->buf.be16));
  	if (ret)
  		return ret;
  
  	/* Enable scale if gain is not equal to one */
  	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
  				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
  				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
  					     !(gain_int == 1 && gain_frac == 0)));
  }

To provide the enough resolution to compute every step (e.g., 0xFFFF and
0xFFFE) with the arbitrary user input.

> > +	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > +				&st->buf.be16, sizeof(st->buf.be16));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enable scale if gain is not one. */
> 
> "...is not equal to one."
> 
> Also be consistent with the style for one-line comments. Choose one and
> use it everywhere. Usual cases:
> - my one-line comment
> - My one-line comment
> - My one-line comment.
> 
Ack.
> 
> > +	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > +				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > +				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > +					     !(gain_int == 1 && gain_frac == 0)));
> > +}
> 
> ...
> 
> > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> 
> Can be named without leading double underscore? Preference is to use
> the suffix, like _no_pm (but you can find better one).
> 
Since there is one usage of this method, can be merged into ad4062_read_chan_raw.
> > +{
> > +	struct i3c_device *i3cdev = st->i3cdev;
> > +	struct i3c_priv_xfer t[2] = {
> > +		{
> > +			.data.out = &st->reg_addr_conv,
> > +			.len = sizeof(st->reg_addr_conv),
> > +			.rnw = false,
> > +		},
> > +		{
> > +			.data.in = &st->buf.be32,
> > +			.len = sizeof(st->buf.be32),
> > +			.rnw = true,
> > +		}
> > +	};
> > +	int ret;
> > +
> > +	reinit_completion(&st->completion);
> > +	/* Change address pointer to trigger conversion */
> > +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> 
> Why array? Just split them on per transfer and use separately. This gives a bit
> odd feeling that the two goes together, but no. They are semi-related as we
> have a special condition after the first one.
> 
For this commit sure, but in the next a fallback method is introduced
for when the gp1 gpio line is not connected.
There are two register to trigger and read samples:

* write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
* write CONV_TRIGGER - [conversion] -> read value -> write ...

The first allows almost twice the sampling frequency, but does not work
with the fallback because In-Band-Interrupt for CONV_READ are not
yielded.

> > +	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 = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
> > +	if (ret)
> > +		return ret;
> > +	*val = get_unaligned_be32(st->buf.bytes);
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
> > +				    long info)
> 
> The parameters are split in a logical way here...
> 
> (however preference is
> 
> static int ad4062_read_raw_dispatch(struct ad4062_state *st,
> 				    int *val, int *val2, long info)
> 
> to fit 80 characters)
> 
> ...
Ack.
> 
> > +static int ad4062_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int *val,
> > +			   int *val2, long info)
> 
> ...but here. Why not
> 
> static int ad4062_read_raw(struct iio_dev *indio_dev,
> 			   struct iio_chan_spec const *chan,
> 			   int *val, int *val2, long info)
> 
> ?
> 
Ack.
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (info == IIO_CHAN_INFO_SCALE)
> > +		return ad4062_get_chan_scale(indio_dev, val, val2);
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	ret = ad4062_read_raw_dispatch(st, val, val2, info);
> > +
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> 
> 	return ret ?: IIO_VAL_INT;
> 
> > +}
> 
> ...
Ack.
> 
> > +static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> > +				     unsigned int writeval, unsigned int *readval)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (readval)
> > +		ret = regmap_read(st->regmap, reg, readval);
> > +	else
> > +		ret = regmap_write(st->regmap, reg, writeval);
> > +
> > +	return ret;
> 
> Do you expand this in the following patches? If not, ret is not needed.
> Just return directly.
> 
Not anymore, will just return.
> > +}
> 
> ...
> 
> > +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> > +{
> > +	struct device *dev = &st->i3cdev->dev;
> > +	int ret;
> > +
> > +	ret = devm_regulator_get_enable(dev, "vio");
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to enable vio voltage\n");
> > +
> > +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	*ref_sel = st->vref_uv == -ENODEV;
> 
> _uV ?
> 
Ack.
> > +	if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {
> 
> You already has the second part
> 
> 	if (st->vref_uV < 0 && !*ref_sel) {
> 
> I believe this is better to understand as we check that ref_sel is not chosen.
> 
Ack.
> > +		return dev_err_probe(dev, st->vref_uv,
> > +				     "Failed to enable and read ref voltage\n");
> 
> > +	} else if (st->vref_uv == -ENODEV) {
> 
> Redundant 'else'
> 
> 	if (*ref_sel) {
> 
> (also in similar way as above)
> 
> I don't know if the above was asked specifically, but if so, I ask
> the requestor(s) to reconsider.
> 
Ack. Asked for returning error first, good path return 0 and not return
ret;
> > +		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> > +		if (st->vref_uv < 0)
> > +			return dev_err_probe(dev, st->vref_uv,
> > +					     "Failed to enable and read vdd voltage\n");
> > +	} else {
> > +		ret = devm_regulator_get_enable(dev, "vdd");
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to enable vdd regulator\n");
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_runtime_resume(struct device *dev)
> > +{
> > +	struct ad4062_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
> > +				AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	fsleep(4000);
> 
> 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> 
Will add
	/* Wait device functional blocks to power up */
Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
that the device is not ready to switch to acquisition mode for
conversions.
> > +	return 0;
> > +}
> 
> ...
> 
> > +static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops, ad4062_runtime_suspend,
> > +				 ad4062_runtime_resume, NULL);
> 
> I think the logical split is slightly better:
> 
> static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
> 				 ad4062_runtime_suspend, ad4062_runtime_resume, NULL);
> 
> OR
Ack.
> 
> static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
> 				 ad4062_runtime_suspend,
> 				 ad4062_runtime_resume,
> 				 NULL);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Best Regards,
Jorge

Re: [PATCH v2 5/9] iio: adc: ad4062: Add IIO Trigger support

[Jorge Marques]

On Mon, Nov 24, 2025 at 11:36:12AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 10:18:04AM +0100, Jorge Marques wrote:
Hi Andy,
> > Adds support for IIO Trigger. Optionally, gp1 is assigned as Data Ready
> > signal, if not present, fallback to an I3C IBI with the same role.
> > The software trigger is allocated by the device, but must be attached by
> > the user before enabling the buffer. The purpose is to not impede
> > removing the driver due to the increased reference count when
> > iio_trigger_set_immutable or iio_trigger_get is used.
> 
> We refer to the functions as func(). Mind the parentheses.
> 
> ...
> 
Ack.
> > +	struct ad4062_state *st = container_of(work, struct ad4062_state,
> > +					       trig_conv);
> 
> I think the
> 
> 	struct ad4062_state *st =
> 		container_of(work, struct ad4062_state, trig_conv);
> 
> reads better.
> 
Ack.
> > +	int ret;
> 
> ...
> 
> > +	/* Read current conversion, if at reg CONV_READ, stop bit triggers
> > +	 * next sample and does not need writing the address.
> > +	 */
> 
> /*
>  * The multi-line comment style is as in
>  * this example. Please, check and update.
>  */
> 
Ack.
> > +static irqreturn_t ad4062_poll_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	schedule_work(&st->trig_conv);
> > +
> > +	return IRQ_HANDLED;
> >  }
> 
> ...
> 
> > +static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4062_set_operation_mode(st, st->mode);
> > +	if (ret)
> > +		goto out_mode_error;
> > +
> > +	/* CONV_READ requires read to trigger first sample. */
> > +	struct i3c_priv_xfer t[2] = {
> > +		{
> > +			.data.out = &st->reg_addr_conv,
> > +			.len = sizeof(st->reg_addr_conv),
> > +			.rnw = false,
> > +		},
> > +		{
> > +			.data.in = &st->buf.be32,
> > +			.len = sizeof(st->buf.be32),
> > +			.rnw = true,
> > +		}
> > +	};
> > +
> > +	ret = i3c_device_do_priv_xfers(st->i3cdev, t, st->gpo_irq[1] ? 2 : 1);
> > +	if (ret)
> > +		goto out_mode_error;
> > +	return 0;
> > +
> > +out_mode_error:
> > +	pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > +
> > +	return ret;
> 
> I guess with ACQUIRE() this function will look better, because the explicit
> reference count bumping (with an associated comment) is more descriptive on
> what's going on here with PM. Same for other related functions.
> 
Oh this one slipped through my fingers, should be using ACQUIRE as well, thanks
> > +}
> 
> ...
> 
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "Failed to request i3c ibi\n");
> >  
> > +	INIT_WORK(&st->trig_conv, ad4062_trigger_work);
> 
> This is mixture of devm_*() and non-devm_*() calls. How did you (stress) test
> the removal and error paths here? Wouldn't devm-helpers.h APIs help here to
> make / keep order correct?
> 
Oh yeah, missed this helper

  	ret = devm_work_autocancel(dev, &st->trig_conv, ad4062_trigger_work);
  	if (ret)
  		return ret;

the path was missing clean-up, and did cause issue if there was work
being done on detach

  ERROR: iio:device0: Unable to get next block: Connection timed out (110)
  8<--- cut here ---
  Unable to handle kernel paging request at virtual address bf00715c when execute
  [bf00715c] *pgd=0b43f811, *pte=00000000, *ppte=00000000
  Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
  Modules linked in: ad4062 regmap_i3c nvmem_axi_sysid [last unloaded: adi_i3c_master]
  CPU: 0 UID: 0 PID: 8033 Comm: kworker/0:6 Not tainted 6.12.0+ #4
  Hardware name: Xilinx Zynq Platform
  Workqueue: events ad4062_trigger_work [ad4062]
  PC is at 0xbf00715c
  LR is at wait_for_completion_timeout+0xf0/0x118
  pc : [<bf00715c>]    lr : [<c07ec920>]    psr: 60000013
  sp : e0911ec0  ip : 00000000  fp : dfb960e0
  r10: 00000011  r9 : 00000001  r8 : c4b89d40
  r7 : c49e4500  r6 : c49da040  r5 : 00000001  r4 : e0911ef4
  r3 : cb455500  r2 : 00000000  r1 : 00000000  r0 : 00000000
  Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none

thanks,

Best regards,
Jorge
> >  	return devm_iio_device_register(dev, indio_dev);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 7/9] iio: adc: ad4062: Add IIO Events support

[Jorge Marques]

On Mon, Nov 24, 2025 at 12:33:12PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 10:18:06AM +0100, Jorge Marques wrote:
> > Adds support for IIO Events. Optionally, gp0 is assigned as Threshold
> > Either signal, if not present, fallback to an I3C IBI with the same
> > role.
> 
> ...
> 
Hi Andy,
> > +static ssize_t ad4062_events_frequency_store(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct ad4062_state *st = iio_priv(indio_dev);
> > +	int val, ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> > +
> > +	ret = kstrtoint(buf, 10, &val);
> > +	if (ret < 0)
> > +		goto out_release;
> > +
> > +	st->events_frequency = find_closest_descending(val, ad4062_conversion_freqs,
> > +						       ARRAY_SIZE(ad4062_conversion_freqs));
> > +	ret = 0;
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : len;
> 
> 	return ret ?: len;
> 
Ack
> > +}
> 
> ...
> 
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644, ad4062_events_frequency_show,
> > +		       ad4062_events_frequency_store, 0);
> 
> IIO_DEVICE_ATTR_RW()
> 
Sure
> ...
> 
> >  {
> >  	struct ad4062_state *st = i3cdev_get_drvdata(i3cdev);
> >  
> > -	if (iio_buffer_enabled(st->indio_dev))
> > -		iio_trigger_poll_nested(st->trigger);
> > -	else
> > -		complete(&st->completion);
> > +	if (st->wait_event) {
> > +		iio_push_event(st->indio_dev,
> > +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> > +						    IIO_EV_TYPE_THRESH,
> > +						    IIO_EV_DIR_EITHER),
> > +			       iio_get_time_ns(st->indio_dev));
> > +	} else {
> > +		if (iio_buffer_enabled(st->indio_dev))
> > +			iio_trigger_poll_nested(st->trigger);
> > +		else
> > +			complete(&st->completion);
> > +	}
> 
> Less ping-pong:ish if you simply add a new code.
> 
> 	if (st->wait_event) {
> 		iio_push_event(st->indio_dev,
> 			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> 						    IIO_EV_TYPE_THRESH,
> 						    IIO_EV_DIR_EITHER),
> 			       iio_get_time_ns(st->indio_dev));
> 
> 		return;
> 	}
> 
> >  }
> 
Sure.
> ...
> 
> > +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> > +{
> > +	int ret = 0;
> 
> Unneeded assignment.
Ack.
> > +	if (!enable) {
> > +		pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > +		return 0;
> > +	}
> 
> Just split to two functions and drop parameter 'enable',
>
Sure.
> > +	ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > +	ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_get_noresume(&st->i3cdev->dev);
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event == state) {
> > +		ret = 0;
> > +		goto out_release;
> > +	}
> > +
> > +	ret = ad4062_monitor_mode_enable(st, state);
> > +	if (!ret)
> > +		st->wait_event = state;
> 
> Please use regular patter to check for errors first.
> 
> 	if (st->wait_event == state)
> 		ret = 0;
> 	else
> 		ret = ad4062_monitor_mode_enable(st, state);
> 	if (ret)
> 		goto out_release;
> 
> 	st->wait_event = state;
> 
> Always think about readability first and then about size of the source code.
> 
Sure.
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> 
> ...
> 
> > +static int ad4062_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 ad4062_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +	if (st->wait_event) {
> > +		ret = -EBUSY;
> > +		goto out_release;
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		ret = __ad4062_read_event_info_value(st, dir, val);
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		ret = __ad4062_read_event_info_hysteresis(st, dir, val);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> 
> 	return ret ?: IIO_VAL_INT;
> 
> > +}
Ack.
> 
> ...
> 
> > +static int __ad4062_write_event_info_value(struct ad4062_state *st,
> > +					   enum iio_event_direction dir, int val)
> > +{
> > +	u8 reg;
> > +
> > +	if (val > 2047 || val < -2048)
> > +		return -EINVAL;
> 
> There was already magic '11', perhaps define it and use there and here?
> 
> #define x11	11 // needs a good name
> 
> 	if (val > BIT(x11) || val < -BIT(x11))
> 	
Not magic number, but max and min signed 12-bit, maybe

	if (val != sign_extend32(val, 11))
		return -EINVAL;
to not look like magic numbers, or 
  	if (val < (-BIT(11)) || val > BIT(11) - 1)
  		return -EINVAL;

For Hysteresis I will change from

	if (val >= BIT(7))
to 
	if (val & ~GENMASK(6,0))

I believe iio only passes positive to the hysteresis, but is a little clearer.

> > +	if (dir == IIO_EV_DIR_RISING)
> > +		reg = AD4062_REG_MAX_LIMIT;
> > +	else
> > +		reg = AD4062_REG_MIN_LIMIT;
> > +	put_unaligned_be16(val, st->buf.bytes);
> > +
> > +	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> > +				 sizeof(st->buf.be16));
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Best regards,
Jorge

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Jorge Marques]

On Mon, Nov 24, 2025 at 12:40:37PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:
> > When gp0 or gp1 is not taken as an interrupt, expose them as gpo if
Hi Andy,
> 
> GPO
Ack.
> 
> > gpio-contoller is set in the devicetree.
> 
> Why can't gpio-regmap be used?
> 
Because the device register values (0x5, 0x6) does not fit the gpio-regmap.
It writes the mask for high and 0 for low.
But low is 01[01] and
    high   01[10]

A different series would need to extend the gpio-regmap ops, but if you
implement your custom reg read/write, then you save at most ~5 lines...
I will add that to the commit message.
> ...
> 
> > +static int ad4062_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +	struct ad4062_state *st = gpiochip_get_data(gc);
> > +	unsigned int reg_val;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4062_REG_GP_CONF, &reg_val);
> > +	if (ret)
> > +		return 0;
Should have been
  		return ret;
> 
> > +	if (st->gpo_irq[offset])
> > +		return -ENODEV;
> 
> Consider using valid_mask instead (.init_valid_mask() callback).
> Hmm... And it seems it's in place. I didn't get what is here then and
> why we need to do it after accessing the HW? If there are side-effects
> they must be described.
> 
True, this is not necessary the valid mask does the same.
> > +	if (offset)
> > +		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_1, reg_val);
> > +	else
> > +		reg_val = FIELD_GET(AD4062_REG_GP_CONF_MODE_MSK_0, reg_val);
> > +
> > +	return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
> 
> 	return !!(reg_val == AD4062_GP_STATIC_HIGH);
> 
> also will work.
>
 	return reg_val == AD4062_GP_STATIC_HIGH;
> > +}
> 
> > +static int ad4062_gpio_init_valid_mask(struct gpio_chip *gc,
> > +				       unsigned long *valid_mask,
> > +				       unsigned int ngpios)
> > +{
> > +	struct ad4062_state *st = gpiochip_get_data(gc);
> > +
> > +	bitmap_zero(valid_mask, ngpios);
> > +
> > +	if (!st->gpo_irq[0])
> > +		set_bit(0, valid_mask);
> > +	if (!st->gpo_irq[1])
> > +		set_bit(1, valid_mask);
> 
> Why atomic bit set:s?
> 
Not needed, will use

	if (!st->gpo_irq[0])
		*valid_mask |= BIT(0);
	if (!st->gpo_irq[1])
		*valid_mask |= BIT(1);

> 
> > +	return 0;
> > +}
> > +
> > +static int ad4062_gpio_init(struct ad4062_state *st)
> > +{
> > +	struct device *dev = &st->i3cdev->dev;
> > +	struct gpio_chip *gc;
> > +	u8 val, mask;
> > +	int ret;
> 
> > +	if ((st->gpo_irq[0] && st->gpo_irq[1]) ||
> > +	    !device_property_read_bool(dev, "gpio-controller"))
> > +		return 0;
> 
> Do you need this? valid_mask should take care of this.
> 
True, this is not necessary.
> > +	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> > +	if (!gc)
> > +		return -ENOMEM;
> > +
> > +	val = 0;
> > +	mask = 0;
> > +	if (!st->gpo_irq[0]) {
> > +		mask |= AD4062_REG_GP_CONF_MODE_MSK_0;
> > +		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_STATIC_LOW);
> > +	}
> > +	if (!st->gpo_irq[1]) {
> > +		mask |= AD4062_REG_GP_CONF_MODE_MSK_1;
> > +		val |= FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_1, AD4062_GP_STATIC_LOW);
> > +	}
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4062_REG_GP_CONF,
> > +				 mask, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_add_action_or_reset(dev, ad4062_gpio_disable, st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gc->parent = dev;
> > +	gc->label = st->chip->name;
> > +	gc->owner = THIS_MODULE;
> > +	gc->base = -1;
> > +	gc->ngpio = 2;
> > +	gc->init_valid_mask = ad4062_gpio_init_valid_mask;
> > +	gc->get_direction = ad4062_gpio_get_direction;
> > +	gc->set = ad4062_gpio_set;
> > +	gc->get = ad4062_gpio_get;
> > +	gc->can_sleep = true;
> > +
> > +	ret = devm_gpiochip_add_data(dev, gc, st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Unable to register GPIO chip\n");
> > +
> > +	return 0;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Best regards,
Jorge

Re: [PATCH v2 1/9] dt-bindings: iio: adc: Add adi,ad4062

[Jorge Marques]

On Tue, Nov 25, 2025 at 10:50:59AM +0100, Krzysztof Kozlowski wrote:
> On Mon, Nov 24, 2025 at 10:18:00AM +0100, Jorge Marques wrote:
Hi Krzysztof,
> > Add dt-bindings for AD4062 family, devices AD4060/AD4062, low-power with
> > monitor capabilities SAR ADCs. Each variant of the family differs in
> > resolution. The device contains two outputs (gp0, gp1). The outputs can
> > be configured for range of options, such as threshold and data ready.
> > The device uses a 2-wire I3C interface.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad4062.yaml    | 123 +++++++++++++++++++++
> >  MAINTAINERS                                        |   6 +
> >  2 files changed, 129 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
> > new file mode 100644
> > index 0000000000000..a25af66dd64d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
> > @@ -0,0 +1,123 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2024 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4062.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD4062 ADC family device driver
> > +
> > +maintainers:
> > +  - Jorge Marques <jorge.marques@analog.com>
> > +
> > +description: |
> > +  Analog Devices AD4062 Single Channel Precision SAR ADC family
> > +
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4060.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4062.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ad4060
> > +      - adi,ad4062
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    description:
> > +      The interrupt pins are digital outputs that can be configured at runtime
> > +      as multiple interrupt signals. Each can be configured as GP_INTR, RDY,
> > +      DEV_EN, logic low, logic high and DEV_RDY (GP1 only). RDY is the
> > +      active-low data ready signal, indicates when new ADC data are ready to
> > +      read. DEV_EN synchronizes the enable and power-down states of signal
> > +      chain devices with the ADC sampling instant. DEV_RDY is an active-high
> > +      signal that indicates when the device is ready to accept serial interface
> > +      communications. In GP_INTR mode, the interrupt outputs one of the
> > +      threshold detection interrupt signals (MIN_INTR, MAX_INTR or either).
> > +    minItems: 1
> 
> So the wire/pin can be physically disconnected?
> 
Yes, the device can yield those interrupts as through I3C
In-Band-Interrupts (IBI), which are messages sent in the I3C bus by the
target (this device). I consider as a fallback, because the overhead for
those bus interrupts are much higher. The user can have all interrupts
as IBIs, and re-use the pins as generic GPOs.
> > +    items:
> > +      - description:
> > +          gp0, interrupt line for GP0 pin, cannot be configured as DEV_RDY.
> 
> Write concise statements - duplicating gp0 is not helping. "GP0 pin,
> cannot be configured as DEV_RDY."
> 
> "GP1 pin, can be configured to any setting."
> 
> 
Ok
> > +      - description:
> > +          gp1, interrupt line for GP1 pin, can be configured to any setting.
> > +
> > +  interrupt-names:
> 
> Why this is not matching interrupts in number of items?
> 
Is missing

    minItems: 1

thanks,
> > +    items:
> > +      - const: gp0
> > +      - const: gp1
> > +
> > +  gpio-controller:
> > +    description:
> > +      Marks the device node as a GPIO controller. GPs not listed in
> > +      interrupt-names are exposed as a GPO.
> 
> "not listed as interrupts are..."
> 
Ack.
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +    description:
> > +      The first cell is the GPIO number and the second cell specifies
> > +      GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
> 
> Best regards,
> Krzysztof
> 

Best regards,
Jorge

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Andy Shevchenko]

On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:

...

> > > +#define AD4062_MON_VAL_MAX_GAIN		1999970
> > 
> > This is decimal...
> > 
> > > +#define AD4062_MON_VAL_MIDDLE_POINT	0x8000
> > 
> > ...and this is hexadecimal. Can you make these consistent?
> > Also, is there any explanation of the number above? To me
> > it looks like 2000000 - 30. Is it so? Or is this a fraction
> > number multiplied by 1000000 or so? In any case some elaboration
> > would be good to have.
> > 
> Since this is not a magic number, I will use directly below.
> It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000

Okay, at least it will explain the value.

...

> > > +	if (val < 1 || val > BIT(st->chip->max_avg + 1))
> > 
> > in_range() ?
> > 
> > 	in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> > 
> > if I am not mistaken. Also note, the GENMASK() approach makes possible
> > to have all 32 bits set, however it's most unlikely to happen here anyway.
> > 
> Sure, but requires locals to not trigger suspicious usage of sizeof.
>   	// ...
>   	const u32 _max = GENMASK(st->chip->max_avg, 0);
>   	const u32 _min = 1;
>   	int ret;
>   
>   	if (in_range(val, _min, _max))
> > > +		return -EINVAL;

It's fine.

...

> > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > +{
> > > +	/* See datasheet page 31 */
> > > +	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > +
> > > +	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> > 
> > Why u64?
> > 
> > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > more than 4 billions?
> > 
> This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> than 4 seconds, even though I do timeout after 1 seconds in the raw
> acquisition.

Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
and that fits u32. Can you refactor to avoid 64-bit arithmetics?

> > > +}

...

> > > +static int ad4062_soft_reset(struct ad4062_state *st)
> > > +{
> > > +	u8 val = AD4062_SOFT_RESET;
> > > +	int ret;
> > > +
> > > +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Wait AD4062 treset time */
> > > +	fsleep(5000);
> > 
> > 5 * USEC_PER_MSEC
> > 
> > This gives a hint on the units without even a need to comment or look somewhere
> > else.
> >
> // TODO
> Since the device functional blocks are powered when voltage is supplied,
> here we can stick with the treset datasheet value 60ns (ndelay(60)).

Add a comment and it will work for me, thanks!

> > > +	return 0;
> > > +}

...

> > > +static const int ad4062_oversampling_avail[] = {
> > > +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> > 
> > It's not easy to count them at glance, please add a comment with indices.
> > 
> Ack, will use
>   static const int ad4062_oversampling_avail[] = {
>           BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
>   	BIT(9), BIT(10), BIT(11), BIT(12),
>   };

Of course you can use bit notations, but what I meant is to have

	1, 2, 4, 8, 16, 32, 64, 128,		/*  0 -  7 */
	256, 512, 1024, 2048, 4096,		/*  8 - 12 */

(or something alike).

> > > +};

...

> > > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > > +{
> > > +	u16 gain;
> > > +	int ret;
> > > +
> > > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > > +			       &st->buf.be16, sizeof(st->buf.be16));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	gain = get_unaligned_be16(st->buf.bytes);
> > > +
> > > +	/* From datasheet: code out = code in × mon_val/0x8000 */
> > > +	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;
> > 
> > > +	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
> > > +				AD4062_MON_VAL_MIDDLE_POINT);
> > 
> > I don't see the need for 64-bit division. Can you elaborate what I miss here?
> > 
> > > +	return IIO_VAL_INT_PLUS_NANO;
> > > +}
> > 
> Can be improved to
> 
>   static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
>   {
>   	int ret;
>   
>   	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
>   			       &st->buf.be16, sizeof(st->buf.be16));
>   	if (ret)
>   		return ret;
>   
>   	/* From datasheet: code out = code in × mon_val/0x8000 */
>   	*val = get_unaligned_be16(st->buf.bytes) * 2;
>   	*val2 = 16;
>   
>   	return IIO_VAL_FRACTIONAL_LOG2;
>   }

Much better, thanks!

...

> > > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> > 
> > Forgot to wrap this line.
> > 
> ack
> > > +{
> > > +	u64 gain;
> > > +	int ret;
> > > +
> > > +	if (gain_int < 0 || gain_frac < 0)
> > > +		return -EINVAL;
> > > +
> > > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > 
> > > +
> > 
> > Redundant blank line.
> > 
> Ack.
> > > +	if (gain > AD4062_MON_VAL_MAX_GAIN)
> > > +		return -EINVAL;
> > > +
> > > +	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > +						 MICRO),
> > > +			   st->buf.bytes);
> > 
> > Also in doubt here about 64-bit division.
> > 
> This can be slightly improved to
> 
>   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
>   				      int gain_frac)
>   {
>   	u32 gain;
>   	int ret;
>   
>   	if (gain_int < 0 || gain_frac < 0)
>   		return -EINVAL;
>   
>   	gain = gain_int * MICRO + gain_frac;
>   	if (gain > 1999970)

But this magic should be changed to what you explained to me
(as in 0xffff/0x8000 with the proper precision, and this
 can be done in 32-bit space).

Or even better

	if (gain_int < 0 || gain_int > 1)
		return -EINVAL;

	if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
		return -EINVAL;

>   		return -EINVAL;
>   
>   	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
>   						 MICRO),

...with temporary variable at minimum.

But again, I still don't see the need for 64-bit space.

>   			   st->buf.bytes);
>   
>   	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
>   				&st->buf.be16, sizeof(st->buf.be16));
>   	if (ret)
>   		return ret;
>   
>   	/* Enable scale if gain is not equal to one */
>   	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
>   				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
>   				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
>   					     !(gain_int == 1 && gain_frac == 0)));
>   }
> 
> To provide the enough resolution to compute every step (e.g., 0xFFFF and
> 0xFFFE) with the arbitrary user input.

...

> > > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> > 
> > Can be named without leading double underscore? Preference is to use
> > the suffix, like _no_pm (but you can find better one).
> > 
> Since there is one usage of this method, can be merged into ad4062_read_chan_raw.

Good choice!

...

> > > +	struct i3c_priv_xfer t[2] = {
> > > +		{
> > > +			.data.out = &st->reg_addr_conv,
> > > +			.len = sizeof(st->reg_addr_conv),
> > > +			.rnw = false,
> > > +		},
> > > +		{
> > > +			.data.in = &st->buf.be32,
> > > +			.len = sizeof(st->buf.be32),
> > > +			.rnw = true,
> > > +		}
> > > +	};

> > > +	/* Change address pointer to trigger conversion */
> > > +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> > 
> > Why array? Just split them on per transfer and use separately. This gives a bit
> > odd feeling that the two goes together, but no. They are semi-related as we
> > have a special condition after the first one.
> > 
> For this commit sure, but in the next a fallback method is introduced
> for when the gp1 gpio line is not connected.
> There are two register to trigger and read samples:
> 
> * write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
> * write CONV_TRIGGER - [conversion] -> read value -> write ...
> 
> The first allows almost twice the sampling frequency, but does not work
> with the fallback because In-Band-Interrupt for CONV_READ are not
> yielded.

Do you mean that the same array is reused differently? If so, then okay.

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

...

> > > +	fsleep(4000);
> > 
> > 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> > 
> Will add
> 	/* Wait device functional blocks to power up */
> Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
> that the device is not ready to switch to acquisition mode for
> conversions.

Good!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] iio: adc: ad4062: Add IIO Events support

[Andy Shevchenko]

On Wed, Nov 26, 2025 at 04:00:36PM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 12:33:12PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:06AM +0100, Jorge Marques wrote:

...

> > > +static int __ad4062_write_event_info_value(struct ad4062_state *st,
> > > +					   enum iio_event_direction dir, int val)
> > > +{
> > > +	u8 reg;
> > > +
> > > +	if (val > 2047 || val < -2048)
> > > +		return -EINVAL;
> > 
> > There was already magic '11', perhaps define it and use there and here?
> > 
> > #define x11	11 // needs a good name
> > 
> > 	if (val > BIT(x11) || val < -BIT(x11))
> > 	
> Not magic number, but max and min signed 12-bit, maybe
> 
> 	if (val != sign_extend32(val, 11))

If you go this way, the 11 still needs a definition.

> 		return -EINVAL;
> to not look like magic numbers, or 
>   	if (val < (-BIT(11)) || val > BIT(11) - 1)
>   		return -EINVAL;
> For Hysteresis I will change from
> 
> 	if (val >= BIT(7))
> to 
> 	if (val & ~GENMASK(6,0))

Not sure about this. If it's a HW-based limit, the

	val > (BIT(x) - 1)

says that this is limited by x-bit size of the register (field).

So, I leave it to Jonathan (my personal preference here is BIT(x) - 1 approach).

> I believe iio only passes positive to the hysteresis, but is a little clearer.
> 
> > > +	if (dir == IIO_EV_DIR_RISING)
> > > +		reg = AD4062_REG_MAX_LIMIT;
> > > +	else
> > > +		reg = AD4062_REG_MIN_LIMIT;
> > > +	put_unaligned_be16(val, st->buf.bytes);
> > > +
> > > +	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> > > +				 sizeof(st->buf.be16));
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Andy Shevchenko]

On Wed, Nov 26, 2025 at 04:55:41PM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 12:40:37PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:

...

> > Why can't gpio-regmap be used?
> > 
> Because the device register values (0x5, 0x6) does not fit the gpio-regmap.
> It writes the mask for high and 0 for low.
> But low is 01[01] and
>     high   01[10]
> 
> A different series would need to extend the gpio-regmap ops, but if you
> implement your custom reg read/write, then you save at most ~5 lines...
> I will add that to the commit message.

OK.

...

> > > +	return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
> > 
> > 	return !!(reg_val == AD4062_GP_STATIC_HIGH);
> > 
> > also will work.
> >
>  	return reg_val == AD4062_GP_STATIC_HIGH;

Hmm... This will include implicit bool->int. The !! guarantees values 0 or 1,
but I don't remember about implicit bool->int case.

...

> > > +static int ad4062_gpio_init_valid_mask(struct gpio_chip *gc,
> > > +				       unsigned long *valid_mask,
> > > +				       unsigned int ngpios)
> > > +{
> > > +	struct ad4062_state *st = gpiochip_get_data(gc);
> > > +
> > > +	bitmap_zero(valid_mask, ngpios);
> > > +
> > > +	if (!st->gpo_irq[0])
> > > +		set_bit(0, valid_mask);
> > > +	if (!st->gpo_irq[1])
> > > +		set_bit(1, valid_mask);
> > 
> > Why atomic bit set:s?
> > 
> Not needed, will use

Note, bitops are xxx_bit() -- atomic, __xxx_bit() -- non-atomic,
that's what I had in mind.

> 	if (!st->gpo_irq[0])
> 		*valid_mask |= BIT(0);
> 	if (!st->gpo_irq[1])
> 		*valid_mask |= BIT(1);

Can't it be rather something like

	for (unsigned int i = 0; i < ...; i++)
		__assign_bit(i, valid_mask, st->gpo_irq[i]);

?
This shorter and does the same independently on the length of the bitmask
(and effectively the array size of gpo_irq)

> > > +	return 0;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Jorge Marques]

On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> 
> ...
> 
> > > > +#define AD4062_MON_VAL_MAX_GAIN		1999970
> > > 
> > > This is decimal...
> > > 
> > > > +#define AD4062_MON_VAL_MIDDLE_POINT	0x8000
> > > 
> > > ...and this is hexadecimal. Can you make these consistent?
> > > Also, is there any explanation of the number above? To me
> > > it looks like 2000000 - 30. Is it so? Or is this a fraction
> > > number multiplied by 1000000 or so? In any case some elaboration
> > > would be good to have.
> > > 
> > Since this is not a magic number, I will use directly below.
> > It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000
> 
> Okay, at least it will explain the value.
> 
> ...
> 
> > > > +	if (val < 1 || val > BIT(st->chip->max_avg + 1))
> > > 
> > > in_range() ?
> > > 
> > > 	in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> > > 
> > > if I am not mistaken. Also note, the GENMASK() approach makes possible
> > > to have all 32 bits set, however it's most unlikely to happen here anyway.
> > > 
> > Sure, but requires locals to not trigger suspicious usage of sizeof.
> >   	// ...
> >   	const u32 _max = GENMASK(st->chip->max_avg, 0);
> >   	const u32 _min = 1;
> >   	int ret;
> >   
> >   	if (in_range(val, _min, _max))
> > > > +		return -EINVAL;
> 
> It's fine.
> 
> ...
> 
> > > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > > +{
> > > > +	/* See datasheet page 31 */
> > > > +	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > > +
> > > > +	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> > > 
> > > Why u64?
> > > 
> > > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > > more than 4 billions?
> > > 
> > This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> > than 4 seconds, even though I do timeout after 1 seconds in the raw
> > acquisition.
> 
> Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
> and that fits u32. Can you refactor to avoid 64-bit arithmetics?
>

Ok, any frequency lower than 1 Hz does not make sense.

  static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
  {
  	/* See datasheet page 31 */
  	u32 period = NSEC_PER_SEC / fosc;
  	u32 n_avg = BIT(oversamp_ratio) - 1;
  
  	/* Result is less than 1 Hz */
  	if (n_avg >= fosc)
  		return 1;
  	return NSEC_PER_SEC / (n_avg * period + AD4062_TCONV_NS);
  }

> > > > +}
> 
> ...
> 
> > > > +static int ad4062_soft_reset(struct ad4062_state *st)
> > > > +{
> > > > +	u8 val = AD4062_SOFT_RESET;
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* Wait AD4062 treset time */
> > > > +	fsleep(5000);
> > > 
> > > 5 * USEC_PER_MSEC
> > > 
> > > This gives a hint on the units without even a need to comment or look somewhere
> > > else.
> > >
> > // TODO
> > Since the device functional blocks are powered when voltage is supplied,
> > here we can stick with the treset datasheet value 60ns (ndelay(60)).
> 
> Add a comment and it will work for me, thanks!
> 
> > > > +	return 0;
> > > > +}
> 
> ...
> 
> > > > +static const int ad4062_oversampling_avail[] = {
> > > > +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> > > 
> > > It's not easy to count them at glance, please add a comment with indices.
> > > 
> > Ack, will use
> >   static const int ad4062_oversampling_avail[] = {
> >           BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
> >   	BIT(9), BIT(10), BIT(11), BIT(12),
> >   };
> 
> Of course you can use bit notations, but what I meant is to have
> 
> 	1, 2, 4, 8, 16, 32, 64, 128,		/*  0 -  7 */
> 	256, 512, 1024, 2048, 4096,		/*  8 - 12 */
> 
> (or something alike).
> 
Ack
> > > > +};
> 
> ...
> 
> > > > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > > > +{
> > > > +	u16 gain;
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > > > +			       &st->buf.be16, sizeof(st->buf.be16));
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	gain = get_unaligned_be16(st->buf.bytes);
> > > > +
> > > > +	/* From datasheet: code out = code in × mon_val/0x8000 */
> > > > +	*val = gain / AD4062_MON_VAL_MIDDLE_POINT;
> > > 
> > > > +	*val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
> > > > +				AD4062_MON_VAL_MIDDLE_POINT);
> > > 
> > > I don't see the need for 64-bit division. Can you elaborate what I miss here?
> > > 
> > > > +	return IIO_VAL_INT_PLUS_NANO;
> > > > +}
> > > 
> > Can be improved to
> > 
> >   static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> >   {
> >   	int ret;
> >   
> >   	ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> >   			       &st->buf.be16, sizeof(st->buf.be16));
> >   	if (ret)
> >   		return ret;
> >   
> >   	/* From datasheet: code out = code in × mon_val/0x8000 */
> >   	*val = get_unaligned_be16(st->buf.bytes) * 2;
> >   	*val2 = 16;
> >   
> >   	return IIO_VAL_FRACTIONAL_LOG2;
> >   }
> 
> Much better, thanks!
> 
> ...
> 
> > > > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> > > 
> > > Forgot to wrap this line.
> > > 
> > ack
> > > > +{
> > > > +	u64 gain;
> > > > +	int ret;
> > > > +
> > > > +	if (gain_int < 0 || gain_frac < 0)
> > > > +		return -EINVAL;
> > > > +
> > > > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > > 
> > > > +
> > > 
> > > Redundant blank line.
> > > 
> > Ack.
> > > > +	if (gain > AD4062_MON_VAL_MAX_GAIN)
> > > > +		return -EINVAL;
> > > > +
> > > > +	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > > +						 MICRO),
> > > > +			   st->buf.bytes);
> > > 
> > > Also in doubt here about 64-bit division.
> > > 
> > This can be slightly improved to
> > 
> >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> >   				      int gain_frac)
> >   {
> >   	u32 gain;
> >   	int ret;
> >   
> >   	if (gain_int < 0 || gain_frac < 0)
> >   		return -EINVAL;
> >   
> >   	gain = gain_int * MICRO + gain_frac;
> >   	if (gain > 1999970)
> 
> But this magic should be changed to what you explained to me
> (as in 0xffff/0x8000 with the proper precision, and this
>  can be done in 32-bit space).
> 
> Or even better
> 
> 	if (gain_int < 0 || gain_int > 1)
> 		return -EINVAL;
> 
> 	if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> 		return -EINVAL;
> 
gain_frac would be 999999 max, or 999970 for the limit that fits in the
register after the math. I think > 1.999.970 is self explanatory.
> >   		return -EINVAL;
> >   
> >   	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> >   						 MICRO),
> 
> ...with temporary variable at minimum.
> 
> But again, I still don't see the need for 64-bit space.
> 

Well, by dividing mon_val and micro values by a common divisor the
operation fit in 32-bits:

  static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
                                        int gain_frac)
  {
          const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
          const u32 micro = MICRO / 64;
          const u32 gain = gain_int * MICRO + gain_frac;
          int ret;

          if (gain_int < 0 || gain_frac < 0)
                  return -EINVAL;

          if (gain > 1999970)
                  return -EINVAL;

          put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);

          ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
                                  &st->buf.be16, sizeof(st->buf.be16));
          if (ret)
                  return ret;

          /* Enable scale if gain is not equal to one */
          return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
                                    AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
                                    FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
                                               !(gain_int == 1 && gain_frac == 0)));
  }


> >   			   st->buf.bytes);
> >   
> >   	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> >   				&st->buf.be16, sizeof(st->buf.be16));
> >   	if (ret)
> >   		return ret;
> >   
> >   	/* Enable scale if gain is not equal to one */
> >   	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> >   				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> >   				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> >   					     !(gain_int == 1 && gain_frac == 0)));
> >   }
> > 
> > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > 0xFFFE) with the arbitrary user input.
> 
> ...
> 
> > > > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> > > 
> > > Can be named without leading double underscore? Preference is to use
> > > the suffix, like _no_pm (but you can find better one).
> > > 
> > Since there is one usage of this method, can be merged into ad4062_read_chan_raw.
> 
> Good choice!
> 
> ...
> 
> > > > +	struct i3c_priv_xfer t[2] = {
> > > > +		{
> > > > +			.data.out = &st->reg_addr_conv,
> > > > +			.len = sizeof(st->reg_addr_conv),
> > > > +			.rnw = false,
> > > > +		},
> > > > +		{
> > > > +			.data.in = &st->buf.be32,
> > > > +			.len = sizeof(st->buf.be32),
> > > > +			.rnw = true,
> > > > +		}
> > > > +	};
> 
> > > > +	/* Change address pointer to trigger conversion */
> > > > +	ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> > > 
> > > Why array? Just split them on per transfer and use separately. This gives a bit
> > > odd feeling that the two goes together, but no. They are semi-related as we
> > > have a special condition after the first one.
> > > 
> > For this commit sure, but in the next a fallback method is introduced
> > for when the gp1 gpio line is not connected.
> > There are two register to trigger and read samples:
> > 
> > * write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
> > * write CONV_TRIGGER - [conversion] -> read value -> write ...
> > 
> > The first allows almost twice the sampling frequency, but does not work
> > with the fallback because In-Band-Interrupt for CONV_READ are not
> > yielded.
> 
> Do you mean that the same array is reused differently? If so, then okay.
> 
Yes
> > > > +	if (ret)
> > > > +		return ret;
> 
> ...
> 
> > > > +	fsleep(4000);
> > > 
> > > 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> > > 
> > Will add
> > 	/* Wait device functional blocks to power up */
> > Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
> > that the device is not ready to switch to acquisition mode for
> > conversions.
> 
> Good!
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Best regards,
Jorge

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Andy Shevchenko]

On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:

Please, remove the context you are agree with or which has no need
to be answered, it helps to parse and reply.

...

> > > > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > > > +{
> > > > > +	/* See datasheet page 31 */
> > > > > +	u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > > > +
> > > > > +	return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> > > > 
> > > > Why u64?
> > > > 
> > > > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > > > more than 4 billions?
> > > > 
> > > This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> > > than 4 seconds, even though I do timeout after 1 seconds in the raw
> > > acquisition.
> > 
> > Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
> > and that fits u32. Can you refactor to avoid 64-bit arithmetics?
> 
> Ok, any frequency lower than 1 Hz does not make sense.

Depends on the cases, we have sub-Hz sensors or some other stuff.
So, "...does not make sense in _this_ case." That's what I implied.

>   static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)

Shouldn't fosc be unsigned?

>   {
>   	/* See datasheet page 31 */

It's fine, but better to add a formula here or more information about
the calculations done in the function.

>   	u32 period = NSEC_PER_SEC / fosc;

period_ns ?

(We usually add units to this kind of variables for better understanding
 of the calculations)

>   	u32 n_avg = BIT(oversamp_ratio) - 1;
>   
>   	/* Result is less than 1 Hz */
>   	if (n_avg >= fosc)
>   		return 1;

+ blank line.

>   	return NSEC_PER_SEC / (n_avg * period + AD4062_TCONV_NS);
>   }

LGTM, thanks!

> > > > > +}

...

> > >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > >   				      int gain_frac)
> > >   {
> > >   	u32 gain;
> > >   	int ret;
> > >   
> > >   	if (gain_int < 0 || gain_frac < 0)
> > >   		return -EINVAL;
> > >   
> > >   	gain = gain_int * MICRO + gain_frac;
> > >   	if (gain > 1999970)
> > 
> > But this magic should be changed to what you explained to me
> > (as in 0xffff/0x8000 with the proper precision, and this
> >  can be done in 32-bit space).
> > 
> > Or even better
> > 
> > 	if (gain_int < 0 || gain_int > 1)
> > 		return -EINVAL;
> > 
> > 	if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> > 		return -EINVAL;

> gain_frac would be 999999 max, or 999970 for the limit that fits in the
> register after the math. I think > 1.999.970 is self explanatory.

On the place of unprepared reader this is a complete magic number without
scale, without understanding where it came from, etc.

So, can you define it as a derivative from the other constants and with
a comment perhaps?

> > >   		return -EINVAL;
> > >   
> > >   	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > >   						 MICRO),
> > 
> > ...with temporary variable at minimum.
> > 
> > But again, I still don't see the need for 64-bit space.
> 
> Well, by dividing mon_val and micro values by a common divisor the
> operation fit in 32-bits:
> 
>   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
>                                         int gain_frac)
>   {

	/* Divide numerator and denumerator by known great common divider */

>           const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
>           const u32 micro = MICRO / 64;

Yep, I suggested the same in another patch under review (not yours) for
the similar cases where we definitely may easily avoid overflow.

Alternatively you can use gcd().

>           const u32 gain = gain_int * MICRO + gain_frac;
>           int ret;
> 
>           if (gain_int < 0 || gain_frac < 0)
>                   return -EINVAL;
> 
>           if (gain > 1999970)
>                   return -EINVAL;
> 
>           put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
> 
>           ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
>                                   &st->buf.be16, sizeof(st->buf.be16));
>           if (ret)
>                   return ret;
> 
>           /* Enable scale if gain is not equal to one */
>           return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
>                                     AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
>                                     FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
>                                                !(gain_int == 1 && gain_frac == 0)));

Btw, I think you can move this check up and save in a temporary variable which
might affect the binary size of the compiled object as accesses to the gain_int
and gain_frac will be grouped in the same place with potential of the reusing
the CPU register(s)..

>   }

> > >   			   st->buf.bytes);
> > >   
> > >   	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > >   				&st->buf.be16, sizeof(st->buf.be16));
> > >   	if (ret)
> > >   		return ret;
> > >   
> > >   	/* Enable scale if gain is not equal to one */
> > >   	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > >   				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > >   				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > >   					     !(gain_int == 1 && gain_frac == 0)));
> > >   }
> > > 
> > > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > > 0xFFFE) with the arbitrary user input.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Jorge Marques]

On Fri, Nov 28, 2025 at 09:25:50PM +0200, Andy Shevchenko wrote:
Hi Andy,

> On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> > On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > > > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> 
> >   static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
> 
> Shouldn't fosc be unsigned?
> 
Yep
> >   {
> >   	/* See datasheet page 31 */
> 
> It's fine, but better to add a formula here or more information about
> the calculations done in the function.
> 
> >   	u32 period = NSEC_PER_SEC / fosc;
> 
> period_ns ?
> 
> (We usually add units to this kind of variables for better understanding
>  of the calculations)
> 
Ack.
> > > >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > >   				      int gain_frac)
> > > >   {
> > > >   ...
> > > >   
> > > >   	if (gain > 1999970)
> > > 
> > > But this magic should be changed to what you explained to me
> > > (as in 0xffff/0x8000 with the proper precision, and this
> > >  can be done in 32-bit space).
> > > 
> > > Or even better
> > > 
> > > 	if (gain_int < 0 || gain_int > 1)
> > > 		return -EINVAL;
> > > 
> > > 	if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> > > 		return -EINVAL;
> 
> > gain_frac would be 999999 max, or 999970 for the limit that fits in the
> > register after the math. I think > 1.999.970 is self explanatory.
> 
> On the place of unprepared reader this is a complete magic number without
> scale, without understanding where it came from, etc.
> 
> So, can you define it as a derivative from the other constants and with
> a comment perhaps?
> 
(my proposal is after all your comments below)
> > > >   		return -EINVAL;
> > > >   
> > > >   	put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > >   						 MICRO),
> > > 
> > > ...with temporary variable at minimum.
> > > 
> > > But again, I still don't see the need for 64-bit space.
> > 
> > Well, by dividing mon_val and micro values by a common divisor the
> > operation fit in 32-bits:
> > 
> >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> >                                         int gain_frac)
> >   {
> 
> 	/* Divide numerator and denumerator by known great common divider */
> 
> >           const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
> >           const u32 micro = MICRO / 64;
> 
> Yep, I suggested the same in another patch under review (not yours) for
> the similar cases where we definitely may easily avoid overflow.
> 
> Alternatively you can use gcd().
> 
> >           put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
> 
> Btw, I think you can move this check up and save in a temporary variable which
> might affect the binary size of the compiled object as accesses to the gain_int
> and gain_frac will be grouped in the same place with potential of the reusing
> the CPU register(s)..
> 
> >   }
> 
I believe this is clear and fits all points:

 	/* Divide numerator and denumerator by known great common divider */
	const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
	const u32 micro = MICRO / 64;
	const u32 gain_fp = gain_int * MICRO + gain_frac;
	const u32 reg_val = DIV_ROUND_CLOSEST(gain_fp * mon_val, micro);
	int ret;

	/* Checks if the gain is in range and the value fits the field */
	if (gain_int < 0 || gain_int > 1 || reg_val > BIT(16) - 1)
		return -EINVAL;

	put_unaligned_be16(reg_val, st->buf.bytes);

Explains 64 value. Checks if is in range [0, 2), then if fits the
register field for corner case of range (1.999970, 2) (0x10000). Full
formula is in the previous method ad4062_get_chan_calibscale.


> > > >   			   st->buf.bytes);
> > > >   
> > > >   	ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > > >   				&st->buf.be16, sizeof(st->buf.be16));
> > > >   	if (ret)
> > > >   		return ret;
> > > >   
> > > >   	/* Enable scale if gain is not equal to one */
> > > >   	return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > > >   				  AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > >   				  FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > >   					     !(gain_int == 1 && gain_frac == 0)));
> > > >   }
> > > > 
> > > > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > > > 0xFFFE) with the arbitrary user input.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Best Regards,
Jorge

Re: [PATCH v2 7/9] iio: adc: ad4062: Add IIO Events support

[Jorge Marques]

On Thu, Nov 27, 2025 at 11:13:28AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 26, 2025 at 04:00:36PM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 12:33:12PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 24, 2025 at 10:18:06AM +0100, Jorge Marques wrote:
> 
> ...
> 
Hi Andy,
> > > > +static int __ad4062_write_event_info_value(struct ad4062_state *st,
> > > > +					   enum iio_event_direction dir, int val)
> > > > +{
> > > > +	u8 reg;
> > > > +
> > > > +	if (val > 2047 || val < -2048)
> > > > +		return -EINVAL;
> > > 
> > > There was already magic '11', perhaps define it and use there and here?
> > > 
> > > #define x11	11 // needs a good name
> > > 
> > > 	if (val > BIT(x11) || val < -BIT(x11))
> > > 	
> > Not magic number, but max and min signed 12-bit, maybe
> > 
> > 	if (val != sign_extend32(val, 11))
> 
> If you go this way, the 11 still needs a definition.
> 
Sure, I will go with AD4062_LIMIT_BITS, then usage 
	*val = sign_extend32(get_unaligned_be16(st->buf.bytes),
			     AD4062_LIMIT_BITS-1);
> > 		return -EINVAL;
> > to not look like magic numbers, or 
> >   	if (val < (-BIT(11)) || val > BIT(11) - 1)
> >   		return -EINVAL;
> > For Hysteresis I will change from
> > 
> > 	if (val >= BIT(7))
> > to 
> > 	if (val & ~GENMASK(6,0))
> 
> Not sure about this. If it's a HW-based limit, the
> 
> 	val > (BIT(x) - 1)
> 
> says that this is limited by x-bit size of the register (field).
> 
> So, I leave it to Jonathan (my personal preference here is BIT(x) - 1 approach).
> 
I don't have a preferance, will use BIT(7) - 1
> > I believe iio only passes positive to the hysteresis, but is a little clearer.
> > 
> > > > +	if (dir == IIO_EV_DIR_RISING)
> > > > +		reg = AD4062_REG_MAX_LIMIT;
> > > > +	else
> > > > +		reg = AD4062_REG_MIN_LIMIT;
> > > > +	put_unaligned_be16(val, st->buf.bytes);
> > > > +
> > > > +	return regmap_bulk_write(st->regmap, reg, &st->buf.be16,
> > > > +				 sizeof(st->buf.be16));
> > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Best Regards,
Jorge

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Jorge Marques]

On Thu, Nov 27, 2025 at 11:20:54AM +0200, Andy Shevchenko wrote:
Hi Andy,
> On Wed, Nov 26, 2025 at 04:55:41PM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 12:40:37PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:
> 
> ...
> 
> > > > +	return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
> > > 
> > > 	return !!(reg_val == AD4062_GP_STATIC_HIGH);
> > > 
> > > also will work.
> > >
> >  	return reg_val == AD4062_GP_STATIC_HIGH;
> 
> Hmm... This will include implicit bool->int. The !! guarantees values 0 or 1,
> but I don't remember about implicit bool->int case.
> 
> ...
I don't think the implicit bool->int is an issue, grepping `return .* == .*;`
matches a few methods that return int.
Experimenting with the _Bool type (gcc 15, clang 19, any std version),

	int main()
	{
	    int a = 1;
	    int b = 2;
	
	    return (_Bool)(a == b);
	}

with
gcc -Wall -W -pedantic -std=c23 -c test.c
clang -Wall -Wextra -Wbool-conversion -std=c11 -O2 test.c

also doesn't raise warnings.

> 
> > > > +static int ad4062_gpio_init_valid_mask(struct gpio_chip *gc,
> > > > +				       unsigned long *valid_mask,
> > > > +				       unsigned int ngpios)
> > > > +{
> > > > +	struct ad4062_state *st = gpiochip_get_data(gc);
> > > > +
> > > > +	bitmap_zero(valid_mask, ngpios);
> > > > +
> > > > +	if (!st->gpo_irq[0])
> > > > +		set_bit(0, valid_mask);
> > > > +	if (!st->gpo_irq[1])
> > > > +		set_bit(1, valid_mask);
> > > 
> > > Why atomic bit set:s?
> > > 
> > Not needed, will use
> 
> Note, bitops are xxx_bit() -- atomic, __xxx_bit() -- non-atomic,
> that's what I had in mind.
> 
> > 	if (!st->gpo_irq[0])
> > 		*valid_mask |= BIT(0);
> > 	if (!st->gpo_irq[1])
> > 		*valid_mask |= BIT(1);
> 
> Can't it be rather something like
> 
> 	for (unsigned int i = 0; i < ...; i++)
> 		__assign_bit(i, valid_mask, st->gpo_irq[i]);
> 
> ?
> This shorter and does the same independently on the length of the bitmask
> (and effectively the array size of gpo_irq)
> 
Sure, just
 		__assign_bit(i, valid_mask, !st->gpo_irq[i]);

"Set as valid gpo if not used as irq"
> > > > +	return 0;
> > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
Best Regards,
Jorge

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Andy Shevchenko]

On Thu, Dec 4, 2025 at 11:38 PM Jorge Marques <gastmaier@gmail.com> wrote:
> On Thu, Nov 27, 2025 at 11:20:54AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 04:55:41PM +0100, Jorge Marques wrote:
> > > On Mon, Nov 24, 2025 at 12:40:37PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:

...

> > > > > +       return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
> > > >
> > > >   return !!(reg_val == AD4062_GP_STATIC_HIGH);
> > > >
> > > > also will work.
> > > >
> > >     return reg_val == AD4062_GP_STATIC_HIGH;
> >
> > Hmm... This will include implicit bool->int. The !! guarantees values 0 or 1,
> > but I don't remember about implicit bool->int case.

> I don't think the implicit bool->int is an issue, grepping `return .* == .*;`
> matches a few methods that return int.

Yes, the Q here is the value of true _always_ be promoted to 1?

> Experimenting with the _Bool type (gcc 15, clang 19, any std version),
>
>         int main()
>         {
>             int a = 1;
>             int b = 2;
>
>             return (_Bool)(a == b);
>         }
>
> with
> gcc -Wall -W -pedantic -std=c23 -c test.c
> clang -Wall -Wextra -Wbool-conversion -std=c11 -O2 test.c
>
> also doesn't raise warnings.

Of course, because before even looking into warnings the entire code
degrades to return 0. I.o.w., the test case is not correct. But don't
hurry up to fix it, you won't get warnings anyway, it's all about C
standard and not about (in)correctness of the code. See above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Andy Shevchenko]

On Thu, Dec 4, 2025 at 11:37 PM Jorge Marques <gastmaier@gmail.com> wrote:
> On Fri, Nov 28, 2025 at 09:25:50PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 28, 2025 at 07:50:02PM +0100, Jorge Marques wrote:
> > > On Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> > > > > On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:

...

> > > > >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > > > >                                       int gain_frac)
> > > > >   {
> > > > >   ...
> > > > >
> > > > >         if (gain > 1999970)
> > > >
> > > > But this magic should be changed to what you explained to me
> > > > (as in 0xffff/0x8000 with the proper precision, and this
> > > >  can be done in 32-bit space).
> > > >
> > > > Or even better
> > > >
> > > >   if (gain_int < 0 || gain_int > 1)
> > > >           return -EINVAL;
> > > >
> > > >   if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
> > > >           return -EINVAL;
> >
> > > gain_frac would be 999999 max, or 999970 for the limit that fits in the
> > > register after the math. I think > 1.999.970 is self explanatory.
> >
> > On the place of unprepared reader this is a complete magic number without
> > scale, without understanding where it came from, etc.
> >
> > So, can you define it as a derivative from the other constants and with
> > a comment perhaps?
> >
> (my proposal is after all your comments below)
> > > > >                 return -EINVAL;
> > > > >
> > > > >         put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > > >                                                  MICRO),
> > > >
> > > > ...with temporary variable at minimum.
> > > >
> > > > But again, I still don't see the need for 64-bit space.
> > >
> > > Well, by dividing mon_val and micro values by a common divisor the
> > > operation fit in 32-bits:
> > >
> > >   static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> > >                                         int gain_frac)
> > >   {
> >
> >       /* Divide numerator and denumerator by known great common divider */
> >
> > >           const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
> > >           const u32 micro = MICRO / 64;
> >
> > Yep, I suggested the same in another patch under review (not yours) for
> > the similar cases where we definitely may easily avoid overflow.
> >
> > Alternatively you can use gcd().
> >
> > >           put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), st->buf.bytes);
> >
> > Btw, I think you can move this check up and save in a temporary variable which
> > might affect the binary size of the compiled object as accesses to the gain_int
> > and gain_frac will be grouped in the same place with potential of the reusing
> > the CPU register(s)..
> >
> > >   }
> >
> I believe this is clear and fits all points:
>
>         /* Divide numerator and denumerator by known great common divider */
>         const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
>         const u32 micro = MICRO / 64;
>         const u32 gain_fp = gain_int * MICRO + gain_frac;
>         const u32 reg_val = DIV_ROUND_CLOSEST(gain_fp * mon_val, micro);
>         int ret;
>
>         /* Checks if the gain is in range and the value fits the field */
>         if (gain_int < 0 || gain_int > 1 || reg_val > BIT(16) - 1)
>                 return -EINVAL;
>
>         put_unaligned_be16(reg_val, st->buf.bytes);
>
> Explains 64 value. Checks if is in range [0, 2), then if fits the
> register field for corner case of range (1.999970, 2) (0x10000). Full
> formula is in the previous method ad4062_get_chan_calibscale.

Yes, LGTM now, thanks.

> > > > >                            st->buf.bytes);
> > > > >
> > > > >         ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> > > > >                                 &st->buf.be16, sizeof(st->buf.be16));
> > > > >         if (ret)
> > > > >                 return ret;
> > > > >
> > > > >         /* Enable scale if gain is not equal to one */
> > > > >         return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> > > > >                                   AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > > >                                   FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> > > > >                                              !(gain_int == 1 && gain_frac == 0)));
> > > > >   }
> > > > >
> > > > > To provide the enough resolution to compute every step (e.g., 0xFFFF and
> > > > > 0xFFFE) with the arbitrary user input.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Jorge Marques]

On Fri, Dec 05, 2025 at 12:21:31AM +0200, Andy Shevchenko wrote:
> On Thu, Dec 4, 2025 at 11:38 PM Jorge Marques <gastmaier@gmail.com> wrote:
> > On Thu, Nov 27, 2025 at 11:20:54AM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 26, 2025 at 04:55:41PM +0100, Jorge Marques wrote:
> > > > On Mon, Nov 24, 2025 at 12:40:37PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:
> 
> ...
> 
> > > > > > +       return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
> > > > >
> > > > >   return !!(reg_val == AD4062_GP_STATIC_HIGH);
> > > > >
> > > > > also will work.
> > > > >
> > > >     return reg_val == AD4062_GP_STATIC_HIGH;
> > >
> > > Hmm... This will include implicit bool->int. The !! guarantees values 0 or 1,
> > > but I don't remember about implicit bool->int case.
> 
> > I don't think the implicit bool->int is an issue, grepping `return .* == .*;`
> > matches a few methods that return int.
> 
> Yes, the Q here is the value of true _always_ be promoted to 1?
> 
Hi Andy,

The relational operator result has type int (c99 6.5.9 Equality
operators); and when any scalar value is converted to _Bool, the result
is 0 if the value compares equal to 0; otherwise, the result is 1 (c99
6.3.1.2).
https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf

No conversion warnings even when forcing _Bool type.
There are many usages like this, for example:

drivers/iio/accel/adxl313_core.c @ int adxl313_is_act_inact_ac()
drivers/iio/light/opt4060.c @ int opt4060_read_event_config()
drivers/iio/light/tsl2772.c @ int tsl2772_device_id_verify()
lib/zstd/compress/zstd_fast.c @ int ZSTD_match4Found_branch()

I cannot find many legitimate usage of relational operator with the
double negation.
  git ls-files | xargs grep -s 'return !!' | grep '=='

> > Experimenting with the _Bool type (gcc 15, clang 19, any std version),
> >
> >         int main()
> >         {
> >             int a = 1;
> >             int b = 2;
> >
> >             return (_Bool)(a == b);
> >         }
> >
> > with
> > gcc -Wall -W -pedantic -std=c23 -c test.c
> > clang -Wall -Wextra -Wbool-conversion -std=c11 -O2 test.c
> >
> > also doesn't raise warnings.
> 
> Of course, because before even looking into warnings the entire code
> degrades to return 0. I.o.w., the test case is not correct. But don't
> hurry up to fix it, you won't get warnings anyway, it's all about C
> standard and not about (in)correctness of the code. See above.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
Best Regards,
Jorge

Re: [PATCH v2 9/9] iio: adc: ad4062: Add GPIO Controller support

[Andy Shevchenko]

On Fri, Dec 5, 2025 at 1:53 PM Jorge Marques <gastmaier@gmail.com> wrote:
> On Fri, Dec 05, 2025 at 12:21:31AM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 4, 2025 at 11:38 PM Jorge Marques <gastmaier@gmail.com> wrote:
> > > On Thu, Nov 27, 2025 at 11:20:54AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 26, 2025 at 04:55:41PM +0100, Jorge Marques wrote:
> > > > > On Mon, Nov 24, 2025 at 12:40:37PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Nov 24, 2025 at 10:18:08AM +0100, Jorge Marques wrote:

...

> > > > > > > +       return reg_val == AD4062_GP_STATIC_HIGH ? 1 : 0;
> > > > > >
> > > > > >   return !!(reg_val == AD4062_GP_STATIC_HIGH);
> > > > > >
> > > > > > also will work.
> > > > > >
> > > > >     return reg_val == AD4062_GP_STATIC_HIGH;
> > > >
> > > > Hmm... This will include implicit bool->int. The !! guarantees values 0 or 1,
> > > > but I don't remember about implicit bool->int case.
> >
> > > I don't think the implicit bool->int is an issue, grepping `return .* == .*;`
> > > matches a few methods that return int.
> >
> > Yes, the Q here is the value of true _always_ be promoted to 1?
>
> The relational operator result has type int (c99 6.5.9 Equality
> operators); and when any scalar value is converted to _Bool, the result
> is 0 if the value compares equal to 0; otherwise, the result is 1 (c99
> 6.3.1.2).
> https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf

Okay, thanks for checking this!
So, let's go with a simplified variant then.

> No conversion warnings even when forcing _Bool type.
> There are many usages like this, for example:
>
> drivers/iio/accel/adxl313_core.c @ int adxl313_is_act_inact_ac()
> drivers/iio/light/opt4060.c @ int opt4060_read_event_config()
> drivers/iio/light/tsl2772.c @ int tsl2772_device_id_verify()
> lib/zstd/compress/zstd_fast.c @ int ZSTD_match4Found_branch()
>
> I cannot find many legitimate usage of relational operator with the
> double negation.
>   git ls-files | xargs grep -s 'return !!' | grep '=='

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/9] iio: adc: Add support for ad4062

[Jonathan Cameron]

On Wed, 26 Nov 2025 12:40:00 +0100
Jorge Marques <gastmaier@gmail.com> wrote:

> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:  
> > > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > > register (SAR) analog-to-digital converter (ADC) with low-power and
> > > threshold monitoring modes.  
> > 
> > ...
> >   
> Hi Andy,
> > > +#define AD4062_SOFT_RESET	0x81  
> > 
> > The grouping seems a bit strange. Haven't you forgotten a blank line here?
> > Ditto for other similar cases.
> >   
> Ack.

Side note. For efficiency, if you agree with something just delete that
block of the thread and don't say so.  Reviewers should be safe to assume
that anything the author agrees with will just be fixed in the next version.

That lets us focus in very fast on the key discussions.

> >   
> > > +struct ad4062_state {
> > > +	const struct ad4062_chip_info *chip;
> > > +	const struct ad4062_bus_ops *ops;
> > > +	enum ad4062_operation_mode mode;
> > > +	struct completion completion;
> > > +	struct iio_trigger *trigger;
> > > +	struct iio_dev *indio_dev;
> > > +	struct i3c_device *i3cdev;
> > > +	struct regmap *regmap;
> > > +	u16 sampling_frequency;
> > > +	int vref_uv;
> > > +	int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> > > +	u8 oversamp_ratio;
> > > +	union {
> > > +		__be32 be32;
> > > +		__be16 be16;
> > > +		u8 bytes[4];
> > > +	} buf __aligned(IIO_DMA_MINALIGN);
> > > +	u8 reg_addr_conv;  
> > 
> > Can't we group u8:s to save a few bytes of memory?
> >   
> Sure
> 
>   struct ad4062_state {
>   	// ...
>   	union {
>   		__be32 be32;
>   		__be16 be16;
>   		u8 bytes[4];
>   	} buf __aligned(IIO_DMA_MINALIGN);
>   	u16 sampling_frequency;
>   	u8 oversamp_ratio;
>   	u8 reg_addr_conv;

Unless my assumption is wrong and those 3 values are passed
as buffers for DMA (or otherwise very carefully protected
against access racing with the DMA buffers) then this is very wrong.
Refresh your memory on why we do the __aligned(IIO_DMA_MINALIGN) and
exactly how that works.

Short answer, nothing must come after it in the structure as it
only forces the start of the buffer, not it's end and hence the
following data ends up in the same cacheline and fun data corruption
is the result.

>   };