Skip to contentSkip to conversation

Add support for AD4170

RFC PATCH In review Marcelo Schmitt 6 participants 18 messages Dec 18, 2024 – Jan 14, 2025

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover Add support for AD4170

Add support for AD4170.
Initial driver for ad4170 was inpired from ad4130. Then I picked it up from ADI Linux repo and changed a lot. Clock provider support is the same from ad7173.

Most disruptive things are:
- Draft support for negative/bipolar voltage reference supply.
- IIO channels sharing setup number will share configurations.
- Draft ADC documentation to help clarify/explain why so many possible ADC input configurations.

This is big so not expecting to receive any review on this any time soon. Happy holly days.

Ana-Maria Cusco (2):
include: dt-bindings: iio: adc: Add defines for AD4170
iio: adc: Add support for AD4170

Marcelo Schmitt (2):
dt-bindings: iio: adc: Add AD4170
Documentation: iio: Add ADC documentation

 .../bindings/iio/adc/adi,ad4170.yaml          |  473 ++++
 Documentation/iio/iio_adc.rst                 |  280 +++
 Documentation/iio/index.rst                   |    1 +
 drivers/iio/adc/Kconfig                       |   16 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/ad4170.c                      | 2049 +++++++++++++++++
 drivers/iio/adc/ad4170.h                      |  316 +++
 include/dt-bindings/iio/adc/adi,ad4170.h      |   96 +
 8 files changed, 3232 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
 create mode 100644 Documentation/iio/iio_adc.rst
 create mode 100644 drivers/iio/adc/ad4170.c
 create mode 100644 drivers/iio/adc/ad4170.h
 create mode 100644 include/dt-bindings/iio/adc/adi,ad4170.h

base-commit: a61ff7eac77e86de828fe28c4e42b8ae9ec2b195

Signature · 1 line
2.45.2
#1 lore raw

RFC PATCH 1/4 include: dt-bindings: iio: adc: Add defines for AD4170

From: Ana-Maria Cusco <[email protected]>

Add defines to improve readability of AD4170 device tree nodes.

---
 include/dt-bindings/iio/adc/adi,ad4170.h | 96 ++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 include/dt-bindings/iio/adc/adi,ad4170.h
include/dt-bindings/iio/adc/adi,ad4170.h new file +96 -0
new file mode 100644
index 000000000000..a508b37e9a46
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * AD4170 ADC
+ *
+ * Copyright 2024 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#ifndef _DT_BINDINGS_IIO_ADC_AD4170_H_
+#define _DT_BINDINGS_IIO_ADC_AD4170_H_
+
+/*
+ * Excitation Current Chopping Control.
+ * Use for adi,chop-iexc
+ */
+#define AD4170_MISC_CHOP_IEXC_OFF	0
+/* Chopping of Iout_A and Iout_B Excitation Currents. */
+#define AD4170_MISC_CHOP_IEXC_AB	1
+/* Chopping of Iout_C and Iout_D Excitation Currents. */
+#define AD4170_MISC_CHOP_IEXC_CD	2
+/* Chopping of Both Pairs of Excitation Currents. */
+#define AD4170_MISC_CHOP_IEXC_ABCD	3
+
+/*
+ * Chopping
+ * Use for adi,chop-adc
+ */
+/* No Chopping. */
+#define AD4170_MISC_CHOP_ADC_OFF		0
+/* Chops Internal Mux. */
+#define AD4170_MISC_CHOP_ADC_MUX		1
+/* Chops AC Excitation Using 4 GPIO Pins. */
+#define AD4170_MISC_CHOP_ADC_ACX_4PIN		2
+/* Chops AC Excitation Using 2 GPIO Pins. */
+#define AD4170_MISC_CHOP_ADC_ACX_2PIN		3
+
+/*
+ * Reference Selection Mode
+ * Use for adi,reference-select
+ */
+#define AD4170_AFE_REFIN_REFIN1		0
+#define AD4170_AFE_REFIN_REFIN2		1
+#define AD4170_AFE_REFIN_REFOUT		2
+#define AD4170_AFE_REFIN_AVDD		3
+
+/*
+ * Definitios for describing channel selections
+ * Use for adi,channel-map
+ */
+#define AD4170_MAP_AIN0			0
+#define AD4170_MAP_AIN1			1
+#define AD4170_MAP_AIN2			2
+#define AD4170_MAP_AIN3			3
+#define AD4170_MAP_AIN4			4
+#define AD4170_MAP_AIN5			5
+#define AD4170_MAP_AIN6			6
+#define AD4170_MAP_AIN7			7
+#define AD4170_MAP_AIN8			8
+#define AD4170_MAP_TEMP_SENSOR_P	17
+#define AD4170_MAP_TEMP_SENSOR_N	17
+#define AD4170_MAP_AVDD_AVSS_P		18
+#define AD4170_MAP_AVDD_AVSS_N		18
+#define AD4170_MAP_IOVDD_DGND_P		19
+#define AD4170_MAP_IOVDD_DGND_N		19
+#define AD4170_MAP_DAC_P		20
+#define AD4170_MAP_DAC_N		20
+#define AD4170_MAP_ALDO			21
+#define AD4170_MAP_DLDO			22
+#define AD4170_MAP_AVSS			23
+#define AD4170_MAP_DGND			24
+#define AD4170_MAP_REFIN1_P		25
+#define AD4170_MAP_REFIN1_N		26
+#define AD4170_MAP_REFIN2_P		27
+#define AD4170_MAP_REFIN2_N		28
+#define AD4170_MAP_REFOUT		29
+
+/*
+ * Definitios for describing excitation current output pin selections
+ * Use for adi,excitation-pin-0/1/2/3
+ */
+#define AD4170_CURRENT_IOUT_AIN0			0
+#define AD4170_CURRENT_IOUT_AIN1			1
+#define AD4170_CURRENT_IOUT_AIN2			2
+#define AD4170_CURRENT_IOUT_AIN3			3
+#define AD4170_CURRENT_IOUT_AIN4			4
+#define AD4170_CURRENT_IOUT_AIN5			5
+#define AD4170_CURRENT_IOUT_AIN6			6
+#define AD4170_CURRENT_IOUT_AIN7			7
+#define AD4170_CURRENT_IOUT_AIN8			8
+#define AD4170_CURRENT_IOUT_GPIO0			17
+#define AD4170_CURRENT_IOUT_GPIO1			18
+#define AD4170_CURRENT_IOUT_GPIO2			19
+#define AD4170_CURRENT_IOUT_GPIO3			20
+
+#endif /* _DT_BINDINGS_IIO_ADC_AD4170_H_ */
Signature · 1 line
2.45.2
#2 lore raw

Re: [RFC PATCH 1/4] include: dt-bindings: iio: adc: Add defines for AD4170

Quoted 3 lines · Marcelo Schmitt

From: Ana-Maria Cusco <[email protected]>

Add defines to improve readability of AD4170 device tree nodes.

Binding headers define the ABI between driver and DTS. Improving readability of DTS is not their purpose.

Explain why this is binding. Also, I don't get why this is separate from device binding - see git log history how various IDs are being added.

Quoted 15 lines
---
 include/dt-bindings/iio/adc/adi,ad4170.h | 96 ++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 include/dt-bindings/iio/adc/adi,ad4170.h
include/dt-bindings/iio/adc/adi,ad4170.h new file +0 -0
new file mode 100644
index 000000000000..a508b37e9a46

include/dt-bindings/iio/adc/adi,ad4170.h line 7

Quoted 7 lines
include/dt-bindings/iio/adc/adi,ad4170.h new file +7 -0
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * AD4170 ADC
+ *
+ * Copyright 2024 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.

No, you cannot say something else than SPDX. Anyway, drop license boilerplate text.

include/dt-bindings/iio/adc/adi,ad4170.h line 12

Quoted 5 lines
include/dt-bindings/iio/adc/adi,ad4170.h new file +5 -0
+ */
+
+#ifndef _DT_BINDINGS_IIO_ADC_AD4170_H_
+#define _DT_BINDINGS_IIO_ADC_AD4170_H_
+

Best regards,
Krzysztof

#3 lore raw

RFC PATCH 2/4 dt-bindings: iio: adc: Add AD4170

In reply to Marcelo Schmitt

Add device tree documentation for AD4170 sigma-delta ADCs.

---
 .../bindings/iio/adc/adi,ad4170.yaml          | 473 ++++++++++++++++++
 1 file changed, 473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +473 -0
new file mode 100644
index 000000000000..8c5defc614ee
@@ -0,0 +1,473 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4170 Analog to Digital Converter
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD4170 Analog to Digital Converter.
+  Specifications can be found at:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4170
+
+  avss-supply:
+    description:
+      Referece voltage supply for AVDD. AVSS can be set below 0V to provide a
+      bipolar power supply to AD4170-4. Must be −2.625V at minimum, 0V maximum.
+      If not specified, this is assumed to be analog ground.
+
+  avdd-supply:
+    description:
+      A supply of 4.75V to 5.25V relative to AVSS that powers the chip (AVDD).
+
+  iovdd-supply:
+    description: 1.7V to 5.25V reference supply to the serial interface (IOVDD).
+
+  refin1p-supply:
+    description: REFIN+ supply that can be used as reference for conversion.
+
+  refin1n-supply:
+    description: REFIN- supply that can be used as reference for conversion.
+
+  refin2p-supply:
+    description: REFIN2+ supply that can be used as reference for conversion.
+
+  refin2n-supply:
+    description: REFIN2- supply that can be used as reference for conversion.
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: |
+      Optional external clock source. Can include one clock source: external
+      clock or external crystal.
+
+  clock-names:
+    enum:
+      - ext-clk
+      - xtal
+
+  '#clock-cells':
+    const: 0
+
+  adi,gpio0-power-down-switch:
+    type: boolean
+    description:
+      Describes whether GPIO0 is used as a switch to disconnect bridge circuits
+      from AVSS. Pin defaults to GPIO if this property is not present.
+
+  adi,gpio1-power-down-switch:
+    type: boolean
+    description:
+      Describes whether GPIO1 is used as a switch to disconnect bridge circuits
+      from AVSS. Pin defaults to GPIO if this property is not present.
+
+  adi,vbias-pins:
+    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 9
+    items:
+      minimum: 0
+      maximum: 8
+
+  adi,dig-aux1:
+    description:
+      Describes whether DIG_AUX1 pin will operate as data ready output,
+      synchronization output signal (SYNC_OUT), or if it will be disabled.
+      A value of 0 indicates DIG_AUX1 pin disabled. High impedance.
+      A value of 1 indicates DIG_AUX1 is configured as ADC data ready output.
+      A value of 1 indicates DIG_AUX1 is configured as SYNC_OUT output.
+      If this property is absent, DIG_AUX1 pin is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 0
+
+  adi,dig-aux2:
+    description:
+      Describes whether DIG_AUX2 pin will function as DAC LDAC input,
+      synchronization start input (START), or if it will be disabled.
+      A value of 0 indicates DIG_AUX2 pin is disabled. High impedance.
+      A value of 1 indicates DIG_AUX2 pin is configured as active-low LDAC input
+      for the DAC.
+      A value of 2 indicates DIG_AUX2 pin is configured as START input.
+      If this property is absent, DIG_AUX2 pin is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 0
+
+  adi,sync-option:
+    description:
+      Describes how ADC conversions are going to be synchronized. A value of 1
+      indicates the SYNC_IN pin will function as a synchronization input that
+      allows the user to control the start of sampling by pulling SYNC_IN high.
+      Use option number 2 to set the alternate synchronization functionality
+      which allows per channel conversion start control when multiple channels
+      are enabled. Option number 0 disables synchronization.
+      A value of 0 indicates no synchronization. SYNC_IN pin disabled.
+      A value of 1 indicates standard synchronization functionality.
+      A value of 2 indicates alternate synchronization functionality.
+      If this property is absent, no synchronization is performed.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 1
+
+  adi,excitation-pin-0:
+    description: |
+      Specifies the pin to apply excitation current 0 (IOUT0). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT0 to GPIO0.
+      18: Output excitation current IOUT0 to GPIO1.
+      19: Output excitation current IOUT0 to GPIO2.
+      20: Output excitation current IOUT0 to GPIO3.
+      If this property is absent, IOUT0 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-1:
+    description: |
+      Specifies the pin to apply excitation current 1 (IOUT1). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT1 to GPIO0.
+      18: Output excitation current IOUT1 to GPIO1.
+      19: Output excitation current IOUT1 to GPIO2.
+      20: Output excitation current IOUT1 to GPIO3.
+      If this property is absent, IOUT1 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-2:
+    description: |
+      Specifies the pin to apply excitation current 2 (IOUT2). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT2 to GPIO0.
+      18: Output excitation current IOUT2 to GPIO1.
+      19: Output excitation current IOUT2 to GPIO2.
+      20: Output excitation current IOUT2 to GPIO3.
+      If this property is absent, IOUT2 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-3:
+    description: |
+      Specifies the pin to apply excitation current 3 (IOUT3). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT3 to GPIO0.
+      18: Output excitation current IOUT3 to GPIO1.
+      19: Output excitation current IOUT3 to GPIO2.
+      20: Output excitation current IOUT3 to GPIO3.
+      If this property is absent, IOUT3 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-current-0-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT0 output pin
+      specified in adi,excitation-pin-0.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-1-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT1 output pin
+      specified in adi,excitation-pin-1.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-2-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT2 output pin
+      specified in adi,excitation-pin-2.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-3-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT3 output pin
+      specified in adi,excitation-pin-3.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,chop-iexc:
+    description: |
+      Specifies the chopping/swapping functionality for excitation currents.
+      0: No Chopping of Excitation Currents.
+      1: Chop/swap IOUT0 and IOUT1 (pair AB) excitation currents.
+      2: Chop/swap IOUT2 and IOUT3 (pair CD) excitation currents.
+      3: Chop/swap both pairs (pair AB and pair CD) of excitation currents.
+      If this property is absent, no chopping is performed.
+      There are macros for the above values in dt-bindings/iio/adi,ad4170.h.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2, 3]
+    default: 0
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":
+    $ref: adc.yaml
+    type: object
+    unevaluatedProperties: false
+    description: |
+      Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number. The device can have up to 16 channels numbered
+          from 0 to 15.
+        items:
+          minimum: 0
+          maximum: 15
+
+      diff-channels:
+        description: |
+          This property is used for defining the inputs of a differential
+          voltage channel. The first value is the positive input and the second
+          value is the negative input of the channel.
+
+          Besides the analog input pins AIN0 to AIN8, there are special inputs
+          that can be selected with the following values:
+            17: Temperature sensor input
+            18: (AVDD-AVSS)/5
+            19: (IOVDD-DGND)/5
+            20: DAC output
+            21: ALDO
+            22: DLDO
+            23: AVSS
+            24: DGND
+            25: REFIN+
+            26: REFIN-
+            27: REFIN2+
+            28: REFIN2-
+            29: REFOUT
+
+          There are macros for those values in dt-bindings/iio/adi,ad4170.h.
+
+        items:
+          minimum: 0
+          maximum: 31
+
+      single-channel: true
+
+      common-mode-channel: true
+
+      bipolar: true
+
+      adi,config-setup-number:
+        description: |
+          Specifies which of the 8 setups are used to configure the channel.
+          A setup comprises of: AFE, FILTER, FILTER_FS, MISC, OFFSET, and GAIN
+          registers. More than one channel can use the same configuration setup
+          number in which case they will share the settings of the above
+          mentioned registers.
+        items:
+          minimum: 0
+          maximum: 7
+
+      adi,chop-adc:
+        description: |
+          Specifies the chopping/swapping functionality for a channel setup.
+          Macros for adi,chop-adc values are available in
+          dt-bindings/iio/adi,ad4170.h. When enabled, the analog inputs are
+          continuously swapped and a conversion is generated for each time a
+          swap occurs. The analog input pins are connected in one direction,
+          sampled, swapped, sampled again, and then the conversion results are
+          averaged. The input swap minimizes system offset and offset drift.
+          This property also specifies whether AC excitation using 2 or 4 GPIOs
+          are going to be used.
+          0: No channel chop.
+          1: Chop/swap the channel inputs.
+          2: AC Excitation using 4 GPIOs.
+          3: AC Excitation using 2 GPIOs.
+          If this property is absent, no chopping is performed.
+        $ref: /schemas/types.yaml#/definitions/uint16
+        enum: [0, 1, 2, 3]
+        default: 0
+
+      adi,burnout-current-nanoamp:
+        description: |
+          Current in nanoamps to be applied for this channel. Burnout currents
+          are only active when the channel is selected for conversion.
+        enum: [0, 100, 2000, 10000]
+        default: 0
+
+      adi,buffered-negative:
+        description: Enable precharge buffer, full buffer, or skip reference
+          buffering of the negative voltage reference. Because the output
+          impedance of the source driving the voltage reference inputs may be
+          dynamic, RC combinations of those inputs can cause DC gain errors if
+          the reference inputs go unbuffered into the ADC. Enable reference
+          buffering if the provided reference source has dynamic high impedance
+          output.
+        enum: [0, 1, 2]
+        default: 0
+
+      adi,buffered-positive:
+        description: Enable precharge buffer, full buffer, or skip reference
+          buffering of the positive voltage reference. Because the output
+          impedance of the source driving the voltage reference inputs may be
+          dynamic, RC combinations of those inputs can cause DC gain errors if
+          the reference inputs go unbuffered into the ADC. Enable reference
+          buffering if the provided reference source has dynamic high impedance
+          output.
+        enum: [0, 1, 2]
+        default: 0
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on the specific
+          channel. Valid values are:
+          0: Differential reference voltage REFIN+ - REFIN−.
+          1: Differential reference voltage REFIN2+ - REFIN2−.
+          2: Internal 2.5V referece (REFOUT) relative to AVSS.
+          3: Analog supply voltage (AVDD) relative relative AVSS.
+          If this field is left empty, the internal reference is selected.
+        $ref: /schemas/types.yaml#/definitions/uint8
+        enum: [0, 1, 2, 3]
+        default: 2
+
+    required:
+      - reg
+      - adi,config-setup-number
+
+    allOf:
+      - oneOf:
+          - required: [single-channel]
+            properties:
+              diff-channels: false
+          - required: [diff-channels]
+            properties:
+              single-channel: false
+              common-mode-channel: false
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - iovdd-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/iio/adc/adi,ad4170.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad4170";
+            reg = <0>;
+            avdd-supply = <&avdd>;
+            iovdd-supply = <&iovdd>;
+            spi-max-frequency = <20000000>;
+            interrupt-parent = <&gpio_in>;
+            interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
+            adi,dig-aux1 = /bits/ 8 <1>;
+            adi,dig-aux2 = /bits/ 8 <0>;
+            adi,sync-option = /bits/ 8 <0>;
+            adi,excitation-pin-0 = <19>;
+            adi,excitation-current-0-microamp = <10>;
+            adi,excitation-pin-1 = <20>;
+            adi,excitation-current-1-microamp = <10>;
+            adi,chop-iexc = /bits/ 8 <1>;
+            adi,vbias-pins = <5 6>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            // Sample AIN0 with respect to AIN1 throughout AVDD/AVSS input range
+            // Fully differential. If AVSS < 0V, Fully differential true bipolar
+            channel@0 {
+                reg = <0>;
+                bipolar;
+                diff-channels = <AD4170_MAP_AIN0 AD4170_MAP_AIN1>;
+                adi,config-setup-number = <0>;
+                adi,reference-select = /bits/ 8 <3>;
+                adi,burnout-current-nanoamp = <100>;
+            };
+            // Sample AIN2 with respect to DGND throughout AVDD/DGND input range
+            // Peseudo-differential unipolar (fig. 2a)
+            channel@1 {
+                reg = <1>;
+                single-channel = <AD4170_MAP_AIN2>;
+                common-mode-channel = <AD4170_MAP_DGND>;
+                adi,config-setup-number = <1>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN3 with respect to 2.5V throughout AVDD/AVSS input range
+            // Pseudo-differential bipolar (fig. 2b)
+            channel@2 {
+                reg = <2>;
+                bipolar;
+                single-channel = <AD4170_MAP_AIN3>;
+                common-mode-channel = <AD4170_MAP_REFOUT>;
+                adi,config-setup-number = <2>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN4 with respect to DGND throughout AVDD/AVSS input range
+            // Pseudo-differential true bipolar if AVSS < 0V (fig. 2c)
+            channel@3 {
+                reg = <3>;
+                bipolar;
+                single-channel = <AD4170_MAP_AIN4>;
+                common-mode-channel = <AD4170_MAP_DGND>;
+                adi,config-setup-number = <3>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN5 with respect to 2.5V throughout AVDD/REFOUT input range
+            // Pseudo-differential unipolar (AD4170 datasheet page 46 example)
+            channel@4 {
+                reg = <4>;
+                single-channel = <AD4170_MAP_AIN5>;
+                common-mode-channel = <AD4170_MAP_REFOUT>;
+                adi,config-setup-number = <4>;
+                adi,reference-select = /bits/ 8 <2>;
+            };
+            // Sample AIN6 with respect to REFIN+ throughout AVDD/AVSS input range
+            // Pseudo-differential unipolar
+            channel@5 {
+                reg = <5>;
+                single-channel = <AD4170_MAP_AIN6>;
+                common-mode-channel = <AD4170_MAP_REFIN1_P>;
+                adi,config-setup-number = <4>;
+                adi,reference-select = /bits/ 8 <2>;
+            };
+            // Sample AIN7 with respect to DGND throughout REFIN+/REFIN- input range
+            // Pseudo-differential bipolar
+            channel@6 {
+                reg = <6>;
+                bipolar;
+                diff-channels = <AD4170_MAP_AIN7 AD4170_MAP_DGND>;
+                adi,config-setup-number = <5>;
+                adi,reference-select = /bits/ 8 <0>;
+            };
+        };
+    };
+...
+
Signature · 1 line
2.45.2
#4 lore raw

Re: [RFC PATCH 2/4] dt-bindings: iio: adc: Add AD4170

Quoted 8 lines · Marcelo Schmitt

Add device tree documentation for AD4170 sigma-delta ADCs.

---
 .../bindings/iio/adc/adi,ad4170.yaml          | 473 ++++++++++++++++++
 1 file changed, 473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml: adi,config-setup-number: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/caadb73da62e80877eab8b0287d996b52266d912.1734530280.git.marcelo.schmitt@analog.com

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

maintainer
#5 lore raw

Re: [RFC PATCH 2/4] dt-bindings: iio: adc: Add AD4170

In reply to Marcelo Schmitt
Quoted 2 lines · Marcelo Schmitt

Add device tree documentation for AD4170 sigma-delta ADCs.

The usual question with long lists of properties. Any of these should be run-time controlled (by userspace) instead? If they might vary by the user for given h/w design, then should be user controlled instead.

Quoted 56 lines
---
 .../bindings/iio/adc/adi,ad4170.yaml          | 473 ++++++++++++++++++
 1 file changed, 473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +44 -0
new file mode 100644
index 000000000000..8c5defc614ee
@@ -0,0 +1,473 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4170 Analog to Digital Converter
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD4170 Analog to Digital Converter.
+  Specifications can be found at:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4170
+
+  avss-supply:
+    description:
+      Referece voltage supply for AVDD. AVSS can be set below 0V to provide a
+      bipolar power supply to AD4170-4. Must be −2.625V at minimum, 0V maximum.
+      If not specified, this is assumed to be analog ground.
+
+  avdd-supply:
+    description:
+      A supply of 4.75V to 5.25V relative to AVSS that powers the chip (AVDD).
+
+  iovdd-supply:
+    description: 1.7V to 5.25V reference supply to the serial interface (IOVDD).
+
+  refin1p-supply:
+    description: REFIN+ supply that can be used as reference for conversion.
+
+  refin1n-supply:
+    description: REFIN- supply that can be used as reference for conversion.
+
+  refin2p-supply:
+    description: REFIN2+ supply that can be used as reference for conversion.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 54

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+
+  refin2n-supply:
+    description: REFIN2- supply that can be used as reference for conversion.
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: |

Don't need '|' if no formatting to preserve.

Quoted 164 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +164 -0
+      Optional external clock source. Can include one clock source: external
+      clock or external crystal.
+
+  clock-names:
+    enum:
+      - ext-clk
+      - xtal
+
+  '#clock-cells':
+    const: 0
+
+  adi,gpio0-power-down-switch:
+    type: boolean
+    description:
+      Describes whether GPIO0 is used as a switch to disconnect bridge circuits
+      from AVSS. Pin defaults to GPIO if this property is not present.
+
+  adi,gpio1-power-down-switch:
+    type: boolean
+    description:
+      Describes whether GPIO1 is used as a switch to disconnect bridge circuits
+      from AVSS. Pin defaults to GPIO if this property is not present.
+
+  adi,vbias-pins:
+    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 9
+    items:
+      minimum: 0
+      maximum: 8
+
+  adi,dig-aux1:
+    description:
+      Describes whether DIG_AUX1 pin will operate as data ready output,
+      synchronization output signal (SYNC_OUT), or if it will be disabled.
+      A value of 0 indicates DIG_AUX1 pin disabled. High impedance.
+      A value of 1 indicates DIG_AUX1 is configured as ADC data ready output.
+      A value of 1 indicates DIG_AUX1 is configured as SYNC_OUT output.
+      If this property is absent, DIG_AUX1 pin is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 0
+
+  adi,dig-aux2:
+    description:
+      Describes whether DIG_AUX2 pin will function as DAC LDAC input,
+      synchronization start input (START), or if it will be disabled.
+      A value of 0 indicates DIG_AUX2 pin is disabled. High impedance.
+      A value of 1 indicates DIG_AUX2 pin is configured as active-low LDAC input
+      for the DAC.
+      A value of 2 indicates DIG_AUX2 pin is configured as START input.
+      If this property is absent, DIG_AUX2 pin is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 0
+
+  adi,sync-option:
+    description:
+      Describes how ADC conversions are going to be synchronized. A value of 1
+      indicates the SYNC_IN pin will function as a synchronization input that
+      allows the user to control the start of sampling by pulling SYNC_IN high.
+      Use option number 2 to set the alternate synchronization functionality
+      which allows per channel conversion start control when multiple channels
+      are enabled. Option number 0 disables synchronization.
+      A value of 0 indicates no synchronization. SYNC_IN pin disabled.
+      A value of 1 indicates standard synchronization functionality.
+      A value of 2 indicates alternate synchronization functionality.
+      If this property is absent, no synchronization is performed.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 1
+
+  adi,excitation-pin-0:
+    description: |
+      Specifies the pin to apply excitation current 0 (IOUT0). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT0 to GPIO0.
+      18: Output excitation current IOUT0 to GPIO1.
+      19: Output excitation current IOUT0 to GPIO2.
+      20: Output excitation current IOUT0 to GPIO3.
+      If this property is absent, IOUT0 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-1:
+    description: |
+      Specifies the pin to apply excitation current 1 (IOUT1). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT1 to GPIO0.
+      18: Output excitation current IOUT1 to GPIO1.
+      19: Output excitation current IOUT1 to GPIO2.
+      20: Output excitation current IOUT1 to GPIO3.
+      If this property is absent, IOUT1 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-2:
+    description: |
+      Specifies the pin to apply excitation current 2 (IOUT2). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT2 to GPIO0.
+      18: Output excitation current IOUT2 to GPIO1.
+      19: Output excitation current IOUT2 to GPIO2.
+      20: Output excitation current IOUT2 to GPIO3.
+      If this property is absent, IOUT2 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-3:
+    description: |
+      Specifies the pin to apply excitation current 3 (IOUT3). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT3 to GPIO0.
+      18: Output excitation current IOUT3 to GPIO1.
+      19: Output excitation current IOUT3 to GPIO2.
+      20: Output excitation current IOUT3 to GPIO3.
+      If this property is absent, IOUT3 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-current-0-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT0 output pin
+      specified in adi,excitation-pin-0.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-1-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT1 output pin
+      specified in adi,excitation-pin-1.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-2-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT2 output pin
+      specified in adi,excitation-pin-2.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-3-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT3 output pin
+      specified in adi,excitation-pin-3.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,chop-iexc:
+    description: |
+      Specifies the chopping/swapping functionality for excitation currents.
+      0: No Chopping of Excitation Currents.
+      1: Chop/swap IOUT0 and IOUT1 (pair AB) excitation currents.
+      2: Chop/swap IOUT2 and IOUT3 (pair CD) excitation currents.
+      3: Chop/swap both pairs (pair AB and pair CD) of excitation currents.
+      If this property is absent, no chopping is performed.
+      There are macros for the above values in dt-bindings/iio/adi,ad4170.h.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2, 3]

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 228

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+    default: 0
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":

Unit-addresses are typically hex, not decimal.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 239

Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +11 -0
+    $ref: adc.yaml
+    type: object
+    unevaluatedProperties: false
+    description: |
+      Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number. The device can have up to 16 channels numbered
+          from 0 to 15.

Don't need to say in prose what the schema says. IOW, drop the 2nd sentence.

Quoted 237 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +234 -0
+        items:
+          minimum: 0
+          maximum: 15
+
+      diff-channels:
+        description: |
+          This property is used for defining the inputs of a differential
+          voltage channel. The first value is the positive input and the second
+          value is the negative input of the channel.
+
+          Besides the analog input pins AIN0 to AIN8, there are special inputs
+          that can be selected with the following values:
+            17: Temperature sensor input
+            18: (AVDD-AVSS)/5
+            19: (IOVDD-DGND)/5
+            20: DAC output
+            21: ALDO
+            22: DLDO
+            23: AVSS
+            24: DGND
+            25: REFIN+
+            26: REFIN-
+            27: REFIN2+
+            28: REFIN2-
+            29: REFOUT
+
+          There are macros for those values in dt-bindings/iio/adi,ad4170.h.
+
+        items:
+          minimum: 0
+          maximum: 31
+
+      single-channel: true
+
+      common-mode-channel: true
+
+      bipolar: true
+
+      adi,config-setup-number:
+        description: |
+          Specifies which of the 8 setups are used to configure the channel.
+          A setup comprises of: AFE, FILTER, FILTER_FS, MISC, OFFSET, and GAIN
+          registers. More than one channel can use the same configuration setup
+          number in which case they will share the settings of the above
+          mentioned registers.
+        items:
+          minimum: 0
+          maximum: 7
+
+      adi,chop-adc:
+        description: |
+          Specifies the chopping/swapping functionality for a channel setup.
+          Macros for adi,chop-adc values are available in
+          dt-bindings/iio/adi,ad4170.h. When enabled, the analog inputs are
+          continuously swapped and a conversion is generated for each time a
+          swap occurs. The analog input pins are connected in one direction,
+          sampled, swapped, sampled again, and then the conversion results are
+          averaged. The input swap minimizes system offset and offset drift.
+          This property also specifies whether AC excitation using 2 or 4 GPIOs
+          are going to be used.
+          0: No channel chop.
+          1: Chop/swap the channel inputs.
+          2: AC Excitation using 4 GPIOs.
+          3: AC Excitation using 2 GPIOs.
+          If this property is absent, no chopping is performed.
+        $ref: /schemas/types.yaml#/definitions/uint16
+        enum: [0, 1, 2, 3]
+        default: 0
+
+      adi,burnout-current-nanoamp:
+        description: |
+          Current in nanoamps to be applied for this channel. Burnout currents
+          are only active when the channel is selected for conversion.
+        enum: [0, 100, 2000, 10000]
+        default: 0
+
+      adi,buffered-negative:
+        description: Enable precharge buffer, full buffer, or skip reference
+          buffering of the negative voltage reference. Because the output
+          impedance of the source driving the voltage reference inputs may be
+          dynamic, RC combinations of those inputs can cause DC gain errors if
+          the reference inputs go unbuffered into the ADC. Enable reference
+          buffering if the provided reference source has dynamic high impedance
+          output.
+        enum: [0, 1, 2]
+        default: 0
+
+      adi,buffered-positive:
+        description: Enable precharge buffer, full buffer, or skip reference
+          buffering of the positive voltage reference. Because the output
+          impedance of the source driving the voltage reference inputs may be
+          dynamic, RC combinations of those inputs can cause DC gain errors if
+          the reference inputs go unbuffered into the ADC. Enable reference
+          buffering if the provided reference source has dynamic high impedance
+          output.
+        enum: [0, 1, 2]
+        default: 0
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on the specific
+          channel. Valid values are:
+          0: Differential reference voltage REFIN+ - REFIN−.
+          1: Differential reference voltage REFIN2+ - REFIN2−.
+          2: Internal 2.5V referece (REFOUT) relative to AVSS.
+          3: Analog supply voltage (AVDD) relative relative AVSS.
+          If this field is left empty, the internal reference is selected.
+        $ref: /schemas/types.yaml#/definitions/uint8
+        enum: [0, 1, 2, 3]
+        default: 2
+
+    required:
+      - reg
+      - adi,config-setup-number
+
+    allOf:
+      - oneOf:
+          - required: [single-channel]
+            properties:
+              diff-channels: false
+          - required: [diff-channels]
+            properties:
+              single-channel: false
+              common-mode-channel: false
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - iovdd-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/iio/adc/adi,ad4170.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad4170";
+            reg = <0>;
+            avdd-supply = <&avdd>;
+            iovdd-supply = <&iovdd>;
+            spi-max-frequency = <20000000>;
+            interrupt-parent = <&gpio_in>;
+            interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
+            adi,dig-aux1 = /bits/ 8 <1>;
+            adi,dig-aux2 = /bits/ 8 <0>;
+            adi,sync-option = /bits/ 8 <0>;
+            adi,excitation-pin-0 = <19>;
+            adi,excitation-current-0-microamp = <10>;
+            adi,excitation-pin-1 = <20>;
+            adi,excitation-current-1-microamp = <10>;
+            adi,chop-iexc = /bits/ 8 <1>;
+            adi,vbias-pins = <5 6>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            // Sample AIN0 with respect to AIN1 throughout AVDD/AVSS input range
+            // Fully differential. If AVSS < 0V, Fully differential true bipolar
+            channel@0 {
+                reg = <0>;
+                bipolar;
+                diff-channels = <AD4170_MAP_AIN0 AD4170_MAP_AIN1>;
+                adi,config-setup-number = <0>;
+                adi,reference-select = /bits/ 8 <3>;
+                adi,burnout-current-nanoamp = <100>;
+            };
+            // Sample AIN2 with respect to DGND throughout AVDD/DGND input range
+            // Peseudo-differential unipolar (fig. 2a)
+            channel@1 {
+                reg = <1>;
+                single-channel = <AD4170_MAP_AIN2>;
+                common-mode-channel = <AD4170_MAP_DGND>;
+                adi,config-setup-number = <1>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN3 with respect to 2.5V throughout AVDD/AVSS input range
+            // Pseudo-differential bipolar (fig. 2b)
+            channel@2 {
+                reg = <2>;
+                bipolar;
+                single-channel = <AD4170_MAP_AIN3>;
+                common-mode-channel = <AD4170_MAP_REFOUT>;
+                adi,config-setup-number = <2>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN4 with respect to DGND throughout AVDD/AVSS input range
+            // Pseudo-differential true bipolar if AVSS < 0V (fig. 2c)
+            channel@3 {
+                reg = <3>;
+                bipolar;
+                single-channel = <AD4170_MAP_AIN4>;
+                common-mode-channel = <AD4170_MAP_DGND>;
+                adi,config-setup-number = <3>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN5 with respect to 2.5V throughout AVDD/REFOUT input range
+            // Pseudo-differential unipolar (AD4170 datasheet page 46 example)
+            channel@4 {
+                reg = <4>;
+                single-channel = <AD4170_MAP_AIN5>;
+                common-mode-channel = <AD4170_MAP_REFOUT>;
+                adi,config-setup-number = <4>;
+                adi,reference-select = /bits/ 8 <2>;
+            };
+            // Sample AIN6 with respect to REFIN+ throughout AVDD/AVSS input range
+            // Pseudo-differential unipolar
+            channel@5 {
+                reg = <5>;
+                single-channel = <AD4170_MAP_AIN6>;
+                common-mode-channel = <AD4170_MAP_REFIN1_P>;
+                adi,config-setup-number = <4>;
+                adi,reference-select = /bits/ 8 <2>;
+            };
+            // Sample AIN7 with respect to DGND throughout REFIN+/REFIN- input range
+            // Pseudo-differential bipolar
+            channel@6 {
+                reg = <6>;
+                bipolar;
+                diff-channels = <AD4170_MAP_AIN7 AD4170_MAP_DGND>;
+                adi,config-setup-number = <5>;
+                adi,reference-select = /bits/ 8 <0>;
+            };
+        };
+    };
+...
+
Signature · 1 line
2.45.2
maintainer
#6 lore raw

Re: [RFC PATCH 2/4] dt-bindings: iio: adc: Add AD4170

Quoted 7 lines · On Wed, 18 Dec 2024 13:48:04 -0600 Rob Herring <[email protected]> wrote:
Quoted 2 lines · Marcelo Schmitt

Add device tree documentation for AD4170 sigma-delta ADCs.

The usual question with long lists of properties. Any of these should be run-time controlled (by userspace) instead? If they might vary by the user for given h/w design, then should be user controlled instead.

Hi Rob,

I took a look and mostly make sense in DT as they are hardware design things as it's all about driving an external bridge circuit for which there will be a correct configuration.

There were a few exceptions however hiding in amongst them!

ADCs that support Wheatstone bridge operations are probably my least favourite drivers to review. Very very fiddly as they have complex (and random seeming) control functions for circuitry that is off chip and only somewhat standard.

Jonathan

maintainer
#7 lore raw

Re: [RFC PATCH 2/4] dt-bindings: iio: adc: Add AD4170

In reply to Marcelo Schmitt
Quoted 74 lines · On Wed, 18 Dec 2024 11:37:42 -0300 Marcelo Schmitt…

Add device tree documentation for AD4170 sigma-delta ADCs.

---
 .../bindings/iio/adc/adi,ad4170.yaml          | 473 ++++++++++++++++++
 1 file changed, 473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +60 -0
new file mode 100644
index 000000000000..8c5defc614ee
@@ -0,0 +1,473 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4170 Analog to Digital Converter
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD4170 Analog to Digital Converter.
+  Specifications can be found at:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4170
+
+  avss-supply:
+    description:
+      Referece voltage supply for AVDD. AVSS can be set below 0V to provide a
+      bipolar power supply to AD4170-4. Must be −2.625V at minimum, 0V maximum.
+      If not specified, this is assumed to be analog ground.
+
+  avdd-supply:
+    description:
+      A supply of 4.75V to 5.25V relative to AVSS that powers the chip (AVDD).
+
+  iovdd-supply:
+    description: 1.7V to 5.25V reference supply to the serial interface (IOVDD).
+
+  refin1p-supply:
+    description: REFIN+ supply that can be used as reference for conversion.
+
+  refin1n-supply:
+    description: REFIN- supply that can be used as reference for conversion.
+
+  refin2p-supply:
+    description: REFIN2+ supply that can be used as reference for conversion.
+
+  refin2n-supply:
+    description: REFIN2- supply that can be used as reference for conversion.
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: |
+      Optional external clock source. Can include one clock source: external
+      clock or external crystal.
+
+  clock-names:
+    enum:
+      - ext-clk

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 70

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+      - xtal
+
+  '#clock-cells':
+    const: 0
+
+  adi,gpio0-power-down-switch:
+    type: boolean
+    description:
+      Describes whether GPIO0 is used as a switch to disconnect bridge circuits
+      from AVSS. Pin defaults to GPIO if this property is not present.

This is interesting because it's power control for external bridges. Maybe should be described as a regulator (output)? Any bridge circuit will need to be an IIO consumer anyway so we can work out the channel scaling so that would then consume this regulator as it's power supply.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 80

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+
+  adi,gpio1-power-down-switch:
+    type: boolean
+    description:
+      Describes whether GPIO1 is used as a switch to disconnect bridge circuits
+      from AVSS. Pin defaults to GPIO if this property is not present.
+
+  adi,vbias-pins:
+    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
+    $ref: /schemas/types.yaml#/definitions/uint32-array

This is described as being relevant if we have say a thermocouple wired up. For that I'd expect it to both be channel specific and us to need a bunch of other description.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 87

Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +7 -0
+    minItems: 1
+    maxItems: 9
+    items:
+      minimum: 0
+      maximum: 8
+
+  adi,dig-aux1:

If it's a data ready signal, then interrupt/ interrupt-names probably appropriate.

Sync is messier as you need a binding to describe where that goes but we have provided a specific property for this in the past. Try to match up with an existing one. Trickier as there are multiple of these here.

The high impedance is what you do if not an interrupt and the sync out not set.

Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +9 -0
+    description:
+      Describes whether DIG_AUX1 pin will operate as data ready output,
+      synchronization output signal (SYNC_OUT), or if it will be disabled.
+      A value of 0 indicates DIG_AUX1 pin disabled. High impedance.
+      A value of 1 indicates DIG_AUX1 is configured as ADC data ready output.
+      A value of 1 indicates DIG_AUX1 is configured as SYNC_OUT output.
+      If this property is absent, DIG_AUX1 pin is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 106

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+    default: 0
+
+  adi,dig-aux2:
+    description:
+      Describes whether DIG_AUX2 pin will function as DAC LDAC input,
+      synchronization start input (START), or if it will be disabled.
+      A value of 0 indicates DIG_AUX2 pin is disabled. High impedance.
+      A value of 1 indicates DIG_AUX2 pin is configured as active-low LDAC input
+      for the DAC.
+      A value of 2 indicates DIG_AUX2 pin is configured as START input.

Ah. the other side of sync.
Also DAC toggle pin. If we are using it as a dac toggle, needs a gpio binding. Start will need a specific property. High impedance is what we do if the others not set.

Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +6 -0
+      If this property is absent, DIG_AUX2 pin is disabled.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 0
+
+  adi,sync-option:

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 122

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+    description:
+      Describes how ADC conversions are going to be synchronized. A value of 1
+      indicates the SYNC_IN pin will function as a synchronization input that
+      allows the user to control the start of sampling by pulling SYNC_IN high.
+      Use option number 2 to set the alternate synchronization functionality
+      which allows per channel conversion start control when multiple channels
+      are enabled. Option number 0 disables synchronization.
+      A value of 0 indicates no synchronization. SYNC_IN pin disabled.
+      A value of 1 indicates standard synchronization functionality.
+      A value of 2 indicates alternate synchronization functionality.

This is a software choice. I can't see why it belongs in DT unless the assumption is some external timing circuitry is driving this. To actually control mode 2 timing would be interesting as doesn't correspond to triggering etc in iio which is matching standard synchronization assuming I got right idea from quick read of the datasheet section on this).

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 123

Quoted 1 line
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +1 -0
+      If this property is absent, no synchronization is performed.

Inconsistent with default.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 127

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +4 -0
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2]
+    default: 1
+

All the bridge related excitation control etc makes me wonder if we need a way to describe the bridge rather than the modes of this device. I can't immediately see how to do it though as I'm not sure this stuff is that standard across devices.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 131

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +4 -0
+  adi,excitation-pin-0:
+    description: |
+      Specifies the pin to apply excitation current 0 (IOUT0). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.

Add some flavour for this. What is it for? Basically make a clear argument it is all about the external circuitry.

Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +8 -0
+      17: Output excitation current IOUT0 to GPIO0.
+      18: Output excitation current IOUT0 to GPIO1.
+      19: Output excitation current IOUT0 to GPIO2.
+      20: Output excitation current IOUT0 to GPIO3.
+      If this property is absent, IOUT0 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 149

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+
+  adi,excitation-pin-1:
+    description: |
+      Specifies the pin to apply excitation current 1 (IOUT1). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT1 to GPIO0.
+      18: Output excitation current IOUT1 to GPIO1.
+      19: Output excitation current IOUT1 to GPIO2.
+      20: Output excitation current IOUT1 to GPIO3.
+      If this property is absent, IOUT1 is not routed to any pin.

Default doesn't make much sense if that is intent. Just drop it.

Maybe see if there is a sensible path to sharing the text for these given repitition. If they were always there I'd suggest an array, but we need the 'not in use' value and magic numbers for that are nasty.

Quoted 24 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +24 -0
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-2:
+    description: |
+      Specifies the pin to apply excitation current 2 (IOUT2). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT2 to GPIO0.
+      18: Output excitation current IOUT2 to GPIO1.
+      19: Output excitation current IOUT2 to GPIO2.
+      20: Output excitation current IOUT2 to GPIO3.
+      If this property is absent, IOUT2 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-pin-3:
+    description: |
+      Specifies the pin to apply excitation current 3 (IOUT3). Besides the
+      analog pins 0 to 8, the excitation current can be applied to GPIO pins.
+      17: Output excitation current IOUT3 to GPIO0.
+      18: Output excitation current IOUT3 to GPIO1.
+      19: Output excitation current IOUT3 to GPIO2.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 183

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+      20: Output excitation current IOUT3 to GPIO3.
+      If this property is absent, IOUT3 is not routed to any pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20]
+    default: 0
+
+  adi,excitation-current-0-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT0 output pin
+      specified in adi,excitation-pin-0.

As above, see if you can combine entries and add a bit of flavour on the why.

Quoted 59 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +59 -0
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-1-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT1 output pin
+      specified in adi,excitation-pin-1.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-2-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT2 output pin
+      specified in adi,excitation-pin-2.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,excitation-current-3-microamp:
+    description: |
+      Excitation current in microamps to be applied to IOUT3 output pin
+      specified in adi,excitation-pin-3.
+    enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
+    default: 0
+
+  adi,chop-iexc:
+    description: |
+      Specifies the chopping/swapping functionality for excitation currents.
+      0: No Chopping of Excitation Currents.
+      1: Chop/swap IOUT0 and IOUT1 (pair AB) excitation currents.
+      2: Chop/swap IOUT2 and IOUT3 (pair CD) excitation currents.
+      3: Chop/swap both pairs (pair AB and pair CD) of excitation currents.
+      If this property is absent, no chopping is performed.
+      There are macros for the above values in dt-bindings/iio/adi,ad4170.h.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    enum: [0, 1, 2, 3]
+    default: 0
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":
+    $ref: adc.yaml
+    type: object
+    unevaluatedProperties: false
+    description: |
+      Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number. The device can have up to 16 channels numbered
+          from 0 to 15.
+        items:
+          minimum: 0
+          maximum: 15

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 252

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+
+      diff-channels:
+        description: |
+          This property is used for defining the inputs of a differential
+          voltage channel. The first value is the positive input and the second
+          value is the negative input of the channel.
+
+          Besides the analog input pins AIN0 to AIN8, there are special inputs
+          that can be selected with the following values:
+            17: Temperature sensor input

I guess we can't stop this being set to silly value (so one side of temperature against anything else).

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 264

Quoted 12 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +12 -0
+            18: (AVDD-AVSS)/5
+            19: (IOVDD-DGND)/5
+            20: DAC output
+            21: ALDO
+            22: DLDO
+            23: AVSS
+            24: DGND
+            25: REFIN+
+            26: REFIN-
+            27: REFIN2+
+            28: REFIN2-
+            29: REFOUT

Most of these are typically used for calibration. I'd be tempted to drop them from the channel configuration. Or maybe keep them also provide a way to override and set these up anyway as needed.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 270

Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +6 -0
+
+          There are macros for those values in dt-bindings/iio/adi,ad4170.h.
+
+        items:
+          minimum: 0
+          maximum: 31

Use an enum. We don't want the reserved values.

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +4 -0
+
+      single-channel: true
+
+      common-mode-channel: true

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 284

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+
+      bipolar: true
+
+      adi,config-setup-number:
+        description: |
+          Specifies which of the 8 setups are used to configure the channel.
+          A setup comprises of: AFE, FILTER, FILTER_FS, MISC, OFFSET, and GAIN
+          registers. More than one channel can use the same configuration setup
+          number in which case they will share the settings of the above
+          mentioned registers.

We have always done this in the driver previously. It can be complex due to need to match equivalent configs, but most of this is stuff we want
to control. It doesn't lead to simple code, but it does give a lot of flexibility as things like filters and gains etc are not characteristics of the external wiring (though good choices are related to it).

Quoted 18 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +18 -0
+        items:
+          minimum: 0
+          maximum: 7
+
+      adi,chop-adc:
+        description: |
+          Specifies the chopping/swapping functionality for a channel setup.
+          Macros for adi,chop-adc values are available in
+          dt-bindings/iio/adi,ad4170.h. When enabled, the analog inputs are
+          continuously swapped and a conversion is generated for each time a
+          swap occurs. The analog input pins are connected in one direction,
+          sampled, swapped, sampled again, and then the conversion results are
+          averaged. The input swap minimizes system offset and offset drift.
+          This property also specifies whether AC excitation using 2 or 4 GPIOs
+          are going to be used.
+          0: No channel chop.
+          1: Chop/swap the channel inputs.
+          2: AC Excitation using 4 GPIOs.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 312

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+          3: AC Excitation using 2 GPIOs.
+          If this property is absent, no chopping is performed.
+        $ref: /schemas/types.yaml#/definitions/uint16
+        enum: [0, 1, 2, 3]
+        default: 0
+
+      adi,burnout-current-nanoamp:
+        description: |
+          Current in nanoamps to be applied for this channel. Burnout currents
+          are only active when the channel is selected for conversion.

All about open wire detection. So a runtime configuration thing.

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 323

Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +11 -0
+        enum: [0, 100, 2000, 10000]
+        default: 0
+
+      adi,buffered-negative:
+        description: Enable precharge buffer, full buffer, or skip reference
+          buffering of the negative voltage reference. Because the output
+          impedance of the source driving the voltage reference inputs may be
+          dynamic, RC combinations of those inputs can cause DC gain errors if
+          the reference inputs go unbuffered into the ADC. Enable reference
+          buffering if the provided reference source has dynamic high impedance
+          output.

Say why you would not enable it

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 324

Quoted 1 line
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +1 -0
+        enum: [0, 1, 2]

Value meanings?

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 327

Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +3 -0
+        default: 0
+
+      adi,buffered-positive:

Probably put positive first. Feels more natural!

Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +9 -0
+        description: Enable precharge buffer, full buffer, or skip reference
+          buffering of the positive voltage reference. Because the output
+          impedance of the source driving the voltage reference inputs may be
+          dynamic, RC combinations of those inputs can cause DC gain errors if
+          the reference inputs go unbuffered into the ADC. Enable reference
+          buffering if the provided reference source has dynamic high impedance
+          output.
+        enum: [0, 1, 2]
+        default: 0

Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 346

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10 -0
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on the specific
+          channel. Valid values are:
+          0: Differential reference voltage REFIN+ - REFIN−.
+          1: Differential reference voltage REFIN2+ - REFIN2−.
+          2: Internal 2.5V referece (REFOUT) relative to AVSS.
+          3: Analog supply voltage (AVDD) relative relative AVSS.
+          If this field is left empty, the internal reference is selected.

There is an argument that this might be something you want to change at runtime but it's pretty unlikely so I'm fine with this in DT.

Hmm. I'm not a fan of trying to review these AFE + bridge supporting ADCs. So fiddly and with so many weird options that only make sense if I dig out my memory of analog circuits courses from years ago :(

Jonathan

Quoted 127 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +127 -0
+        $ref: /schemas/types.yaml#/definitions/uint8
+        enum: [0, 1, 2, 3]
+        default: 2
+
+    required:
+      - reg
+      - adi,config-setup-number
+
+    allOf:
+      - oneOf:
+          - required: [single-channel]
+            properties:
+              diff-channels: false
+          - required: [diff-channels]
+            properties:
+              single-channel: false
+              common-mode-channel: false
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - iovdd-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/iio/adc/adi,ad4170.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad4170";
+            reg = <0>;
+            avdd-supply = <&avdd>;
+            iovdd-supply = <&iovdd>;
+            spi-max-frequency = <20000000>;
+            interrupt-parent = <&gpio_in>;
+            interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
+            adi,dig-aux1 = /bits/ 8 <1>;
+            adi,dig-aux2 = /bits/ 8 <0>;
+            adi,sync-option = /bits/ 8 <0>;
+            adi,excitation-pin-0 = <19>;
+            adi,excitation-current-0-microamp = <10>;
+            adi,excitation-pin-1 = <20>;
+            adi,excitation-current-1-microamp = <10>;
+            adi,chop-iexc = /bits/ 8 <1>;
+            adi,vbias-pins = <5 6>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            // Sample AIN0 with respect to AIN1 throughout AVDD/AVSS input range
+            // Fully differential. If AVSS < 0V, Fully differential true bipolar
+            channel@0 {
+                reg = <0>;
+                bipolar;
+                diff-channels = <AD4170_MAP_AIN0 AD4170_MAP_AIN1>;
+                adi,config-setup-number = <0>;
+                adi,reference-select = /bits/ 8 <3>;
+                adi,burnout-current-nanoamp = <100>;
+            };
+            // Sample AIN2 with respect to DGND throughout AVDD/DGND input range
+            // Peseudo-differential unipolar (fig. 2a)
+            channel@1 {
+                reg = <1>;
+                single-channel = <AD4170_MAP_AIN2>;
+                common-mode-channel = <AD4170_MAP_DGND>;
+                adi,config-setup-number = <1>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN3 with respect to 2.5V throughout AVDD/AVSS input range
+            // Pseudo-differential bipolar (fig. 2b)
+            channel@2 {
+                reg = <2>;
+                bipolar;
+                single-channel = <AD4170_MAP_AIN3>;
+                common-mode-channel = <AD4170_MAP_REFOUT>;
+                adi,config-setup-number = <2>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN4 with respect to DGND throughout AVDD/AVSS input range
+            // Pseudo-differential true bipolar if AVSS < 0V (fig. 2c)
+            channel@3 {
+                reg = <3>;
+                bipolar;
+                single-channel = <AD4170_MAP_AIN4>;
+                common-mode-channel = <AD4170_MAP_DGND>;
+                adi,config-setup-number = <3>;
+                adi,reference-select = /bits/ 8 <3>;
+            };
+            // Sample AIN5 with respect to 2.5V throughout AVDD/REFOUT input range
+            // Pseudo-differential unipolar (AD4170 datasheet page 46 example)
+            channel@4 {
+                reg = <4>;
+                single-channel = <AD4170_MAP_AIN5>;
+                common-mode-channel = <AD4170_MAP_REFOUT>;
+                adi,config-setup-number = <4>;
+                adi,reference-select = /bits/ 8 <2>;
+            };
+            // Sample AIN6 with respect to REFIN+ throughout AVDD/AVSS input range
+            // Pseudo-differential unipolar
+            channel@5 {
+                reg = <5>;
+                single-channel = <AD4170_MAP_AIN6>;
+                common-mode-channel = <AD4170_MAP_REFIN1_P>;
+                adi,config-setup-number = <4>;
+                adi,reference-select = /bits/ 8 <2>;
+            };
+            // Sample AIN7 with respect to DGND throughout REFIN+/REFIN- input range
+            // Pseudo-differential bipolar
+            channel@6 {
+                reg = <6>;
+                bipolar;
+                diff-channels = <AD4170_MAP_AIN7 AD4170_MAP_DGND>;
+                adi,config-setup-number = <5>;
+                adi,reference-select = /bits/ 8 <0>;
+            };
+        };
+    };
+...
+
#8 lore raw

RFC PATCH 3/4 iio: adc: Add support for AD4170

In reply to Marcelo Schmitt

From: Ana-Maria Cusco <[email protected]>

From: Ana-Maria Cusco <[email protected]>

Add support for the AD4170 ADC with the following features:
- Single-shot read (read_raw), scale, sampling freq
- Multi channel buffer support
- Buffered capture in triggered mode
- Gain and offset calibration
- chop_iexc and chop_adc device configuration
- Powerdown switch configuration

---
 drivers/iio/adc/Kconfig  |   16 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4170.c | 2049 ++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad4170.h |  316 ++++++
 4 files changed, 2382 insertions(+)
 create mode 100644 drivers/iio/adc/ad4170.c
 create mode 100644 drivers/iio/adc/ad4170.h
drivers/iio/adc/Kconfig +16 -0
index 849c90203071..997473ac958f 100644
@@ -48,6 +48,22 @@ config AD4130
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4130.
 
+
+config AD4170
+	tristate "Analog Device AD4170 ADC Driver"
+	depends on SPI
+	depends on GPIOLIB
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	select REGMAP_SPI
+	depends on COMMON_CLK
+	help
+	  Say yes here to build support for Analog Devices AD4170 SPI analog
+	  to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4170.
+
 config AD4695
 	tristate "Analog Device AD4695 ADC Driver"
 	depends on SPI
drivers/iio/adc/Makefile +1 -0
index ee19afba62b7..64c72b595580 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4130) += ad4130.o
+obj-$(CONFIG_AD4170) += ad4170.o
 obj-$(CONFIG_AD4695) += ad4695.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o
drivers/iio/adc/ad4170.c new file +2049 -0

2,049 lines not on this page · raw message

drivers/iio/adc/ad4170.h new file +316 -0

316 lines not on this page · raw message

Signature · 1 line
2.45.2
#9 lore raw

Re: [RFC PATCH 3/4] iio: adc: Add support for AD4170

drivers/iio/adc/ad4170.c line 39

Quoted 12 lines · Marcelo Schmitt
drivers/iio/adc/ad4170.c new file +12 -0
+#include <asm/div64.h>
+#include <linux/unaligned.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include "ad4170.h"
+

No bindings header included? No usage of binding defines (I did look for them in case you have some incorrect include via other header)? So not a binding...

Best regards,
Krzysztof

maintainer
#10 lore raw

Re: [RFC PATCH 3/4] iio: adc: Add support for AD4170

Quoted 17 lines · On Thu, 19 Dec 2024 10:25:42 +0100 Krzysztof Kozlowski <[email protected]> wrote:

drivers/iio/adc/ad4170.c line 39

Quoted 12 lines · Marcelo Schmitt
drivers/iio/adc/ad4170.c new file +12 -0
+#include <asm/div64.h>
+#include <linux/unaligned.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include "ad4170.h"
+  

No bindings header included? No usage of binding defines (I did look for them in case you have some incorrect include via other header)? So not a binding...

It's in the local header in this patch.

Quoted 6 lines
drivers/iio/adc/ad4170.h new file +0 -0
new file mode 100644
index 000000000000..5b24788314b1

drivers/iio/adc/ad4170.h line 8

Quoted 8 lines
drivers/iio/adc/ad4170.h new file +8 -0
@@ -0,0 +1,316 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * AD4170 ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+
+#include <dt-bindings/iio/adc/adi,ad4170.h>

Agreed on the lack of them being used as widely as they should be though!

Quoted 4 lines

Best regards,
Krzysztof

maintainer
#11 lore raw

Re: [RFC PATCH 3/4] iio: adc: Add support for AD4170

In reply to Marcelo Schmitt
Quoted 3 lines · On Wed, 18 Dec 2024 11:37:59 -0300 Marcelo Schmitt…

From: Ana-Maria Cusco <[email protected]>

From: Ana-Maria Cusco <[email protected]>

Two froms!

Quoted 14 lines

Add support for the AD4170 ADC with the following features:
- Single-shot read (read_raw), scale, sampling freq
- Multi channel buffer support
- Buffered capture in triggered mode
- Gain and offset calibration
- chop_iexc and chop_adc device configuration
- Powerdown switch configuration

Quoted 5 lines
---
 drivers/iio/adc/Kconfig  |   16 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4170.c | 2049 ++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad4170.h |  316 ++++++

Don't have a separate header if only 1 c file. It just adds unnecessary boilerplate and can lead to separations that make little sense.

It's a big driver and I think the changes requested to bindings will make it bigger and more complex still. Hence not the most detailed of reviews follows.

Jonathan

Quoted 6 lines
drivers/iio/adc/ad4170.c new file +0 -0
new file mode 100644
index 000000000000..20b74575f2cb
@@ -0,0 +1,2049 @@

...

drivers/iio/adc/ad4170.c line 65

Quoted 13 lines
drivers/iio/adc/ad4170.c new file +13 -0
+
+static const char *const ad4170_clk_sel[] = {
+	"ext-clk", "xtal"
+};
+
+static const char *const ad4170_i_out_pin_dt_props[] = {
+	"adi,excitation-pin-0",
+	"adi,excitation-pin-1",
+	"adi,excitation-pin-2",
+	"adi,excitation-pin-3",
+};
+
+static const char *const ad4170_i_out_val_dt_props[] = {

Push this down into the place it is used. Same for similar examples of once use static const data.

Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5 -0
+	"adi,excitation-current-0-microamp",
+	"adi,excitation-current-1-microamp",
+	"adi,excitation-current-2-microamp",
+	"adi,excitation-current-3-microamp",
+};

drivers/iio/adc/ad4170.c line 80

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+
+#define AD4170_SINC5_FS_PADDING		8
+#define AD4170_SINC3_FS_OFFSET		2
+
+#define AD4170_SINC3_MIN_FS		4
+#define AD4170_SINC3_MAX_FS		65532
+#define AD4170_SINC5_MIN_FS		1
+#define AD4170_SINC5_MAX_FS		256
+
+/* Make separate fs tables? One for SINC5 other for SINC5+AVG and SINC3? */

That sounds simpler than this.

Quoted 37 lines
drivers/iio/adc/ad4170.c new file +36 -0
+static const unsigned int ad4170_filt_fs_tbl[] = {
+	1, /* SINC5 minimum filter_fs */
+	2,
+	4, /* SINC5+AVG and SINC3 minimum filter_fs */
+	8,
+	12,
+	16,
+	20,
+	40,
+	48,
+	80,
+	100,
+	256, /* SINC5 maximum filter_fs */
+	500,
+	1000,
+	5000,
+	8332,
+	10000,
+	25000,
+	50000,
+	65532 /* SINC5+AVG and SINC3 maximum filter_fs */
+};
 
+struct ad4170_state {
+	struct regmap *regmap;
+	struct spi_device *spi;
+	struct regulator_bulk_data supplies[7];
+	struct mutex lock; /* Protect filter, PGA, GPIO, chan read, chan config */
+	struct iio_chan_spec chans[AD4170_MAX_CHANNELS];
+	struct ad4170_chan_info chans_info[AD4170_MAX_CHANNELS];
+	struct ad4170_setup setups[AD4170_MAX_SETUPS];
+	u32 mclk_hz;
+	enum ad4170_pin_function pins_fn[AD4170_NUM_ANALOG_PINS];
+	enum ad4170_gpio_function gpio_fn[AD4170_NUM_GPIO_PINS];
+	unsigned int clock_ctrl;
+	struct clk *ext_clk;
+	struct clk_hw int_clk_hw;

drivers/iio/adc/ad4170.c line 163

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][ARRAY_SIZE(ad4170_filt_fs_tbl)][2];
+	struct completion completion;
+	struct iio_trigger *trig;
+	u32 data[AD4170_MAX_CHANNELS];
+
+	struct spi_transfer xfer;
+	struct spi_message msg;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the transfer buffers
+	 * to live in their own cache lines.

Comment doesn't match code. write_tx_buf is not in the cacheline you intend.

Quoted 29 lines
drivers/iio/adc/ad4170.c new file +28 -0
+	 */
+	u8 reg_write_tx_buf[6];
+	__be32 reg_read_rx_buf __aligned(IIO_DMA_MINALIGN);
+	__be16 reg_read_tx_buf;
+};
+
 
+static const struct regmap_config ad4170_regmap_config = {
+	.reg_bits = 14,
+	.val_bits = 32,
+	.reg_format_endian = REGMAP_ENDIAN_BIG,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.reg_read = ad4170_reg_read,
+	.reg_write = ad4170_reg_write,
+};
+
+static int ad4170_set_mode(struct ad4170_state *st, unsigned int mode)
+{
+	return regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
+				  AD4170_REG_CTRL_MODE_MSK,
+				  FIELD_PREP(AD4170_REG_CTRL_MODE_MSK, mode));
+}
+
+/* 8 possible setups (0-7)*/
+static int ad4170_write_setup(struct ad4170_state *st, unsigned int setup_num,
+			      struct ad4170_setup *setup)
+{
+	int ret;
+

drivers/iio/adc/ad4170.c line 338

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	/*
+	 * It is recommended to place the ADC in standby mode or idle mode to
+	 * write to OFFSET and GAIN registers.
+	 */
+	ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_IDLE);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4170_MISC_REG(setup_num), setup->misc);
+	if (ret < 0)

regmap always returns 0 on success so you can just do if (ret)

Quoted 16 lines
drivers/iio/adc/ad4170.c new file +16 -0
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4170_AFE_REG(setup_num), setup->afe);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4170_FILTER_REG(setup_num),
+			   setup->filter);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4170_FILTER_FS_REG(setup_num),
+			   setup->filter_fs);
+	if (ret < 0)
+		return ret;
+

drivers/iio/adc/ad4170.c line 364

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	ret = regmap_write(st->regmap, AD4170_OFFSET_REG(setup_num),
+			   setup->offset);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4170_GAIN_REG(setup_num), setup->gain);
+	if (ret < 0)
+		return ret;
+
+	return 0;

Advantage of above is then return regmap_write() etc is obviously the same as the earlier calls.

Quoted 25 lines
drivers/iio/adc/ad4170.c new file +24 -0
+}
 
+static int ad4170_set_filter_type(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int val)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	struct ad4170_chan_info *chan_info = &st->chans_info[chan->address];
+	struct ad4170_setup *setup = &st->setups[chan_info->setup_num];
+	unsigned int old_filter_fs, old_filter, filter_type_conf;
+	int ret = 0;
+
+	switch (val) {
+	case AD4170_SINC5_AVG:
+		filter_type_conf = AD4170_SINC5_AVG_CONF;
+		break;
+	case AD4170_SINC5:
+		filter_type_conf = AD4170_SINC5_CONF;
+		break;
+	case AD4170_SINC3:
+		filter_type_conf = AD4170_SINC3_CONF;
+		break;
+	default:
+		return -EINVAL;
+	}

drivers/iio/adc/ad4170.c line 415

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+
+	/*
+	 * The filters provide the same ODR for a given filter_fs value but
+	 * there are different minimum and maximum filter_fs limits for each
+	 * filter. The filter_fs value will be adjusted if the current filter_fs
+	 * is out of the limits of the just requested filter. Since the
+	 * filter_fs value affects the ODR (sampling_frequency), changing the
+	 * filter may lead to a change in the sampling frequency.
+	 */
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {

As below on this magic going away. Though in this case not worth a helper function as no early returns.

Quoted 44 lines
drivers/iio/adc/ad4170.c new file +43 -0
+		old_filter = setup->filter;
+		old_filter_fs = setup->filter_fs;
+		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3) {
+			if (setup->filter_fs < AD4170_SINC3_MIN_FS)
+				setup->filter_fs = AD4170_SINC3_MIN_FS;
+			if (setup->filter_fs > AD4170_SINC3_MAX_FS)
+				setup->filter_fs = AD4170_SINC3_MAX_FS;
+
+		} else if (val == AD4170_SINC5) {
+			if (setup->filter_fs < AD4170_SINC5_MIN_FS)
+				setup->filter_fs = AD4170_SINC5_MIN_FS;
+			if (setup->filter_fs > AD4170_SINC5_MAX_FS)
+				setup->filter_fs = AD4170_SINC5_MAX_FS;
+		}
+
+		setup->filter &= ~AD4170_SETUPS_FILTER_TYPE_MSK;
+		setup->filter |= FIELD_PREP(AD4170_SETUPS_FILTER_TYPE_MSK,
+					    filter_type_conf);
+
+		guard(mutex)(&st->lock);
+		ret = ad4170_write_setup(st, chan_info->setup_num, setup);
+		if (ret) {
+			setup->filter = old_filter;
+			setup->filter_fs = old_filter_fs;
+		}
+		return ret;
+	}
+	unreachable();
+}
 
+
+static int ad4170_read_sample(struct iio_dev *indio_dev, unsigned int channel,
+			      int *val)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	struct ad4170_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4170_setup *setup = &st->setups[chan_info->setup_num];
+	int precision_bits = ad4170_channel_template.scan_type.realbits;
+	int ret;
+
+	guard(mutex)(&st->lock);
+	ret = ad4170_set_channel_enable(st, channel, true);
+	if (ret)
+		return ret;

drivers/iio/adc/ad4170.c line 546

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+
+	reinit_completion(&st->completion);
+
+	ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_SINGLE);
+	if (ret)
+		return ret;
+
+	ret = wait_for_completion_timeout(&st->completion, HZ);
+	if (!ret)
+		goto out;

Odd to let this go, but for other cases not re enable the channel. Add a comment on why you are doing this.

Quoted 14 lines
drivers/iio/adc/ad4170.c new file +14 -0
+
+	ret = regmap_read(st->regmap, AD4170_DATA_24b_REG, val);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(AD4170_AFE_BIPOLAR_MSK, setup->afe))
+		*val = sign_extend32(*val, precision_bits - 1);
+out:
+	ret = ad4170_set_channel_enable(st, channel, false);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT;
+}

...

Quoted 21 lines
drivers/iio/adc/ad4170.c new file +21 -0
+/*
+ * Receives the device state, the channel spec, a reference selection, and
+ * returns the magnitude of the allowed input range in µV.
+ * Verifies whether the channel configuration is valid by checking the provided
+ * input type, polarity, and voltage references result in a sane input range.
+ * Returns negative error code on failure.
+ */
+static int ad4170_get_input_range(struct ad4170_state *st,
+				  struct iio_chan_spec const *chan,
+				  unsigned int ref_sel)
+{
+	struct ad4170_chan_info *chan_info = &st->chans_info[chan->address];
+	struct ad4170_setup *setup = &st->setups[chan_info->setup_num];
+	bool bipolar = FIELD_GET(AD4170_AFE_BIPOLAR_MSK, setup->afe);
+	int refp, refn, ain_voltage, ret;
+
+	switch (ref_sel) {
+	case AD4170_AFE_REFIN_REFIN1:
+		refp = regulator_get_voltage(st->supplies[AD4170_REFIN1P_SUP].consumer);
+		refn = regulator_get_voltage(st->supplies[AD4170_REFIN1N_SUP].consumer);
+		break;

drivers/iio/adc/ad4170.c line 745

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	case AD4170_AFE_REFIN_REFIN2:
+		refp = regulator_get_voltage(st->supplies[AD4170_REFIN2P_SUP].consumer);
+		refn = regulator_get_voltage(st->supplies[AD4170_REFIN2N_SUP].consumer);
+		break;
+	case AD4170_AFE_REFIN_AVDD:
+		refp = regulator_get_voltage(st->supplies[AD4170_AVDD_SUP].consumer);
+		ret = regulator_get_voltage(st->supplies[AD4170_AVSS_SUP].consumer);
+		/*
+		 * TODO AVSS is actually optional.
+		 * Should we handle -EPROBE_DEFER here?

Not here. That should have been handled when originally getting the regulator.

Quoted 21 lines
drivers/iio/adc/ad4170.c new file +21 -0
+		 */
+		if (ret < 0)
+			ret = 0; /* Assume AVSS at GND if not provided */
+
+		refn = ret;
+		break;
+	case AD4170_AFE_REFIN_REFOUT:
+		refn = regulator_get_voltage(st->supplies[AD4170_AVSS_SUP].consumer);
+		if (refn < 0)
+			refn = 0;
+
+		/* REFOUT is 2.5 V relative to AVSS */
+		/* avss-supply is never above 0V. */
+		refp = AD4170_INT_REF_2_5V - refn;
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (refp < 0)
+		return refp;
+

drivers/iio/adc/ad4170.c line 776

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	if (refn < 0)
+		return refn;
+
+	/*
+	 * Find out the analog input range from the channel type, polarity, and
+	 * voltage reference selection.
+	 * AD4170 channels are either differential or pseudo-differential.
+	 */
+	/* Differential Input Voltage Range: −VREF/gain to +VREF/gain (datasheet page 6) */
+	/* Single-Ended Input Voltage Range: 0 to VREF/gain (datasheet page 6) */

Checkpatch is going to moan. Just make a larger multiline comment without appropriate formatting to have same affect as here.

drivers/iio/adc/ad4170.c line 786

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	if (chan->differential) {
+		if (!bipolar)
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "Invalid channel %lu setup.\n",
+					     chan->address);
+
+		/* Differential bipolar channel */
+		/* avss-supply is never above 0V. */
+		/* Assuming refin1n-supply not above 0V. */
+		/* Assuming refin2n-supply not above 0V. */

More to combine into a block comment with some extra formatting.

Quoted 44 lines
drivers/iio/adc/ad4170.c new file +44 -0
+		return refp + refn;
+	}
+	/*
+	 * Some configurations can lead to invalid setups.
+	 * For example, if AVSS = -2.5V, REF_SELECT set to REFOUT (REFOUT/AVSS),
+	 * and single-ended channel configuration set, then the input range
+	 * should go from 0V to +VREF (single-ended - datasheet pg 10), but
+	 * REFOUT/AVSS range would be -2.5V to 0V.
+	 * Check the positive reference is higher than 0V for pseudo-diff
+	 * channels.
+	 */
+	if (bipolar) {
+		/* Pseudo-differential bipolar channel */
+		/* Input allowed to swing from GND to +VREF */
+		if (refp <= 0)
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "Invalid setup for channel %lu.\n",
+					     chan->address);
+
+		return refp;
+	}
+
+	/* Pseudo-differential unipolar channel */
+	/* Input allowed to swing from IN- to +VREF */
+	if (refp <= 0)
+		return dev_err_probe(&st->spi->dev, -EINVAL,
+				     "Invalid setup for channel %lu.\n",
+				     chan->address);
+
+	ret = ad4170_get_AINM_voltage(st, chan->channel2, &ain_voltage);
+	if (ret < 0)
+		return ret;
+
+	if (refp - ain_voltage <= 0)
+		return dev_err_probe(&st->spi->dev, -EINVAL,
+				     "Invalid setup for channel %lu.\n",
+				     chan->address);
+
+	return refp - ain_voltage;
+}
+
+static int ad4170_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long info)

drivers/iio/adc/ad4170.c line 840

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	struct ad4170_chan_info *chan_info = &st->chans_info[chan->address];
+	struct ad4170_setup *setup = &st->setups[chan_info->setup_num];
+	enum ad4170_filter_type f_type;
+	unsigned int pga, fs_idx;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)

Sorry to say that I think this is going away. Please open code the claim and release. If I finish reviewing the backlog I'll start work on ripping this out. The unreachable() here is one of the reasons it is a pain and not worth it

Quoted 29 lines
drivers/iio/adc/ad4170.c new file +28 -0
+			return ad4170_read_sample(indio_dev, chan->address, val);
+		unreachable();
+	case IIO_CHAN_INFO_SCALE:
+		pga = FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);
+		*val = chan_info->scale_tbl[pga][0];
+		*val2 = chan_info->scale_tbl[pga][1];
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OFFSET:
+		pga = FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);
+		*val = chan_info->offset_tbl[pga];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		f_type = ad4170_filter_to_filter_type(setup->filter);
+		fs_idx = find_closest(setup->filter_fs, ad4170_filt_fs_tbl,
+				      ARRAY_SIZE(ad4170_filt_fs_tbl));
+		*val = st->sps_tbl[f_type][fs_idx][0];
+		*val2 = st->sps_tbl[f_type][fs_idx][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		*val = setup->offset;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = setup->gain;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
 

drivers/iio/adc/ad4170.c

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +8 -0
 
 
+
+static int ad4170_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {

It's a pain but this is almost certainly going away. Easiest solution is use the opencoded claim and release but then factor out the bit in this scope as a __ad4170_write_raw() so that you can still do direct returns etc.

Quoted 20 lines
drivers/iio/adc/ad4170.c new file +20 -0
+		switch (info) {
+		case IIO_CHAN_INFO_SCALE:
+			return ad4170_set_pga(indio_dev, st, chan->address, val,
+					      val2);
+		case IIO_CHAN_INFO_SAMP_FREQ:
+			return ad4170_set_channel_freq(st, chan->address, val,
+						       val2);
+		case IIO_CHAN_INFO_CALIBBIAS:
+			return ad4170_set_calib_offset(indio_dev, chan->address,
+						       val);
+		case IIO_CHAN_INFO_CALIBSCALE:
+			return ad4170_set_calib_gain(indio_dev, chan->address,
+						     val);
+		default:
+			return -EINVAL;
+		}
+	}
+	unreachable();
+}
+

drivers/iio/adc/ad4170.c line 1137

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+static int ad4170_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *active_scan_mask)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	unsigned int channel;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	for_each_set_bit(channel, active_scan_mask, indio_dev->num_channels) {

Can't do this any more... Try building on latest kernel. You will need to use an accessor function to get num_channels.

drivers/iio/adc/ad4170.c line 650

Quoted 6 lines
drivers/iio/adc/ad4170.c new file +6 -0
+		ret = ad4170_set_channel_enable(st, channel, true);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}

c int ad4170_parse_channels(struct iio_dev *indio_dev)

Quoted 9 lines
drivers/iio/adc/ad4170.c new file +9 -0
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	int chan_num, i, ret;
+
+	chan_num = 0;
+	device_for_each_child_node_scoped(dev, child) {
+		ret = ad4170_parse_channel_node(indio_dev, child, chan_num);
+		if (ret)

drivers/iio/adc/ad4170.c line 1411

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+			return ret;
+
+		chan_num++;
+	}
+	for (i = 0; i < AD4170_MAX_CHANNELS; i++)
+		if (st->chans[i].scan_index == 0)
+			break;
+
+	/*
+	 * When more than one channel is enabled, channel 0 must always be used.

Maybe say why?

Quoted 8 lines
drivers/iio/adc/ad4170.c new file +8 -0
+	 */
+	if (chan_num > 1 && i == AD4170_MAX_CHANNELS)
+		return dev_err_probe(dev, -EINVAL,
+				     "Channel 0 must be configured\n");
+
+	indio_dev->channels = st->chans;
+	indio_dev->num_channels = chan_num;
+	return 0;

drivers/iio/adc/ad4170.c

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +9 -0
+}
 
+
+static int ad4170_clk_output_is_enabled(struct clk_hw *hw)
+{
+	struct ad4170_state *st = clk_hw_to_ad4170(hw);
+	u32 clk_sel;
+
+	clk_sel = FIELD_GET(AD4170_CLOCK_CTRL_CLOCKSEL_MSK, st->clock_ctrl);
+	return clk_sel == AD4170_INTERNAL_OSC_OUTPUT;

That is a bool not an int.

Quoted 6 lines
drivers/iio/adc/ad4170.c new file +5 -0
+}
 
+static int ad4170_parse_pw_switch(struct ad4170_state *st, struct device *dev)
+{
+	bool pdsw0, pdsw1;
+

drivers/iio/adc/ad4170.c line 1632

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	pdsw0 = device_property_read_bool(dev, "adi,gpio0-power-down-switch");
+	pdsw1 = device_property_read_bool(dev, "adi,gpio1-power-down-switch");
+
+	/*
+	 * Error if a GPIO is assigned to be both excitation current output and
+	 * power switch.
+	 */
+	if (pdsw0) {
+		if (st->gpio_fn[0] != AD4170_GPIO_UNASIGNED)
+			return dev_err_probe(&st->spi->dev, -EINVAL,

Can't you use the dev that was passed in?

Quoted 21 lines
drivers/iio/adc/ad4170.c new file +21 -0
+					     "GPIO 0 already used with fn %u\n",
+					     st->gpio_fn[0]);
+
+		st->gpio_fn[0] = AD4170_GPIO_PW_DOW_SWITCH;
+	}
+	if (pdsw1) {
+		if (st->gpio_fn[1] != AD4170_GPIO_UNASIGNED)
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "GPIO 1 already used with fn %u\n",
+					     st->gpio_fn[1]);
+
+		st->gpio_fn[1] = AD4170_GPIO_PW_DOW_SWITCH;
+	}
+	return regmap_update_bits(st->regmap, AD4170_POWER_DOWN_SW_REG,
+				 AD4170_POWER_DOWN_SW_PDSW0_MSK |
+				 AD4170_POWER_DOWN_SW_PDSW1_MSK,
+				 pdsw0 | pdsw1);
+}
+
+static int ad4170_parse_vbias(struct ad4170_state *st, struct device *dev)
+{

drivers/iio/adc/ad4170.c line 1663

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	u32 vbias_pins[AD4170_MAX_ANALOG_PINS];
+	unsigned int i, val;
+	u32 num_vbias_pins;
+	int ret;
+
+	ret = device_property_count_u32(dev, "adi,vbias-pins");
+	if (ret > 0) {
+		if (ret > AD4170_MAX_ANALOG_PINS)
+			return dev_err_probe(dev, -EINVAL,
+					     "Too many vbias pins %u\n", ret);

Look at what dev_err_probe() does... You never want to print ret explicitly as it does the job better than this.

drivers/iio/adc/ad4170.c line 1675

Quoted 12 lines
drivers/iio/adc/ad4170.c new file +12 -0
+
+		num_vbias_pins = ret;
+		ret = device_property_read_u32_array(dev, "adi,vbias-pins",
+						     vbias_pins,
+						     num_vbias_pins);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to read vbias pins\n");
+	}
+	for (i = 0; i < num_vbias_pins; i++) {
+		if (st->pins_fn[vbias_pins[i]] != AD4170_PIN_UNASIGNED)
+			return dev_err_probe(&st->spi->dev, -EINVAL,

dev?

Quoted 29 lines
drivers/iio/adc/ad4170.c new file +29 -0
+					     "AIN%d already used with fn %u\n",
+					     vbias_pins[i], st->pins_fn[i]);
+
+		val |= BIT(vbias_pins[i]);
+		st->pins_fn[vbias_pins[i]] = AD4170_PIN_VBIAS;
+	}
+	return regmap_write(st->regmap, AD4170_V_BIAS_REG, val);
+}
+
+static int ad4170_parse_firmware(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	int ret, i;
+	u8 aux;
+
+	ret = ad4170_clock_select(indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to setup device clock\n");
+
+	ret = regmap_write(st->regmap, AD4170_CLOCK_CTRL_REG, st->clock_ctrl);
+	if (ret < 0)
+		return ret;
+
+	ret = ad4170_parse_pin_muxing(indio_dev);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++)

drivers/iio/adc/ad4170.c line 1714

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+		st->pins_fn[i] = AD4170_PIN_UNASIGNED;
+
+	for (i = 0; i < AD4170_NUM_GPIO_PINS; i++)
+		st->gpio_fn[i] = AD4170_GPIO_UNASIGNED;
+
+	ret = ad4170_parse_exc_current(indio_dev);
+	if (ret)
+		return ret;
+// write to misc reg must be delaied because misc register contains settings
+// that are channel specific.

looks like left over debug comment. Make sure to address these before posting.

Quoted 33 lines
drivers/iio/adc/ad4170.c new file +33 -0
+
+	ret = ad4170_parse_pw_switch(st, dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to config power down switches\n");
+
+	ret = ad4170_parse_vbias(st, dev);
+	if (ret < 0)
+		return ret;
+
+	ret = ad4170_parse_channels(indio_dev);
+	if (ret)
+		return ret;
+
+	aux = AD4170_MISC_CHOP_IEXC_OFF;
+	ret = device_property_read_u8(dev, "adi,chop-iexc", &aux);
+	if (!ret) {
+		ret = ad4170_find_table_index(ad4170_iexc_chop_tbl, aux);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Invalid adi,chop-iexc config: %u\n",
+					      aux);
+	}
+
+	/* Set excitation current chop config to first channel setup config */
+	st->setups[indio_dev->channels[0].address].misc |=
+		FIELD_PREP(AD4170_MISC_CHOP_IEXC_MSK, aux);
+	return 0;
+}
+
+static int ad4170_initial_config(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);

drivers/iio/adc/ad4170.c line 1757

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	struct ad4170_chan_info *chan_info;
+	struct iio_chan_spec const *chan;
+	struct ad4170_setup *setup;
+	unsigned int val;
+	unsigned int i;
+	int ret;
+
+	ad4170_fill_sps_tbl(st);
+
+	/* Put ADC in IDLE mode */

Code is pretty self explanatory. So drop the comment as something that might bitrot.

drivers/iio/adc/ad4170.c line 1762

Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5 -0
+	ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_IDLE);
+	if (ret)
+		return ret;
+
+	/* Setup channels. */

Similar with this one.

drivers/iio/adc/ad4170.c

Quoted 1 line
drivers/iio/adc/ad4170.c new file +1 -0
+	for (i = 0; i < indio_dev->num_channels; i++) {

Can reduce the scope of some of the local variables by pulling them in here.

drivers/iio/adc/ad4170.c line 1766

Quoted 3 lines
drivers/iio/adc/ad4170.c new file +3 -0
+		chan = &indio_dev->channels[i];
+		chan_info = &st->chans_info[chan->address];
+		setup = &st->setups[chan_info->setup_num];

To match with the other drivers that support hardware where channels have to share some config, you will need to do the setup stuff alongside a config entry allocator that checks for matches. It's fiddly but there are examples in tree. Some of the other ADI folk should be able to point you at a good example to follow

Quoted 37 lines
drivers/iio/adc/ad4170.c new file +37 -0
+
+		ret = regmap_update_bits(st->regmap,
+					 AD4170_CHAN_SETUP_REG(chan->address),
+					 AD4170_CHANNEL_SETUP_SETUP_MSK,
+					 FIELD_PREP(AD4170_CHANNEL_SETUP_SETUP_MSK,
+						    chan_info->setup_num));
+		if (ret)
+			return ret;
+
+		setup->gain = AD4170_GAIN_REG_DEFAULT;
+		ret = ad4170_write_setup(st, chan_info->setup_num, setup);
+		if (ret)
+			return ret;
+
+		val = FIELD_PREP(AD4170_CHANNEL_MAPN_AINP_MSK, chan->channel) |
+		      FIELD_PREP(AD4170_CHANNEL_MAPN_AINM_MSK, chan->channel2);
+
+		ret = regmap_write(st->regmap, AD4170_CHAN_MAP_REG(i), val);
+		if (ret < 0)
+			return ret;
+
+		ret = ad4170_set_channel_freq(st, chan->address,
+					      AD4170_MAX_SAMP_RATE, 0);
+		if (ret)
+			return ret;
+
+		ret = ad4170_fill_scale_tbl(indio_dev, i);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_write(st->regmap, AD4170_CHANNEL_EN_REG, 0);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Configure channels to share the same data output register, i.e. data

drivers/iio/adc/ad4170.c line 1813

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	 * can be read from the same register address regardless of channel
+	 * number.
+	 */
+	ret = regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
+				 AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK,
+				 AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK);
+	if (ret)
+		return ret;
+
+	return 0;

return regmap_update_bits...

Quoted 24 lines
drivers/iio/adc/ad4170.c new file +23 -0
+}
 
+static void ad4170_prepare_message(struct ad4170_state *st)
+{
+	/*
+	 * Continuous data register read is enabled on buffer postenable so
+	 * no instruction phase is needed meaning we don't need to send the
+	 * register address to read data. Transfer only needs the read buffer.
+	 */
+	st->xfer.rx_buf = &st->reg_read_rx_buf;
+	st->xfer.bits_per_word = ad4170_channel_template.scan_type.storagebits;
+	st->xfer.len = BITS_TO_BYTES(ad4170_channel_template.scan_type.storagebits);
+
+	spi_message_init_with_transfers(&st->msg, &st->xfer, 1);
+}
+
+static int ad4170_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_CONT);
+	if (ret < 0)
+		return ret;

drivers/iio/adc/ad4170.c line 1864

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+
+	ret = regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
+				 AD4170_REG_CTRL_CONT_READ_MSK,
+				 FIELD_PREP(AD4170_REG_CTRL_CONT_READ_MSK,
+					    AD4170_ADC_CTRL_CONT_READ_ENABLE));
+
+	if (ret < 0)
+		return ret;
+
+	return spi_optimize_message(st->spi, &st->msg);

This is a little odd. You have an optimized message, but also use regmap and never seem to use st->msg.

Half done conversion?

drivers/iio/adc/ad4170.c line 1875

Quoted 11 lines
drivers/iio/adc/ad4170.c new file +11 -0
+}
+
+static int ad4170_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret, i;
+
+	spi_unoptimize_message(&st->msg);
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		ret = ad4170_set_channel_enable(st, i, false);

I guess harmless to disable channels that were never enabled. Maybe add a comment on that.

Quoted 19 lines
drivers/iio/adc/ad4170.c new file +19 -0
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(st->regmap, AD4170_ADC_CTRL_REG,
+				 AD4170_REG_CTRL_CONT_READ_MSK,
+				 FIELD_PREP(AD4170_REG_CTRL_CONT_READ_MSK,
+					    AD4170_ADC_CTRL_CONT_READ_DISABLE));
+	if (ret < 0)
+		return ret;
+
+	return ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_IDLE);
+}
+
+static const struct iio_buffer_setup_ops ad4170_buffer_ops = {
+	.postenable = ad4170_buffer_postenable,
+	.predisable = ad4170_buffer_predisable,
+};
+

drivers/iio/adc/ad4170.c line 1904

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+static irqreturn_t ad4170_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret, i = 0;
+	int scan_index;
+
+	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {

Try building on current kernel... then fix it up ;) There is a new iterator for this.

drivers/iio/adc/ad4170.c line 1913

Quoted 9 lines
drivers/iio/adc/ad4170.c new file +9 -0
+		/* Read register data */
+		ret = regmap_read(st->regmap, AD4170_DATA_24b_REG, &st->data[i]);
+		if (ret)
+			goto out;
+		i++;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
+					   iio_get_time_ns(indio_dev));

Used the pollfunc that gets a timestamp but grabbed a new one here which makes no sense. Figure out when it makes more sense to get that timestamp and only use that.

drivers/iio/adc/ad4170.c line 1924

Quoted 11 lines
drivers/iio/adc/ad4170.c new file +11 -0
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int ad4170_triggered_buffer_setup(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret;
+
+	indio_dev->modes |= INDIO_BUFFER_TRIGGERED;

That's set appropriately by devm_iio_trigger_buffer_setup() so no need I think to set it here.

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+
+	st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-trig%d",
+					  indio_dev->name,
+					  iio_device_id(indio_dev));
+	if (!st->trig)
+		return -ENOMEM;
+
+	st->trig->ops = &ad4170_trigger_ops;
+	st->trig->dev.parent = indio_dev->dev.parent;
+

drivers/iio/adc/ad4170.c line 1944

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	iio_trigger_set_drvdata(st->trig, indio_dev);
+	ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(st->trig);
+
+	ret = request_irq(st->spi->irq,
+			  &ad4170_interrupt,
+			  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

Interrupt direction is a thing for firmware to control, not the driver so don't set it here.

Quoted 34 lines
drivers/iio/adc/ad4170.c new file +34 -0
+			  indio_dev->name, indio_dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_triggered_buffer_setup(indio_dev->dev.parent, indio_dev,
+					       &iio_pollfunc_store_time,
+					       &ad4170_trigger_handler,
+					       &ad4170_buffer_ops);
+}
+
+static int ad4170_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad4170_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	devm_mutex_init(dev, &st->lock);
+
+	indio_dev->name = AD4170_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad4170_info;
+
+	st->spi = spi;
+	st->regmap = devm_regmap_init(dev, NULL, st, &ad4170_regmap_config);
+
+	st->supplies[AD4170_AVDD_SUP].supply = "avdd";
+	st->supplies[AD4170_AVSS_SUP].supply = "avss";
+	st->supplies[AD4170_IOVDD_SUP].supply = "iovdd";

drivers/iio/adc/ad4170.c line 1988

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	st->supplies[AD4170_REFIN1P_SUP].supply = "refin1p";
+	st->supplies[AD4170_REFIN1N_SUP].supply = "refin1n";
+	st->supplies[AD4170_REFIN2P_SUP].supply = "refin2p";
+	st->supplies[AD4170_REFIN2N_SUP].supply = "refin2n";
+
+	/*
+	 * If a regulator is not available, it will be set to a dummy regulator.
+	 * Each channel reference is checked with regulator_get_voltage() before
+	 * setting attributes so if any channel uses a dummy supply the driver
+	 * probe will fail.

If you are using an optional regulator to provide an attribute and it's not there then we should not be registering the attribute (to indicate to userspace it is not supported). You are doing complex handling of channel setup in here anyway so shouldn't be too hard to filter for this

Quoted 13 lines
drivers/iio/adc/ad4170.c new file +13 -0
+	 */
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->supplies),
+				      st->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get supplies\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(st->supplies), st->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable supplies\n");
+
+	ret = devm_add_action_or_reset(dev, ad4170_disable_supplies, st);
+	if (ret)
+		return dev_err_probe(dev, ret,

drivers/iio/adc/ad4170.c line 2011

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+				     "Failed to add supplies disable action\n");
+
+	ret = ad4170_soft_reset(st);
+	if (ret)
+		return ret;
+
+	ret = ad4170_parse_firmware(indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to parse firmware\n");

Check wrapping throughout. This easily fits on line line under 80 chars.

Quoted 18 lines
drivers/iio/adc/ad4170.c new file +18 -0
+
+	ret = ad4170_initial_config(indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to setup device\n");
+
+	init_completion(&st->completion);
+
+	ret = ad4170_triggered_buffer_setup(indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup read buffer\n");
+
+	ad4170_prepare_message(st);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id ad4170_of_match[] = {

drivers/iio/adc/ad4170.c line 2039

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	{
+		.compatible = "adi,ad4170",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad4170_of_match);
+
+static struct spi_driver ad4170_driver = {
+	.driver = {
+		.name = AD4170_NAME,

As below. String here preferred over the define.

drivers/iio/adc/ad4170.c line 2041

Quoted 2 lines
drivers/iio/adc/ad4170.c new file +2 -0
+		.of_match_table = ad4170_of_match,
+	},

Need the id_table of spi_match_ids as well so autoprobing works. (odd piece of history that we can't unwind easily).

drivers/iio/adc/ad4170.h

Quoted 13 lines
drivers/iio/adc/ad4170.c new file +8 -0
+	.probe = ad4170_probe,
+};
+module_spi_driver(ad4170_driver);
+
+MODULE_AUTHOR("Ana-Maria Cusco <[email protected]>");
+MODULE_AUTHOR("Marcelo Schmitt <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD4170 SPI driver");
+MODULE_LICENSE("GPL");
drivers/iio/adc/ad4170.h new file +0 -0
new file mode 100644
index 000000000000..5b24788314b1

As mentioned at the top, just put this stuff at top of the c file.

drivers/iio/adc/ad4170.h line 10

Quoted 11 lines
drivers/iio/adc/ad4170.h new file +10 -0
@@ -0,0 +1,316 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * AD4170 ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+
+#include <dt-bindings/iio/adc/adi,ad4170.h>
+
+#define AD4170_NAME				"ad4170"

Use the string inline rather than via a define like this. Easier to review and often there is no specific reason why the string should be the same in all the places it is used.

drivers/iio/adc/ad4170.h line 14

Quoted 4 lines
drivers/iio/adc/ad4170.h new file +4 -0
+
+#define AD4170_READ_MASK BIT(14)
+/* AD4170 registers */
+#define AD4170_INTERFACE_CONFIG_A_REG		0x00

Clunky name. I'd shorten it.

Quoted 9 lines
drivers/iio/adc/ad4170.h new file +9 -0
+#define AD4170_STATUS_REG			0x14
+#define AD4170_DATA_24b_REG			0x1c
+#define AD4170_PIN_MUXING_REG			0x68
+#define AD4170_CLOCK_CTRL_REG			0x6A
+#define AD4170_POWER_DOWN_SW_REG		0x6e
+#define AD4170_ADC_CTRL_REG			0x70
+#define AD4170_CHANNEL_EN_REG			0x78
+#define AD4170_CHAN_SETUP_REG(x)		(0x80 + 4 * (x))
+#define AD4170_CHAN_MAP_REG(x)			(0x82 + 4 * (x))

drivers/iio/adc/ad4170.h line 33

Quoted 10 lines
drivers/iio/adc/ad4170.h new file +10 -0
+#define AD4170_MISC_REG(x)			(0xc0 + 14 * (x))
+#define AD4170_AFE_REG(x)			(0xc2 + 14 * (x))
+#define AD4170_FILTER_REG(x)			(0xc4 + 14 * (x))
+#define AD4170_FILTER_FS_REG(x)			(0xc6 + 14 * (x))
+#define AD4170_OFFSET_REG(x)			(0xc8 + 14 * (x))
+#define AD4170_GAIN_REG(x)			(0xcb + 14 * (x))
+#define AD4170_V_BIAS_REG			0x134
+#define AD4170_CURRENT_SRC_REG(x)		(0x138 + 2 * (x))
+
+/* AD4170_REG_INTERFACE_CONFIG_A */

Name the fields to make it obvious what they are in. Just the config_A bit should do or INTCONFA or some other shortening.

drivers/iio/adc/ad4170.h line 35

Quoted 2 lines
drivers/iio/adc/ad4170.h new file +2 -0
+#define AD4170_SW_RESET_MSK		(BIT(7) | BIT(0))
+#define AD4170_IFACE_CONFIG_ADDR_ASCENSION_MSK	BIT(5)

That made me go look at the datasheet. Weird naming. I'd call it ADDR_INC_NDEC or something like that.

drivers/iio/adc/ad4170.h line 47

Quoted 12 lines
drivers/iio/adc/ad4170.h new file +12 -0
+#define AD4170_RESET_SLEEP_US	        1000
+#define AD4170_INT_REF_2_5V		2500000
+
+/* AD4170_REG_PIN_MUXING */
+#define AD4170_PIN_MUXING_DIG_AUX2_CTRL_MSK		GENMASK(7, 6)
+#define AD4170_PIN_MUXING_DIG_AUX1_CTRL_MSK		GENMASK(5, 4)
+#define AD4170_PIN_MUXING_SYNC_CTRL_MSK			GENMASK(3, 2)
+
+/* AD4170_REG_CLOCK_CTRL */
+#define AD4170_INT_FREQ_16MHZ				16000000
+#define AD4170_EXT_FREQ_MHZ_MIN				1000000
+#define AD4170_EXT_FREQ_MHZ_MAX				17000000

Generally don't mix field definitions with values they map to. Put these magic values later, or put them inline where they are used. They are real numbers afterall. MEGA define in units.h is your friend as well for making them more readable.

drivers/iio/adc/ad4170.h line 53

Quoted 6 lines
drivers/iio/adc/ad4170.h new file +6 -0
+#define AD4170_CLOCK_CTRL_CLOCKSEL_MSK			GENMASK(1, 0)
+
+#define AD4170_INTERNAL_OSC				0x0
+#define AD4170_INTERNAL_OSC_OUTPUT			0x1
+#define AD4170_EXTERNAL_OSC				0x2
+#define AD4170_EXTERNAL_XTAL				0x3

Name these so it's obvious where they go. So after the field name.

I'll not look closely at the rest of the defines but suggestion is to rethink them to make it easier to:
1) Figure out which register a field is in.
2) Figure out what values can be set in that field.

Quoted 70 lines
drivers/iio/adc/ad4170.h new file +70 -0
+
+/* AD4170_REG_POWER_DOWN_SW */
+#define AD4170_POWER_DOWN_SW_PDSW1_MSK				BIT(1)
+#define AD4170_POWER_DOWN_SW_PDSW0_MSK				BIT(0)
+
+/* AD4170_REG_ADC_CTRL */
+#define AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK			BIT(7)
+#define AD4170_REG_CTRL_CONT_READ_MSK				GENMASK(5, 4)
+#define AD4170_REG_CTRL_MODE_MSK				GENMASK(3, 0)
+
+/* AD4170_REG_CHANNEL_EN */
+#define AD4170_CHANNEL_EN(ch)					BIT(ch)
+
+/* AD4170_REG_ADC_CHANNEL_SETUP */
+#define AD4170_CHANNEL_SETUP_SETUP_MSK				GENMASK(2, 0)
+
+/* AD4170_REG_ADC_CHANNEL_MAP */
+#define AD4170_CHANNEL_MAPN_AINP_MSK				GENMASK(12, 8)
+#define AD4170_CHANNEL_MAPN_AINM_MSK				GENMASK(4, 0)
+
+/* AD4170_REG_ADC_SETUPS_MISC */
+#define AD4170_MISC_CHOP_IEXC_MSK				GENMASK(15, 14)
+#define AD4170_MISC_CHOP_ADC_MSK				GENMASK(9, 8)
+#define AD4170_MISC_BURNOUT_MSK					GENMASK(1, 0)
+
+/* AD4170_REG_ADC_SETUPS_AFE */
+#define AD4170_AFE_REF_BUF_M_MSK				GENMASK(11, 10)
+#define AD4170_AFE_REF_BUF_P_MSK				GENMASK(9, 8)
+#define AD4170_AFE_REF_SELECT_MSK				GENMASK(6, 5)
+#define AD4170_AFE_BIPOLAR_MSK					BIT(4)
+#define AD4170_AFE_PGA_GAIN_MSK					GENMASK(3, 0)
+
+/* AD4170_REG_ADC_SETUPS_FILTER */
+#define AD4170_SETUPS_POST_FILTER_SEL_MSK			GENMASK(7, 4)
+#define AD4170_SETUPS_FILTER_TYPE_MSK				GENMASK(3, 0)
+
+/* AD4170 REG_OFFSET*/
+#define AD4170_OFFSET_MSK					GENMASK(23, 0)
+
+/* AD4170 REG_GAIN*/
+#define AD4170_GAIN_MSK						GENMASK(23, 0)
+#define AD4170_GAIN_REG_DEFAULT					0x555555
+
+/* AD4170_REG_CURRENT_SOURCE */
+#define AD4170_CURRENT_SRC_I_OUT_PIN_MSK			GENMASK(12, 8)
+#define AD4170_CURRENT_SRC_I_OUT_VAL_MSK			GENMASK(2, 0)
+
+/* AD4170_REG_FIR_CONTROL */
+#define AD4170_FIR_CONTROL_IIR_MODE_MSK				BIT(15)
+
+#define AD4170_MAX_CHANNELS				16
+#define AD4170_MAX_ANALOG_PINS				8
+#define AD4170_MAX_SETUPS				8
+#define AD4170_INVALID_SETUP_SLOT			9
+#define AD4170_NUM_CURRENT_SRC				4
+#define AD4170_MAX_SAMP_RATE				125000
+
+#define AD4170_NUM_ANALOG_PINS				9
+#define AD4170_NUM_GPIO_PINS				4
+
+#define AD4170_DIG_AUX1_DISABLED	0
+#define AD4170_DIG_AUX1_RDY		1
+#define AD4170_DIG_AUX1_SYNC		2
+
+#define AD4170_DIG_AUX2_DISABLED	0
+#define AD4170_DIG_AUX2_LDAC		1
+#define AD4170_DIG_AUX2_SYNC		2
+
+#define AD4170_SYNC_DISABLED		0
+#define AD4170_SYNC_STANDARD		1

drivers/iio/adc/ad4170.h line 133

Quoted 10 lines
drivers/iio/adc/ad4170.h new file +10 -0
+#define AD4170_SYNC_ALTERNATE		2
+
+#define AD4170_I_OUT_0UA		0
+#define AD4170_I_OUT_10UA		10
+#define AD4170_I_OUT_50UA		50
+#define AD4170_I_OUT_100UA		100
+#define AD4170_I_OUT_250UA		250
+#define AD4170_I_OUT_500UA		500
+#define AD4170_I_OUT_1000UA		1000
+#define AD4170_I_OUT_1500UA		1500

Defines for not so magic numbers where the value is the name aren't normally all that useful.

Quoted 16 lines
drivers/iio/adc/ad4170.h new file +16 -0
+
+#define AD4170_BURNOUT_OFF		0
+#define AD4170_BURNOUT_100NA		100
+#define AD4170_BURNOUT_2000NA		2000
+#define AD4170_BURNOUT_10000NA		10000
+
+#define AD4170_PGA_OPTIONS		10
+
+enum ad4170_pin_function {
+	AD4170_PIN_UNASIGNED,
+	AD4170_PIN_ANALOG_IN,
+	AD4170_PIN_CURRENT_OUT,
+	AD4170_PIN_VBIAS
+};
+
+enum ad4170_gpio_function {

drivers/iio/adc/ad4170.h line 159

Quoted 10 lines
drivers/iio/adc/ad4170.h new file +10 -0
+	AD4170_GPIO_UNASIGNED,
+	AD4170_GPIO_PW_DOW_SWITCH,
+	AD4170_GPIO_AC_EXCITATION,
+	AD4170_GPIO_OUTPUT,
+	AD4170_GPIO_CHANNEL,
+};
+
+#define AD4170_ADC_CTRL_CONT_READ_DISABLE		0
+#define AD4170_ADC_CTRL_CONT_READ_ENABLE		1
+//#define AD4170_ADC_CTRL_CONT_READ_TRANSMIT		2

?

drivers/iio/adc/ad4170.h line 170

Quoted 11 lines
drivers/iio/adc/ad4170.h new file +11 -0
+
+#define AD4170_ADC_CTRL_MODE_CONT			0
+#define AD4170_ADC_CTRL_MODE_SINGLE			4
+#define AD4170_ADC_CTRL_MODE_IDLE			7
+
+/**
+ * @enum ad4170_ref_buf
+ * @brief REFIN Buffer Enable.
+ */
+enum ad4170_ref_buf {
+	/** Pre-charge Buffer. */

Use longer defines and drop the comments as they should then be unnecessary.

Quoted 32 lines
drivers/iio/adc/ad4170.h new file +32 -0
+	AD4170_REF_BUF_PRE,
+	/** Full Buffer.*/
+	AD4170_REF_BUF_FULL,
+	/** Bypass */
+	AD4170_REF_BUF_BYPASS
+};
+
+/**
+ * @enum ad4170_filter_type
+ * @brief Filter Mode for Sinc-Based Filters.
+ */
+enum ad4170_filter_type {
+	AD4170_SINC5_AVG,
+	AD4170_SINC5,
+	AD4170_SINC3,
+};
+
+/* ADC Register Lengths */
+static const unsigned int ad4170_reg_size[] = {
+	[AD4170_INTERFACE_CONFIG_A_REG] = 1,
+	[AD4170_STATUS_REG]	= 2,
+	[AD4170_DATA_24b_REG]	= 3,
+	[AD4170_PIN_MUXING_REG]	= 2,
+	[AD4170_CLOCK_CTRL_REG]	= 2,
+	[AD4170_POWER_DOWN_SW_REG]	= 2,
+	[AD4170_ADC_CTRL_REG]	= 2,
+	[AD4170_CHANNEL_EN_REG]	= 2,
+	/*
+	 * CHANNEL_SETUP and CHANNEL_MAP register are all 2 byte size each and
+	 * their addresses are interleaved such that we have CHANNEL_SETUP0
+	 * address followed by CHANNEL_MAP0 address, followed by CHANNEL_SETUP1,
+	 * and so on until CHANNEL_MAP15.

drivers/iio/adc/ad4170.h line 212

Quoted 10 lines
drivers/iio/adc/ad4170.h new file +10 -0
+	 * Thus, initialize the register size for them only once.
+	 */
+	[AD4170_CHAN_SETUP_REG(0) ... AD4170_CHAN_MAP_REG(AD4170_MAX_CHANNELS - 1)] = 2,
+	/*
+	 * MISC, AFE, FILTER, FILTER_FS, OFFSET, and GAIN register addresses are
+	 * also interleaved but MISC, AFE, FILTER, FILTER_FS, OFFSET are 16-bit
+	 * while OFFSET, GAIN are 24-bit registers so we can't init them all to
+	 * the same size.
+	 */
+	/* Init MISC register size */

Maybe use a macro for each config set?
Would avoid confusion of having them out of register order here.

Quoted 50 lines
drivers/iio/adc/ad4170.h new file +50 -0
+	[AD4170_MISC_REG(0)] = 2,
+	[AD4170_MISC_REG(1)] = 2,
+	[AD4170_MISC_REG(2)] = 2,
+	[AD4170_MISC_REG(3)] = 2,
+	[AD4170_MISC_REG(4)] = 2,
+	[AD4170_MISC_REG(5)] = 2,
+	[AD4170_MISC_REG(6)] = 2,
+	[AD4170_MISC_REG(7)] = 2,
+	/* Init AFE register size */
+	[AD4170_AFE_REG(0)] = 2,
+	[AD4170_AFE_REG(1)] = 2,
+	[AD4170_AFE_REG(2)] = 2,
+	[AD4170_AFE_REG(3)] = 2,
+	[AD4170_AFE_REG(4)] = 2,
+	[AD4170_AFE_REG(5)] = 2,
+	[AD4170_AFE_REG(6)] = 2,
+	[AD4170_AFE_REG(7)] = 2,
+	/* Init FILTER register size */
+	[AD4170_FILTER_REG(0)]	= 2,
+	[AD4170_FILTER_REG(1)]	= 2,
+	[AD4170_FILTER_REG(2)]	= 2,
+	[AD4170_FILTER_REG(3)]	= 2,
+	[AD4170_FILTER_REG(4)]	= 2,
+	[AD4170_FILTER_REG(5)]	= 2,
+	[AD4170_FILTER_REG(6)]	= 2,
+	[AD4170_FILTER_REG(7)]	= 2,
+	/* Init FILTER_FS register size */
+	[AD4170_FILTER_FS_REG(0)]	= 2,
+	[AD4170_FILTER_FS_REG(1)]	= 2,
+	[AD4170_FILTER_FS_REG(2)]	= 2,
+	[AD4170_FILTER_FS_REG(3)]	= 2,
+	[AD4170_FILTER_FS_REG(4)]	= 2,
+	[AD4170_FILTER_FS_REG(5)]	= 2,
+	[AD4170_FILTER_FS_REG(6)]	= 2,
+	[AD4170_FILTER_FS_REG(7)]	= 2,
+	/* Init OFFSET register size */
+	[AD4170_OFFSET_REG(0)]	= 3,
+	[AD4170_OFFSET_REG(1)]	= 3,
+	[AD4170_OFFSET_REG(2)]	= 3,
+	[AD4170_OFFSET_REG(3)]	= 3,
+	[AD4170_OFFSET_REG(4)]	= 3,
+	[AD4170_OFFSET_REG(5)]	= 3,
+	[AD4170_OFFSET_REG(6)]	= 3,
+	[AD4170_OFFSET_REG(7)]	= 3,
+	/* Init GAIN register size */
+	[AD4170_GAIN_REG(0)]	= 3,
+	[AD4170_GAIN_REG(1)]	= 3,
+	[AD4170_GAIN_REG(2)]	= 3,
+	[AD4170_GAIN_REG(3)]	= 3,
+	[AD4170_GAIN_REG(4)]	= 3,

drivers/iio/adc/ad4170.h line 312

Quoted 10 lines
drivers/iio/adc/ad4170.h new file +9 -0
+	[AD4170_GAIN_REG(5)]	= 3,
+	[AD4170_GAIN_REG(6)]	= 3,
+	[AD4170_GAIN_REG(7)]	= 3,
+	[AD4170_V_BIAS_REG]	= 2,
+	[AD4170_CURRENT_SRC_REG(0) ... AD4170_CURRENT_SRC_REG(AD4170_NUM_CURRENT_SRC - 1)] = 2,
+};
 
+
+static const unsigned int ad4170_burnout_current_na_tbl[] = {
+	AD4170_BURNOUT_OFF,

Maybe just call it 0NA which is off :)

Quoted 4 lines
drivers/iio/adc/ad4170.h new file +4 -0
+	AD4170_BURNOUT_100NA,
+	AD4170_BURNOUT_2000NA,
+	AD4170_BURNOUT_10000NA,
+};
reviewer
#12 lore raw

Re: [RFC PATCH 3/4] iio: adc: Add support for AD4170

In reply to Marcelo Schmitt
Quoted 17 lines · Marcelo Schmitt

From: Ana-Maria Cusco <[email protected]>

From: Ana-Maria Cusco <[email protected]>

Add support for the AD4170 ADC with the following features:
- Single-shot read (read_raw), scale, sampling freq
- Multi channel buffer support
- Buffered capture in triggered mode
- Gain and offset calibration
- chop_iexc and chop_adc device configuration
- Powerdown switch configuration

Quoted 4 lines
---
 drivers/iio/adc/Kconfig  |   16 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4170.c | 2049 ++++++++++++++++++++++++++++++++++++++

I know it is a pain to split this up into multiple patches, but this is really too huge to try to review all at once! 500 lines changed or less per patch is much more manageable.

Just commenting on a few things that jupmed out at me for now...

Quoted 19 lines
 drivers/iio/adc/ad4170.h |  316 ++++++
 4 files changed, 2382 insertions(+)
 create mode 100644 drivers/iio/adc/ad4170.c
 create mode 100644 drivers/iio/adc/ad4170.h
drivers/iio/adc/ad4170.c new file +12 -0
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>

drivers/iio/adc/ad4170.c line 28

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+#include <linux/util_macros.h>
+
+#include <asm/div64.h>

Usually don't use asm but rather linux/math64.h.

Quoted 21 lines
drivers/iio/adc/ad4170.c new file +19 -0
+#include <linux/unaligned.h>
+
 
 
+struct ad4170_state {
+	struct regmap *regmap;
+	struct spi_device *spi;
+	struct regulator_bulk_data supplies[7];
+	struct mutex lock; /* Protect filter, PGA, GPIO, chan read, chan config */
+	struct iio_chan_spec chans[AD4170_MAX_CHANNELS];
+	struct ad4170_chan_info chans_info[AD4170_MAX_CHANNELS];
+	struct ad4170_setup setups[AD4170_MAX_SETUPS];
+	u32 mclk_hz;
+	enum ad4170_pin_function pins_fn[AD4170_NUM_ANALOG_PINS];
+	enum ad4170_gpio_function gpio_fn[AD4170_NUM_GPIO_PINS];
+	unsigned int clock_ctrl;
+	struct clk *ext_clk;
+	struct clk_hw int_clk_hw;
+	int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][ARRAY_SIZE(ad4170_filt_fs_tbl)][2];
+	struct completion completion;
+	struct iio_trigger *trig;

drivers/iio/adc/ad4170.c line 166

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	u32 data[AD4170_MAX_CHANNELS];
+
+	struct spi_transfer xfer;
+	struct spi_message msg;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the transfer buffers
+	 * to live in their own cache lines.
+	 */
+	u8 reg_write_tx_buf[6];
+	__be32 reg_read_rx_buf __aligned(IIO_DMA_MINALIGN);

Looks like __align() needs to be moved up one line.

Quoted 19 lines
drivers/iio/adc/ad4170.c new file +17 -0
+	__be16 reg_read_tx_buf;
+};
+
 
 
+static int ad4170_get_input_range(struct ad4170_state *st,
+				  struct iio_chan_spec const *chan,
+				  unsigned int ref_sel)
+{
+	struct ad4170_chan_info *chan_info = &st->chans_info[chan->address];
+	struct ad4170_setup *setup = &st->setups[chan_info->setup_num];
+	bool bipolar = FIELD_GET(AD4170_AFE_BIPOLAR_MSK, setup->afe);
+	int refp, refn, ain_voltage, ret;
+
+	switch (ref_sel) {
+	case AD4170_AFE_REFIN_REFIN1:
+		refp = regulator_get_voltage(st->supplies[AD4170_REFIN1P_SUP].consumer);
+		refn = regulator_get_voltage(st->supplies[AD4170_REFIN1N_SUP].consumer);
+		break;

drivers/iio/adc/ad4170.c line 745

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+	case AD4170_AFE_REFIN_REFIN2:
+		refp = regulator_get_voltage(st->supplies[AD4170_REFIN2P_SUP].consumer);
+		refn = regulator_get_voltage(st->supplies[AD4170_REFIN2N_SUP].consumer);
+		break;
+	case AD4170_AFE_REFIN_AVDD:
+		refp = regulator_get_voltage(st->supplies[AD4170_AVDD_SUP].consumer);
+		ret = regulator_get_voltage(st->supplies[AD4170_AVSS_SUP].consumer);
+		/*
+		 * TODO AVSS is actually optional.
+		 * Should we handle -EPROBE_DEFER here?

Since this is calling regulator_get_voltage(), we won't get EPROBE_DEFER here.

Quoted 13 lines
drivers/iio/adc/ad4170.c new file +11 -0
+		 */
 
 
+
+static int ad4170_clock_select(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	int ret;
+
+	st->mclk_hz = AD4170_INT_FREQ_16MHZ;
+	ret = device_property_match_property_string(dev, "clock-names",
+						    ad4170_clk_sel,

drivers/iio/adc/ad4170.c line 1587

Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10 -0
+						    ARRAY_SIZE(ad4170_clk_sel));
+	if (ret < 0) {
+		/* Use internal clock reference */
+		st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
+					     AD4170_INTERNAL_OSC_OUTPUT);
+		return ad4170_register_clk_provider(indio_dev);
+	}
+
+	/* Use external clock reference */
+	st->ext_clk = devm_clk_get(dev, ad4170_clk_sel[ret]);

devm_clk_get_enabled() ?

drivers/iio/adc/ad4170.c line 1590

Quoted 3 lines
drivers/iio/adc/ad4170.c new file +3 -0
+	if (IS_ERR(st->ext_clk)) {
+		if (PTR_ERR(st->ext_clk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;

dev_err_probe() already does this check.

Quoted 100 lines
(quoted diff) +98 -0
+
+		return dev_err_probe(dev, PTR_ERR(st->ext_clk),
+				     "Failed to get external clock\n");
+	}
+	st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
+				     AD4170_EXTERNAL_OSC + ret);
+
+	ret = clk_prepare_enable(st->ext_clk);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable external clock\n");
+
+	ret = devm_add_action_or_reset(dev, ad4170_clk_disable_unprepare,
+				       st->ext_clk);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add clock unwind action\n");
+
+	st->mclk_hz = clk_get_rate(st->ext_clk);
+	if (st->mclk_hz < AD4170_EXT_FREQ_MHZ_MIN ||
+	    st->mclk_hz > AD4170_EXT_FREQ_MHZ_MAX) {
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid external clock frequency %u\n",
+				     st->mclk_hz);
+	}
+	return 0;
+}
 
 
+
+/* ADC Register Lengths */
+static const unsigned int ad4170_reg_size[] = {
+	[AD4170_INTERFACE_CONFIG_A_REG] = 1,
+	[AD4170_STATUS_REG]	= 2,
+	[AD4170_DATA_24b_REG]	= 3,
+	[AD4170_PIN_MUXING_REG]	= 2,
+	[AD4170_CLOCK_CTRL_REG]	= 2,
+	[AD4170_POWER_DOWN_SW_REG]	= 2,
+	[AD4170_ADC_CTRL_REG]	= 2,
+	[AD4170_CHANNEL_EN_REG]	= 2,
+	/*
+	 * CHANNEL_SETUP and CHANNEL_MAP register are all 2 byte size each and
+	 * their addresses are interleaved such that we have CHANNEL_SETUP0
+	 * address followed by CHANNEL_MAP0 address, followed by CHANNEL_SETUP1,
+	 * and so on until CHANNEL_MAP15.
+	 * Thus, initialize the register size for them only once.
+	 */
+	[AD4170_CHAN_SETUP_REG(0) ... AD4170_CHAN_MAP_REG(AD4170_MAX_CHANNELS - 1)] = 2,
+	/*
+	 * MISC, AFE, FILTER, FILTER_FS, OFFSET, and GAIN register addresses are
+	 * also interleaved but MISC, AFE, FILTER, FILTER_FS, OFFSET are 16-bit
+	 * while OFFSET, GAIN are 24-bit registers so we can't init them all to
+	 * the same size.
+	 */
+	/* Init MISC register size */
+	[AD4170_MISC_REG(0)] = 2,
+	[AD4170_MISC_REG(1)] = 2,
+	[AD4170_MISC_REG(2)] = 2,
+	[AD4170_MISC_REG(3)] = 2,
+	[AD4170_MISC_REG(4)] = 2,
+	[AD4170_MISC_REG(5)] = 2,
+	[AD4170_MISC_REG(6)] = 2,
+	[AD4170_MISC_REG(7)] = 2,
+	/* Init AFE register size */
+	[AD4170_AFE_REG(0)] = 2,
+	[AD4170_AFE_REG(1)] = 2,
+	[AD4170_AFE_REG(2)] = 2,
+	[AD4170_AFE_REG(3)] = 2,
+	[AD4170_AFE_REG(4)] = 2,
+	[AD4170_AFE_REG(5)] = 2,
+	[AD4170_AFE_REG(6)] = 2,
+	[AD4170_AFE_REG(7)] = 2,
+	/* Init FILTER register size */
+	[AD4170_FILTER_REG(0)]	= 2,
+	[AD4170_FILTER_REG(1)]	= 2,
+	[AD4170_FILTER_REG(2)]	= 2,
+	[AD4170_FILTER_REG(3)]	= 2,
+	[AD4170_FILTER_REG(4)]	= 2,
+	[AD4170_FILTER_REG(5)]	= 2,
+	[AD4170_FILTER_REG(6)]	= 2,
+	[AD4170_FILTER_REG(7)]	= 2,
+	/* Init FILTER_FS register size */
+	[AD4170_FILTER_FS_REG(0)]	= 2,
+	[AD4170_FILTER_FS_REG(1)]	= 2,
+	[AD4170_FILTER_FS_REG(2)]	= 2,
+	[AD4170_FILTER_FS_REG(3)]	= 2,
+	[AD4170_FILTER_FS_REG(4)]	= 2,
+	[AD4170_FILTER_FS_REG(5)]	= 2,
+	[AD4170_FILTER_FS_REG(6)]	= 2,
+	[AD4170_FILTER_FS_REG(7)]	= 2,
+	/* Init OFFSET register size */
+	[AD4170_OFFSET_REG(0)]	= 3,
+	[AD4170_OFFSET_REG(1)]	= 3,
+	[AD4170_OFFSET_REG(2)]	= 3,
+	[AD4170_OFFSET_REG(3)]	= 3,
+	[AD4170_OFFSET_REG(4)]	= 3,
+	[AD4170_OFFSET_REG(5)]	= 3,
+	[AD4170_OFFSET_REG(6)]	= 3,
+	[AD4170_OFFSET_REG(7)]	= 3,
+	/* Init GAIN register size */

Review

Quoted 10 lines
(quoted diff) +10 -0
+	[AD4170_GAIN_REG(0)]	= 3,
+	[AD4170_GAIN_REG(1)]	= 3,
+	[AD4170_GAIN_REG(2)]	= 3,
+	[AD4170_GAIN_REG(3)]	= 3,
+	[AD4170_GAIN_REG(4)]	= 3,
+	[AD4170_GAIN_REG(5)]	= 3,
+	[AD4170_GAIN_REG(6)]	= 3,
+	[AD4170_GAIN_REG(7)]	= 3,
+	[AD4170_V_BIAS_REG]	= 2,
+	[AD4170_CURRENT_SRC_REG(0) ... AD4170_CURRENT_SRC_REG(AD4170_NUM_CURRENT_SRC - 1)] = 2,

Another way to do this is to make 3 regmaps, one for each word size.

It's a bit more work to set up in the first place, but then makes using it easier. (see ad4695 for an example)

Quoted 1 line

+};

#13 lore raw

RFC PATCH 4/4 Documentation: iio: Add ADC documentation

In reply to Marcelo Schmitt

ADCs can have different input configurations such that developers can get confused when trying to model some of them into IIO channels.

For example, some differential ADCs can have their channels configured as pseudo-differential channels. In that configuration, only one input connects to the signal of interest as opposed to using two inputs of a differential input configuration. Datasheets sometimes also refer to pseudo-differential inputs as single-ended inputs even though they have distinct physical configuration and measurement procedure. There has been some previous discussion in the mailing list about pseudo-differential and single-ended channels [1].

Documenting the many possible ADC channel configurations should provide two benefits:
A) Consolidate the knowledge from [2] and from [1], and hopefully reduce the reviewing time of forthcoming ADC drivers.
B) Help Linux developers figure out quicker how to better support differential ADCs, specially those that can have channels configured as pseudo-differential inputs.

Add documentation about common ADC characteristics and IIO support for them.

[1]: https://lore.kernel.org/linux-iio/[email protected]/
[2]: https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.

---
 Documentation/iio/iio_adc.rst | 280 ++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst   |   1 +
 2 files changed, 281 insertions(+)
 create mode 100644 Documentation/iio/iio_adc.rst
Documentation/iio/iio_adc.rst new file +280 -0
new file mode 100644
index 000000000000..43b8cad547c9
@@ -0,0 +1,280 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+IIO Abstractions for ADCs
+=========================
+
+1. Overview
+===========
+
+The IIO subsystem supports many Analog to Digital Converters (ADCs). Some ADCs
+have features and characteristics that are supported in specific ways by IIO
+device drivers. This documentation describes common ADC features and explains
+how they are (should be?) supported by the IIO subsystem.
+
+1. ADC Channel Types
+====================
+
+ADCs can have distinct types of inputs, each of them measuring analog voltages
+in a slightly different way. An ADC digitizes the analog input voltage over a
+span given by the provided voltage reference, the input type, and the input
+polarity. The input range allowed to an ADC channel is needed to determine the
+scale factor and offset needed to obtain the measured value in real-world
+units (millivolts for voltage measurement, milliamps for current measurement,
+etc.).
+
+There are three types of ADC inputs (single-ended, differential,
+pseudo-differential) and two possible polarities (unipolar, bipolar). The input
+type (single-ended, differential, pseudo-differential) is one channel
+characteristic, and is completely independent of the polarity (unipolar,
+bipolar) aspect. A comprehensive article about ADC input types (on which this
+doc is heavily based on) can be found at
+https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.
+
+1.1 Single-ended channels
+-------------------------
+
+Single-ended channels digitize the analog input voltage relative to ground and
+can be either unipolar or bipolar.
+
+1.1.1 Single-ended Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  ---------- VREF -------------
+      ´ `           ´ `                  _____________
+    /     \       /     \               /             |
+   /       \     /       \         --- <  IN    ADC   |
+            \   /         \   /         \             |
+             `-´           `-´           \       VREF |
+  -------- GND (0V) -----------           +-----------+
+                                                  ^
+                                                  |
+                                             External VREF
+
+The input voltage to a **single-ended unipolar** channel is allowed to swing
+from GND to VREF (where VREF is a voltage reference with electrical potential
+higher than system ground). The maximum input voltage is also called VFS
+(full-scale input voltage), with VFS being determined by VREF. The voltage
+reference may be provided from an external supply or derived from the chip power
+source.
+
+A single-ended unipolar channel could be described in device tree like the
+following example::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+        };
+    };
+
+See ``Documentation/devicetree/bindings/iio/adc/adc.yaml`` for the complete
+documentation of ADC specific device tree properties.
+
+
+1.1.2 Single-ended Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  ---------- +VREF ------------
+      ´ `           ´ `                  _____________________
+    /     \       /     \               /                     |
+   /       \     /       \         --- <  IN          ADC     |
+            \   /         \   /         \                     |
+             `-´           `-´           \       +VREF  -VREF |
+  ---------- -VREF ------------           +-------------------+
+                                                  ^       ^
+                                                  |       |
+                             External +VREF ------+  External -VREF
+
+For a **single-ended bipolar** channel, the analog voltage input can go from
+-VREF to +VREF (where -VREF is the voltage reference that has the lower
+electrical potential while +VREF is the reference with the higher one). Some ADC
+chips derive the lower reference from +VREF, others get it from a separate
+input.  Often, +VREF and -VREF are symmetric but they don't need to be so. When
+-VREF is lower than system ground, these inputs are also called single-ended
+true bipolar.
+
+Here's an example device tree description of a single-ended bipolar channel.
+::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            bipolar;
+        };
+    };
+
+1.2 Differential channels
+-------------------------
+
+A differential voltage measurement digitizes the voltage level at the positive
+input (IN+) relative to the negative input (IN-) over the -VREF to +VREF span.
+In other words, a differential channel measures how many volts IN+ is away from
+IN- (IN+ - IN-).
+
+1.2.1 Differential Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------
+    ´ `       ´ `               +-------------------+
+  /     \   /     \   /        /                    |
+         `-´       `-´    --- <  IN+                |
+  -------- -VREF ------        |                    |
+                               |            ADC     |
+  -------- +VREF ------        |                    |
+        ´ `       ´ `     --- <  IN-                |
+  \   /     \   /     \        \       +VREF  -VREF |
+   `-´       `-´                +-------------------+
+  -------- -VREF ------                  ^       ^
+                                         |       +---- External -VREF
+                                  External +VREF
+
+The analog signals to **differential bipolar** inputs are also allowed to swing
+from -VREF to +VREF. If -VREF is below system GND, these are also called
+differential true bipolar inputs.
+
+Device tree example of a differential bipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            bipolar;
+            diff-channels = <0 1>;
+        };
+    };
+
+In the ADC driver, `differential = 1` is set into `struct iio_chan_spec` for the
+channel. See ``include/linux/iio/iio.h`` for more information.
+
+1.2.2 Differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+For **differential unipolar** channels, the analog voltage at the positive input
+must also be higher than the voltage at the negative input. Thus, the actual
+input range allowed to a differential unipolar channel is IN- to +VREF. Because
+IN+ is allowed to swing with the measured analog signal and the input setup must
+guarantee IN+ will not go below IN- (nor IN- will raise above IN+), most
+differential unipolar channel setups have IN- fixed to a known voltage that does
+not fall within the voltage range expected for the measured signal. This leads
+to a setup that is equivalent to a pseudo-differential channel. Thus,
+differential unipolar channels are actually pseudo-differential unipolar
+channels.
+
+1.3 Pseudo-differential Channels
+--------------------------------
+
+There is a third ADC input type which is called pseudo-differential or
+single-ended to differential configuration. A pseudo-differential channel is
+similar to a differential channel in that it also measures IN+ relative to IN-.
+However, unlike differential channels, the negative input is limited to a narrow
+voltage range while only IN+ is allowed to swing. A pseudo-differential channel
+can be made out from a differential pair of inputs by restricting the negative
+input to a known voltage while allowing only the positive input to swing. Aside
+from that, some parts have a COM pin that allows single-ended inputs to be
+referenced to a common-mode voltage, making them pseudo-differential channels.
+
+1.3.1 Pseudo-differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------          +-------------------+
+    ´ `       ´ `               /                    |
+  /     \   /     \   /    --- <  IN+                |
+         `-´       `-´          |                    |
+  --------- IN- -------         |            ADC     |
+                                |                    |
+  Common-mode voltage -->  --- <  IN-                |
+                                \       +VREF  -VREF |
+                                 +-------------------+
+                                          ^       ^
+                                          |       +---- External -VREF
+                                   External +VREF
+
+A **pseudo-differential unipolar** input has the limitations a differential
+unipolar channel would have, meaning the analog voltage to the positive input
+IN+ must stay within IN- to +VREF. The fixed voltage to IN- is sometimes called
+common-mode voltage and it must be within -VREF to +VREF as would be expected
+from the signal to any differential channel negative input.
+
+In pseudo-differential configuration, the voltage measured from IN+ is not
+relative to GND (as it would be for a single-ended channel) but to IN-, which
+causes the measurement to always be offset by IN- volts. To allow applications
+to calculate IN+ voltage with respect to system ground, the IIO channel may
+provide an `_offset` attribute to report the channel offset to user space.
+
+Device tree example for pseudo-differential unipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            single-channel = <0>;
+            common-mode-channel = <1>;
+        };
+    };
+
+Do not set `differential` in the channel `iio_chan_spec` struct of
+pseudo-differential channels.
+
+1.3.2 Pseudo-differential Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------          +-------------------+
+    ´ `       ´ `               /                    |
+  /     \   /     \   /    --- <  IN+                |
+         `-´       `-´          |                    |
+  -------- -VREF ------         |            ADC     |
+                                |                    |
+  Common-mode voltage -->  --- <  IN-                |
+                                \       +VREF  -VREF |
+                                 +-------------------+
+                                          ^       ^
+                                          |       +---- External -VREF
+                                   External +VREF
+
+A **pseudo-differential bipolar** input is not limited by the level at IN- but
+it will be limited to -VREF or to GND on the lower end of the input range
+depending on the particular ADC. Similar to their unipolar counter parts,
+pseudo-differential bipolar channels may define an `_offset` attribute to
+provide the read offset relative to GND.
+
+Device tree example for pseudo-differential bipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            bipolar;
+            single-channel = <0>;
+            common-mode-channel = <1>;
+        };
+    };
+
+Again, the `differential` field of `struct iio_chan_spec` is not set for
+pseudo-differential channels.
Documentation/iio/index.rst +1 -0
index 074dbbf7ba0a..15f62d304eaa 100644
@@ -7,6 +7,7 @@ Industrial I/O
 .. toctree::
    :maxdepth: 1
 
+   iio_adc
    iio_configfs
    iio_devbuf
    iio_dmabuf_api
Signature · 1 line
2.45.2
reviewer
#14 lore raw

Re: [RFC PATCH 4/4] Documentation: iio: Add ADC documentation

Quoted 20 lines · Marcelo Schmitt

ADCs can have different input configurations such that developers can get confused when trying to model some of them into IIO channels.

For example, some differential ADCs can have their channels configured as pseudo-differential channels. In that configuration, only one input connects to the signal of interest as opposed to using two inputs of a differential input configuration. Datasheets sometimes also refer to pseudo-differential inputs as single-ended inputs even though they have distinct physical configuration and measurement procedure. There has been some previous discussion in the mailing list about pseudo-differential and single-ended channels [1].

Documenting the many possible ADC channel configurations should provide two benefits:
A) Consolidate the knowledge from [2] and from [1], and hopefully reduce the reviewing time of forthcoming ADC drivers.
B) Help Linux developers figure out quicker how to better support differential ADCs, specially those that can have channels configured as pseudo-differential inputs.

Quoted 7 lines

Add documentation about common ADC characteristics and IIO support for them.

[1]: https://lore.kernel.org/linux-iio/[email protected]/
[2]: https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.

---

This is really nice to have!

Quoted 27 lines
 Documentation/iio/iio_adc.rst | 280 ++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst   |   1 +
 2 files changed, 281 insertions(+)
 create mode 100644 Documentation/iio/iio_adc.rst
Documentation/iio/iio_adc.rst new file +16 -0
new file mode 100644
index 000000000000..43b8cad547c9
@@ -0,0 +1,280 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+IIO Abstractions for ADCs
+=========================
+
+1. Overview
+===========
+
+The IIO subsystem supports many Analog to Digital Converters (ADCs). Some ADCs
+have features and characteristics that are supported in specific ways by IIO
+device drivers. This documentation describes common ADC features and explains
+how they are (should be?) supported by the IIO subsystem.
+
+1. ADC Channel Types
+====================

Documentation/iio/iio_adc.rst line 26

Quoted 10 lines
Documentation/iio/iio_adc.rst new file +10 -0
+
+ADCs can have distinct types of inputs, each of them measuring analog voltages
+in a slightly different way. An ADC digitizes the analog input voltage over a
+span given by the provided voltage reference, the input type, and the input
+polarity. The input range allowed to an ADC channel is needed to determine the
+scale factor and offset needed to obtain the measured value in real-world
+units (millivolts for voltage measurement, milliamps for current measurement,
+etc.).
+
+There are three types of ADC inputs (single-ended, differential,
                  ^
                  | general
Quoted 69 lines
Documentation/iio/iio_adc.rst new file +69 -0
+pseudo-differential) and two possible polarities (unipolar, bipolar). The input
+type (single-ended, differential, pseudo-differential) is one channel
+characteristic, and is completely independent of the polarity (unipolar,
+bipolar) aspect. A comprehensive article about ADC input types (on which this
+doc is heavily based on) can be found at
+https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.
+
+1.1 Single-ended channels
+-------------------------
+
+Single-ended channels digitize the analog input voltage relative to ground and
+can be either unipolar or bipolar.
+
+1.1.1 Single-ended Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  ---------- VREF -------------
+      ´ `           ´ `                  _____________
+    /     \       /     \               /             |
+   /       \     /       \         --- <  IN    ADC   |
+            \   /         \   /         \             |
+             `-´           `-´           \       VREF |
+  -------- GND (0V) -----------           +-----------+
+                                                  ^
+                                                  |
+                                             External VREF
+
+The input voltage to a **single-ended unipolar** channel is allowed to swing
+from GND to VREF (where VREF is a voltage reference with electrical potential
+higher than system ground). The maximum input voltage is also called VFS
+(full-scale input voltage), with VFS being determined by VREF. The voltage
+reference may be provided from an external supply or derived from the chip power
+source.
+
+A single-ended unipolar channel could be described in device tree like the
+following example::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+        };
+    };
+
+See ``Documentation/devicetree/bindings/iio/adc/adc.yaml`` for the complete
+documentation of ADC specific device tree properties.
+
+
+1.1.2 Single-ended Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  ---------- +VREF ------------
+      ´ `           ´ `                  _____________________
+    /     \       /     \               /                     |
+   /       \     /       \         --- <  IN          ADC     |
+            \   /         \   /         \                     |
+             `-´           `-´           \       +VREF  -VREF |
+  ---------- -VREF ------------           +-------------------+
+                                                  ^       ^
+                                                  |       |
+                             External +VREF ------+  External -VREF
+

Documentation/iio/iio_adc.rst line 105

Quoted 10 lines
Documentation/iio/iio_adc.rst new file +10 -0
+For a **single-ended bipolar** channel, the analog voltage input can go from
+-VREF to +VREF (where -VREF is the voltage reference that has the lower
+electrical potential while +VREF is the reference with the higher one). Some ADC
+chips derive the lower reference from +VREF, others get it from a separate
+input.  Often, +VREF and -VREF are symmetric but they don't need to be so. When
+-VREF is lower than system ground, these inputs are also called single-ended
+true bipolar.
+
+Here's an example device tree description of a single-ended bipolar channel.
+::

To be consistent with other sections, put :: at the end of the text.

Here's an example device tree description of a single-ended bipolar channel::

Quoted 9 lines
Documentation/iio/iio_adc.rst new file +9 -0
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            bipolar;

Documentation/iio/iio_adc.rst line 124

Quoted 10 lines
Documentation/iio/iio_adc.rst new file +10 -0
+        };
+    };
+
+1.2 Differential channels
+-------------------------
+
+A differential voltage measurement digitizes the voltage level at the positive
+input (IN+) relative to the negative input (IN-) over the -VREF to +VREF span.
+In other words, a differential channel measures how many volts IN+ is away from
+IN- (IN+ - IN-).

Suggest using the word "difference" or the "the potential difference between" instead of saying "is away from".

Quoted 33 lines
Documentation/iio/iio_adc.rst new file +33 -0
+
+1.2.1 Differential Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------
+    ´ `       ´ `               +-------------------+
+  /     \   /     \   /        /                    |
+         `-´       `-´    --- <  IN+                |
+  -------- -VREF ------        |                    |
+                               |            ADC     |
+  -------- +VREF ------        |                    |
+        ´ `       ´ `     --- <  IN-                |
+  \   /     \   /     \        \       +VREF  -VREF |
+   `-´       `-´                +-------------------+
+  -------- -VREF ------                  ^       ^
+                                         |       +---- External -VREF
+                                  External +VREF
+
+The analog signals to **differential bipolar** inputs are also allowed to swing
+from -VREF to +VREF. If -VREF is below system GND, these are also called
+differential true bipolar inputs.
+
+Device tree example of a differential bipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;

Documentation/iio/iio_adc.rst line 167

Quoted 10 lines
Documentation/iio/iio_adc.rst new file +10 -0
+            bipolar;
+            diff-channels = <0 1>;
+        };
+    };
+
+In the ADC driver, `differential = 1` is set into `struct iio_chan_spec` for the
+channel. See ``include/linux/iio/iio.h`` for more information.
+
+1.2.2 Differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To be consistent with the other sections, move unipolar before bipolar.

Documentation/iio/iio_adc.rst line 178

Quoted 11 lines
Documentation/iio/iio_adc.rst new file +11 -0
+
+For **differential unipolar** channels, the analog voltage at the positive input
+must also be higher than the voltage at the negative input. Thus, the actual
+input range allowed to a differential unipolar channel is IN- to +VREF. Because
+IN+ is allowed to swing with the measured analog signal and the input setup must
+guarantee IN+ will not go below IN- (nor IN- will raise above IN+), most
+differential unipolar channel setups have IN- fixed to a known voltage that does
+not fall within the voltage range expected for the measured signal. This leads
+to a setup that is equivalent to a pseudo-differential channel. Thus,
+differential unipolar channels are actually pseudo-differential unipolar
+channels.

The diagrams are really helpful, so please add a diagram in this section as well.

Documentation/iio/iio_adc.rst line 191

Quoted 13 lines
Documentation/iio/iio_adc.rst new file +13 -0
+
+1.3 Pseudo-differential Channels
+--------------------------------
+
+There is a third ADC input type which is called pseudo-differential or
+single-ended to differential configuration. A pseudo-differential channel is
+similar to a differential channel in that it also measures IN+ relative to IN-.
+However, unlike differential channels, the negative input is limited to a narrow
+voltage range while only IN+ is allowed to swing. A pseudo-differential channel
+can be made out from a differential pair of inputs by restricting the negative
+input to a known voltage while allowing only the positive input to swing. Aside
+from that, some parts have a COM pin that allows single-ended inputs to be
+referenced to a common-mode voltage, making them pseudo-differential channels.

I think it would be helpful to mention here that the common mode input voltage is usually described in the devicetree as a voltage regulator since it is basically a constant voltage source, e.g. ``com-supply`` to correspond to the example COM pin.

Documentation/iio/iio_adc.rst line 202

Quoted 11 lines
Documentation/iio/iio_adc.rst new file +11 -0
+
+1.3.1 Pseudo-differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------          +-------------------+
+    ´ `       ´ `               /                    |
+  /     \   /     \   /    --- <  IN+                |
+         `-´       `-´          |                    |
+  --------- IN- -------         |            ADC     |

The bottom rail should be GND, not IN-. Typically, the common mode voltage is VREF / 2. In other words it is halfway between the two rails.

Documentation/iio/iio_adc.rst line 208

Quoted 6 lines
Documentation/iio/iio_adc.rst new file +6 -0
+                                |                    |
+  Common-mode voltage -->  --- <  IN-                |
+                                \       +VREF  -VREF |
+                                 +-------------------+
+                                          ^       ^
+                                          |       +---- External -VREF

This is unipolar, so would not expect -VREF here.

Documentation/iio/iio_adc.rst line 221

Quoted 13 lines
Documentation/iio/iio_adc.rst new file +13 -0
+                                   External +VREF
+
+A **pseudo-differential unipolar** input has the limitations a differential
+unipolar channel would have, meaning the analog voltage to the positive input
+IN+ must stay within IN- to +VREF. The fixed voltage to IN- is sometimes called
+common-mode voltage and it must be within -VREF to +VREF as would be expected
+from the signal to any differential channel negative input.
+
+In pseudo-differential configuration, the voltage measured from IN+ is not
+relative to GND (as it would be for a single-ended channel) but to IN-, which
+causes the measurement to always be offset by IN- volts. To allow applications
+to calculate IN+ voltage with respect to system ground, the IIO channel may
+provide an `_offset` attribute to report the channel offset to user space.

In some chips though, the common mode voltage may be GND. (Example is AD7944 that calls this "ground sense"). So in that case, there is no common mode supply or ``_offset`` attribute.

Quoted 6 lines
Documentation/iio/iio_adc.rst new file +6 -0
+
+Device tree example for pseudo-differential unipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;

Documentation/iio/iio_adc.rst line 237

Quoted 10 lines
Documentation/iio/iio_adc.rst new file +10 -0
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            single-channel = <0>;
+            common-mode-channel = <1>;
+        };
+    };
+
+Do not set `differential` in the channel `iio_chan_spec` struct of

Needs `` for .rst formatting.

Quoted 48 lines
Documentation/iio/iio_adc.rst new file +43 -0
+pseudo-differential channels.
+
+1.3.2 Pseudo-differential Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------          +-------------------+
+    ´ `       ´ `               /                    |
+  /     \   /     \   /    --- <  IN+                |
+         `-´       `-´          |                    |
+  -------- -VREF ------         |            ADC     |
+                                |                    |
+  Common-mode voltage -->  --- <  IN-                |
+                                \       +VREF  -VREF |
+                                 +-------------------+
+                                          ^       ^
+                                          |       +---- External -VREF
+                                   External +VREF
+
+A **pseudo-differential bipolar** input is not limited by the level at IN- but
+it will be limited to -VREF or to GND on the lower end of the input range
+depending on the particular ADC. Similar to their unipolar counter parts,
+pseudo-differential bipolar channels may define an `_offset` attribute to
+provide the read offset relative to GND.
+
+Device tree example for pseudo-differential bipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            bipolar;
+            single-channel = <0>;
+            common-mode-channel = <1>;
+        };
+    };
+
+Again, the `differential` field of `struct iio_chan_spec` is not set for
+pseudo-differential channels.
Documentation/iio/index.rst +0 -0
index 074dbbf7ba0a..15f62d304eaa 100644

Documentation/iio/index.rst Industrial I/O · line 10

Quoted 4 lines
Documentation/iio/index.rst +1 -0
@@ -7,6 +7,7 @@ Industrial I/O
 .. toctree::
    :maxdepth: 1
 
+   iio_adc

Maybe make this iio_adc_inputs in case we make a general adc page in the future.

Quoted 3 lines
Documentation/iio/index.rst +0 -0
    iio_configfs
    iio_devbuf
    iio_dmabuf_api
maintainer
#15 lore raw

Re: [RFC PATCH 4/4] Documentation: iio: Add ADC documentation

Quoted 23 lines · On Wed, 18 Dec 2024 14:46:14 -0600 David Lechner <[email protected]> wrote:
Quoted 22 lines · Marcelo Schmitt

ADCs can have different input configurations such that developers can get confused when trying to model some of them into IIO channels.

For example, some differential ADCs can have their channels configured as pseudo-differential channels. In that configuration, only one input connects to the signal of interest as opposed to using two inputs of a differential input configuration. Datasheets sometimes also refer to pseudo-differential inputs as single-ended inputs even though they have distinct physical configuration and measurement procedure. There has been some previous discussion in the mailing list about pseudo-differential and single-ended channels [1].

Documenting the many possible ADC channel configurations should provide two benefits:
A) Consolidate the knowledge from [2] and from [1], and hopefully reduce the reviewing time of forthcoming ADC drivers.
B) Help Linux developers figure out quicker how to better support differential ADCs, specially those that can have channels configured as pseudo-differential inputs.

Add documentation about common ADC characteristics and IIO support for them.

Quoted 7 lines
Quoted 5 lines

[1]: https://lore.kernel.org/linux-iio/[email protected]/
[2]: https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.

---

This is really nice to have!

Agreed. I end up looking this up from time to time, so a local set of definitions makes complete sense to have.

Thanks for doing this!
A few really minor additions to David's comments inline.

Thanks

Jonathan

Quoted 28 lines
Quoted 28 lines
 Documentation/iio/iio_adc.rst | 280 ++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst   |   1 +
 2 files changed, 281 insertions(+)
 create mode 100644 Documentation/iio/iio_adc.rst
Documentation/iio/iio_adc.rst new file +16 -0
new file mode 100644
index 000000000000..43b8cad547c9
@@ -0,0 +1,280 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+IIO Abstractions for ADCs
+=========================
+
+1. Overview
+===========
+
+The IIO subsystem supports many Analog to Digital Converters (ADCs). Some ADCs
+have features and characteristics that are supported in specific ways by IIO
+device drivers. This documentation describes common ADC features and explains
+how they are (should be?) supported by the IIO subsystem.
+
+1. ADC Channel Types
+====================
Quoted 8 lines

Documentation/iio/iio_adc.rst line 24

Quoted 8 lines
Documentation/iio/iio_adc.rst new file +8 -0
+
+ADCs can have distinct types of inputs, each of them measuring analog voltages
+in a slightly different way. An ADC digitizes the analog input voltage over a
+span given by the provided voltage reference, the input type, and the input
+polarity. The input range allowed to an ADC channel is needed to determine the
+scale factor and offset needed to obtain the measured value in real-world
+units (millivolts for voltage measurement, milliamps for current measurement,
+etc.).

Add some 'weasel' words in here. There are more complex non linear ADCs and ones only capable of reaching some fraction of the reference voltage. Maybe throw in a "generally" somewhere.

Quoted 12 lines

Documentation/iio/iio_adc.rst line 26

Quoted 2 lines
Documentation/iio/iio_adc.rst new file +2 -0
+
+There are three types of ADC inputs (single-ended, differential,  
                  ^
                  | general
Quoted 7 lines
Documentation/iio/iio_adc.rst new file +7 -0
+pseudo-differential) and two possible polarities (unipolar, bipolar). The input
+type (single-ended, differential, pseudo-differential) is one channel
+characteristic, and is completely independent of the polarity (unipolar,
+bipolar) aspect. A comprehensive article about ADC input types (on which this
+doc is heavily based on) can be found at
+https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.
+

...

Quoted 37 lines
Quoted 37 lines
Documentation/iio/iio_adc.rst new file +37 -0
+
+1.2.1 Differential Bipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
+  -------- +VREF ------
+    ´ `       ´ `               +-------------------+
+  /     \   /     \   /        /                    |
+         `-´       `-´    --- <  IN+                |
+  -------- -VREF ------        |                    |
+                               |            ADC     |
+  -------- +VREF ------        |                    |
+        ´ `       ´ `     --- <  IN-                |
+  \   /     \   /     \        \       +VREF  -VREF |
+   `-´       `-´                +-------------------+
+  -------- -VREF ------                  ^       ^
+                                         |       +---- External -VREF
+                                  External +VREF
+
+The analog signals to **differential bipolar** inputs are also allowed to swing
+from -VREF to +VREF. If -VREF is below system GND, these are also called
+differential true bipolar inputs.
+
+Device tree example of a differential bipolar channel::
+
+    adc@0 {
+        ...
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+            bipolar;
+            diff-channels = <0 1>;
+        };
+    };
Quoted 8 lines

Documentation/iio/iio_adc.rst line 167

Quoted 6 lines
Documentation/iio/iio_adc.rst new file +6 -0
+
+In the ADC driver, `differential = 1` is set into `struct iio_chan_spec` for the
+channel. See ``include/linux/iio/iio.h`` for more information.
+
+1.2.2 Differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  

To be consistent with the other sections, move unipolar before bipolar.

I'm not sure I agree on this. It's the 'weird one', so some logic in introducing it after bipolar differential.

Quoted 11 lines

Documentation/iio/index.rst Industrial I/O · line 10

Quoted 9 lines
Documentation/iio/index.rst +1 -0
index 074dbbf7ba0a..15f62d304eaa 100644
@@ -7,6 +7,7 @@ Industrial I/O
 .. toctree::
    :maxdepth: 1
 
+   iio_adc  

Maybe make this iio_adc_inputs in case we make a general adc page in the future.

Can rename if we do.

Quoted 6 lines
Quoted 4 lines
Documentation/iio/index.rst +0 -0
    iio_configfs
    iio_devbuf
    iio_dmabuf_api  
#16 lore raw

Re: [RFC PATCH 4/4] Documentation: iio: Add ADC documentation

Quoted 7 lines · Jonathan Cameron
Quoted 4 lines · On Wed, 18 Dec 2024 14:46:14 -0600 David Lechner <[email protected]> wrote:
Quoted 3 lines · Marcelo Schmitt

ADCs can have different input configurations such that developers can get confused when trying to model some of them into IIO channels.

...

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

Add documentation about common ADC characteristics and IIO support for them.

[1]: https://lore.kernel.org/linux-iio/[email protected]/
[2]: https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html.

---

...

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

+1. ADC Channel Types
+====================

Documentation/iio/iio_adc.rst new file +4 -0
+
+ADCs can have distinct types of inputs, each of them measuring analog voltages
+in a slightly different way. An ADC digitizes the analog input voltage over a
+span given by the provided voltage reference, the input type, and the input
Quoted 8 lines
Quoted 4 lines

Documentation/iio/iio_adc.rst line 24

Quoted 4 lines
Documentation/iio/iio_adc.rst new file +4 -0
+polarity. The input range allowed to an ADC channel is needed to determine the
+scale factor and offset needed to obtain the measured value in real-world
+units (millivolts for voltage measurement, milliamps for current measurement,
+etc.).

Add some 'weasel' words in here. There are more complex non linear ADCs and ones only capable of reaching some fraction of the reference voltage. Maybe throw in a "generally" somewhere.

V2 will come with a short paragraph with a disclaimer for complex ADCs :)

Thanks,
Marcelo

#17 lore raw

Re: [RFC PATCH 4/4] Documentation: iio: Add ADC documentation

In reply to David Lechner

Hi David, thank you for your suggestions.
I think I've applied most of them and will soon send a v2 only with the docs. Replying here mostly on the comments I didn't comply with.

Quoted 4 lines · David Lechner
Quoted 3 lines · Marcelo Schmitt

ADCs can have different input configurations such that developers can get confused when trying to model some of them into IIO channels.

...

Quoted 3 lines
Quoted 3 lines

Add documentation about common ADC characteristics and IIO support for them.

...

Quoted 7 lines

Documentation/iio/iio_adc.rst line 167

Quoted 5 lines
Documentation/iio/iio_adc.rst new file +5 -0
+In the ADC driver, `differential = 1` is set into `struct iio_chan_spec` for the
+channel. See ``include/linux/iio/iio.h`` for more information.
+
+1.2.2 Differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To be consistent with the other sections, move unipolar before bipolar.

I had differential unipolar before differential bipolar on a preliminary version (not sent to the mailing list), but it lead to a much more intricate explanation of differential unipolar. That's why I prefer to keep the Differential Unipolar section after Differential Bipolar.

Quoted 14 lines

Documentation/iio/iio_adc.rst line 178

Quoted 12 lines
Documentation/iio/iio_adc.rst new file +11 -0
+
+For **differential unipolar** channels, the analog voltage at the positive input
+must also be higher than the voltage at the negative input. Thus, the actual
+input range allowed to a differential unipolar channel is IN- to +VREF. Because
+IN+ is allowed to swing with the measured analog signal and the input setup must
+guarantee IN+ will not go below IN- (nor IN- will raise above IN+), most
+differential unipolar channel setups have IN- fixed to a known voltage that does
+not fall within the voltage range expected for the measured signal. This leads
+to a setup that is equivalent to a pseudo-differential channel. Thus,
+differential unipolar channels are actually pseudo-differential unipolar
+channels.

The diagrams are really helpful, so please add a diagram in this section as well.

There is no diagram for Differential Unipolar. What would be the Differential Unipolar diagram is the diagram for Pseudo-Differential Unipolar. Having Differential Unipolar section here also makes it closer to the Pseudo-Differential Unipolar diagram.

Quoted 1 line

...

Quoted 6 lines
Quoted 6 lines
Documentation/iio/iio_adc.rst new file +6 -0
+
+1.3.1 Pseudo-differential Unipolar Channels
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+::
+
Quoted 8 lines

Documentation/iio/iio_adc.rst line 202

Quoted 5 lines
Documentation/iio/iio_adc.rst new file +5 -0
+  -------- +VREF ------          +-------------------+
+    ´ `       ´ `               /                    |
+  /     \   /     \   /    --- <  IN+                |
+         `-´       `-´          |                    |
+  --------- IN- -------         |            ADC     |

The bottom rail should be GND, not IN-. Typically, the common mode voltage is VREF / 2. In other words it is halfway between the two rails.

IN- may be above GND (e.g. at VREF / 2 as a typical common mode voltage). In that case, the minimum voltage for IN+ (i.e. the bottom rail) would be VREF / 2. The generic constraint would be that IN+ does not fall below IN-.
See bipolar/unipolar configuration section of AD4170 datasheet page 46.
https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf
On that example, VREF is actually a differential voltage reference that is 2.5V nominal voltage (halfway between 5V AVDD and 0V AVSS).
If IN+ is allowed to go below IN-, then this becomes Pseudo-differential Bipolar.

Quoted 9 lines

Documentation/iio/iio_adc.rst line 208

Quoted 7 lines
Documentation/iio/iio_adc.rst new file +6 -0
+                                |                    |
+  Common-mode voltage -->  --- <  IN-                |
+                                \       +VREF  -VREF |
+                                 +-------------------+
+                                          ^       ^
+                                          |       +---- External -VREF

This is unipolar, so would not expect -VREF here.

I think IN- could in theory be negative (bellow GND) if the ADC inputs are true bipolar inputs. Though, I have never seen such thing so can't say for sure. Anyway, -VREF is not doing anything on this setup so I omitted it in v2.

Quoted 10 lines
Quoted 10 lines
Documentation/iio/iio_adc.rst new file +9 -0
+                                   External +VREF
+
+A **pseudo-differential unipolar** input has the limitations a differential
+unipolar channel would have, meaning the analog voltage to the positive input
+IN+ must stay within IN- to +VREF. The fixed voltage to IN- is sometimes called
+common-mode voltage and it must be within -VREF to +VREF as would be expected
+from the signal to any differential channel negative input.
+
+In pseudo-differential configuration, the voltage measured from IN+ is not
Quoted 9 lines

Documentation/iio/iio_adc.rst line 221

Quoted 4 lines
Documentation/iio/iio_adc.rst new file +4 -0
+relative to GND (as it would be for a single-ended channel) but to IN-, which
+causes the measurement to always be offset by IN- volts. To allow applications
+to calculate IN+ voltage with respect to system ground, the IIO channel may
+provide an `_offset` attribute to report the channel offset to user space.

In some chips though, the common mode voltage may be GND. (Example is AD7944 that calls this "ground sense"). So in that case, there is no common mode supply or ``_offset`` attribute.

Added a comment about it omitting the ``_offset`` in those cases. My understanding is that, because the common mode voltage (or ground sense in AD7944's case) is at GND, the ``_offset`` is always zero and that's why the ``_offset`` attribute is not needed in that case. Whenever the common mode voltage is at something other than GND, we would need ``_offset`` to be able to get the voltage relative to GND.
Oh well, that's just another way of saying what you already told me I guess.