* [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
@ 2025-10-13 7:27 ` Jorge Marques
2025-10-13 19:50 ` Conor Dooley
2025-10-18 15:11 ` Jonathan Cameron
2025-10-13 7:28 ` [PATCH 2/7] docs: iio: New docs for ad4062 driver Jorge Marques
` (5 subsequent siblings)
6 siblings, 2 replies; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:27 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, 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
granuality. 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 | 83 ++++++++++++++++++++++
MAINTAINERS | 6 ++
2 files changed, 89 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 0000000000000000000000000000000000000000..dcf86088fc4f32de7ad681561a09bad2755af04c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
@@ -0,0 +1,83 @@
+# 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:
+ minItems: 1
+ maxItems: 2
+
+ interrupt-names:
+ items:
+ - const: gp0
+ description: Signal coming from the GP0 pin.
+ - const: gp1
+ description: Signal coming from the GP1 pin.
+
+ 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>;
+
+ ad4062: adc@0,2ee007c0000 {
+ reg = <0x0 0x2ee 0x7c0000>;
+ vdd-supply = <&vdd>;
+ vio-supply = <&vio>;
+ ref-supply = <&ref>;
+
+ gp1-gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+ gp0-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
+ interrupt-parent = <&gpio>;
+ interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
+ <0 1 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-names = "gp0", "gp1";
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index f090c2f6e63a0d255a025885cc4573f5802ef159..afbfaeba5387b9fbfa9bf1443a059c47dd596d45 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1400,6 +1400,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.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062
2025-10-13 7:27 ` [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062 Jorge Marques
@ 2025-10-13 19:50 ` Conor Dooley
2025-10-26 16:35 ` Jorge Marques
2025-10-18 15:11 ` Jonathan Cameron
1 sibling, 1 reply; 31+ messages in thread
From: Conor Dooley @ 2025-10-13 19:50 UTC (permalink / raw)
To: Jorge Marques
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
[-- Attachment #1: Type: text/plain, Size: 4457 bytes --]
On Mon, Oct 13, 2025 at 09:27:59AM +0200, 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
> granuality. 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 | 83 ++++++++++++++++++++++
> MAINTAINERS | 6 ++
> 2 files changed, 89 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 0000000000000000000000000000000000000000..dcf86088fc4f32de7ad681561a09bad2755af04c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
> @@ -0,0 +1,83 @@
> +# 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:
> + minItems: 1
> + maxItems: 2
> +
> + interrupt-names:
> + items:
> + - const: gp0
> + description: Signal coming from the GP0 pin.
> + - const: gp1
> + description: Signal coming from the GP1 pin.
Please move the descriptions to the interrupts property, by creating an
items list there. I think more information should probably be provided
about them, than just "signal coming from", perhaps referencing the
ability for what the signal actually represents being controllable at
runtime.
> +
> + 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>;
> +
> + ad4062: adc@0,2ee007c0000 {
Remove the ad4062 label here, since there are no users.
Cheers,
Conor.
pw-bot: changes-requested
> + reg = <0x0 0x2ee 0x7c0000>;
> + vdd-supply = <&vdd>;
> + vio-supply = <&vio>;
> + ref-supply = <&ref>;
> +
> + gp1-gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
> + gp0-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> + interrupt-parent = <&gpio>;
> + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> + <0 1 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-names = "gp0", "gp1";
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f090c2f6e63a0d255a025885cc4573f5802ef159..afbfaeba5387b9fbfa9bf1443a059c47dd596d45 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1400,6 +1400,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.49.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062
2025-10-13 19:50 ` Conor Dooley
@ 2025-10-26 16:35 ` Jorge Marques
0 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-10-26 16:35 UTC (permalink / raw)
To: Conor Dooley
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Mon, Oct 13, 2025 at 08:50:31PM +0100, Conor Dooley wrote:
> On Mon, Oct 13, 2025 at 09:27:59AM +0200, 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
> > granuality. 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 | 83 ++++++++++++++++++++++
> > MAINTAINERS | 6 ++
> > 2 files changed, 89 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 0000000000000000000000000000000000000000..dcf86088fc4f32de7ad681561a09bad2755af04c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4062.yaml
> > @@ -0,0 +1,83 @@
> > +# 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:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + interrupt-names:
> > + items:
> > + - const: gp0
> > + description: Signal coming from the GP0 pin.
> > + - const: gp1
> > + description: Signal coming from the GP1 pin.
Hi Conor,
>
> Please move the descriptions to the interrupts property, by creating an
> items list there. I think more information should probably be provided
> about them, than just "signal coming from", perhaps referencing the
> ability for what the signal actually represents being controllable at
> runtime.
I will add a short description of all mode that can be configured to
during runtime. Since both can be configured to any mode except gp0 as
dev_rdy, I will add an description to Interrupts, and then for each
item, just say that for gp0 cannot be dev_rdy, aka:
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
>
> > +
> > + 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>;
> > +
> > + ad4062: adc@0,2ee007c0000 {
>
> Remove the ad4062 label here, since there are no users.
Ack.
>
> Cheers,
> Conor.
>
> pw-bot: changes-requested
>
Best regards,
Jorge
> > + reg = <0x0 0x2ee 0x7c0000>;
> > + vdd-supply = <&vdd>;
> > + vio-supply = <&vio>;
> > + ref-supply = <&ref>;
> > +
> > + gp1-gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
> > + gp0-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> > + interrupt-parent = <&gpio>;
> > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > + interrupt-names = "gp0", "gp1";
> > + };
> > + };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f090c2f6e63a0d255a025885cc4573f5802ef159..afbfaeba5387b9fbfa9bf1443a059c47dd596d45 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1400,6 +1400,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.49.0
> >
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062
2025-10-13 7:27 ` [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062 Jorge Marques
2025-10-13 19:50 ` Conor Dooley
@ 2025-10-18 15:11 ` Jonathan Cameron
2025-10-26 16:37 ` Jorge Marques
1 sibling, 1 reply; 31+ messages in thread
From: Jonathan Cameron @ 2025-10-18 15:11 UTC (permalink / raw)
To: Jorge Marques
Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-iio, devicetree,
linux-kernel, linux-doc
On Mon, 13 Oct 2025 09:27:59 +0200
Jorge Marques <jorge.marques@analog.com> wrote:
> Add dt-bindings for AD4062 family, devices AD4060/AD4062, low-power with
> monitor capabilities SAR ADCs. Each variant of the family differs in
> granuality.
Resolution? I'm not sure what granularity means for an ADC.
> 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>
Otherwise nice simple binding. Nothing to add to Conor's review.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062
2025-10-18 15:11 ` Jonathan Cameron
@ 2025-10-26 16:37 ` Jorge Marques
0 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-10-26 16:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Sat, Oct 18, 2025 at 04:11:43PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:27:59 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
>
> > Add dt-bindings for AD4062 family, devices AD4060/AD4062, low-power with
> > monitor capabilities SAR ADCs. Each variant of the family differs in
> > granuality.
>
> Resolution? I'm not sure what granularity means for an ADC.
'Resolution' is the correct term here, thanks.
>
> > 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>
> Otherwise nice simple binding. Nothing to add to Conor's review.
Best Regards,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
2025-10-13 7:27 ` [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062 Jorge Marques
@ 2025-10-13 7:28 ` Jorge Marques
2025-10-18 15:21 ` Jonathan Cameron
2025-10-13 7:28 ` [PATCH 3/7] iio: adc: Add support for ad4062 Jorge Marques
` (4 subsequent siblings)
6 siblings, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, 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 | 89 ++++++++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 90 insertions(+)
diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
new file mode 100644
index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
--- /dev/null
+++ b/Documentation/iio/ad4062.rst
@@ -0,0 +1,89 @@
+.. 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:
+
+- 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 scale factor to multiply the raw value.
+ * - ``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``
+ - Returs the channel scale in reference to the reference voltage
+ ``ref-supply``.
+
+Also contain the following device attributes:
+
+.. list-table:: Device attributes
+ :header-rows: 1
+
+ * - Attribute
+ - Description
+ * - ``samling_frequency``
+ - Sets the sets the device internal sample rate, used in the burst
+ averaging mode.
+ * - ``sampling_frequency_available``
+ - Lists the available device internal sample rates.
+
+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
diff --git a/MAINTAINERS b/MAINTAINERS
index afbfaeba5387b9fbfa9bf1443a059c47dd596d45..ce012c6c719023d3c0355676a335a55d92cf424c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1405,6 +1405,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.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-10-13 7:28 ` [PATCH 2/7] docs: iio: New docs for ad4062 driver Jorge Marques
@ 2025-10-18 15:21 ` Jonathan Cameron
2025-10-28 15:31 ` Jorge Marques
0 siblings, 1 reply; 31+ messages in thread
From: Jonathan Cameron @ 2025-10-18 15:21 UTC (permalink / raw)
To: Jorge Marques
Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-iio, devicetree,
linux-kernel, linux-doc
On Mon, 13 Oct 2025 09:28:00 +0200
Jorge Marques <jorge.marques@analog.com> wrote:
> This adds a new page to document how to use the ad4062 ADC driver.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jorge,
Various comments inline.
Thanks,
Jonathan
> ---
> Documentation/iio/ad4062.rst | 89 ++++++++++++++++++++++++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 90 insertions(+)
>
> diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
> --- /dev/null
> +++ b/Documentation/iio/ad4062.rst
> @@ -0,0 +1,89 @@
> +.. 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.
This raises a question on whether it makes sense for the binding to support providing
gpios from the start (as alternative to interrupts). Seems like the two pins
are completely interchangeable so one might well be 'left' for use by some other
device that needs a gpio pin.
I don't mind that much if we want to leave the binding support for that for later
but in the ideal case we'd have it from the start (even if the driver doesn't
support it until we have a user).
> +
> +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:
> +
> +- GP1: Is assigned the role of Data Ready signal.
I assume that's only the case if GP1 is provided? If GP0 is the only one
we should allow use that for data ready. As long as the DT allows that it is
permissible for the driver to not do so for now.
> +
> +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 scale factor to multiply the raw value.
That's confusing. This should be hardware 'tweak' to compensate for
calibration or similar. The text doesn't make it clear where that multiply
is happening. Sounds too much like _scale.
> + * - ``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``
> + - Returs the channel scale in reference to the reference voltage
Spell check needed. Also this describes why it might take different values
but not the bit users care about which is the standard ABI thing of
Real value in mV = _raw * _scale
> + ``ref-supply``.
> +
> +Also contain the following device attributes:
> +
> +.. list-table:: Device attributes
> + :header-rows: 1
> +
> + * - Attribute
> + - Description
> + * - ``samling_frequency``
Check these.. sampling_frequency.
> + - Sets the sets the device internal sample rate, used in the burst
> + averaging mode.
It's not use otherwise? That's unusual ABI. I'd expect it to give
the right value at least when burst mode isn't used. Or is burst mode
the only way we do buffered capture?
> + * - ``sampling_frequency_available``
> + - Lists the available device internal sample rates.
> +
> +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.
It feels like it should be easy to use the other GPO pin in this case if that
is present.
> +
> +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
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afbfaeba5387b9fbfa9bf1443a059c47dd596d45..ce012c6c719023d3c0355676a335a55d92cf424c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1405,6 +1405,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>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-10-18 15:21 ` Jonathan Cameron
@ 2025-10-28 15:31 ` Jorge Marques
2025-11-02 12:37 ` Jonathan Cameron
0 siblings, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-10-28 15:31 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Sat, Oct 18, 2025 at 04:21:13PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:00 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
>
> > This adds a new page to document how to use the ad4062 ADC driver.
> >
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> Hi Jorge,
>
> Various comments inline.
>
> Thanks,
>
> Jonathan
>
Hi Jonathan,
> > ---
> > Documentation/iio/ad4062.rst | 89 ++++++++++++++++++++++++++++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 90 insertions(+)
> >
> > diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
> > --- /dev/null
> > +++ b/Documentation/iio/ad4062.rst
> > @@ -0,0 +1,89 @@
> > +.. 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.
> This raises a question on whether it makes sense for the binding to support providing
> gpios from the start (as alternative to interrupts). Seems like the two pins
> are completely interchangeable so one might well be 'left' for use by some other
> device that needs a gpio pin.
>
> I don't mind that much if we want to leave the binding support for that for later
> but in the ideal case we'd have it from the start (even if the driver doesn't
> support it until we have a user).
Yep, they are almost interchangeable except GP0 cannot be DEV_DRY (device is
ready to accept serial interface communications). The question is how to
represent that in the devicetree.
I am ok with adding `gpio-controller` as an optional property. If
present, and if the gp0/1 is not taken by the interrupt-names property,
it fallback to expose as a gpo0 (they cannot be set as gpi).
For the other roles, based on
https://elixir.bootlin.com/linux/v6.18-rc3/source/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml#L109
We would have
adi,gp0-mode = "dev-en";
adi,gp1-mode = "rdy";
Some examples:
// gp0: threshold-either (default), gp1: data ready (default)
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";
};
// gp0: user GPO, gp1: data ready
adc@0,2ee007c0000 {
reg = <0x0 0x2ee 0x7c0000>;
vdd-supply = <&vdd>;
vio-supply = <&vio>;
ref-supply = <&ref>;
gpio-controller;
interrupt-parent = <&gpio>;
interrupts = <0 1 IRQ_TYPE_EDGE_FALLING>;
interrupt-names = "gp1";
};
// gp0: threshold crossed high value, g1: unused
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_FALLING>;
interrupt-names = "gp0";
adi,gp0-mode = "thresh-high"
};
And the driver constraints the valid configurations.
>
> > +
> > +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:
> > +
> > +- GP1: Is assigned the role of Data Ready signal.
>
> I assume that's only the case if GP1 is provided? If GP0 is the only one
> we should allow use that for data ready. As long as the DT allows that it is
> permissible for the driver to not do so for now.
>
Suggested `adi,gp*-mode` should solve.
> > +
> > +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 scale factor to multiply the raw value.
> That's confusing. This should be hardware 'tweak' to compensate for
> calibration or similar. The text doesn't make it clear where that multiply
> is happening. Sounds too much like _scale.
The user does raw * _scale * _calibscale to get the value in volts.
_calibscale is 1 by default and serves two purposes:
* small hw corrections (matches ABI); or
* an arbitrary user set scale, ideally also for corrections, but not
constrained.
For the prior, the device does support computing the appropriate
_calibscale value, but it is not implemented.
If it was implemented, it would occur once during driver initialization
and then _calibscale default value would be approximately 1, (e.g.,
0.997).
>
> > + * - ``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``
> > + - Returs the channel scale in reference to the reference voltage
>
> Spell check needed. Also this describes why it might take different values
> but not the bit users care about which is the standard ABI thing of
> Real value in mV = _raw * _scale
>
Ack, will describe
raw * _scale * _calibscale
> > + ``ref-supply``.
> > +
> > +Also contain the following device attributes:
> > +
> > +.. list-table:: Device attributes
> > + :header-rows: 1
> > +
> > + * - Attribute
> > + - Description
> > + * - ``samling_frequency``
>
> Check these.. sampling_frequency.
>
> > + - Sets the sets the device internal sample rate, used in the burst
> > + averaging mode.
>
> It's not use otherwise? That's unusual ABI. I'd expect it to give
> the right value at least when burst mode isn't used. Or is burst mode
> the only way we do buffered capture?
>
It is not used otherwise, the device internal sample rate is used only
to evenly distribute readings used in the burst averaging mode.
There is no sampling trigger to evenly space the adc conversion reading,
e.g.,:
Oversampling 4, sampling ratio 2Hz
Sampling trigger | | | | |
ADC conversion ++++ ++++ ++++ ++++ ++++
ADC data ready * * * * *
Oversampling 4, sampling ratio 1Hz
Sampling trigger | | | | |
ADC conversion + + + + + + + + + + + + + + + + + + + +
ADC data ready * * * * *
For this driver, the sampling trigger is a register access (the stop bit
of the i3c transfer to be exact), so in buffer mode it reads as fast as
possible.
> > + * - ``sampling_frequency_available``
> > + - Lists the available device internal sample rates.
> > +
> > +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.
>
> It feels like it should be easy to use the other GPO pin in this case if that
> is present.
>
adi-gp0-mode should solve.
I wouldn't auto-set the mode by the position in the interrupt-names
array, but let me know your opinion.
Best regards,
Jorge
> > +
> > +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
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index afbfaeba5387b9fbfa9bf1443a059c47dd596d45..ce012c6c719023d3c0355676a335a55d92cf424c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1405,6 +1405,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>
> >
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-10-28 15:31 ` Jorge Marques
@ 2025-11-02 12:37 ` Jonathan Cameron
2025-11-03 10:19 ` Jorge Marques
0 siblings, 1 reply; 31+ messages in thread
From: Jonathan Cameron @ 2025-11-02 12:37 UTC (permalink / raw)
To: Jorge Marques
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Tue, 28 Oct 2025 16:31:46 +0100
Jorge Marques <gastmaier@gmail.com> wrote:
> On Sat, Oct 18, 2025 at 04:21:13PM +0100, Jonathan Cameron wrote:
> > On Mon, 13 Oct 2025 09:28:00 +0200
> > Jorge Marques <jorge.marques@analog.com> wrote:
> >
> > > This adds a new page to document how to use the ad4062 ADC driver.
> > >
> > > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > Hi Jorge,
> >
> > Various comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Hi Jonathan,
>
> > > ---
> > > Documentation/iio/ad4062.rst | 89 ++++++++++++++++++++++++++++++++++++++++++++
> > > MAINTAINERS | 1 +
> > > 2 files changed, 90 insertions(+)
> > >
> > > diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
> > > --- /dev/null
> > > +++ b/Documentation/iio/ad4062.rst
> > > @@ -0,0 +1,89 @@
> > > +.. 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.
> > This raises a question on whether it makes sense for the binding to support providing
> > gpios from the start (as alternative to interrupts). Seems like the two pins
> > are completely interchangeable so one might well be 'left' for use by some other
> > device that needs a gpio pin.
> >
> > I don't mind that much if we want to leave the binding support for that for later
> > but in the ideal case we'd have it from the start (even if the driver doesn't
> > support it until we have a user).
>
> Yep, they are almost interchangeable except GP0 cannot be DEV_DRY (device is
> ready to accept serial interface communications). The question is how to
> represent that in the devicetree.
>
> I am ok with adding `gpio-controller` as an optional property. If
> present, and if the gp0/1 is not taken by the interrupt-names property,
> it fallback to expose as a gpo0 (they cannot be set as gpi).
>
> For the other roles, based on
> https://elixir.bootlin.com/linux/v6.18-rc3/source/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml#L109
> We would have
>
> adi,gp0-mode = "dev-en";
> adi,gp1-mode = "rdy";
>
> Some examples:
>
> // gp0: threshold-either (default), gp1: data ready (default)
> 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";
> };
>
> // gp0: user GPO, gp1: data ready
> adc@0,2ee007c0000 {
> reg = <0x0 0x2ee 0x7c0000>;
> vdd-supply = <&vdd>;
> vio-supply = <&vio>;
> ref-supply = <&ref>;
>
> gpio-controller;
>
> interrupt-parent = <&gpio>;
> interrupts = <0 1 IRQ_TYPE_EDGE_FALLING>;
> interrupt-names = "gp1";
> };
>
> // gp0: threshold crossed high value, g1: unused
> 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_FALLING>;
> interrupt-names = "gp0";
>
> adi,gp0-mode = "thresh-high"
> };
>
>
> And the driver constraints the valid configurations.
More or less looks good We have other drivers effectively doing this.
Whether we actively handle the final case or not in driver probably doesn't matter.
Wtihout the gpio-controller specified nothing can use the gpios anyway so
if we support them they simply won't get used.
I'm not sure on the gpi0-mode though. Why is that useful? That feels like
a choice the driver should make dependent on what is available to it.
So if we've specified it is in interrupt, only the modes that might be
an interrupt are available to the driver.
The only case I can see being useful is dev_en as that's a specifically timed
control signal for the analog front end.
So a specific flag for that probably makes sense - I'm not sure if we'd
ever have an AFE where it would make sense to drive it from a hardware
signal on the ADC like this one AND from a gpio, so I don't think we need
to support binding this as a gpio in that case.
>
> >
> > > +
> > > +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:
> > > +
> > > +- GP1: Is assigned the role of Data Ready signal.
> >
> > I assume that's only the case if GP1 is provided? If GP0 is the only one
> > we should allow use that for data ready. As long as the DT allows that it is
> > permissible for the driver to not do so for now.
> >
>
> Suggested `adi,gp*-mode` should solve.
>
> > > +
> > > +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 scale factor to multiply the raw value.
> > That's confusing. This should be hardware 'tweak' to compensate for
> > calibration or similar. The text doesn't make it clear where that multiply
> > is happening. Sounds too much like _scale.
>
> The user does raw * _scale * _calibscale to get the value in volts.
That's not ABI compliant so no general purpose user space is going to do that.
So a hard no for this being what userspace needs to apply.
I'm not particularly keen on calibscale for things other than tweaking so
that raw * _scale is in milivolts.
Normally if we have other forms of controllable scaling it's a question
of wrapping up any such scan factors into a writeable _scale.
>
> _calibscale is 1 by default and serves two purposes:
>
> * small hw corrections (matches ABI); or
> * an arbitrary user set scale, ideally also for corrections, but not
> constrained.
>
> For the prior, the device does support computing the appropriate
> _calibscale value, but it is not implemented.
>
> If it was implemented, it would occur once during driver initialization
> and then _calibscale default value would be approximately 1, (e.g.,
> 0.997).
>
> >
> > > + * - ``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``
> > > + - Returs the channel scale in reference to the reference voltage
> >
> > Spell check needed. Also this describes why it might take different values
> > but not the bit users care about which is the standard ABI thing of
> > Real value in mV = _raw * _scale
> >
> Ack, will describe
>
> raw * _scale * _calibscale
As above, no to this. It must be just raw * _scale.
>
> > > + ``ref-supply``.
> > > +
> > > +Also contain the following device attributes:
> > > +
> > > +.. list-table:: Device attributes
> > > + :header-rows: 1
> > > +
> > > + * - Attribute
> > > + - Description
> > > + * - ``samling_frequency``
> >
> > Check these.. sampling_frequency.
> >
> > > + - Sets the sets the device internal sample rate, used in the burst
> > > + averaging mode.
> >
> > It's not use otherwise? That's unusual ABI. I'd expect it to give
> > the right value at least when burst mode isn't used. Or is burst mode
> > the only way we do buffered capture?
> >
>
> It is not used otherwise, the device internal sample rate is used only
> to evenly distribute readings used in the burst averaging mode.
> There is no sampling trigger to evenly space the adc conversion reading,
In event of no internal trigger, sampling_frequency is normally
1/duration of a single scan (or sample if a per channel attribute).
> e.g.,:
>
> Oversampling 4, sampling ratio 2Hz
> Sampling trigger | | | | |
> ADC conversion ++++ ++++ ++++ ++++ ++++
> ADC data ready * * * * *
>
> Oversampling 4, sampling ratio 1Hz
> Sampling trigger | | | | |
> ADC conversion + + + + + + + + + + + + + + + + + + + +
> ADC data ready * * * * *
>
> For this driver, the sampling trigger is a register access (the stop bit
> of the i3c transfer to be exact), so in buffer mode it reads as fast as
> possible.
>
> > > + * - ``sampling_frequency_available``
> > > + - Lists the available device internal sample rates.
> > > +
> > > +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.
> >
> > It feels like it should be easy to use the other GPO pin in this case if that
> > is present.
> >
> adi-gp0-mode should solve.
>
> I wouldn't auto-set the mode by the position in the interrupt-names
> array, but let me know your opinion.
Mode when multiple are equally valid (for a given device wiring)
is purely down to what the driver wants to do. There is no benefit in
encoding that in dt.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-11-02 12:37 ` Jonathan Cameron
@ 2025-11-03 10:19 ` Jorge Marques
2025-11-03 12:22 ` Jorge Marques
2025-11-09 12:31 ` Jonathan Cameron
0 siblings, 2 replies; 31+ messages in thread
From: Jorge Marques @ 2025-11-03 10:19 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Sun, Nov 02, 2025 at 12:37:27PM +0000, Jonathan Cameron wrote:
> On Tue, 28 Oct 2025 16:31:46 +0100
> Jorge Marques <gastmaier@gmail.com> wrote:
>
> > On Sat, Oct 18, 2025 at 04:21:13PM +0100, Jonathan Cameron wrote:
> > > On Mon, 13 Oct 2025 09:28:00 +0200
> > > Jorge Marques <jorge.marques@analog.com> wrote:
> > >
> > > > This adds a new page to document how to use the ad4062 ADC driver.
> > > >
> > > > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > > Hi Jorge,
> > >
> > > Various comments inline.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> >
> > Hi Jonathan,
> >
> > > > ---
> > > > Documentation/iio/ad4062.rst | 89 ++++++++++++++++++++++++++++++++++++++++++++
> > > > MAINTAINERS | 1 +
> > > > 2 files changed, 90 insertions(+)
> > > >
> > > > diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
> > > > --- /dev/null
> > > > +++ b/Documentation/iio/ad4062.rst
> > > > @@ -0,0 +1,89 @@
> > > > +.. 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.
> > > This raises a question on whether it makes sense for the binding to support providing
> > > gpios from the start (as alternative to interrupts). Seems like the two pins
> > > are completely interchangeable so one might well be 'left' for use by some other
> > > device that needs a gpio pin.
> > >
> > > I don't mind that much if we want to leave the binding support for that for later
> > > but in the ideal case we'd have it from the start (even if the driver doesn't
> > > support it until we have a user).
> >
> > Yep, they are almost interchangeable except GP0 cannot be DEV_DRY (device is
> > ready to accept serial interface communications). The question is how to
> > represent that in the devicetree.
> >
> > I am ok with adding `gpio-controller` as an optional property. If
> > present, and if the gp0/1 is not taken by the interrupt-names property,
> > it fallback to expose as a gpo0 (they cannot be set as gpi).
> >
> > For the other roles, based on
> > https://elixir.bootlin.com/linux/v6.18-rc3/source/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml#L109
> > We would have
> >
> > adi,gp0-mode = "dev-en";
> > adi,gp1-mode = "rdy";
> >
> > Some examples:
> >
> > // gp0: threshold-either (default), gp1: data ready (default)
> > 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";
> > };
> >
> > // gp0: user GPO, gp1: data ready
> > adc@0,2ee007c0000 {
> > reg = <0x0 0x2ee 0x7c0000>;
> > vdd-supply = <&vdd>;
> > vio-supply = <&vio>;
> > ref-supply = <&ref>;
> >
> > gpio-controller;
> >
> > interrupt-parent = <&gpio>;
> > interrupts = <0 1 IRQ_TYPE_EDGE_FALLING>;
> > interrupt-names = "gp1";
> > };
> >
> > // gp0: threshold crossed high value, g1: unused
> > 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_FALLING>;
> > interrupt-names = "gp0";
> >
> > adi,gp0-mode = "thresh-high"
> > };
> >
> >
> > And the driver constraints the valid configurations.
>
Hi Jonathan,
A comment on the gpio modes and sampling frequency.
Ack on `raw * _scale`.
> More or less looks good We have other drivers effectively doing this.
> Whether we actively handle the final case or not in driver probably doesn't matter.
> Wtihout the gpio-controller specified nothing can use the gpios anyway so
> if we support them they simply won't get used.
>
> I'm not sure on the gpi0-mode though. Why is that useful? That feels like
> a choice the driver should make dependent on what is available to it.
> So if we've specified it is in interrupt, only the modes that might be
> an interrupt are available to the driver.
>
> The only case I can see being useful is dev_en as that's a specifically timed
> control signal for the analog front end.
>
> So a specific flag for that probably makes sense - I'm not sure if we'd
> ever have an AFE where it would make sense to drive it from a hardware
> signal on the ADC like this one AND from a gpio, so I don't think we need
> to support binding this as a gpio in that case.
>
So, by default gp0 is set threshold-either, and gp1 data ready.
The `adi,gp*-mode` would be useful to invert for example, have gp0 as
data ready and gp1 as threshold-either.
On the threshold, there are 3 different crossings:
* either: the reading crosses either the upper or lower boundary.
* max: crosses the upper boundary.
* min: crosses the lower boundary.
To not make the `adi,gp*-mode` attribute necessary I can:
* make the driver set either, max, min roles based on the
values written to the IIO attributes, e.g. writing -1 for
IIO_EV_INFO_HYSTERESIS, or a max/min value high/low enough?
* dev_en use case is to shutdown/turn-on an AFE, amplifiers,
but I wouldn't support in this series.
So, I believe the solution for the dt-binding is add the optional
`gpio-controller`, and if set, expose the unused (not in the
interrupts-names) as GPOutput. And not support inversion gp0<->gp1 of
the mode, the specific threshold crossing (max, min, either), nor the
dev_en use case.
This then doesn't change the series much, just appends a commit adding
the gpio-controller support.
> >
> > >
> > > > +
> > > > +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:
> > > > +
> > > > +- GP1: Is assigned the role of Data Ready signal.
> > >
> > > I assume that's only the case if GP1 is provided? If GP0 is the only one
> > > we should allow use that for data ready. As long as the DT allows that it is
> > > permissible for the driver to not do so for now.
> > >
> >
> > Suggested `adi,gp*-mode` should solve.
> >
> > > > +
> > > > +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 scale factor to multiply the raw value.
> > > That's confusing. This should be hardware 'tweak' to compensate for
> > > calibration or similar. The text doesn't make it clear where that multiply
> > > is happening. Sounds too much like _scale.
> >
> > The user does raw * _scale * _calibscale to get the value in volts.
>
> That's not ABI compliant so no general purpose user space is going to do that.
> So a hard no for this being what userspace needs to apply.
>
> I'm not particularly keen on calibscale for things other than tweaking so
> that raw * _scale is in milivolts.
>
> Normally if we have other forms of controllable scaling it's a question
> of wrapping up any such scan factors into a writeable _scale.
>
Ack, will update to raw * _scale, embedding the _calibscale into the
_scale value.
> >
> > _calibscale is 1 by default and serves two purposes:
> >
> > * small hw corrections (matches ABI); or
> > * an arbitrary user set scale, ideally also for corrections, but not
> > constrained.
> >
> > For the prior, the device does support computing the appropriate
> > _calibscale value, but it is not implemented.
> >
> > If it was implemented, it would occur once during driver initialization
> > and then _calibscale default value would be approximately 1, (e.g.,
> > 0.997).
> >
> > >
> > > > + * - ``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``
> > > > + - Returs the channel scale in reference to the reference voltage
> > >
> > > Spell check needed. Also this describes why it might take different values
> > > but not the bit users care about which is the standard ABI thing of
> > > Real value in mV = _raw * _scale
> > >
> > Ack, will describe
> >
> > raw * _scale * _calibscale
>
> As above, no to this. It must be just raw * _scale.
>
Ack.
> >
> > > > + ``ref-supply``.
> > > > +
> > > > +Also contain the following device attributes:
> > > > +
> > > > +.. list-table:: Device attributes
> > > > + :header-rows: 1
> > > > +
> > > > + * - Attribute
> > > > + - Description
> > > > + * - ``samling_frequency``
> > >
> > > Check these.. sampling_frequency.
> > >
> > > > + - Sets the sets the device internal sample rate, used in the burst
> > > > + averaging mode.
> > >
> > > It's not use otherwise? That's unusual ABI. I'd expect it to give
> > > the right value at least when burst mode isn't used. Or is burst mode
> > > the only way we do buffered capture?
> > >
> >
> > It is not used otherwise, the device internal sample rate is used only
> > to evenly distribute readings used in the burst averaging mode.
> > There is no sampling trigger to evenly space the adc conversion reading,
>
> In event of no internal trigger, sampling_frequency is normally
> 1/duration of a single scan (or sample if a per channel attribute).
>
>
The duration of a single scan (duration between the convert-start high
edge until RDY falling edge), would be ((n_avg - 1) / fosc + tconv)
(datasheet fig.56, p.31) , where fosc is the internal timer frequency
directly exposed in this series. I can make the driver use this formula,
just setting it will be less straight forward, since currently a clear
enum of the values the register supports is returned by the
sampling_frequency_available attribute. Instead, it will scale based on
the number of samples per burst (averaging ratio/n_avg). tconv is typ
270ns and max 320ns.
> > e.g.,:
> >
> > Oversampling 4, sampling ratio 2Hz
> > Sampling trigger | | | | |
> > ADC conversion ++++ ++++ ++++ ++++ ++++
> > ADC data ready * * * * *
> >
> > Oversampling 4, sampling ratio 1Hz
> > Sampling trigger | | | | |
> > ADC conversion + + + + + + + + + + + + + + + + + + + +
> > ADC data ready * * * * *
> >
> > For this driver, the sampling trigger is a register access (the stop bit
> > of the i3c transfer to be exact), so in buffer mode it reads as fast as
> > possible.
> >
> > > > + * - ``sampling_frequency_available``
> > > > + - Lists the available device internal sample rates.
> > > > +
> > > > +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.
> > >
> > > It feels like it should be easy to use the other GPO pin in this case if that
> > > is present.
> > >
> > adi-gp0-mode should solve.
> >
> > I wouldn't auto-set the mode by the position in the interrupt-names
> > array, but let me know your opinion.
>
> Mode when multiple are equally valid (for a given device wiring)
> is purely down to what the driver wants to do. There is no benefit in
> encoding that in dt.
> Thanks,
>
> Jonathan
>
Best Regards,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-11-03 10:19 ` Jorge Marques
@ 2025-11-03 12:22 ` Jorge Marques
2025-11-09 12:16 ` Jonathan Cameron
2025-11-09 12:31 ` Jonathan Cameron
1 sibling, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-11-03 12:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Mon, Nov 03, 2025 at 11:19:50AM +0100, Jorge Marques wrote:
> On Sun, Nov 02, 2025 at 12:37:27PM +0000, Jonathan Cameron wrote:
> > On Tue, 28 Oct 2025 16:31:46 +0100
> > Jorge Marques <gastmaier@gmail.com> wrote:
> >
> > > On Sat, Oct 18, 2025 at 04:21:13PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 13 Oct 2025 09:28:00 +0200
> > > > Jorge Marques <jorge.marques@analog.com> wrote:
> > > >
> > > > > This adds a new page to document how to use the ad4062 ADC driver.
> > > > >
> > > > > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > > > Hi Jorge,
> > > >
> > > > Various comments inline.
> > > >
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >
> > >
> > > Hi Jonathan,
> > >
> > > > > ---
> > > > > Documentation/iio/ad4062.rst | 89 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > MAINTAINERS | 1 +
> > > > > 2 files changed, 90 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
> > > > > --- /dev/null
> > > > > +++ b/Documentation/iio/ad4062.rst
> > > > > @@ -0,0 +1,89 @@
> > > > > +.. 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.
> > > > This raises a question on whether it makes sense for the binding to support providing
> > > > gpios from the start (as alternative to interrupts). Seems like the two pins
> > > > are completely interchangeable so one might well be 'left' for use by some other
> > > > device that needs a gpio pin.
> > > >
> > > > I don't mind that much if we want to leave the binding support for that for later
> > > > but in the ideal case we'd have it from the start (even if the driver doesn't
> > > > support it until we have a user).
> > >
> > > Yep, they are almost interchangeable except GP0 cannot be DEV_DRY (device is
> > > ready to accept serial interface communications). The question is how to
> > > represent that in the devicetree.
> > >
> > > I am ok with adding `gpio-controller` as an optional property. If
> > > present, and if the gp0/1 is not taken by the interrupt-names property,
> > > it fallback to expose as a gpo0 (they cannot be set as gpi).
> > >
> > > For the other roles, based on
> > > https://elixir.bootlin.com/linux/v6.18-rc3/source/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml#L109
> > > We would have
> > >
> > > adi,gp0-mode = "dev-en";
> > > adi,gp1-mode = "rdy";
> > >
> > > Some examples:
> > >
> > > // gp0: threshold-either (default), gp1: data ready (default)
> > > 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";
> > > };
> > >
> > > // gp0: user GPO, gp1: data ready
> > > adc@0,2ee007c0000 {
> > > reg = <0x0 0x2ee 0x7c0000>;
> > > vdd-supply = <&vdd>;
> > > vio-supply = <&vio>;
> > > ref-supply = <&ref>;
> > >
> > > gpio-controller;
> > >
> > > interrupt-parent = <&gpio>;
> > > interrupts = <0 1 IRQ_TYPE_EDGE_FALLING>;
> > > interrupt-names = "gp1";
> > > };
> > >
> > > // gp0: threshold crossed high value, g1: unused
> > > 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_FALLING>;
> > > interrupt-names = "gp0";
> > >
> > > adi,gp0-mode = "thresh-high"
> > > };
> > >
> > >
> > > And the driver constraints the valid configurations.
> >
>
> Hi Jonathan,
>
> A comment on the gpio modes and sampling frequency.
> Ack on `raw * _scale`.
>
> > More or less looks good We have other drivers effectively doing this.
> > Whether we actively handle the final case or not in driver probably doesn't matter.
> > Wtihout the gpio-controller specified nothing can use the gpios anyway so
> > if we support them they simply won't get used.
> >
> > I'm not sure on the gpi0-mode though. Why is that useful? That feels like
> > a choice the driver should make dependent on what is available to it.
> > So if we've specified it is in interrupt, only the modes that might be
> > an interrupt are available to the driver.
> >
> > The only case I can see being useful is dev_en as that's a specifically timed
> > control signal for the analog front end.
> >
> > So a specific flag for that probably makes sense - I'm not sure if we'd
> > ever have an AFE where it would make sense to drive it from a hardware
> > signal on the ADC like this one AND from a gpio, so I don't think we need
> > to support binding this as a gpio in that case.
> >
>
> So, by default gp0 is set threshold-either, and gp1 data ready.
> The `adi,gp*-mode` would be useful to invert for example, have gp0 as
> data ready and gp1 as threshold-either.
>
> On the threshold, there are 3 different crossings:
> * either: the reading crosses either the upper or lower boundary.
> * max: crosses the upper boundary.
> * min: crosses the lower boundary.
>
> To not make the `adi,gp*-mode` attribute necessary I can:
> * make the driver set either, max, min roles based on the
> values written to the IIO attributes, e.g. writing -1 for
> IIO_EV_INFO_HYSTERESIS, or a max/min value high/low enough?
> * dev_en use case is to shutdown/turn-on an AFE, amplifiers,
> but I wouldn't support in this series.
>
> So, I believe the solution for the dt-binding is add the optional
> `gpio-controller`, and if set, expose the unused (not in the
> interrupts-names) as GPOutput. And not support inversion gp0<->gp1 of
> the mode, the specific threshold crossing (max, min, either), nor the
> dev_en use case.
>
> This then doesn't change the series much, just appends a commit adding
> the gpio-controller support.
>
> > >
> > > >
> > > > > +
> > > > > +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:
> > > > > +
> > > > > +- GP1: Is assigned the role of Data Ready signal.
> > > >
> > > > I assume that's only the case if GP1 is provided? If GP0 is the only one
> > > > we should allow use that for data ready. As long as the DT allows that it is
> > > > permissible for the driver to not do so for now.
> > > >
> > >
> > > Suggested `adi,gp*-mode` should solve.
> > >
> > > > > +
> > > > > +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 scale factor to multiply the raw value.
> > > > That's confusing. This should be hardware 'tweak' to compensate for
> > > > calibration or similar. The text doesn't make it clear where that multiply
> > > > is happening. Sounds too much like _scale.
> > >
> > > The user does raw * _scale * _calibscale to get the value in volts.
> >
> > That's not ABI compliant so no general purpose user space is going to do that.
> > So a hard no for this being what userspace needs to apply.
> >
> > I'm not particularly keen on calibscale for things other than tweaking so
> > that raw * _scale is in milivolts.
> >
> > Normally if we have other forms of controllable scaling it's a question
> > of wrapping up any such scan factors into a writeable _scale.
> >
>
> Ack, will update to raw * _scale, embedding the _calibscale into the
> _scale value.
>
Fixup on my side, there is no need to "embed _calibscale into _scale".
The only change that needs to be made is re-wording the doc to clarify
that it is the hw that compensates for calibration errors:
* - ``in_voltage_calibscale``
- - Sets the scale factor to multiply the raw value.
+ - Sets the gain scaling factor that the hardware applies to the sample,
+ to compensate for system gain error.
(datasheet p22, Gain Scaling)
The mv reading is raw * _scale.
Best regards,
Jorge
> > >
> > > _calibscale is 1 by default and serves two purposes:
> > >
> > > * small hw corrections (matches ABI); or
> > > * an arbitrary user set scale, ideally also for corrections, but not
> > > constrained.
> > >
> > > For the prior, the device does support computing the appropriate
> > > _calibscale value, but it is not implemented.
> > >
> > > If it was implemented, it would occur once during driver initialization
> > > and then _calibscale default value would be approximately 1, (e.g.,
> > > 0.997).
> > >
> > > >
> > > > > + * - ``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``
> > > > > + - Returs the channel scale in reference to the reference voltage
> > > >
> > > > Spell check needed. Also this describes why it might take different values
> > > > but not the bit users care about which is the standard ABI thing of
> > > > Real value in mV = _raw * _scale
> > > >
> > > Ack, will describe
> > >
> > > raw * _scale * _calibscale
> >
> > As above, no to this. It must be just raw * _scale.
> >
>
> Ack.
>
> > >
> > > > > + ``ref-supply``.
> > > > > +
> > > > > +Also contain the following device attributes:
> > > > > +
> > > > > +.. list-table:: Device attributes
> > > > > + :header-rows: 1
> > > > > +
> > > > > + * - Attribute
> > > > > + - Description
> > > > > + * - ``samling_frequency``
> > > >
> > > > Check these.. sampling_frequency.
> > > >
> > > > > + - Sets the sets the device internal sample rate, used in the burst
> > > > > + averaging mode.
> > > >
> > > > It's not use otherwise? That's unusual ABI. I'd expect it to give
> > > > the right value at least when burst mode isn't used. Or is burst mode
> > > > the only way we do buffered capture?
> > > >
> > >
> > > It is not used otherwise, the device internal sample rate is used only
> > > to evenly distribute readings used in the burst averaging mode.
> > > There is no sampling trigger to evenly space the adc conversion reading,
> >
> > In event of no internal trigger, sampling_frequency is normally
> > 1/duration of a single scan (or sample if a per channel attribute).
> >
> >
>
> The duration of a single scan (duration between the convert-start high
> edge until RDY falling edge), would be ((n_avg - 1) / fosc + tconv)
> (datasheet fig.56, p.31) , where fosc is the internal timer frequency
> directly exposed in this series. I can make the driver use this formula,
> just setting it will be less straight forward, since currently a clear
> enum of the values the register supports is returned by the
> sampling_frequency_available attribute. Instead, it will scale based on
> the number of samples per burst (averaging ratio/n_avg). tconv is typ
> 270ns and max 320ns.
>
> > > e.g.,:
> > >
> > > Oversampling 4, sampling ratio 2Hz
> > > Sampling trigger | | | | |
> > > ADC conversion ++++ ++++ ++++ ++++ ++++
> > > ADC data ready * * * * *
> > >
> > > Oversampling 4, sampling ratio 1Hz
> > > Sampling trigger | | | | |
> > > ADC conversion + + + + + + + + + + + + + + + + + + + +
> > > ADC data ready * * * * *
> > >
> > > For this driver, the sampling trigger is a register access (the stop bit
> > > of the i3c transfer to be exact), so in buffer mode it reads as fast as
> > > possible.
> > >
> > > > > + * - ``sampling_frequency_available``
> > > > > + - Lists the available device internal sample rates.
> > > > > +
> > > > > +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.
> > > >
> > > > It feels like it should be easy to use the other GPO pin in this case if that
> > > > is present.
> > > >
> > > adi-gp0-mode should solve.
> > >
> > > I wouldn't auto-set the mode by the position in the interrupt-names
> > > array, but let me know your opinion.
> >
> > Mode when multiple are equally valid (for a given device wiring)
> > is purely down to what the driver wants to do. There is no benefit in
> > encoding that in dt.
> > Thanks,
> >
> > Jonathan
> >
>
> Best Regards,
> Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-11-03 12:22 ` Jorge Marques
@ 2025-11-09 12:16 ` Jonathan Cameron
0 siblings, 0 replies; 31+ messages in thread
From: Jonathan Cameron @ 2025-11-09 12:16 UTC (permalink / raw)
To: Jorge Marques
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
> > > > > > +
> > > > > > +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 scale factor to multiply the raw value.
> > > > > That's confusing. This should be hardware 'tweak' to compensate for
> > > > > calibration or similar. The text doesn't make it clear where that multiply
> > > > > is happening. Sounds too much like _scale.
> > > >
> > > > The user does raw * _scale * _calibscale to get the value in volts.
> > >
> > > That's not ABI compliant so no general purpose user space is going to do that.
> > > So a hard no for this being what userspace needs to apply.
> > >
> > > I'm not particularly keen on calibscale for things other than tweaking so
> > > that raw * _scale is in milivolts.
> > >
> > > Normally if we have other forms of controllable scaling it's a question
> > > of wrapping up any such scan factors into a writeable _scale.
> > >
> >
> > Ack, will update to raw * _scale, embedding the _calibscale into the
> > _scale value.
> >
>
> Fixup on my side, there is no need to "embed _calibscale into _scale".
> The only change that needs to be made is re-wording the doc to clarify
> that it is the hw that compensates for calibration errors:
>
> * - ``in_voltage_calibscale``
> - - Sets the scale factor to multiply the raw value.
> + - Sets the gain scaling factor that the hardware applies to the sample,
> + to compensate for system gain error.
>
> (datasheet p22, Gain Scaling)
> The mv reading is raw * _scale.
Good. That aligns with normal use of calibscale.
>
> Best regards,
> Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver
2025-11-03 10:19 ` Jorge Marques
2025-11-03 12:22 ` Jorge Marques
@ 2025-11-09 12:31 ` Jonathan Cameron
1 sibling, 0 replies; 31+ messages in thread
From: Jonathan Cameron @ 2025-11-09 12:31 UTC (permalink / raw)
To: Jorge Marques
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
> > > > > +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.
> > > > This raises a question on whether it makes sense for the binding to support providing
> > > > gpios from the start (as alternative to interrupts). Seems like the two pins
> > > > are completely interchangeable so one might well be 'left' for use by some other
> > > > device that needs a gpio pin.
> > > >
> > > > I don't mind that much if we want to leave the binding support for that for later
> > > > but in the ideal case we'd have it from the start (even if the driver doesn't
> > > > support it until we have a user).
> > >
> > > Yep, they are almost interchangeable except GP0 cannot be DEV_DRY (device is
> > > ready to accept serial interface communications). The question is how to
> > > represent that in the devicetree.
> > >
> > > I am ok with adding `gpio-controller` as an optional property. If
> > > present, and if the gp0/1 is not taken by the interrupt-names property,
> > > it fallback to expose as a gpo0 (they cannot be set as gpi).
> > >
> > > For the other roles, based on
> > > https://elixir.bootlin.com/linux/v6.18-rc3/source/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml#L109
> > > We would have
> > >
> > > adi,gp0-mode = "dev-en";
> > > adi,gp1-mode = "rdy";
> > >
> > > Some examples:
> > >
> > > // gp0: threshold-either (default), gp1: data ready (default)
> > > 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";
> > > };
> > >
> > > // gp0: user GPO, gp1: data ready
> > > adc@0,2ee007c0000 {
> > > reg = <0x0 0x2ee 0x7c0000>;
> > > vdd-supply = <&vdd>;
> > > vio-supply = <&vio>;
> > > ref-supply = <&ref>;
> > >
> > > gpio-controller;
> > >
> > > interrupt-parent = <&gpio>;
> > > interrupts = <0 1 IRQ_TYPE_EDGE_FALLING>;
> > > interrupt-names = "gp1";
> > > };
> > >
> > > // gp0: threshold crossed high value, g1: unused
> > > 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_FALLING>;
> > > interrupt-names = "gp0";
> > >
> > > adi,gp0-mode = "thresh-high"
> > > };
> > >
> > >
> > > And the driver constraints the valid configurations.
> >
>
> Hi Jonathan,
>
> A comment on the gpio modes and sampling frequency.
> Ack on `raw * _scale`.
>
> > More or less looks good We have other drivers effectively doing this.
> > Whether we actively handle the final case or not in driver probably doesn't matter.
> > Wtihout the gpio-controller specified nothing can use the gpios anyway so
> > if we support them they simply won't get used.
> >
> > I'm not sure on the gpi0-mode though. Why is that useful? That feels like
> > a choice the driver should make dependent on what is available to it.
> > So if we've specified it is in interrupt, only the modes that might be
> > an interrupt are available to the driver.
> >
> > The only case I can see being useful is dev_en as that's a specifically timed
> > control signal for the analog front end.
> >
> > So a specific flag for that probably makes sense - I'm not sure if we'd
> > ever have an AFE where it would make sense to drive it from a hardware
> > signal on the ADC like this one AND from a gpio, so I don't think we need
> > to support binding this as a gpio in that case.
> >
>
> So, by default gp0 is set threshold-either, and gp1 data ready.
> The `adi,gp*-mode` would be useful to invert for example, have gp0 as
> data ready and gp1 as threshold-either.
>
> On the threshold, there are 3 different crossings:
> * either: the reading crosses either the upper or lower boundary.
> * max: crosses the upper boundary.
> * min: crosses the lower boundary.
>
> To not make the `adi,gp*-mode` attribute necessary I can:
> * make the driver set either, max, min roles based on the
> values written to the IIO attributes, e.g. writing -1 for
> IIO_EV_INFO_HYSTERESIS, or a max/min value high/low enough?
This confuses me a little. Which direction do the thresholds apply in?
Looks from the datasheet like these are standard in that
crossing the min threshold triggers the interrupt when the value
goes below that threshold (and hysteresis means the detector doesn't
reset until the value goes above min + hysteresis)
So for that it's a falling threshold with normal hysteresis.
For the max on it will be a rising threshold with hysteresis applying below
that. The ABI docs describe this directional use of hysteresis.
Either case seems to correspond to both other interrupts enabled at the
same time. Helpfully it's the or of the control bits so easy to support
as no obvious coupling between them. They seem to have separate status
bits as well.
> * dev_en use case is to shutdown/turn-on an AFE, amplifiers,
> but I wouldn't support in this series.
Often that can be tied to runtime PM, but I agree that is better
left for a future series.
>
> So, I believe the solution for the dt-binding is add the optional
> `gpio-controller`, and if set, expose the unused (not in the
> interrupts-names) as GPOutput. And not support inversion gp0<->gp1 of
> the mode, the specific threshold crossing (max, min, either), nor the
> dev_en use case.
>
> This then doesn't change the series much, just appends a commit adding
> the gpio-controller support.
Agreed.
...
>
> > >
> > > > > + ``ref-supply``.
> > > > > +
> > > > > +Also contain the following device attributes:
> > > > > +
> > > > > +.. list-table:: Device attributes
> > > > > + :header-rows: 1
> > > > > +
> > > > > + * - Attribute
> > > > > + - Description
> > > > > + * - ``samling_frequency``
> > > >
> > > > Check these.. sampling_frequency.
> > > >
> > > > > + - Sets the sets the device internal sample rate, used in the burst
> > > > > + averaging mode.
> > > >
> > > > It's not use otherwise? That's unusual ABI. I'd expect it to give
> > > > the right value at least when burst mode isn't used. Or is burst mode
> > > > the only way we do buffered capture?
> > > >
> > >
> > > It is not used otherwise, the device internal sample rate is used only
> > > to evenly distribute readings used in the burst averaging mode.
> > > There is no sampling trigger to evenly space the adc conversion reading,
> >
> > In event of no internal trigger, sampling_frequency is normally
> > 1/duration of a single scan (or sample if a per channel attribute).
> >
> >
>
> The duration of a single scan (duration between the convert-start high
> edge until RDY falling edge), would be ((n_avg - 1) / fosc + tconv)
> (datasheet fig.56, p.31) , where fosc is the internal timer frequency
> directly exposed in this series. I can make the driver use this formula,
> just setting it will be less straight forward, since currently a clear
> enum of the values the register supports is returned by the
> sampling_frequency_available attribute. Instead, it will scale based on
> the number of samples per burst (averaging ratio/n_avg). tconv is typ
> 270ns and max 320ns.
Agreed this can get a bit fiddly :(
There are also some slightly nasty race conditions around updating
available attributes. We had a solution for that a while back but
I'm not sure what happened to it and the complexity only really applies
when using in kernel consumers for those values rather than sysfs where
we can apply a driver local lock.
That n_avg seems to be oversampling - hence indeed we have to deal
with the affect of that on the sampling frequency (it's documented
in the ABI as sampling frequency is for the total time for oversampling
to deliver data).
Jonathan
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/7] iio: adc: Add support for ad4062
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
2025-10-13 7:27 ` [PATCH 1/7] dt-bindings: iio: adc: Add adi,ad4062 Jorge Marques
2025-10-13 7:28 ` [PATCH 2/7] docs: iio: New docs for ad4062 driver Jorge Marques
@ 2025-10-13 7:28 ` Jorge Marques
2025-10-18 16:10 ` Jonathan Cameron
2025-10-13 7:28 ` [PATCH 4/7] docs: iio: ad4062: Add IIO Trigger support Jorge Marques
` (3 subsequent siblings)
6 siblings, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, Jorge Marques
The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
register (SAR) analog-to-digital converter (ADC) that enables low-power,
high-density data acquisition solutions without sacrificing precision.
This ADC offers a unique balance of performance and power efficiency,
plus innovative features for seamlessly switching between
high-resolution and low-power modes tailored to the immediate needs of
the system. The AD4060/AD4062 are ideal for battery-powered, compact
data acquisition and edge sensing applications.
Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4062.c | 905 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 918 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index ce012c6c719023d3c0355676a335a55d92cf424c..ab4c95e331014c18895dc13699d616a91a080f8e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1406,6 +1406,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 58a14e6833f60008511838176b8b3ec0789dc95b..490c01d701bdd1543809cdefad4ed5573c051c24 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 d008f78dc010aff5f76f838283be294e0ebc27dd..ed71b2549c647b9d59d999e107ca1ee256d8ec04 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 0000000000000000000000000000000000000000..e55a69c62694a71a4e29f29b9a2bfeec3b16c990
--- /dev/null
+++ b/drivers/iio/adc/ad4062.c
@@ -0,0 +1,905 @@
+// 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>
+
+#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_INTERFACE_STATUS_NOT_RDY BIT(7)
+#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_MODES_DATA_FORMAT BIT(7)
+#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_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_TIMER_CONFIG_300KSPS 0x2
+#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_RDY BIT(7)
+#define AD4062_REG_DEVICE_STATUS_DEVICE_RESET BIT(6)
+#define AD4062_REG_MIN_SAMPLE 0x45
+#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 0x61
+
+#define AD4062_I3C_VENDOR 0x0177
+
+#define AD4050_MAX_AVG 0x7
+#define AD4062_MAX_AVG 0xB
+#define AD4062_MAX_RATE(x) ((x) == AD4062_2MSPS ? 2000000 : 500000)
+#define AD4062_FS_OFFSET(g) ((g) == AD4062_2MSPS ? 0 : 2)
+#define AD4062_FS(g) (&ad4062_conversion_freqs[AD4062_FS_OFFSET(g)])
+#define AD4062_FS_LEN(g) (ARRAY_SIZE(ad4062_conversion_freqs) - (AD4062_FS_OFFSET(g)))
+#define AD4062_MON_VAL_MAX_GAIN 1999970
+#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
+#define AD4062_T_CNVH_NS 10
+#define AD4062_VIO_3V3 3300000
+#define AD4062_SPI_MAX_ADC_XFER_SPEED(x) ((x) >= AD4062_VIO_3V3 ? 83333333 : 58823529)
+#define AD4062_SPI_MAX_REG_XFER_SPEED 16000000
+
+enum ad4062_grade {
+ AD4062_2MSPS,
+};
+
+enum ad4062_operation_mode {
+ AD4062_SAMPLE_MODE = 0,
+ AD4062_BURST_AVERAGING_MODE = 1,
+ AD4062_MONITOR_MODE = 3,
+};
+
+enum ad4062_gp_mode {
+ AD4062_GP_DISABLED,
+ AD4062_GP_INTR,
+ AD4062_GP_DRDY,
+};
+
+enum ad4062_interrupt_en {
+ AD4062_INTR_EN_NEITHER,
+ AD4062_INTR_EN_MIN,
+ AD4062_INTR_EN_MAX,
+ AD4062_INTR_EN_EITHER,
+};
+
+struct ad4062_chip_info {
+ const struct iio_chan_spec channels[1];
+ const char *name;
+ u16 prod_id;
+ u8 max_avg;
+ u8 grade;
+};
+
+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,
+ },
+};
+
+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;
+ u8 raw[4] __aligned(IIO_DMA_MINALIGN);
+};
+
+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 const char *const 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 */
+};
+
+static int ad4062_conversion_frequency_set(struct ad4062_state *st, u8 val)
+{
+ val += AD4062_FS_OFFSET(st->chip->grade);
+ return regmap_write(st->regmap, AD4062_REG_TIMER_CONFIG,
+ FIELD_PREP(AD4062_REG_TIMER_CONFIG_FS_MASK, val));
+}
+
+static int ad4062_sampling_frequency_get(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+
+ return st->sampling_frequency - AD4062_FS_OFFSET(st->chip->grade);
+}
+
+static int ad4062_sampling_frequency_set(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int val)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+
+ val += AD4062_FS_OFFSET(st->chip->grade);
+ st->sampling_frequency = val;
+
+ return 0;
+}
+
+static const struct iio_enum AD4062_2MSPS_conversion_freq_enum = {
+ .items = AD4062_FS(AD4062_2MSPS),
+ .num_items = AD4062_FS_LEN(AD4062_2MSPS),
+ .set = ad4062_sampling_frequency_set,
+ .get = ad4062_sampling_frequency_get,
+};
+
+#define AD4062_EXT_INFO(grade) \
+static struct iio_chan_spec_ext_info grade##_ext_info[] = { \
+ IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, \
+ &grade##_conversion_freq_enum), \
+ IIO_ENUM_AVAILABLE("sampling_frequency", IIO_SHARED_BY_ALL, \
+ &grade##_conversion_freq_enum), \
+ { } \
+}
+
+AD4062_EXT_INFO(AD4062_2MSPS);
+
+#define AD4062_CHAN(bits, grade) { \
+ .type = IIO_VOLTAGE, \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_CALIBSCALE) | \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .indexed = 1, \
+ .channel = 0, \
+ .has_ext_scan_type = 1, \
+ .ext_scan_type = ad4062_scan_type_##bits##_s, \
+ .num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s), \
+ .ext_info = grade##_ext_info, \
+}
+
+static const struct ad4062_chip_info ad4060_chip_info = {
+ .name = "ad4060",
+ .channels = { AD4062_CHAN(12, AD4062_2MSPS) },
+ .prod_id = 0x7A,
+ .max_avg = AD4050_MAX_AVG,
+ .grade = AD4062_2MSPS,
+};
+
+static const struct ad4062_chip_info ad4062_chip_info = {
+ .name = "ad4062",
+ .channels = { AD4062_CHAN(16, AD4062_2MSPS) },
+ .prod_id = 0x7C,
+ .max_avg = AD4062_MAX_AVG,
+ .grade = AD4062_2MSPS,
+};
+
+static int ad4062_set_oversampling_ratio(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int val)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (val < 1 || val > BIT(st->chip->max_avg + 1))
+ return -EINVAL;
+
+ /* 1 disables oversampling */
+ if (val == 1) {
+ st->mode = AD4062_SAMPLE_MODE;
+ } else {
+ val = ilog2(val);
+ st->mode = AD4062_BURST_AVERAGING_MODE;
+ ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
+ if (ret)
+ return ret;
+ }
+
+ 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);
+ if (ret)
+ return ret;
+
+ *val = BIT(buf + 1);
+
+ 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->raw, 2);
+ if (ret)
+ return ret;
+
+ val = get_unaligned_be16(st->raw);
+ 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->raw, 2);
+ if (ret)
+ return ret;
+
+ val = get_unaligned_be16(st->raw);
+ 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 = 0x81;
+ 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;
+
+ scan_type = iio_get_current_scan_type(indio_dev, chan);
+ if (IS_ERR(scan_type))
+ return PTR_ERR(scan_type);
+
+ u8 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 | AD4062_REG_GP_CONF_MODE_MSK_0,
+ val);
+ if (ret)
+ return ret;
+
+ 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_ADC_MODES,
+ 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;
+
+ return regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
+ AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
+ val);
+}
+
+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 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 0;
+
+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 >= 0) {
+ ret = devm_request_threaded_irq(dev, ret, NULL,
+ ad4062_irq_handler_drdy,
+ IRQF_ONESHOT, indio_dev->name,
+ indio_dev);
+ } else if (ret != -EPROBE_DEFER) {
+ 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);
+ }
+
+ 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)
+{
+ 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;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad4062_get_chan_scale(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ 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;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+static int ad4062_get_chan_calibscale(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+ u16 gain;
+ int ret;
+
+ ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL, &st->raw, 2);
+ if (ret)
+ return ret;
+
+ gain = get_unaligned_be16(&st->raw);
+
+ /* From datasheet: code out = code in × mon_val/0x8000 */
+ *val = gain / 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 iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int gain_int, int gain_frac)
+{
+ struct ad4062_state *st = iio_priv(indio_dev);
+ u64 gain;
+ int ret;
+
+ if (gain_int < 0 || gain_frac < 0)
+ return -EINVAL;
+
+ gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
+
+ if (gain > AD4062_MON_VAL_MAX_GAIN)
+ return -EINVAL;
+
+ put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
+ MICRO),
+ &st->raw);
+
+ ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL, &st->raw, 2);
+ if (ret)
+ return ret;
+
+ /* Enable scale if gain is not one. */
+ return regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
+ 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;
+ u8 addr = AD4062_REG_CONV_TRIGGER;
+ struct i3c_priv_xfer t[2] = {
+ {
+ .data.out = &addr,
+ .len = 1,
+ .rnw = false,
+ },
+ {
+ .data.in = &st->raw,
+ .len = 4,
+ .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->raw);
+ return ret;
+}
+
+static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
+{
+ 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_error;
+
+ ret = __ad4062_read_chan_raw(st, val);
+
+out_error:
+ pm_runtime_put_autosuspend(&st->i3cdev->dev);
+ return ret;
+}
+
+static int ad4062_read_raw_dispatch(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);
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ return ad4062_read_chan_raw(indio_dev, val);
+
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad4062_get_chan_calibscale(indio_dev, chan, val, val2);
+
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ return ad4062_get_oversampling_ratio(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)
+{
+ int ret;
+
+ if (info == IIO_CHAN_INFO_SCALE)
+ return ad4062_get_chan_scale(indio_dev, chan, val, val2);
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad4062_read_raw_dispatch(indio_dev, chan, val, val2, info);
+
+ iio_device_release_direct(indio_dev);
+ return ret ? ret : IIO_VAL_INT;
+}
+
+static int ad4062_write_raw_dispatch(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long info)
+{
+ switch (info) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ return ad4062_set_oversampling_ratio(indio_dev, chan, val);
+
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad4062_set_chan_calibscale(indio_dev, chan, val, val2);
+
+ 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)
+{
+ int ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad4062_write_raw_dispatch(indio_dev, chan, 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 (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ if (readval)
+ ret = regmap_read(st->regmap, reg, readval);
+ else
+ ret = regmap_write(st->regmap, reg, writeval);
+
+ iio_device_release_direct(indio_dev);
+ return ret;
+}
+
+static int 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 uv;
+
+ uv = devm_regulator_get_enable_read_voltage(dev, "vio");
+ if (uv < 0)
+ return dev_err_probe(dev, uv,
+ "Failed to enable and read vio voltage\n");
+
+ uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
+ if (uv < 0)
+ return dev_err_probe(dev, uv,
+ "Failed to enable vdd regulator\n");
+
+ st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+ *ref_sel = st->vref_uv == -ENODEV;
+ if (st->vref_uv == -ENODEV)
+ st->vref_uv = uv;
+ else if (st->vref_uv < 0)
+ return dev_err_probe(dev, st->vref_uv,
+ "Failed to enable and read ref voltage\n");
+ return 0;
+}
+
+static 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 = AD4062_FS_OFFSET(st->chip->grade);
+ st->indio_dev = indio_dev;
+
+ 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 dev_err_probe(dev, ret,
+ "AD4062 fields assertions failed\n");
+
+ 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 void ad4062_remove(struct i3c_device *i3cdev)
+{
+ i3c_device_disable_ibi(i3cdev);
+ i3c_device_free_ibi(i3cdev);
+}
+
+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);
+
+ fsleep(4000);
+ return ret;
+}
+
+static const struct dev_pm_ops ad4062_pm_ops = {
+ SET_RUNTIME_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,
+ .remove = ad4062_remove,
+ .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.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-10-13 7:28 ` [PATCH 3/7] iio: adc: Add support for ad4062 Jorge Marques
@ 2025-10-18 16:10 ` Jonathan Cameron
2025-11-23 19:48 ` Jorge Marques
0 siblings, 1 reply; 31+ messages in thread
From: Jonathan Cameron @ 2025-10-18 16:10 UTC (permalink / raw)
To: Jorge Marques
Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-iio, devicetree,
linux-kernel, linux-doc
On Mon, 13 Oct 2025 09:28:01 +0200
Jorge Marques <jorge.marques@analog.com> wrote:
> The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> register (SAR) analog-to-digital converter (ADC) that enables low-power,
> high-density data acquisition solutions without sacrificing precision.
> This ADC offers a unique balance of performance and power efficiency,
> plus innovative features for seamlessly switching between
> high-resolution and low-power modes tailored to the immediate needs of
> the system. The AD4060/AD4062 are ideal for battery-powered, compact
> data acquisition and edge sensing applications.
Little bit marketing heavy for a patch description.. I'd stick to
type, resolution and typical usecases. Skip the sacrificing precision etc
wording! It'll just make the readers eyes glaze over ;)
The mixing of regmap and direct accesses is a little unfortunate but seems unavoidable.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Various comments inline. In general looks good for a v1.
> diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e55a69c62694a71a4e29f29b9a2bfeec3b16c990
> --- /dev/null
> +++ b/drivers/iio/adc/ad4062.c
> @@ -0,0 +1,905 @@
> +#define AD4062_REG_DEVICE_STATUS_DEVICE_RESET BIT(6)
> +#define AD4062_REG_MIN_SAMPLE 0x45
> +#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 0x61
I'd define this in terms of the reg that it happens to be
#define AD4052_MAX_REG AD6062_REG_CONF_AUTO
> +#define AD4062_VIO_3V3 3300000
This is a real value rather than a magic number so better to not hide it in a define.
Just use the value inline.
> +#define AD4062_SPI_MAX_ADC_XFER_SPEED(x) ((x) >= AD4062_VIO_3V3 ? 83333333 : 58823529)
> +#define AD4062_SPI_MAX_REG_XFER_SPEED 16000000
So these might explain the locking in debugfs functions, but they aren't currently used.
So either use them or drop them.
> +
> +enum ad4062_grade {
> + AD4062_2MSPS,
> +};
> +
> +enum ad4062_operation_mode {
> + AD4062_SAMPLE_MODE = 0,
> + AD4062_BURST_AVERAGING_MODE = 1,
> + AD4062_MONITOR_MODE = 3,
> +};
> +
> +enum ad4062_gp_mode {
> + AD4062_GP_DISABLED,
> + AD4062_GP_INTR,
> + AD4062_GP_DRDY,
> +};
> +
> +enum ad4062_interrupt_en {
> + AD4062_INTR_EN_NEITHER,
Where these correspond to specific register values make sure
to specify those values with = 0x0 etc
I'd use defines unless the type is useful, for example as ad4062_operation_mode above is.
> + AD4062_INTR_EN_MIN,
> + AD4062_INTR_EN_MAX,
> + AD4062_INTR_EN_EITHER,
> +};
> +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;
> + u8 raw[4] __aligned(IIO_DMA_MINALIGN);
As I mention below, many of the uses of this seem to be a single __be16 so
I'd suggestion a union with one of those so we know it's aligned and can use
sizeof() instead of hard coded 2s. If you prefer just add a __be16 variable
after this as that always fits anyway (IIO_DMA_MINALIGN >= 8).
> +};
> +
> +static const struct iio_enum AD4062_2MSPS_conversion_freq_enum = {
> + .items = AD4062_FS(AD4062_2MSPS),
> + .num_items = AD4062_FS_LEN(AD4062_2MSPS),
> + .set = ad4062_sampling_frequency_set,
> + .get = ad4062_sampling_frequency_get,
> +};
> +
> +#define AD4062_EXT_INFO(grade) \
> +static struct iio_chan_spec_ext_info grade##_ext_info[] = { \
> + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, \
> + &grade##_conversion_freq_enum), \
if it's shared by all why can't you use the standard
info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) ?
That enables simple in kernel use etc that is a pain with ext info.
> + IIO_ENUM_AVAILABLE("sampling_frequency", IIO_SHARED_BY_ALL, \
> + &grade##_conversion_freq_enum), \
And this should use the read_avail handling rather than being done this way.
ext_info is for stuff that isn't particularly standard or which doesn't
fit in the normal interfaces (like mount matrix).
> + { } \
> +}
> +
> +AD4062_EXT_INFO(AD4062_2MSPS);
> +
> +#define AD4062_CHAN(bits, grade) { \
> + .type = IIO_VOLTAGE, \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_CALIBSCALE) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .indexed = 1, \
> + .channel = 0, \
> + .has_ext_scan_type = 1, \
> + .ext_scan_type = ad4062_scan_type_##bits##_s, \
> + .num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s), \
> + .ext_info = grade##_ext_info, \
> +}
> +
> +static const struct ad4062_chip_info ad4060_chip_info = {
> + .name = "ad4060",
> + .channels = { AD4062_CHAN(12, AD4062_2MSPS) },
> + .prod_id = 0x7A,
> + .max_avg = AD4050_MAX_AVG,
> + .grade = AD4062_2MSPS,
> +};
> +
> +static const struct ad4062_chip_info ad4062_chip_info = {
> + .name = "ad4062",
> + .channels = { AD4062_CHAN(16, AD4062_2MSPS) },
> + .prod_id = 0x7C,
> + .max_avg = AD4062_MAX_AVG,
> + .grade = AD4062_2MSPS,
We'd normally only introduce elements to chip_info when then differ between
supported chips. For now this .grade seems unnecessary complexity.
> +};
> +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->raw, 2);
You do a lot of __be16 reads in this driver. Maybe use a union for raw so we can
type these correctly and use sizeof() for that 2.
> + if (ret)
> + return ret;
> +
> + val = get_unaligned_be16(st->raw);
> + 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->raw, 2);
> + if (ret)
> + return ret;
> +
> + val = get_unaligned_be16(st->raw);
> + 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_soft_reset(struct ad4062_state *st)
> +{
> + u8 val = 0x81;
That probably wants to a define as definitely a magic number.
> + 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;
> +
> + scan_type = iio_get_current_scan_type(indio_dev, chan);
> + if (IS_ERR(scan_type))
> + return PTR_ERR(scan_type);
> +
> + u8 val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_INTR) |
I'm a bit confused on why this routes anything to GP0 given we haven't requested that interrupt.
> + 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 | AD4062_REG_GP_CONF_MODE_MSK_0,
> + val);
> + if (ret)
> + return ret;
> +
> + 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));
Filling val up here and using it much later seems odd. Probably better moved down.
Also excess brackets.
I'm lost on what this is doing though. Seems to be enabling interrupts that aren't
routed anywhere.
> +
> + ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
> + 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;
> +
> + return regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
> + AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
> + val);
> +}
> +
> +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 >= 0) {
> + ret = devm_request_threaded_irq(dev, ret, NULL,
> + ad4062_irq_handler_drdy,
> + IRQF_ONESHOT, indio_dev->name,
> + indio_dev);
return directly here.
> + } else if (ret != -EPROBE_DEFER) {
I'd prefer we did the error handling first. It's a little fiddly but
something like
if (ret == -EPROBE_DEFER)
return ret;
if (ret < 0) { //I'd prefer an explicit match against whatever indicates nothing provided.
return ...
return devm_request_threaded_irq();
> + 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);
> + }
> +
> + return ret;
> +}
> +
> +static const int ad4062_oversampling_avail[] = {
> + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
I don't mind this style, but you'll likely get feedback that you might as well
use
BIT(0), BIT(1),
etc here as it makes it less likely a typo will hid in these powers of 2.
> +};
> +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> +{
> + struct i3c_device *i3cdev = st->i3cdev;
> + u8 addr = AD4062_REG_CONV_TRIGGER;
> + struct i3c_priv_xfer t[2] = {
> + {
> + .data.out = &addr,
> + .len = 1,
> + .rnw = false,
> + },
> + {
> + .data.in = &st->raw,
> + .len = 4,
> + .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->raw);
> + return ret;
return 0;
Makes it clear it must be 0 if we get here and that this is the 'good' exit path.
> +}
> +
> +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
There is a nice new
ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
let you do this using cleanup.h style.
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
This looks like a perfect example of where those help.
When I catch up with review backlog I plan to look for other
places to use that infrastructure in IIO.
> + if (ret)
> + return ret;
> +
> + ret = ad4062_set_operation_mode(st, st->mode);
> + if (ret)
> + goto out_error;
> +
> + ret = __ad4062_read_chan_raw(st, val);
> +
> +out_error:
> + pm_runtime_put_autosuspend(&st->i3cdev->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 (!iio_device_claim_direct(indio_dev))
Add a comment on why reading registers isn't fine when buffered mode is in use.
If it's just a case of preventing writes that will change the state, then we typically
don't prevent those. It's a debug interface, no problem with people shooting themselves
in the foot.
If you are avoiding disrupting RMW sequences then use a local lock to ensure that
is atomic, not this big heavy one.
> + return -EBUSY;
> +
> + if (readval)
> + ret = regmap_read(st->regmap, reg, readval);
> + else
> + ret = regmap_write(st->regmap, reg, writeval);
> +
> + iio_device_release_direct(indio_dev);
> + return ret;
> +}
> +
> +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> +{
> + struct device *dev = &st->i3cdev->dev;
> + int uv;
> +
> + uv = devm_regulator_get_enable_read_voltage(dev, "vio");
Why are you reading the voltages then throwing them away?
If we don't need to read it just use
devm_regulator_get_enabled()
> + if (uv < 0)
> + return dev_err_probe(dev, uv,
> + "Failed to enable and read vio voltage\n");
> +
> + uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
Unless there is some ordering constraint I don't know of, you shouldn't
fail on being unable to read the vdd voltage unless we are actually using it.
So try ref first and if that succeeds use devm_regulator_get_enabled(), if not
try the voltage reading approach..
> + if (uv < 0)
> + return dev_err_probe(dev, uv,
> + "Failed to enable vdd regulator\n");
> +
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> + *ref_sel = st->vref_uv == -ENODEV;
> + if (st->vref_uv == -ENODEV)
> + st->vref_uv = uv;
> + else if (st->vref_uv < 0)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to enable and read ref voltage\n");
> + return 0;
> +}
> +
> +static 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),
> + {}
Trivial but I'm trying to slowly standardize spacing in IIO for these and
a while back chose at random (more or less) to go with
{ }
> +};
> +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 = AD4062_FS_OFFSET(st->chip->grade);
> + st->indio_dev = indio_dev;
> +
> + 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 dev_err_probe(dev, ret,
> + "AD4062 fields assertions failed\n");
Seems like you print enough in the check_ids function that this print is
perhaps just confusing? Maybe add a print in the only path in there that
doesn't already have one.
> +
> + 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);
Related to below. There is a race on tear down as you'll remove some of the
i3c infrastructure before removing the userspace ABI that might result in
that being used.
> +}
> +
> +static void ad4062_remove(struct i3c_device *i3cdev)
> +{
> + i3c_device_disable_ibi(i3cdev);
> + i3c_device_free_ibi(i3cdev);
You should never mix devm_ based cleanup and manual cleanup.
Basic rule is that the 1st time you add something that needs non devm cleanup
for some reason then you must stop using any devm_ calls after that in probe().
There are some handy helpers for defining your own cleanup functions.
devm_add_action_or_reset() is helpful for extending how far we can use devm_
and typically solves this problem.
> +}
> +
> +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 it failed, little point in sleeping. I'd do
if (ret)
return ret;
fsleep(4000);
return 0;
So the flow on error is clearer.
> +
> + fsleep(4000);
> + return ret;
> +}
> +
> +static const struct dev_pm_ops ad4062_pm_ops = {
> + SET_RUNTIME_PM_OPS(ad4062_runtime_suspend, ad4062_runtime_resume, NULL)
Should have trailing comma.
Also rare that we can't just enable the system suspend/resume stuff that uses
the runtime PM callbacks by using:
DEFINE_RUNTIME_DEV_PM_OPS(). If we can't I'd add a comment on why not alongside
this code.
> +};
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-10-18 16:10 ` Jonathan Cameron
@ 2025-11-23 19:48 ` Jorge Marques
2025-11-24 7:43 ` Andy Shevchenko
0 siblings, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-11-23 19:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:01 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
>
> > The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> > register (SAR) analog-to-digital converter (ADC) that enables low-power,
> > high-density data acquisition solutions without sacrificing precision.
> > This ADC offers a unique balance of performance and power efficiency,
> > plus innovative features for seamlessly switching between
> > high-resolution and low-power modes tailored to the immediate needs of
> > the system. The AD4060/AD4062 are ideal for battery-powered, compact
> > data acquisition and edge sensing applications.
Hi Jonathan,
Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.
>
> Little bit marketing heavy for a patch description.. I'd stick to
> type, resolution and typical usecases. Skip the sacrificing precision etc
> wording! It'll just make the readers eyes glaze over ;)
>
Yep, will tune down a bit. The description was from the website.
> The mixing of regmap and direct accesses is a little unfortunate but seems unavoidable.
>
Yes, it is not possible to use only the regmap due to the adc sample
transfers that require reads without writing the address.
>
> >
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
>
> Various comments inline. In general looks good for a v1.
>
>
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..e55a69c62694a71a4e29f29b9a2bfeec3b16c990
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4062.c
> > @@ -0,0 +1,905 @@
>
> > +#define AD4062_REG_DEVICE_STATUS_DEVICE_RESET BIT(6)
> > +#define AD4062_REG_MIN_SAMPLE 0x45
> > +#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 0x61
>
> I'd define this in terms of the reg that it happens to be
> #define AD4052_MAX_REG AD6062_REG_CONF_AUTO
>
Ack.
>
> > +#define AD4062_VIO_3V3 3300000
>
> This is a real value rather than a magic number so better to not hide it in a define.
> Just use the value inline.
>
Ack.
>
> > +#define AD4062_SPI_MAX_ADC_XFER_SPEED(x) ((x) >= AD4062_VIO_3V3 ? 83333333 : 58823529)
> > +#define AD4062_SPI_MAX_REG_XFER_SPEED 16000000
> So these might explain the locking in debugfs functions, but they aren't currently used.
> So either use them or drop them.
>
Those are dead code for this driver, I will remove as well as others
unused defines.
> > +
> > +enum ad4062_grade {
> > + AD4062_2MSPS,
> > +};
Since only one speed grade is available in the market, this will be
dropped and all usage simplified.
> > +
> > +enum ad4062_operation_mode {
> > + AD4062_SAMPLE_MODE = 0,
> > + AD4062_BURST_AVERAGING_MODE = 1,
> > + AD4062_MONITOR_MODE = 3,
> > +};
> > +
> > +enum ad4062_gp_mode {
> > + AD4062_GP_DISABLED,
> > + AD4062_GP_INTR,
> > + AD4062_GP_DRDY,
> > +};
> > +
> > +enum ad4062_interrupt_en {
> > + AD4062_INTR_EN_NEITHER,
> Where these correspond to specific register values make sure
> to specify those values with = 0x0 etc
>
> I'd use defines unless the type is useful, for example as ad4062_operation_mode above is.
>
Ack.
> > + AD4062_INTR_EN_MIN,
> > + AD4062_INTR_EN_MAX,
> > + AD4062_INTR_EN_EITHER,
> > +};
>
> > +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;
> > + u8 raw[4] __aligned(IIO_DMA_MINALIGN);
> As I mention below, many of the uses of this seem to be a single __be16 so
> I'd suggestion a union with one of those so we know it's aligned and can use
> sizeof() instead of hard coded 2s. If you prefer just add a __be16 variable
> after this as that always fits anyway (IIO_DMA_MINALIGN >= 8).
>
Will do
union {
__be32 be32;
__be16 be16;
u8 bytes[4];
} buf __aligned(IIO_DMA_MINALIGN);
> > +};
>
> > +
> > +static const struct iio_enum AD4062_2MSPS_conversion_freq_enum = {
> > + .items = AD4062_FS(AD4062_2MSPS),
> > + .num_items = AD4062_FS_LEN(AD4062_2MSPS),
> > + .set = ad4062_sampling_frequency_set,
> > + .get = ad4062_sampling_frequency_get,
> > +};
> > +
> > +#define AD4062_EXT_INFO(grade) \
> > +static struct iio_chan_spec_ext_info grade##_ext_info[] = { \
> > + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, \
> > + &grade##_conversion_freq_enum), \
>
> if it's shared by all why can't you use the standard
> info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) ?
> That enables simple in kernel use etc that is a pain with ext info.
>
Yes definitely.
>
> > + IIO_ENUM_AVAILABLE("sampling_frequency", IIO_SHARED_BY_ALL, \
> > + &grade##_conversion_freq_enum), \
>
> And this should use the read_avail handling rather than being done this way.
> ext_info is for stuff that isn't particularly standard or which doesn't
> fit in the normal interfaces (like mount matrix).
>
Ack.
> > + { } \
> > +}
> > +
> > +AD4062_EXT_INFO(AD4062_2MSPS);
> > +
> > +#define AD4062_CHAN(bits, grade) { \
> > + .type = IIO_VOLTAGE, \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_CALIBSCALE) | \
> > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > + .indexed = 1, \
> > + .channel = 0, \
> > + .has_ext_scan_type = 1, \
> > + .ext_scan_type = ad4062_scan_type_##bits##_s, \
> > + .num_ext_scan_type = ARRAY_SIZE(ad4062_scan_type_##bits##_s), \
> > + .ext_info = grade##_ext_info, \
> > +}
> > +
> > +static const struct ad4062_chip_info ad4060_chip_info = {
> > + .name = "ad4060",
> > + .channels = { AD4062_CHAN(12, AD4062_2MSPS) },
> > + .prod_id = 0x7A,
> > + .max_avg = AD4050_MAX_AVG,
> > + .grade = AD4062_2MSPS,
> > +};
> > +
> > +static const struct ad4062_chip_info ad4062_chip_info = {
> > + .name = "ad4062",
> > + .channels = { AD4062_CHAN(16, AD4062_2MSPS) },
> > + .prod_id = 0x7C,
> > + .max_avg = AD4062_MAX_AVG,
> > + .grade = AD4062_2MSPS,
>
> We'd normally only introduce elements to chip_info when then differ between
> supported chips. For now this .grade seems unnecessary complexity.
>
Yes, grade will be completely removed since the is only one speed grade
in the market.
> > +};
>
>
> > +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->raw, 2);
>
> You do a lot of __be16 reads in this driver. Maybe use a union for raw so we can
> type these correctly and use sizeof() for that 2.
Ack.
>
> > + if (ret)
> > + return ret;
> > +
> > + val = get_unaligned_be16(st->raw);
> > + 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->raw, 2);
> > + if (ret)
> > + return ret;
> > +
> > + val = get_unaligned_be16(st->raw);
> > + 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_soft_reset(struct ad4062_state *st)
> > +{
> > + u8 val = 0x81;
>
> That probably wants to a define as definitely a magic number.
>
Ack.
> > + 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;
> > +
> > + scan_type = iio_get_current_scan_type(indio_dev, chan);
> > + if (IS_ERR(scan_type))
> > + return PTR_ERR(scan_type);
> > +
> > + u8 val = FIELD_PREP(AD4062_REG_GP_CONF_MODE_MSK_0, AD4062_GP_INTR) |
>
> I'm a bit confused on why this routes anything to GP0 given we haven't requested that interrupt.
>
Is used in the next commit, on v2 I will be more careful to define things
only for the particular patch.
> > + 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 | AD4062_REG_GP_CONF_MODE_MSK_0,
> > + val);
> > + if (ret)
> > + return ret;
> > +
> > + 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));
>
> Filling val up here and using it much later seems odd. Probably better moved down.
> Also excess brackets.
> I'm lost on what this is doing though. Seems to be enabling interrupts that aren't
> routed anywhere.
Ack, reason same as above, belongs to the next commit.
>
> > +
> > + ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_MODES,
> > + 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;
> > +
> > + return regmap_update_bits(st->regmap, AD4062_REG_INTR_CONF,
> > + AD4062_REG_INTR_CONF_EN_MSK_0 | AD4062_REG_INTR_CONF_EN_MSK_1,
> > + val);
> > +}
>
> > +
> > +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 >= 0) {
> > + ret = devm_request_threaded_irq(dev, ret, NULL,
> > + ad4062_irq_handler_drdy,
> > + IRQF_ONESHOT, indio_dev->name,
> > + indio_dev);
> return directly here.
Ack
> > + } else if (ret != -EPROBE_DEFER) {
> I'd prefer we did the error handling first. It's a little fiddly but
> something like
Ack.
>
> if (ret == -EPROBE_DEFER)
> return ret;
> if (ret < 0) { //I'd prefer an explicit match against whatever indicates nothing provided.
> return ...
>
> return devm_request_threaded_irq();
>
> > + 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);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const int ad4062_oversampling_avail[] = {
> > + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> I don't mind this style, but you'll likely get feedback that you might as well
> use
> BIT(0), BIT(1),
> etc here as it makes it less likely a typo will hid in these powers of 2.
>
I think it is fine as is.
> > +};
>
> > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> > +{
> > + struct i3c_device *i3cdev = st->i3cdev;
> > + u8 addr = AD4062_REG_CONV_TRIGGER;
> > + struct i3c_priv_xfer t[2] = {
> > + {
> > + .data.out = &addr,
> > + .len = 1,
> > + .rnw = false,
> > + },
> > + {
> > + .data.in = &st->raw,
> > + .len = 4,
> > + .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->raw);
> > + return ret;
>
> return 0;
> Makes it clear it must be 0 if we get here and that this is the 'good' exit path.
>
Ack.
> > +}
> > +
> > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > +{
> > + struct ad4062_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> There is a nice new
> ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> let you do this using cleanup.h style.
>
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
>
> This looks like a perfect example of where those help.
>
> When I catch up with review backlog I plan to look for other
> places to use that infrastructure in IIO.
>
I tried implementing, here becomes
ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
At buffer and monitor, since we put the device as active during the
lifetime of the buffer and monitor mode, either I leave as is, or I bump
the counter with pm_runtime_get_noresume, so when the method leaves, the
counter drops to 1 and not 0, then on disable I drop the counter back to
0 and queue the autosuspend with pm_runtime_put_autosuspend.
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad4062_set_operation_mode(st, st->mode);
> > + if (ret)
> > + goto out_error;
> > +
> > + ret = __ad4062_read_chan_raw(st, val);
> > +
> > +out_error:
> > + pm_runtime_put_autosuspend(&st->i3cdev->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 (!iio_device_claim_direct(indio_dev))
> Add a comment on why reading registers isn't fine when buffered mode is in use.
>
> If it's just a case of preventing writes that will change the state, then we typically
> don't prevent those. It's a debug interface, no problem with people shooting themselves
> in the foot.
>
> If you are avoiding disrupting RMW sequences then use a local lock to ensure that
> is atomic, not this big heavy one.
>
I dropped the lock and will let the user mess-up the transfer.
On buffer or monitor mode, a register access will cause the device to
drop of those modes. For the buffer acquisition, it will stop filling up
the buffer. The user can then recover the state by disabling and
enabling again the buffer or monitor mode.
> > + return -EBUSY;
> > +
> > + if (readval)
> > + ret = regmap_read(st->regmap, reg, readval);
> > + else
> > + ret = regmap_write(st->regmap, reg, writeval);
> > +
> > + iio_device_release_direct(indio_dev);
> > + return ret;
> > +}
>
> > +
> > +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> > +{
> > + struct device *dev = &st->i3cdev->dev;
> > + int uv;
> > +
> > + uv = devm_regulator_get_enable_read_voltage(dev, "vio");
>
> Why are you reading the voltages then throwing them away?
> If we don't need to read it just use
> devm_regulator_get_enabled()
>
Ack, I will just enable vio, not read it.
>
> > + if (uv < 0)
> > + return dev_err_probe(dev, uv,
> > + "Failed to enable and read vio voltage\n");
> > +
> > + uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
>
> Unless there is some ordering constraint I don't know of, you shouldn't
> fail on being unable to read the vdd voltage unless we are actually using it.
> So try ref first and if that succeeds use devm_regulator_get_enabled(), if not
> try the voltage reading approach..
>
Ack, I reordered it and won't read vdd if ref exists (!= -ENODEV).
> > + if (uv < 0)
> > + return dev_err_probe(dev, uv,
> > + "Failed to enable vdd regulator\n");
> > +
> > + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> > + *ref_sel = st->vref_uv == -ENODEV;
> > + if (st->vref_uv == -ENODEV)
> > + st->vref_uv = uv;
> > + else if (st->vref_uv < 0)
> > + return dev_err_probe(dev, st->vref_uv,
> > + "Failed to enable and read ref voltage\n");
> > + return 0;
> > +}
> > +
> > +static 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),
> > + {}
> Trivial but I'm trying to slowly standardize spacing in IIO for these and
> a while back chose at random (more or less) to go with
> { }
Ack.
>
> > +};
> > +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 = AD4062_FS_OFFSET(st->chip->grade);
> > + st->indio_dev = indio_dev;
> > +
> > + 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 dev_err_probe(dev, ret,
> > + "AD4062 fields assertions failed\n");
> Seems like you print enough in the check_ids function that this print is
> perhaps just confusing? Maybe add a print in the only path in there that
> doesn't already have one.
Ack, I will remove this error message.
> > +
> > + 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);
>
> Related to below. There is a race on tear down as you'll remove some of the
> i3c infrastructure before removing the userspace ABI that might result in
> that being used.
I submitted a patch at
https://lore.kernel.org/linux-i3c/aRYLc%2F+KAD13g7T7@lizhi-Precision-Tower-5810/T/#t
to allow using the devm_ based cleanup.
As is, the devm_ calls occurs after i3c_device_remove, this that calls
i3c_device_free_ibi and causes an exception if i3c_device_disable_ibi is
not called.
>
> > +}
> > +
> > +static void ad4062_remove(struct i3c_device *i3cdev)
> > +{
> > + i3c_device_disable_ibi(i3cdev);
> > + i3c_device_free_ibi(i3cdev);
> You should never mix devm_ based cleanup and manual cleanup.
>
> Basic rule is that the 1st time you add something that needs non devm cleanup
> for some reason then you must stop using any devm_ calls after that in probe().
>
> There are some handy helpers for defining your own cleanup functions.
> devm_add_action_or_reset() is helpful for extending how far we can use devm_
> and typically solves this problem.
>
Explained above, will use the devm_ helper in v2.
> > +}
>
> > +
> > +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 it failed, little point in sleeping. I'd do
> if (ret)
> return ret;
>
> fsleep(4000);
> return 0;
>
> So the flow on error is clearer.
Ack.
> > +
> > + fsleep(4000);
> > + return ret;
> > +}
> > +
> > +static const struct dev_pm_ops ad4062_pm_ops = {
> > + SET_RUNTIME_PM_OPS(ad4062_runtime_suspend, ad4062_runtime_resume, NULL)
> Should have trailing comma.
>
> Also rare that we can't just enable the system suspend/resume stuff that uses
> the runtime PM callbacks by using:
> DEFINE_RUNTIME_DEV_PM_OPS(). If we can't I'd add a comment on why not alongside
> this code.
>
> > +};
>
>
Ack.
Best regards,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-11-23 19:48 ` Jorge Marques
@ 2025-11-24 7:43 ` Andy Shevchenko
2025-11-24 8:57 ` Jorge Marques
0 siblings, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2025-11-24 7:43 UTC (permalink / raw)
To: Jorge Marques
Cc: Jonathan Cameron, Jorge Marques, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > On Mon, 13 Oct 2025 09:28:01 +0200
> > Jorge Marques <jorge.marques@analog.com> wrote:
> Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.
...
> > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > +{
> > > + struct ad4062_state *st = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > There is a nice new
> > ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > let you do this using cleanup.h style.
> >
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> >
> > This looks like a perfect example of where those help.
> >
> > When I catch up with review backlog I plan to look for other
> > places to use that infrastructure in IIO.
> >
> I tried implementing, here becomes
>
> ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
>
> At buffer and monitor, since we put the device as active during the
> lifetime of the buffer and monitor mode, either I leave as is, or I bump
> the counter with pm_runtime_get_noresume, so when the method leaves, the
> counter drops to 1 and not 0, then on disable I drop the counter back to
> 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = ad4062_set_operation_mode(st, st->mode);
> > > + if (ret)
> > > + goto out_error;
> > > +
> > > + ret = __ad4062_read_chan_raw(st, val);
> > > +
> > > +out_error:
> > > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > + return ret;
> > > +}
I read the above code, I read it again, I don't understand the reasoning.
The ACQUIRE() doesn't change the behaviour of the above code.
If you need to bump the reference counter, it should be done somewhere else
where it affects the flow, or this code has a bug.
If I miss something, please elaborate.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-11-24 7:43 ` Andy Shevchenko
@ 2025-11-24 8:57 ` Jorge Marques
2025-11-24 9:10 ` Andy Shevchenko
2025-11-24 9:11 ` Andy Shevchenko
0 siblings, 2 replies; 31+ messages in thread
From: Jorge Marques @ 2025-11-24 8:57 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Jorge Marques, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > Jorge Marques <jorge.marques@analog.com> wrote:
>
> > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.
>
> ...
>
Hi Andy,
> > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > +{
> > > > + struct ad4062_state *st = iio_priv(indio_dev);
> > > > + int ret;
> > > > +
> > > > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > > There is a nice new
> > > ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > let you do this using cleanup.h style.
> > >
> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > >
> > > This looks like a perfect example of where those help.
> > >
> > > When I catch up with review backlog I plan to look for other
> > > places to use that infrastructure in IIO.
> > >
> > I tried implementing, here becomes
> >
> > ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> >
> > At buffer and monitor, since we put the device as active during the
> > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > counter drops to 1 and not 0, then on disable I drop the counter back to
> > 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = ad4062_set_operation_mode(st, st->mode);
> > > > + if (ret)
> > > > + goto out_error;
> > > > +
> > > > + ret = __ad4062_read_chan_raw(st, val);
> > > > +
> > > > +out_error:
> > > > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > + return ret;
> > > > +}
>
> I read the above code, I read it again, I don't understand the reasoning.
> The ACQUIRE() doesn't change the behaviour of the above code.
>
> If you need to bump the reference counter, it should be done somewhere else
> where it affects the flow, or this code has a bug.
>
> If I miss something, please elaborate.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
The part highlighted does not require bumping the reference counter, but
at the buffer acquisition and monitor mode, to not put the device back
in low power mode during the lifetime of those operations.
Buffer more:
static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
{
struct ad4062_state *st = iio_priv(indio_dev);
int ret;
// [ Some code ]
ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
if (ret)
return ret;
// [ More code ]
pm_runtime_get_noresume(&st->i3cdev->dev);
return 0;
}
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;
}
Monitor mode:
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;
// [ Some code ]
pm_runtime_get_noresume(&st->i3cdev->dev);
return 0;
}
The raw read does not require this behaviour, when the method returns,
the device returns to low power mode:
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);
}
I will submitted v2 shortly.
Best Regards,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-11-24 8:57 ` Jorge Marques
@ 2025-11-24 9:10 ` Andy Shevchenko
2025-12-06 16:31 ` Jonathan Cameron
2025-11-24 9:11 ` Andy Shevchenko
1 sibling, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2025-11-24 9:10 UTC (permalink / raw)
To: Jorge Marques
Cc: Jonathan Cameron, Jorge Marques, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> > > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > > Jorge Marques <jorge.marques@analog.com> wrote:
> > > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.
...
> > > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > > +{
> > > > > + struct ad4062_state *st = iio_priv(indio_dev);
> > > > > + int ret;
> > > > > +
> > > > > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > > > There is a nice new
> > > > ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > > let you do this using cleanup.h style.
> > > >
> > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > > >
> > > > This looks like a perfect example of where those help.
> > > >
> > > > When I catch up with review backlog I plan to look for other
> > > > places to use that infrastructure in IIO.
> > > >
> > > I tried implementing, here becomes
> > >
> > > ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > > ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > >
> > > At buffer and monitor, since we put the device as active during the
> > > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > > counter drops to 1 and not 0, then on disable I drop the counter back to
> > > 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > > >
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + ret = ad4062_set_operation_mode(st, st->mode);
> > > > > + if (ret)
> > > > > + goto out_error;
> > > > > +
> > > > > + ret = __ad4062_read_chan_raw(st, val);
> > > > > +
> > > > > +out_error:
> > > > > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > > + return ret;
> > > > > +}
> >
> > I read the above code, I read it again, I don't understand the reasoning.
> > The ACQUIRE() doesn't change the behaviour of the above code.
> >
> > If you need to bump the reference counter, it should be done somewhere else
> > where it affects the flow, or this code has a bug.
> >
> > If I miss something, please elaborate.
>
> The part highlighted does not require bumping the reference counter, but
> at the buffer acquisition and monitor mode, to not put the device back
> in low power mode during the lifetime of those operations.
>
> Buffer more:
>
> static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> {
> struct ad4062_state *st = iio_priv(indio_dev);
> int ret;
>
> // [ Some code ]
>
> ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> if (ret)
> return ret;
>
> // [ More code ]
> pm_runtime_get_noresume(&st->i3cdev->dev);
Yes, this looks good if it makes the error paths cleaner.
Also consider adding
struct device *dev = &st->i3cdev->dev;
at the top of the functions that use it, it might make code better to read.
> return 0;
> }
>
> 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;
> }
>
> Monitor mode:
>
> 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;
>
> // [ Some code ]
>
> pm_runtime_get_noresume(&st->i3cdev->dev);
> return 0;
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-11-24 9:10 ` Andy Shevchenko
@ 2025-12-06 16:31 ` Jonathan Cameron
0 siblings, 0 replies; 31+ messages in thread
From: Jonathan Cameron @ 2025-12-06 16:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jorge Marques, Jorge Marques, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Mon, 24 Nov 2025 11:10:32 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
> > > On Sun, Nov 23, 2025 at 08:48:09PM +0100, Jorge Marques wrote:
> > > > On Sat, Oct 18, 2025 at 05:10:32PM +0100, Jonathan Cameron wrote:
> > > > > On Mon, 13 Oct 2025 09:28:01 +0200
> > > > > Jorge Marques <jorge.marques@analog.com> wrote:
>
> > > > Mostly acknowledgements and explanations, except a comment on ACQUIRE usage.
>
> ...
>
> > > > > > +static int ad4062_read_chan_raw(struct iio_dev *indio_dev, int *val)
> > > > > > +{
> > > > > > + struct ad4062_state *st = iio_priv(indio_dev);
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > > > > There is a nice new
> > > > > ACQUIRE()/ACQUIRE_ERR() related set of conditional guards defined that
> > > > > let you do this using cleanup.h style.
> > > > >
> > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a0abc39450a3123fd52533a662fbd37e0d1508c
> > > > >
> > > > > This looks like a perfect example of where those help.
> > > > >
> > > > > When I catch up with review backlog I plan to look for other
> > > > > places to use that infrastructure in IIO.
> > > > >
> > > > I tried implementing, here becomes
> > > >
> > > > ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > > > ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > > >
> > > > At buffer and monitor, since we put the device as active during the
> > > > lifetime of the buffer and monitor mode, either I leave as is, or I bump
> > > > the counter with pm_runtime_get_noresume, so when the method leaves, the
> > > > counter drops to 1 and not 0, then on disable I drop the counter back to
> > > > 0 and queue the autosuspend with pm_runtime_put_autosuspend.
> > > > >
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + ret = ad4062_set_operation_mode(st, st->mode);
> > > > > > + if (ret)
> > > > > > + goto out_error;
> > > > > > +
> > > > > > + ret = __ad4062_read_chan_raw(st, val);
> > > > > > +
> > > > > > +out_error:
> > > > > > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > > > > > + return ret;
> > > > > > +}
> > >
> > > I read the above code, I read it again, I don't understand the reasoning.
> > > The ACQUIRE() doesn't change the behaviour of the above code.
> > >
> > > If you need to bump the reference counter, it should be done somewhere else
> > > where it affects the flow, or this code has a bug.
> > >
> > > If I miss something, please elaborate.
> >
> > The part highlighted does not require bumping the reference counter, but
> > at the buffer acquisition and monitor mode, to not put the device back
> > in low power mode during the lifetime of those operations.
> >
> > Buffer more:
> >
> > static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > {
> > struct ad4062_state *st = iio_priv(indio_dev);
> > int ret;
> >
> > // [ Some code ]
> >
> > ACQUIRE(pm_runtime_active_try_enabled, pm)(&st->i3cdev->dev);
> > ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> > if (ret)
> > return ret;
> >
> > // [ More code ]
>
> > pm_runtime_get_noresume(&st->i3cdev->dev);
>
>
> Yes, this looks good if it makes the error paths cleaner.
> Also consider adding
>
> struct device *dev = &st->i3cdev->dev;
>
> at the top of the functions that use it, it might make code better to read.
>
Sorry I'm late to respond to this. If we do end up doing this
Raphael has been posting some new macros to help and I'm rather dubious
about using an acquire to grab a first reference then upgrading with with
a second on the use paths. I think ACQUIRE in this particular place may
just not be appropriate.
Jonathan
> > return 0;
> > }
> >
> > 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;
> > }
> >
> > Monitor mode:
> >
> > 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;
> >
> > // [ Some code ]
> >
> > pm_runtime_get_noresume(&st->i3cdev->dev);
> > return 0;
> > }
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-11-24 8:57 ` Jorge Marques
2025-11-24 9:10 ` Andy Shevchenko
@ 2025-11-24 9:11 ` Andy Shevchenko
2025-11-24 9:24 ` Jorge Marques
1 sibling, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2025-11-24 9:11 UTC (permalink / raw)
To: Jorge Marques
Cc: Jonathan Cameron, Jorge Marques, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
...
> I will submitted v2 shortly.
I think the "shortly" is not needed, please take your time, this seems missed
v6.19-rc1 anyway.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/7] iio: adc: Add support for ad4062
2025-11-24 9:11 ` Andy Shevchenko
@ 2025-11-24 9:24 ` Jorge Marques
0 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-11-24 9:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Jorge Marques, Lars-Peter Clausen,
Michael Hennerich, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-iio, devicetree, linux-kernel, linux-doc
On Mon, Nov 24, 2025 at 11:11:41AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 09:57:26AM +0100, Jorge Marques wrote:
> > On Mon, Nov 24, 2025 at 09:43:09AM +0200, Andy Shevchenko wrote:
>
> ...
>
> > I will submitted v2 shortly.
>
> I think the "shortly" is not needed, please take your time, this seems missed
> v6.19-rc1 anyway.
Hi Andy,
I was submitting when this reached my mailbox. Still, noted the comment
on your next e-mail and no pressure on the release window, I just didn't
have further changes for this revision.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/7] docs: iio: ad4062: Add IIO Trigger support
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
` (2 preceding siblings ...)
2025-10-13 7:28 ` [PATCH 3/7] iio: adc: Add support for ad4062 Jorge Marques
@ 2025-10-13 7:28 ` Jorge Marques
2025-10-13 7:28 ` [PATCH 5/7] iio: adc: " Jorge Marques
` (2 subsequent siblings)
6 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, 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 b486d7fe1916d2963c94581be3696cf58d51ca48..3770740a1a9b1419cd97882a988578e514407f59 100644
--- a/Documentation/iio/ad4062.rst
+++ b/Documentation/iio/ad4062.rst
@@ -81,6 +81,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.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 5/7] iio: adc: ad4062: Add IIO Trigger support
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
` (3 preceding siblings ...)
2025-10-13 7:28 ` [PATCH 4/7] docs: iio: ad4062: Add IIO Trigger support Jorge Marques
@ 2025-10-13 7:28 ` Jorge Marques
2025-10-18 16:14 ` Jonathan Cameron
2025-10-13 7:28 ` [PATCH 6/7] docs: iio: ad4062: Add IIO Events support Jorge Marques
2025-10-13 7:28 ` [PATCH 7/7] iio: adc: " Jorge Marques
6 siblings, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, 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 | 131 ++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 131 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 490c01d701bdd1543809cdefad4ed5573c051c24..c7c17f7e04a2f93664ddad3d37875079a9d5ab24 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 e55a69c62694a71a4e29f29b9a2bfeec3b16c990..40b7c10b8ce7145b010bb11e8e4698baacb6b3d3 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>
@@ -432,7 +436,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_nested(st->trigger);
+ else
+ complete(&st->completion);
return IRQ_HANDLED;
}
@@ -442,7 +449,49 @@ 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(st->trigger);
+ else
+ complete(&st->completion);
+}
+
+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);
+ u8 addr = AD4062_REG_CONV_TRIGGER;
+ int ret;
+
+ /* Read current and trigger next conversion */
+ struct i3c_priv_xfer t[2] = {
+ {
+ .data.in = &st->raw,
+ .len = 4,
+ .rnw = true,
+ },
+ {
+ .data.out = &addr,
+ .len = 1,
+ .rnw = false,
+ }
+ };
+
+ /* Separated transfers to not infeer repeated-start */
+ ret = i3c_device_do_priv_xfers(st->i3cdev, &t[0], 1);
+ if (ret)
+ goto out;
+ ret = i3c_device_do_priv_xfers(st->i3cdev, &t[1], 1);
+ if (ret)
+ goto out;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->raw,
+ pf->timestamp);
+
+out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
}
static int ad4062_request_ibi(struct i3c_device *i3cdev)
@@ -489,6 +538,27 @@ 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;
+
+ 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);
+
+ return devm_iio_trigger_register(dev, st->trigger);
+}
+
static const int ad4062_oversampling_avail[] = {
1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
};
@@ -709,6 +779,52 @@ 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);
+ u8 addr = AD4062_REG_CONV_TRIGGER;
+ 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;
+
+ /* Trigger first conversion */
+ struct i3c_priv_xfer t = {
+ .data.out = &addr,
+ .len = 1,
+ .rnw = false,
+ };
+
+ ret = i3c_device_do_priv_xfers(st->i3cdev, &t, 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_mark_last_busy(&st->i3cdev->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)
{
@@ -843,6 +959,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)
--
2.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 5/7] iio: adc: ad4062: Add IIO Trigger support
2025-10-13 7:28 ` [PATCH 5/7] iio: adc: " Jorge Marques
@ 2025-10-18 16:14 ` Jonathan Cameron
2025-11-23 19:48 ` Jorge Marques
0 siblings, 1 reply; 31+ messages in thread
From: Jonathan Cameron @ 2025-10-18 16:14 UTC (permalink / raw)
To: Jorge Marques
Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-iio, devicetree,
linux-kernel, linux-doc
On Mon, 13 Oct 2025 09:28:03 +0200
Jorge Marques <jorge.marques@analog.com> 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.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
A few things inline.
Thanks,
> diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> index e55a69c62694a71a4e29f29b9a2bfeec3b16c990..40b7c10b8ce7145b010bb11e8e4698baacb6b3d3 100644
> --- a/drivers/iio/adc/ad4062.c
> +++ b/drivers/iio/adc/ad4062.c
> +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);
> + u8 addr = AD4062_REG_CONV_TRIGGER;
> + int ret;
> +
> + /* Read current and trigger next conversion */
> + struct i3c_priv_xfer t[2] = {
> + {
> + .data.in = &st->raw,
If it is safe to use addr on the stack, do we need to have
a dma safe buffer for raw? I'm not sure for i3c!
> + .len = 4,
> + .rnw = true,
> + },
> + {
> + .data.out = &addr,
> + .len = 1,
> + .rnw = false,
> + }
> + };
> +
> + /* Separated transfers to not infeer repeated-start */
> + ret = i3c_device_do_priv_xfers(st->i3cdev, &t[0], 1);
> + if (ret)
> + goto out;
> + ret = i3c_device_do_priv_xfers(st->i3cdev, &t[1], 1);
Add a comment on this. I assume it's setting things up for the next
scan?
> + if (ret)
> + goto out;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->raw,
> + pf->timestamp);
> +
> +out:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> }
> +
> +static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> +
> + pm_runtime_mark_last_busy(&st->i3cdev->dev);
Take a look at the changes across the tree recently.
Now pm_runtime_put_autosuspend() calls pm_runtime_mark_last_busy()
internally to avoid the need for this pair.
> + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> + return 0;
> +}
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 5/7] iio: adc: ad4062: Add IIO Trigger support
2025-10-18 16:14 ` Jonathan Cameron
@ 2025-11-23 19:48 ` Jorge Marques
0 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-11-23 19:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Sat, Oct 18, 2025 at 05:14:25PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:03 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
>
Hi Jonathan,
> > 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>
>
> A few things inline.
> Thanks,
>
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > index e55a69c62694a71a4e29f29b9a2bfeec3b16c990..40b7c10b8ce7145b010bb11e8e4698baacb6b3d3 100644
> > --- a/drivers/iio/adc/ad4062.c
> > +++ b/drivers/iio/adc/ad4062.c
>
> > +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);
> > + u8 addr = AD4062_REG_CONV_TRIGGER;
> > + int ret;
> > +
> > + /* Read current and trigger next conversion */
> > + struct i3c_priv_xfer t[2] = {
> > + {
> > + .data.in = &st->raw,
>
> If it is safe to use addr on the stack, do we need to have
> a dma safe buffer for raw? I'm not sure for i3c!
>
All buffer should be dma aligned, I will use the heap.
I will use a separate buffer that is written once (CONV_READ or
CONV_TRIGGER, depending if gp1 is routed or using the IBI fallback).
> > + .len = 4,
> > + .rnw = true,
> > + },
> > + {
> > + .data.out = &addr,
> > + .len = 1,
> > + .rnw = false,
> > + }
> > + };
> > +
> > + /* Separated transfers to not infeer repeated-start */
> > + ret = i3c_device_do_priv_xfers(st->i3cdev, &t[0], 1);
> > + if (ret)
> > + goto out;
> > + ret = i3c_device_do_priv_xfers(st->i3cdev, &t[1], 1);
>
> Add a comment on this. I assume it's setting things up for the next
> scan?
>
yes, the next scan is triggered after the reading of the current scan.
There are 2 registers that can be used for scans, CONV_READ and
CONV_TRIGGER:
* CONV_READ: Stop bit of the previous read (without write address),
triggers the next scan. Allows roughly twice the sample rate, since
does not requires writing the address every transfer.
* CONV_TRIGGER: The conversion is triggered by writing the address, so
every new conversion requires writing the address again. Only this
registers will issue an IBI data ready.
That means, if GPIO is not routed as the IRQ, in the fallback using IBI,
CONV_TRIGGER needs to be used. v2 will also use schedule_work to avoid
the IRQF_ONESHOT being triggered (next conversion data ready), before
the current irq_handler returns.
> > + if (ret)
> > + goto out;
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, &st->raw,
> > + pf->timestamp);
> > +
> > +out:
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > }
>
> > +
> > +static int ad4062_triggered_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > + struct ad4062_state *st = iio_priv(indio_dev);
> > +
> > + pm_runtime_mark_last_busy(&st->i3cdev->dev);
>
> Take a look at the changes across the tree recently.
> Now pm_runtime_put_autosuspend() calls pm_runtime_mark_last_busy()
> internally to avoid the need for this pair.
>
Ack.
> > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > + return 0;
> > +}
>
>
Best regards,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 6/7] docs: iio: ad4062: Add IIO Events support
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
` (4 preceding siblings ...)
2025-10-13 7:28 ` [PATCH 5/7] iio: adc: " Jorge Marques
@ 2025-10-13 7:28 ` Jorge Marques
2025-10-13 7:28 ` [PATCH 7/7] iio: adc: " Jorge Marques
6 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, Jorge Marques
Explains the IIO Events support.
Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
Documentation/iio/ad4062.rst | 44 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
index 3770740a1a9b1419cd97882a988578e514407f59..f83b2906228ca7fa076a9340a0eff94b5c102c77 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:
+- GP0: Is assigned the role of Threshold Either signal.
- GP1: Is assigned the role of Data Ready signal.
Device attributes
@@ -60,7 +61,7 @@ Also contain the following device attributes:
- Description
* - ``samling_frequency``
- Sets the sets the device internal sample rate, used in the burst
- averaging mode.
+ averaging mode and monitor modes.
* - ``sampling_frequency_available``
- Lists the available device internal sample rates.
@@ -70,8 +71,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 ``interrup-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
@@ -94,9 +97,42 @@ 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
--
2.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 7/7] iio: adc: ad4062: Add IIO Events support
2025-10-13 7:27 [PATCH 0/7] Add support for AD4062 device family Jorge Marques
` (5 preceding siblings ...)
2025-10-13 7:28 ` [PATCH 6/7] docs: iio: ad4062: Add IIO Events support Jorge Marques
@ 2025-10-13 7:28 ` Jorge Marques
2025-10-18 16:26 ` Jonathan Cameron
6 siblings, 1 reply; 31+ messages in thread
From: Jorge Marques @ 2025-10-13 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet
Cc: linux-iio, devicetree, linux-kernel, linux-doc, 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 | 351 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 347 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
index 40b7c10b8ce7145b010bb11e8e4698baacb6b3d3..b5b12f81c71b52f244600ed23dad11253290b868 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>
@@ -172,6 +173,8 @@ struct ad4062_state {
struct i3c_device *i3cdev;
struct regmap *regmap;
u16 sampling_frequency;
+ u16 events_frequency;
+ bool wait_event;
int vref_uv;
u8 raw[4] __aligned(IIO_DMA_MINALIGN);
};
@@ -202,6 +205,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 const char *const ad4062_conversion_freqs[] = {
"2000000", "1000000", "300000", "100000", /* 0 - 3 */
"33300", "10000", "3000", "500", /* 4 - 7 */
@@ -263,6 +286,8 @@ AD4062_EXT_INFO(AD4062_2MSPS);
.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
.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), \
@@ -285,6 +310,82 @@ static const struct ad4062_chip_info ad4062_chip_info = {
.grade = AD4062_2MSPS,
};
+/**
+ * A register access will cause the device to drop from monitor mode
+ * into configuration mode, update the state to reflect that.
+ */
+static void ad4062_exit_monitor_mode(struct ad4062_state *st)
+{
+ if (st->wait_event) {
+ pm_runtime_mark_last_busy(&st->i3cdev->dev);
+ pm_runtime_put_autosuspend(&st->i3cdev->dev);
+ st->wait_event = 0;
+ }
+}
+
+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, "%s\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 ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ad4062_exit_monitor_mode(st);
+
+ ret = __sysfs_match_string(AD4062_FS(st->chip->grade),
+ AD4062_FS_LEN(st->chip->grade), buf);
+ if (ret < 0)
+ goto out_release;
+
+ st->events_frequency = ret;
+
+out_release:
+ iio_device_release_direct(indio_dev);
+ return ret ? ret : len;
+}
+
+static IIO_DEVICE_ATTR(sampling_frequency, 0644, 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)
+{
+ struct ad4062_state *st = iio_priv(dev_to_iio_dev(dev));
+ int ret = 0;
+
+ for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
+ i < AD4062_FS_LEN(st->chip->grade); i++)
+ ret += sysfs_emit_at(buf, ret, "%s ", ad4062_conversion_freqs[i]);
+
+ ret += sysfs_emit_at(buf, ret, "\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 iio_dev *indio_dev,
const struct iio_chan_spec *chan,
unsigned int val)
@@ -431,6 +532,19 @@ static int ad4062_setup(struct iio_dev *indio_dev, struct iio_chan_spec const *c
val);
}
+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;
@@ -449,10 +563,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(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(st->trigger);
+ else
+ complete(&st->completion);
+ }
}
static irqreturn_t ad4062_poll_handler(int irq, void *p)
@@ -523,6 +645,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 >= 0) {
+ ret = devm_request_threaded_irq(dev, ret, NULL,
+ ad4062_irq_handler_thresh,
+ IRQF_ONESHOT, indio_dev->name,
+ indio_dev);
+ if (ret)
+ return ret;
+ } else if (ret != -EPROBE_DEFER) {
+ 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 {
+ return ret;
+ }
+
ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
if (ret >= 0) {
ret = devm_request_threaded_irq(dev, ret, NULL,
@@ -734,6 +874,7 @@ 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)
@@ -741,6 +882,7 @@ static int ad4062_read_raw(struct iio_dev *indio_dev,
if (!iio_device_claim_direct(indio_dev))
return -EBUSY;
+ ad4062_exit_monitor_mode(st);
ret = ad4062_read_raw_dispatch(indio_dev, chan, val, val2, info);
@@ -768,10 +910,12 @@ 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;
+ ad4062_exit_monitor_mode(st);
ret = ad4062_write_raw_dispatch(indio_dev, chan, val, val2, info);
@@ -779,6 +923,196 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
return ret;
}
+static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
+{
+ int ret = 0;
+
+ if (!enable)
+ goto out_suspend;
+
+ ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
+ if (ret)
+ return ret;
+
+ ret = ad4062_conversion_frequency_set(st, st->events_frequency);
+ if (ret)
+ goto out_suspend;
+
+ ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
+ if (ret)
+ goto out_suspend;
+
+ return ret;
+out_suspend:
+ pm_runtime_put_autosuspend(&st->i3cdev->dev);
+ return ret;
+}
+
+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->raw, 2);
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(get_unaligned_be16(st->raw), 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;
+ ad4062_exit_monitor_mode(st);
+
+ 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;
+ }
+
+ 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->raw);
+
+ return regmap_bulk_write(st->regmap, reg, &st->raw, 2);
+}
+
+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;
+ ad4062_exit_monitor_mode(st);
+
+ 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;
+ }
+
+ iio_device_release_direct(indio_dev);
+ return ret;
+}
+
static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
{
struct ad4062_state *st = iio_priv(indio_dev);
@@ -788,6 +1122,7 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
if (ret)
return ret;
+ ad4062_exit_monitor_mode(st);
ret = ad4062_set_operation_mode(st, st->mode);
if (ret)
@@ -833,6 +1168,7 @@ static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
if (!iio_device_claim_direct(indio_dev))
return -EBUSY;
+ ad4062_exit_monitor_mode(st);
if (readval)
ret = regmap_read(st->regmap, reg, readval);
@@ -857,6 +1193,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,
};
@@ -932,8 +1273,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 = AD4062_FS_OFFSET(st->chip->grade);
+ st->events_frequency = AD4062_FS_OFFSET(st->chip->grade);
st->indio_dev = indio_dev;
indio_dev->modes = INDIO_DIRECT_MODE;
--
2.49.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 7/7] iio: adc: ad4062: Add IIO Events support
2025-10-13 7:28 ` [PATCH 7/7] iio: adc: " Jorge Marques
@ 2025-10-18 16:26 ` Jonathan Cameron
2025-11-23 19:48 ` Jorge Marques
0 siblings, 1 reply; 31+ messages in thread
From: Jonathan Cameron @ 2025-10-18 16:26 UTC (permalink / raw)
To: Jorge Marques
Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-iio, devicetree,
linux-kernel, linux-doc
On Mon, 13 Oct 2025 09:28:05 +0200
Jorge Marques <jorge.marques@analog.com> 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.
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
The one bit of this that I'm not sure on is the apparent dropping out
of monitor mode on most userspace interactions that cause register accesses.
That seems like a fairly unintuitive ABI. It might be better to block the access
until the events are turned off. Perhaps I missed something?
Thanks,
Jonathan
> ---
> drivers/iio/adc/ad4062.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 347 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> index 40b7c10b8ce7145b010bb11e8e4698baacb6b3d3..b5b12f81c71b52f244600ed23dad11253290b868 100644
> --- a/drivers/iio/adc/ad4062.c
> +++ b/drivers/iio/adc/ad4062.c
> @@ -13,6 +13,7 @@
> +/**
> + * A register access will cause the device to drop from monitor mode
> + * into configuration mode, update the state to reflect that.
> + */
> +static void ad4062_exit_monitor_mode(struct ad4062_state *st)
> +{
> + if (st->wait_event) {
> + pm_runtime_mark_last_busy(&st->i3cdev->dev);
> + pm_runtime_put_autosuspend(&st->i3cdev->dev);
As elsewhere, no longer need to have the mark_last_busy() call here.
> + st->wait_event = 0;
> + }
> +}
> +static ssize_t sampling_frequency_available_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ad4062_state *st = iio_priv(dev_to_iio_dev(dev));
> + int ret = 0;
> +
> + for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
> + i < AD4062_FS_LEN(st->chip->grade); i++)
> + ret += sysfs_emit_at(buf, ret, "%s ", ad4062_conversion_freqs[i]);
> +
> + ret += sysfs_emit_at(buf, ret, "\n");
> + return ret;
Has slightly ugly format of " \n" at end rather than "\n"
There are various ways to handle this perhaps easiest is something like
for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
i < AD4062_FS_LEN(st->chip->grade); i++)
ret += sysfs_emit_at(buf, ret, "%s%c", ad4062_conversion_freqs[i],
i != (AD4062_FS_LEN(st->chip_grade) - 1) ? "\n", " ");
> +}
> static irqreturn_t ad4062_poll_handler(int irq, void *p)
> @@ -523,6 +645,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 >= 0) {
> + ret = devm_request_threaded_irq(dev, ret, NULL,
> + ad4062_irq_handler_thresh,
> + IRQF_ONESHOT, indio_dev->name,
> + indio_dev);
> + if (ret)
> + return ret;
> + } else if (ret != -EPROBE_DEFER) {
> + 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 {
> + return ret;
As before. I'd prefer error cases handled first. The earlier code suggestion
doesn't quite work but something along those lines should be doable.
> + }
> +
> ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> if (ret >= 0) {
> ret = devm_request_threaded_irq(dev, ret, NULL,
> @@ -779,6 +923,196 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
> return ret;
> }
>
> +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> +{
> + int ret = 0;
> +
> + if (!enable)
> + goto out_suspend;
> +
> + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> + if (ret)
> + return ret;
> +
> + ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> + if (ret)
> + goto out_suspend;
> +
> + ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> + if (ret)
> + goto out_suspend;
> +
> + return ret;
return 0;
> +out_suspend:
> + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> + return ret;
> +}
> static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> {
> struct ad4062_state *st = iio_priv(indio_dev);
> @@ -788,6 +1122,7 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> if (ret)
> return ret;
> + ad4062_exit_monitor_mode(st);
Hmm. So you always exist monitor mode if we enable the buffer. I assume that doesn't
change detection of events because the buffered mode also allows that?
Do we not need something to turn monitor mode on again once we disable buffered capture?
>
> ret = ad4062_set_operation_mode(st, st->mode);
> if (ret)
> @@ -833,6 +1168,7 @@ static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
>
> if (!iio_device_claim_direct(indio_dev))
> return -EBUSY;
> + ad4062_exit_monitor_mode(st);
This probably needs a comment. Not obvious to me how you end up in with it enabled
again after the debugfs read / write finishes.
>
> if (readval)
> ret = regmap_read(st->regmap, reg, readval);
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/7] iio: adc: ad4062: Add IIO Events support
2025-10-18 16:26 ` Jonathan Cameron
@ 2025-11-23 19:48 ` Jorge Marques
0 siblings, 0 replies; 31+ messages in thread
From: Jorge Marques @ 2025-11-23 19:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jorge Marques, Lars-Peter Clausen, Michael Hennerich,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-iio,
devicetree, linux-kernel, linux-doc
On Sat, Oct 18, 2025 at 05:26:25PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 09:28:05 +0200
> Jorge Marques <jorge.marques@analog.com> 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.
> >
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jonathan,
> The one bit of this that I'm not sure on is the apparent dropping out
> of monitor mode on most userspace interactions that cause register accesses.
> That seems like a fairly unintuitive ABI. It might be better to block the access
> until the events are turned off. Perhaps I missed something?
>
I will change the behaviour to block access until monitor mode is disabled:
if (!iio_device_claim_direct(indio_dev))
return -EBUSY;
if (st->wait_event) {
ret = -EBUSY;
goto out_release;
}
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/ad4062.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 347 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad4062.c b/drivers/iio/adc/ad4062.c
> > index 40b7c10b8ce7145b010bb11e8e4698baacb6b3d3..b5b12f81c71b52f244600ed23dad11253290b868 100644
> > --- a/drivers/iio/adc/ad4062.c
> > +++ b/drivers/iio/adc/ad4062.c
> > @@ -13,6 +13,7 @@
>
> > +/**
> > + * A register access will cause the device to drop from monitor mode
> > + * into configuration mode, update the state to reflect that.
> > + */
> > +static void ad4062_exit_monitor_mode(struct ad4062_state *st)
> > +{
> > + if (st->wait_event) {
> > + pm_runtime_mark_last_busy(&st->i3cdev->dev);
>
> > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> As elsewhere, no longer need to have the mark_last_busy() call here.
>
Ack.
> > + st->wait_event = 0;
> > + }
> > +}
>
> > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ad4062_state *st = iio_priv(dev_to_iio_dev(dev));
> > + int ret = 0;
> > +
> > + for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
> > + i < AD4062_FS_LEN(st->chip->grade); i++)
> > + ret += sysfs_emit_at(buf, ret, "%s ", ad4062_conversion_freqs[i]);
> > +
> > + ret += sysfs_emit_at(buf, ret, "\n");
> > + return ret;
>
> Has slightly ugly format of " \n" at end rather than "\n"
> There are various ways to handle this perhaps easiest is something like
> for (u8 i = AD4062_FS_OFFSET(st->chip->grade);
> i < AD4062_FS_LEN(st->chip->grade); i++)
> ret += sysfs_emit_at(buf, ret, "%s%c", ad4062_conversion_freqs[i],
> i != (AD4062_FS_LEN(st->chip_grade) - 1) ? "\n", " ");
Thanks for the suggestion, I will use.
>
>
> > +}
>
> > static irqreturn_t ad4062_poll_handler(int irq, void *p)
> > @@ -523,6 +645,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 >= 0) {
> > + ret = devm_request_threaded_irq(dev, ret, NULL,
> > + ad4062_irq_handler_thresh,
> > + IRQF_ONESHOT, indio_dev->name,
> > + indio_dev);
> > + if (ret)
> > + return ret;
> > + } else if (ret != -EPROBE_DEFER) {
> > + 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 {
> > + return ret;
>
> As before. I'd prefer error cases handled first. The earlier code suggestion
> doesn't quite work but something along those lines should be doable.
>
Ack.
> > + }
> > +
> > ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> > if (ret >= 0) {
> > ret = devm_request_threaded_irq(dev, ret, NULL,
>
> > @@ -779,6 +923,196 @@ static int ad4062_write_raw(struct iio_dev *indio_dev,
> > return ret;
> > }
> >
> > +static int ad4062_monitor_mode_enable(struct ad4062_state *st, bool enable)
> > +{
> > + int ret = 0;
> > +
> > + if (!enable)
> > + goto out_suspend;
> > +
> > + ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad4062_conversion_frequency_set(st, st->events_frequency);
> > + if (ret)
> > + goto out_suspend;
> > +
> > + ret = ad4062_set_operation_mode(st, AD4062_MONITOR_MODE);
> > + if (ret)
> > + goto out_suspend;
> > +
> > + return ret;
> return 0;
>
Ack.
> > +out_suspend:
> > + pm_runtime_put_autosuspend(&st->i3cdev->dev);
> > + return ret;
> > +}
>
> > static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > {
> > struct ad4062_state *st = iio_priv(indio_dev);
> > @@ -788,6 +1122,7 @@ static int ad4062_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > ret = pm_runtime_resume_and_get(&st->i3cdev->dev);
> > if (ret)
> > return ret;
> > + ad4062_exit_monitor_mode(st);
> Hmm. So you always exist monitor mode if we enable the buffer. I assume that doesn't
> change detection of events because the buffered mode also allows that?
>
> Do we not need something to turn monitor mode on again once we disable buffered capture?
No, the buffer mode does not work alongside monitor mode, I will add the
lock
if (st->wait_event)
return -EBUSY;
> >
> > ret = ad4062_set_operation_mode(st, st->mode);
> > if (ret)
> > @@ -833,6 +1168,7 @@ static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg
> >
> > if (!iio_device_claim_direct(indio_dev))
> > return -EBUSY;
> > + ad4062_exit_monitor_mode(st);
>
> This probably needs a comment. Not obvious to me how you end up in with it enabled
> again after the debugfs read / write finishes.
>
You don't. Once a register access is done, it will drop of monitor mode
or sample/buffer mode. But since it is a debug interface, that may be
alright?
> >
> > if (readval)
> > ret = regmap_read(st->regmap, reg, readval);
Best regards,
Jorge
^ permalink raw reply [flat|nested] 31+ messages in thread