[PATCH v3 0/6] Add support for AD411x

[PATCH v3 0/6] Add support for AD411x

[Dumitru Ceclan via B4 Relay]

This patch series adds support for the Analog Devices AD4111, AD4112,
 AD4114, AD4115, AD4116 within the existing AD7173 driver.

  The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications. They integrate an analog front end suitable for processing
fully differential/single-ended and bipolar voltage inputs.

Particularities of the models:
- All ADCs have inputs with a precision voltage divider with a division
ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
shunt resistor.

Discussions from this patch series have concluded with:
-Datasheets mention single-ended and pseudo differential capabilities by
 the means of connecting the negative input of a differential pair (IN-)
 to a constant voltage supply and letting the positive input fluctuate.
 This is not a special operating mode, it is a capability of the
 differential channels to also measure such signals.

-Single-ended and pseudo differential do not need any specific
 configuration and cannot be differentiated from differential usage by
 the driver side =>
	offer adi,channel-type attribute to flag the usage of the channel

-VINCOM is described as a dedicated pin for single-ended channels but as
 seen in AD4116, it is a normal input connected to the cross-point
 multiplexer (VIN10, VINCOM (single-ended or differential pair)).
 This does not mean full functionality in any configuration:
 AD4111:"If any two voltage inputs are paired in a configuration other
 than what is described in this data sheet, the accuracy of the device
 cannot be guaranteed".

-ADCIN15 input pin from AD4116 is specified as the dedicated pin for
 pseudo-differential but from the datasheet it results that this pin is
 also able to measure single-ended and fully differential channels
 ("ADCIN11, ADCIN15. (pseudo differential or differential pair)";
  "An example is to connect the ADCIN15 pin externally to the AVSS
   pin in a single-ended configuration")

 As such, detecting the type of usage of a channel is not possible and
will be the responsability of the user to specify.
 If the user has connected a non 0V (in regards to AVSS) supply to
the negative input pin of a channel in a pseudo differential
configuration, the offset of the measurement from AVSS will not be known
from the driver and will need to be measured by other means.

Datasheets:
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf

This series depends on patches:
(iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify error paths.)
https://lore.kernel.org/all/20240330190849.1321065-6-jic23@kernel.org
(dt-bindings: iio: adc: Add single-channel property)
https://lore.kernel.org/linux-iio/20240514120222.56488-5-alisa.roman@analog.com/

And patch series:
(AD7173 fixes)
https://lore.kernel.org/all/20240521-ad7173-fixes-v1-0-8161cc7f3ad1@analog.com/

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
Changes in v3:

iio: adc: ad7173: fix buffers enablement for ad7176-2
iio: adc: ad7173: Add ad7173_device_info names
iio: adc: ad7173: Remove index from temp channel
- Remove patches, send in separate series

dt-bindings: adc: ad7173: add support for ad411x
- fix VINCOM typo
- switch current channel definition to use single-channel
- remove pseudo-differential from adi,channel-type, specify that
  single-ended will be used for that case as well
- remove diff-channels from required, already defined in adc.yaml
- update constraints to not permit single-channel for models without
  current inputs

iio: adc: ad7173: refactor channel configuration parsing
- remove blank line from commit message

iio: adc: ad7173: refactor ain and vref selection
- remove blank space from commit message

iio: adc: ad7173: add support for special inputs
<no changes>

iio: adc: ad7173: Add support for AD411x devices
- remove pseudo diff channel type
- change current channels dt parsing to single-channel
- change module description from wild-card to "and similar"

iio: adc: ad7173: Reduce device info struct size
<no changes>

- Link to v2: https://lore.kernel.org/r/20240514-ad4111-v2-0-29be6a55efb5@analog.com

Changes in v2:

dt-bindings: adc: ad7173: add support for ad411x
- Add constraint for missing avdd2-supply on ad411x
- Change support for current channels to selecting the actual
   diff-channels input values and activating the
   adi,current-channel property
- Add constraint for adi,current-channel
- Add adi,channel-type to be able to differentiante in the driver
   between single-ended, pseudo-differential and differential channels.
- Update diff-channels description to decribe inputs beside the AINs

iio: adc: ad7173: fix buffers enablement for ad7176-2
- Specify ".has_input_buf = false" for AD7176-2
- Drop fixes tag, specify that configuration bits are read only

iio: adc: ad7173: refactor channel configuration parsing
- Add Link and Suggested-by in commit message

iio: adc: ad7173: refactor ain and vref selection
- Improve commit message to express commit purpose
- Refactor line wrappings due to reduced indent
- Change AINs check to a loop

iio: adc: ad7173: add support for special inputs
- Create patch

iio: adc: ad7173: Add ad7173_device_info names
- Create patch

iio: adc: ad7173: Remove index from temp channel
- Justify in commit message userspace breakage
- Remove index from the correct channel template

iio: adc: ad7173: Add support for AD411x devices
- Add missing validation for VCOM and inputs with voltage divider
- Add missing validation for AD4116 low level inputs
- Add missing ad7173_device_info names
- Add support for setting differential flag depending on the channel type
- Change current channel validation to use actual pin values
- Combine multiple chipID reg values in a single define
		(AD7173_AD4111_AD4112_AD4114_ID)
- Rename num_inputs and num_inputs_with_divider to include voltage
- Add comment to specify that num_voltage_inputs_with_divider does not
   include the VCOM pin.
- Change break to direct returns where possible in switch cases
- Add fix for ad411x gpio's

iio: adc: ad7173: Reduce device info struct size
- Create patch

- Link to v1: https://lore.kernel.org/r/20240401-ad4111-v1-0-34618a9cc502@analog.com

---
Dumitru Ceclan (6):
      dt-bindings: adc: ad7173: add support for ad411x
      iio: adc: ad7173: refactor channel configuration parsing
      iio: adc: ad7173: refactor ain and vref selection
      iio: adc: ad7173: add support for special inputs
      iio: adc: ad7173: Add support for AD411x devices
      iio: adc: ad7173: Reduce device info struct size

 .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 +++++-
 drivers/iio/adc/ad7173.c                           | 437 ++++++++++++++++++---
 2 files changed, 498 insertions(+), 61 deletions(-)
---
base-commit: af5b6543889edceed5eff3b74e3cfeece6c56c5e
change-id: 20240312-ad4111-7eeb34eb4a5f

Best regards,
-- 
Dumitru Ceclan <dumitru.ceclan@analog.com>

[PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Dumitru Ceclan via B4 Relay]

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.

AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
AD4111/AD4112 support current channels, usage is implemented by
 specifying channel reg values bigger than 15.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
 1 file changed, 120 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
index ea6cfcd0aff4..5b1af382dad3 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -19,7 +19,18 @@ description: |
   primarily for measurement of signals close to DC but also delivers
   outstanding performance with input bandwidths out to ~10kHz.
 
+  Analog Devices AD411x ADC's:
+  The AD411X family encompasses a series of low power, low noise, 24-bit,
+  sigma-delta analog-to-digital converters that offer a versatile range of
+  specifications. They integrate an analog front end suitable for processing
+  fully differential/single-ended and bipolar voltage inputs.
+
   Datasheets for supported chips:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
@@ -31,6 +42,11 @@ description: |
 properties:
   compatible:
     enum:
+      - adi,ad4111
+      - adi,ad4112
+      - adi,ad4114
+      - adi,ad4115
+      - adi,ad4116
       - adi,ad7172-2
       - adi,ad7172-4
       - adi,ad7173-8
@@ -129,10 +145,36 @@ patternProperties:
         maximum: 15
 
       diff-channels:
+        description: |
+          For using current channels specify select the current inputs
+           and enable the adi,current-channel property.
+
+          Family AD411x supports a dedicated VINCOM voltage input.
+          To select it set the second channel to 16.
+            (VIN2, VINCOM) -> diff-channels = <2 16>
+
+          There are special values that can be selected besides the voltage
+          analog inputs:
+            21: REF+
+            22: REF−
+          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
+            19: ((AVDD1 − AVSS)/5)+
+            20: ((AVDD1 − AVSS)/5)−
+
         items:
           minimum: 0
           maximum: 31
 
+      single-channel:
+        description: |
+          Models AD4111 and AD4112 support single-ended current channels.
+          To select the desired current input, specify the desired input pair:
+            (IIN2+, IIN2−) -> single-channel = <2>
+
+        items:
+          minimum: 1
+          maximum: 16
+
       adi,reference-select:
         description: |
           Select the reference source to use when converting on
@@ -154,9 +196,26 @@ patternProperties:
           - avdd
         default: refout-avss
 
+      adi,current-channel:
+        description: |
+          Signal that the selected inputs are current channels.
+          Only available on AD4111 and AD4112.
+        type: boolean
+
+      adi,channel-type:
+        description:
+          Used to differentiate between different channel types as the device
+           register configurations are the same for all usage types.
+          Both pseudo-differential and single-ended channels will use the
+           single-ended specifier.
+        $ref: /schemas/types.yaml#/definitions/string
+        enum:
+          - single-ended
+          - differential
+        default: differential
+
     required:
       - reg
-      - diff-channels
 
 required:
   - compatible
@@ -166,7 +225,6 @@ allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
 
   # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
-  # Other models have [0-3] channel registers
   - if:
       properties:
         compatible:
@@ -187,6 +245,37 @@ allOf:
                 - vref
                 - refout-avss
                 - avdd
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad4114
+              - adi,ad4115
+              - adi,ad4116
+              - adi,ad7173-8
+              - adi,ad7175-8
+    then:
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            reg:
+              maximum: 15
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7172-2
+              - adi,ad7175-2
+              - adi,ad7176-2
+              - adi,ad7177-2
+    then:
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
             reg:
               maximum: 3
 
@@ -210,6 +299,35 @@ allOf:
           required:
             - adi,reference-select
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad4111
+              - adi,ad4112
+              - adi,ad4114
+              - adi,ad4115
+              - adi,ad4116
+    then:
+      properties:
+        avdd2-supply: false
+
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              enum:
+                - adi,ad4111
+                - adi,ad4112
+    then:
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            single-channel: false
+            adi,current-channel: false
+
   - if:
       anyOf:
         - required: [clock-names]

-- 
2.43.0

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Conor Dooley]

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

On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> 
> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> AD4111/AD4112 support current channels, usage is implemented by
>  specifying channel reg values bigger than 15.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
>  1 file changed, 120 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> index ea6cfcd0aff4..5b1af382dad3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -19,7 +19,18 @@ description: |
>    primarily for measurement of signals close to DC but also delivers
>    outstanding performance with input bandwidths out to ~10kHz.
>  
> +  Analog Devices AD411x ADC's:
> +  The AD411X family encompasses a series of low power, low noise, 24-bit,
> +  sigma-delta analog-to-digital converters that offer a versatile range of
> +  specifications. They integrate an analog front end suitable for processing
> +  fully differential/single-ended and bipolar voltage inputs.
> +
>    Datasheets for supported chips:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> @@ -31,6 +42,11 @@ description: |
>  properties:
>    compatible:
>      enum:
> +      - adi,ad4111
> +      - adi,ad4112
> +      - adi,ad4114
> +      - adi,ad4115
> +      - adi,ad4116
>        - adi,ad7172-2
>        - adi,ad7172-4
>        - adi,ad7173-8
> @@ -129,10 +145,36 @@ patternProperties:
>          maximum: 15
>  
>        diff-channels:
> +        description: |
> +          For using current channels specify select the current inputs
> +           and enable the adi,current-channel property.
> +
> +          Family AD411x supports a dedicated VINCOM voltage input.
> +          To select it set the second channel to 16.
> +            (VIN2, VINCOM) -> diff-channels = <2 16>
> +
> +          There are special values that can be selected besides the voltage
> +          analog inputs:
> +            21: REF+
> +            22: REF−
> +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
> +            19: ((AVDD1 − AVSS)/5)+
> +            20: ((AVDD1 − AVSS)/5)−
> +
>          items:
>            minimum: 0
>            maximum: 31
>  
> +      single-channel:
> +        description: |
> +          Models AD4111 and AD4112 support single-ended current channels.
> +          To select the desired current input, specify the desired input pair:
> +            (IIN2+, IIN2−) -> single-channel = <2>
> +
> +        items:
> +          minimum: 1
> +          maximum: 16
> +
>        adi,reference-select:
>          description: |
>            Select the reference source to use when converting on
> @@ -154,9 +196,26 @@ patternProperties:
>            - avdd
>          default: refout-avss
>  
> +      adi,current-channel:
> +        description: |
> +          Signal that the selected inputs are current channels.
> +          Only available on AD4111 and AD4112.
> +        type: boolean
> +
> +      adi,channel-type:
> +        description:
> +          Used to differentiate between different channel types as the device
> +           register configurations are the same for all usage types.
> +          Both pseudo-differential and single-ended channels will use the
> +           single-ended specifier.
> +        $ref: /schemas/types.yaml#/definitions/string
> +        enum:
> +          - single-ended
> +          - differential
> +        default: differential

I dunno if my brain just ain't workin' right today, or if this is not
sufficiently explained, but why is this property needed? You've got
diff-channels and single-channels already, why can you not infer the
information you need from them? What should software do with this
information?
Additionally, "pseudo-differential" is not explained in this binding.

Also, what does "the device register configurations are the same for
all uses types" mean? The description here implies that you'd be reading
the registers to determine the configuration, but as far as I understand
it's the job of drivers to actually configure devices.
The only way I could interpret this that makes sense to me is that you're
trying to say that the device doesn't have registers that allow you to
do runtime configuration detection - but that's the norm and I would not
call it out here.

Thanks,
Conor.

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

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Ceclan, Dumitru]

On 27/05/2024 20:48, Conor Dooley wrote:
> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
>>
>> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
>> AD4111/AD4112 support current channels, usage is implemented by
>>  specifying channel reg values bigger than 15.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
>>  1 file changed, 120 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>> index ea6cfcd0aff4..5b1af382dad3 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>> @@ -19,7 +19,18 @@ description: |
>>    primarily for measurement of signals close to DC but also delivers
>>    outstanding performance with input bandwidths out to ~10kHz.
>>  
>> +  Analog Devices AD411x ADC's:
>> +  The AD411X family encompasses a series of low power, low noise, 24-bit,
>> +  sigma-delta analog-to-digital converters that offer a versatile range of
>> +  specifications. They integrate an analog front end suitable for processing
>> +  fully differential/single-ended and bipolar voltage inputs.
>> +
>>    Datasheets for supported chips:
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>> @@ -31,6 +42,11 @@ description: |
>>  properties:
>>    compatible:
>>      enum:
>> +      - adi,ad4111
>> +      - adi,ad4112
>> +      - adi,ad4114
>> +      - adi,ad4115
>> +      - adi,ad4116
>>        - adi,ad7172-2
>>        - adi,ad7172-4
>>        - adi,ad7173-8
>> @@ -129,10 +145,36 @@ patternProperties:
>>          maximum: 15
>>  
>>        diff-channels:
>> +        description: |
>> +          For using current channels specify select the current inputs
>> +           and enable the adi,current-channel property.
>> +
>> +          Family AD411x supports a dedicated VINCOM voltage input.
>> +          To select it set the second channel to 16.
>> +            (VIN2, VINCOM) -> diff-channels = <2 16>
>> +
>> +          There are special values that can be selected besides the voltage
>> +          analog inputs:
>> +            21: REF+
>> +            22: REF−
>> +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>> +            19: ((AVDD1 − AVSS)/5)+
>> +            20: ((AVDD1 − AVSS)/5)−
>> +
>>          items:
>>            minimum: 0
>>            maximum: 31
>>  
>> +      single-channel:
>> +        description: |
>> +          Models AD4111 and AD4112 support single-ended current channels.
>> +          To select the desired current input, specify the desired input pair:
>> +            (IIN2+, IIN2−) -> single-channel = <2>
>> +
>> +        items:
>> +          minimum: 1
>> +          maximum: 16
>> +
>>        adi,reference-select:
>>          description: |
>>            Select the reference source to use when converting on
>> @@ -154,9 +196,26 @@ patternProperties:
>>            - avdd
>>          default: refout-avss
>>  
>> +      adi,current-channel:
>> +        description: |
>> +          Signal that the selected inputs are current channels.
>> +          Only available on AD4111 and AD4112.
>> +        type: boolean
>> +
>> +      adi,channel-type:
>> +        description:
>> +          Used to differentiate between different channel types as the device
>> +           register configurations are the same for all usage types.
>> +          Both pseudo-differential and single-ended channels will use the
>> +           single-ended specifier.
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        enum:
>> +          - single-ended
>> +          - differential
>> +        default: differential
> 
> I dunno if my brain just ain't workin' right today, or if this is not
> sufficiently explained, but why is this property needed? You've got
> diff-channels and single-channels already, why can you not infer the
> information you need from them? What should software do with this
> information?
> Additionally, "pseudo-differential" is not explained in this binding.
> In previous thread we arrived to the conclusion single-ended and
pseudo-differential channels should be marked with the flag
"differential=false" in the IIO channel struct. This cannot
really be inferred as any input pair could be used in that
manner and the only difference would be in external wiring. 

Single-channels cannot be used to define such a channel as
two voltage inputs need to be selected. Also, we are already
using single-channel to define the current channels.

As for explaining the pseudo-differential, should it be explained?
A voltage channel within the context of these families is actually
differential(as there are always two inputs selected).
The single-ended and pseudo-diff use case is actually wiring up a
constant voltage to the selected negative input.

I did not consider that this should be described, as there is no
need for an attribute to describe it.

> Also, what does "the device register configurations are the same for
> all uses types" mean? The description here implies that you'd be reading
> the registers to determine the configuration, but as far as I understand
> it's the job of drivers to actually configure devices.
> The only way I could interpret this that makes sense to me is that you're
> trying to say that the device doesn't have registers that allow you to
> do runtime configuration detection - but that's the norm and I would not
> call it out here.

No, I meant that the same register configuration will be set for
both fully differential and single-ended. 

The user will set diff-channels = <0, 1>, bipolar(or not) and
then they can wire whatever to those pins: 
- a differential signal
- AVSS to 1 and a single-ended signal to 0
- AVSS+offset to 1 and a single-ended signal to 0
	(which is called pseudo-differential in some datasheets)

All these cases will look the same in terms of configuration

> 
> Thanks,
> Conor.

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Conor Dooley]

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

On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> On 27/05/2024 20:48, Conor Dooley wrote:
> > On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>
> >> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> >>
> >> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> >> AD4111/AD4112 support current channels, usage is implemented by
> >>  specifying channel reg values bigger than 15.
> >>
> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >> ---
> >>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
> >>  1 file changed, 120 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> >> index ea6cfcd0aff4..5b1af382dad3 100644
> >> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> >> @@ -19,7 +19,18 @@ description: |
> >>    primarily for measurement of signals close to DC but also delivers
> >>    outstanding performance with input bandwidths out to ~10kHz.
> >>  
> >> +  Analog Devices AD411x ADC's:
> >> +  The AD411X family encompasses a series of low power, low noise, 24-bit,
> >> +  sigma-delta analog-to-digital converters that offer a versatile range of
> >> +  specifications. They integrate an analog front end suitable for processing
> >> +  fully differential/single-ended and bipolar voltage inputs.
> >> +
> >>    Datasheets for supported chips:
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> >>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> >>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> >>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> >> @@ -31,6 +42,11 @@ description: |
> >>  properties:
> >>    compatible:
> >>      enum:
> >> +      - adi,ad4111
> >> +      - adi,ad4112
> >> +      - adi,ad4114
> >> +      - adi,ad4115
> >> +      - adi,ad4116
> >>        - adi,ad7172-2
> >>        - adi,ad7172-4
> >>        - adi,ad7173-8
> >> @@ -129,10 +145,36 @@ patternProperties:
> >>          maximum: 15
> >>  
> >>        diff-channels:
> >> +        description: |
> >> +          For using current channels specify select the current inputs
> >> +           and enable the adi,current-channel property.
> >> +
> >> +          Family AD411x supports a dedicated VINCOM voltage input.
> >> +          To select it set the second channel to 16.
> >> +            (VIN2, VINCOM) -> diff-channels = <2 16>
> >> +
> >> +          There are special values that can be selected besides the voltage
> >> +          analog inputs:
> >> +            21: REF+
> >> +            22: REF−
> >> +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
> >> +            19: ((AVDD1 − AVSS)/5)+
> >> +            20: ((AVDD1 − AVSS)/5)−
> >> +
> >>          items:
> >>            minimum: 0
> >>            maximum: 31
> >>  
> >> +      single-channel:
> >> +        description: |
> >> +          Models AD4111 and AD4112 support single-ended current channels.
> >> +          To select the desired current input, specify the desired input pair:
> >> +            (IIN2+, IIN2−) -> single-channel = <2>
> >> +
> >> +        items:
> >> +          minimum: 1
> >> +          maximum: 16
> >> +
> >>        adi,reference-select:
> >>          description: |
> >>            Select the reference source to use when converting on
> >> @@ -154,9 +196,26 @@ patternProperties:
> >>            - avdd
> >>          default: refout-avss
> >>  
> >> +      adi,current-channel:
> >> +        description: |
> >> +          Signal that the selected inputs are current channels.
> >> +          Only available on AD4111 and AD4112.
> >> +        type: boolean
> >> +
> >> +      adi,channel-type:
> >> +        description:
> >> +          Used to differentiate between different channel types as the device
> >> +           register configurations are the same for all usage types.
> >> +          Both pseudo-differential and single-ended channels will use the
> >> +           single-ended specifier.
> >> +        $ref: /schemas/types.yaml#/definitions/string
> >> +        enum:
> >> +          - single-ended
> >> +          - differential
> >> +        default: differential
> > 
> > I dunno if my brain just ain't workin' right today, or if this is not
> > sufficiently explained, but why is this property needed? You've got
> > diff-channels and single-channels already, why can you not infer the
> > information you need from them? What should software do with this
> > information?
> > Additionally, "pseudo-differential" is not explained in this binding.
> 
> In previous thread we arrived to the conclusion single-ended and
> pseudo-differential channels should be marked with the flag
> "differential=false" in the IIO channel struct. This cannot
> really be inferred as any input pair could be used in that
> manner and the only difference would be in external wiring.
> 
> Single-channels cannot be used to define such a channel as
> two voltage inputs need to be selected. Also, we are already
> using single-channel to define the current channels.

If I understand correctly, the property could be simplified to a flag
then, since it's only the pseudo differential mode that you cannot be
sure of?
You know when you're single-ended based on single-channel, so the
additional info you need is only in the pseudo-differential case.

> As for explaining the pseudo-differential, should it be explained?
> A voltage channel within the context of these families is actually
> differential(as there are always two inputs selected).
> The single-ended and pseudo-diff use case is actually wiring up a
> constant voltage to the selected negative input.
> 
> I did not consider that this should be described, as there is no
> need for an attribute to describe it.

I dunno, adding an explanation of it in the text for the channel type
seems trivial to do. "Both pseudo-differential mode (where the
one of differential inputs is connected to a constant voltage) and
single-ended channels will..."

> > Also, what does "the device register configurations are the same for
> > all uses types" mean? The description here implies that you'd be reading
> > the registers to determine the configuration, but as far as I understand
> > it's the job of drivers to actually configure devices.
> > The only way I could interpret this that makes sense to me is that you're
> > trying to say that the device doesn't have registers that allow you to
> > do runtime configuration detection - but that's the norm and I would not
> > call it out here.
> 
> No, I meant that the same register configuration will be set for
> both fully differential and single-ended. 
> 
> The user will set diff-channels = <0, 1>, bipolar(or not) and
> then they can wire whatever to those pins: 
> - a differential signal
> - AVSS to 1 and a single-ended signal to 0
> - AVSS+offset to 1 and a single-ended signal to 0
> 	(which is called pseudo-differential in some datasheets)
> 
> All these cases will look the same in terms of configuration

In that case, I'd just remove this sentence from the description then.
How you configure the registers to use the device doesn't really have
anything to do with describing the configuration of the hardware.
Given it isn't related to configuration detection at runtime, what
you've got written here just makes it seem like the property is
redundant because the register settings do not change.

Instead, use the description to talk about when the property should be
used and what software should use it to determine, e.g. "Software can
use vendor,channel-type to determine whether or not the measured voltage
is absolute or relative". I pulled that outta my ass, it might not
be what you're actually doing, but I figure you just want to know if
you're measuring from the origin or either side of it.

Cheers,
Conor.

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

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Ceclan, Dumitru]

On 28/05/2024 20:52, Conor Dooley wrote:
> On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
>> On 27/05/2024 20:48, Conor Dooley wrote:
>>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>
>>>> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
>>>>
>>>> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
>>>> AD4111/AD4112 support current channels, usage is implemented by
>>>>  specifying channel reg values bigger than 15.
>>>>
>>>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>> ---
>>>>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
>>>>  1 file changed, 120 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>> index ea6cfcd0aff4..5b1af382dad3 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>> @@ -19,7 +19,18 @@ description: |
>>>>    primarily for measurement of signals close to DC but also delivers
>>>>    outstanding performance with input bandwidths out to ~10kHz.
>>>>  
>>>> +  Analog Devices AD411x ADC's:
>>>> +  The AD411X family encompasses a series of low power, low noise, 24-bit,
>>>> +  sigma-delta analog-to-digital converters that offer a versatile range of
>>>> +  specifications. They integrate an analog front end suitable for processing
>>>> +  fully differential/single-ended and bipolar voltage inputs.
>>>> +
>>>>    Datasheets for supported chips:
>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>>>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>>>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>>>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>>>> @@ -31,6 +42,11 @@ description: |
>>>>  properties:
>>>>    compatible:
>>>>      enum:
>>>> +      - adi,ad4111
>>>> +      - adi,ad4112
>>>> +      - adi,ad4114
>>>> +      - adi,ad4115
>>>> +      - adi,ad4116
>>>>        - adi,ad7172-2
>>>>        - adi,ad7172-4
>>>>        - adi,ad7173-8
>>>> @@ -129,10 +145,36 @@ patternProperties:
>>>>          maximum: 15
>>>>  
>>>>        diff-channels:
>>>> +        description: |
>>>> +          For using current channels specify select the current inputs
>>>> +           and enable the adi,current-channel property.
>>>> +
>>>> +          Family AD411x supports a dedicated VINCOM voltage input.
>>>> +          To select it set the second channel to 16.
>>>> +            (VIN2, VINCOM) -> diff-channels = <2 16>
>>>> +
>>>> +          There are special values that can be selected besides the voltage
>>>> +          analog inputs:
>>>> +            21: REF+
>>>> +            22: REF−
>>>> +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>>>> +            19: ((AVDD1 − AVSS)/5)+
>>>> +            20: ((AVDD1 − AVSS)/5)−
>>>> +
>>>>          items:
>>>>            minimum: 0
>>>>            maximum: 31
>>>>  
>>>> +      single-channel:
>>>> +        description: |
>>>> +          Models AD4111 and AD4112 support single-ended current channels.
>>>> +          To select the desired current input, specify the desired input pair:
>>>> +            (IIN2+, IIN2−) -> single-channel = <2>
>>>> +
>>>> +        items:
>>>> +          minimum: 1
>>>> +          maximum: 16
>>>> +
>>>>        adi,reference-select:
>>>>          description: |
>>>>            Select the reference source to use when converting on
>>>> @@ -154,9 +196,26 @@ patternProperties:
>>>>            - avdd
>>>>          default: refout-avss
>>>>  
>>>> +      adi,current-channel:
>>>> +        description: |
>>>> +          Signal that the selected inputs are current channels.
>>>> +          Only available on AD4111 and AD4112.
>>>> +        type: boolean
>>>> +
>>>> +      adi,channel-type:
>>>> +        description:
>>>> +          Used to differentiate between different channel types as the device
>>>> +           register configurations are the same for all usage types.
>>>> +          Both pseudo-differential and single-ended channels will use the
>>>> +           single-ended specifier.
>>>> +        $ref: /schemas/types.yaml#/definitions/string
>>>> +        enum:
>>>> +          - single-ended
>>>> +          - differential
>>>> +        default: differential
>>>
>>> I dunno if my brain just ain't workin' right today, or if this is not
>>> sufficiently explained, but why is this property needed? You've got
>>> diff-channels and single-channels already, why can you not infer the
>>> information you need from them? What should software do with this
>>> information?
>>> Additionally, "pseudo-differential" is not explained in this binding.
>>
>> In previous thread we arrived to the conclusion single-ended and
>> pseudo-differential channels should be marked with the flag
>> "differential=false" in the IIO channel struct. This cannot
>> really be inferred as any input pair could be used in that
>> manner and the only difference would be in external wiring.
>>
>> Single-channels cannot be used to define such a channel as
>> two voltage inputs need to be selected. Also, we are already
>> using single-channel to define the current channels.
> 
> If I understand correctly, the property could be simplified to a flag
> then, since it's only the pseudo differential mode that you cannot be
> sure of?
> You know when you're single-ended based on single-channel, so the
> additional info you need is only in the pseudo-differential case.
> 
Yes, it could just be a boolean flag. The only thing I have against
that is the awkwardness of having both diff-channels and
differential=false within a channel definition.

No, there is no uncertainty regarding pseudo-differential, it's
basically single-ended.

We cannot use single-channel for voltage channels, two voltage
inputs need to be specified. And again, single-channel will be
used here for the current channels. 

>> As for explaining the pseudo-differential, should it be explained?
>> A voltage channel within the context of these families is actually
>> differential(as there are always two inputs selected).
>> The single-ended and pseudo-diff use case is actually wiring up a
>> constant voltage to the selected negative input.
>>
>> I did not consider that this should be described, as there is no
>> need for an attribute to describe it.
> 
> I dunno, adding an explanation of it in the text for the channel type
> seems trivial to do. "Both pseudo-differential mode (where the
> one of differential inputs is connected to a constant voltage) and
> single-ended channels will..."
>
>>> Also, what does "the device register configurations are the same for
>>> all uses types" mean? The description here implies that you'd be reading
>>> the registers to determine the configuration, but as far as I understand
>>> it's the job of drivers to actually configure devices.
>>> The only way I could interpret this that makes sense to me is that you're
>>> trying to say that the device doesn't have registers that allow you to
>>> do runtime configuration detection - but that's the norm and I would not
>>> call it out here.
>>
>> No, I meant that the same register configuration will be set for
>> both fully differential and single-ended. 
>>
>> The user will set diff-channels = <0, 1>, bipolar(or not) and
>> then they can wire whatever to those pins: 
>> - a differential signal
>> - AVSS to 1 and a single-ended signal to 0
>> - AVSS+offset to 1 and a single-ended signal to 0
>> 	(which is called pseudo-differential in some datasheets)
>>
>> All these cases will look the same in terms of configuration
> 
> In that case, I'd just remove this sentence from the description then.
> How you configure the registers to use the device doesn't really have
> anything to do with describing the configuration of the hardware.
> Given it isn't related to configuration detection at runtime, what
> you've got written here just makes it seem like the property is
> redundant because the register settings do not change.
>
> Instead, use the description to talk about when the property should be
> used and what software should use it to determine, e.g. "Software can
> use vendor,channel-type to determine whether or not the measured voltage
> is absolute or relative". I pulled that outta my ass, it might not
> be what you're actually doing, but I figure you just want to know if
> you're measuring from the origin or either side of it.
>It's more to the "software can this property to correctly mark the channel
as differential or not". Hope this is acceptable. But got it, thanks.

> Cheers,
> Conor.

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Conor Dooley]

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

On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
> On 28/05/2024 20:52, Conor Dooley wrote:
> > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> >> On 27/05/2024 20:48, Conor Dooley wrote:
> >>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> >>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>>> +      adi,channel-type:
> >>>> +        description:
> >>>> +          Used to differentiate between different channel types as the device
> >>>> +           register configurations are the same for all usage types.
> >>>> +          Both pseudo-differential and single-ended channels will use the
> >>>> +           single-ended specifier.
> >>>> +        $ref: /schemas/types.yaml#/definitions/string
> >>>> +        enum:
> >>>> +          - single-ended
> >>>> +          - differential
> >>>> +        default: differential
> >>>
> >>> I dunno if my brain just ain't workin' right today, or if this is not
> >>> sufficiently explained, but why is this property needed? You've got
> >>> diff-channels and single-channels already, why can you not infer the
> >>> information you need from them? What should software do with this
> >>> information?
> >>> Additionally, "pseudo-differential" is not explained in this binding.
> >>
> >> In previous thread we arrived to the conclusion single-ended and
> >> pseudo-differential channels should be marked with the flag
> >> "differential=false" in the IIO channel struct. This cannot
> >> really be inferred as any input pair could be used in that
> >> manner and the only difference would be in external wiring.
> >>
> >> Single-channels cannot be used to define such a channel as
> >> two voltage inputs need to be selected. Also, we are already
> >> using single-channel to define the current channels.
> > 
> > If I understand correctly, the property could be simplified to a flag
> > then, since it's only the pseudo differential mode that you cannot be
> > sure of?
> > You know when you're single-ended based on single-channel, so the
> > additional info you need is only in the pseudo-differential case.
> > 
> Yes, it could just be a boolean flag. The only thing I have against
> that is the awkwardness of having both diff-channels and
> differential=false within a channel definition.

What I was suggesting was more like "adi,pseudo-differential" (you don't
need to set the =false or w/e, flag properties work based on present/not
present). I think that would avoid the awkwardness?

> >> As for explaining the pseudo-differential, should it be explained?
> >> A voltage channel within the context of these families is actually
> >> differential(as there are always two inputs selected).
> >> The single-ended and pseudo-diff use case is actually wiring up a
> >> constant voltage to the selected negative input.
> >>
> >> I did not consider that this should be described, as there is no
> >> need for an attribute to describe it.
> > 
> > I dunno, adding an explanation of it in the text for the channel type
> > seems trivial to do. "Both pseudo-differential mode (where the
> > one of differential inputs is connected to a constant voltage) and
> > single-ended channels will..."
> >
> >>> Also, what does "the device register configurations are the same for
> >>> all uses types" mean? The description here implies that you'd be reading
> >>> the registers to determine the configuration, but as far as I understand
> >>> it's the job of drivers to actually configure devices.
> >>> The only way I could interpret this that makes sense to me is that you're
> >>> trying to say that the device doesn't have registers that allow you to
> >>> do runtime configuration detection - but that's the norm and I would not
> >>> call it out here.
> >>
> >> No, I meant that the same register configuration will be set for
> >> both fully differential and single-ended. 
> >>
> >> The user will set diff-channels = <0, 1>, bipolar(or not) and
> >> then they can wire whatever to those pins: 
> >> - a differential signal
> >> - AVSS to 1 and a single-ended signal to 0
> >> - AVSS+offset to 1 and a single-ended signal to 0
> >> 	(which is called pseudo-differential in some datasheets)
> >>
> >> All these cases will look the same in terms of configuration
> > 
> > In that case, I'd just remove this sentence from the description then.
> > How you configure the registers to use the device doesn't really have
> > anything to do with describing the configuration of the hardware.
> > Given it isn't related to configuration detection at runtime, what
> > you've got written here just makes it seem like the property is
> > redundant because the register settings do not change.
> >
> > Instead, use the description to talk about when the property should be
> > used and what software should use it to determine, e.g. "Software can
> > use vendor,channel-type to determine whether or not the measured voltage
> > is absolute or relative". I pulled that outta my ass, it might not
> > be what you're actually doing, but I figure you just want to know if
> > you're measuring from the origin or either side of it.

> >It's more to the "software can this property to correctly mark the channel

Your quoting is scuffed here, I didn't write this!

> as differential or not". Hope this is acceptable. But got it, thanks.

As long as you've got a description that tells the OS what the property
actually represents, I'm happy.

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

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Nuno Sá]

On Wed, 2024-05-29 at 17:04 +0100, Conor Dooley wrote:
> On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
> > On 28/05/2024 20:52, Conor Dooley wrote:
> > > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> > > > On 27/05/2024 20:48, Conor Dooley wrote:
> > > > > On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay
> > > > > wrote:
> > > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > > > > +      adi,channel-type:
> > > > > > +        description:
> > > > > > +          Used to differentiate between different channel types as the
> > > > > > device
> > > > > > +           register configurations are the same for all usage types.
> > > > > > +          Both pseudo-differential and single-ended channels will use
> > > > > > the
> > > > > > +           single-ended specifier.
> > > > > > +        $ref: /schemas/types.yaml#/definitions/string
> > > > > > +        enum:
> > > > > > +          - single-ended
> > > > > > +          - differential
> > > > > > +        default: differential
> > > > > 
> > > > > I dunno if my brain just ain't workin' right today, or if this is not
> > > > > sufficiently explained, but why is this property needed? You've got
> > > > > diff-channels and single-channels already, why can you not infer the
> > > > > information you need from them? What should software do with this
> > > > > information?
> > > > > Additionally, "pseudo-differential" is not explained in this binding.
> > > > 
> > > > In previous thread we arrived to the conclusion single-ended and
> > > > pseudo-differential channels should be marked with the flag
> > > > "differential=false" in the IIO channel struct. This cannot
> > > > really be inferred as any input pair could be used in that
> > > > manner and the only difference would be in external wiring.
> > > > 
> > > > Single-channels cannot be used to define such a channel as
> > > > two voltage inputs need to be selected. Also, we are already
> > > > using single-channel to define the current channels.
> > > 
> > > If I understand correctly, the property could be simplified to a flag
> > > then, since it's only the pseudo differential mode that you cannot be
> > > sure of?
> > > You know when you're single-ended based on single-channel, so the
> > > additional info you need is only in the pseudo-differential case.
> > > 
> > Yes, it could just be a boolean flag. The only thing I have against
> > that is the awkwardness of having both diff-channels and
> > differential=false within a channel definition.
> 
> What I was suggesting was more like "adi,pseudo-differential" (you don't
> need to set the =false or w/e, flag properties work based on present/not
> present). I think that would avoid the awkwardness?
> 

Yeah, that was also my understanding of your reply... But I think you're also
mentioning to have this flag together with the single-channel property? 

I'm a bit confused because it seems to me that single-channel only gets one input
while we need to select two for pseudo-differential/single-ended. Is this correct
Dumitru?

FWIW, I think we already have that awkwardness in the current form. If I'm not
missing anything, what we have in the driver is pretty much:

if (not diff && single-channel)
	// then current channel
else
	// relies on the channel-type stuff

So, effectively with the above we have

diff-channels = <1 0>;

but then wait, not so fast

adi,channel-type = "single-ended"

To me the above is equally awkward (not sure if there's any precedence in using diff-
channels like this though)...

I would like for this to be explicit... If we have diff-channels, then it's surely
differential.

If not we could use the single-channel property and instead of using an extra flag we
could make the property having either 1 or 2 items. If we have 1, then it's a current
channel. If we have 2, then it's voltage single-ended/pseudo-differential. 

David's suggestion is also pretty good (and I like it's more explicit about what's
going on) so I would likely go with it...

- Nuno Sá


> > > > 

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Ceclan, Dumitru]

On 30/05/2024 10:50, Nuno Sá wrote:
> On Wed, 2024-05-29 at 17:04 +0100, Conor Dooley wrote:
>> On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
>>> On 28/05/2024 20:52, Conor Dooley wrote:
>>>> On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
>>>>> On 27/05/2024 20:48, Conor Dooley wrote:
>>>>>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay
>>>>>> wrote:
>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>>>> +      adi,channel-type:
>>>>>>> +        description:
>>>>>>> +          Used to differentiate between different channel types as the
>>>>>>> device
>>>>>>> +           register configurations are the same for all usage types.
>>>>>>> +          Both pseudo-differential and single-ended channels will use
>>>>>>> the
>>>>>>> +           single-ended specifier.
>>>>>>> +        $ref: /schemas/types.yaml#/definitions/string
>>>>>>> +        enum:
>>>>>>> +          - single-ended
>>>>>>> +          - differential
>>>>>>> +        default: differential
>>>>>>
>>>>>> I dunno if my brain just ain't workin' right today, or if this is not
>>>>>> sufficiently explained, but why is this property needed? You've got
>>>>>> diff-channels and single-channels already, why can you not infer the
>>>>>> information you need from them? What should software do with this
>>>>>> information?
>>>>>> Additionally, "pseudo-differential" is not explained in this binding.
>>>>>
>>>>> In previous thread we arrived to the conclusion single-ended and
>>>>> pseudo-differential channels should be marked with the flag
>>>>> "differential=false" in the IIO channel struct. This cannot
>>>>> really be inferred as any input pair could be used in that
>>>>> manner and the only difference would be in external wiring.
>>>>>
>>>>> Single-channels cannot be used to define such a channel as
>>>>> two voltage inputs need to be selected. Also, we are already
>>>>> using single-channel to define the current channels.
>>>>
>>>> If I understand correctly, the property could be simplified to a flag
>>>> then, since it's only the pseudo differential mode that you cannot be
>>>> sure of?
>>>> You know when you're single-ended based on single-channel, so the
>>>> additional info you need is only in the pseudo-differential case.
>>>>
>>> Yes, it could just be a boolean flag. The only thing I have against
>>> that is the awkwardness of having both diff-channels and
>>> differential=false within a channel definition.
>>
>> What I was suggesting was more like "adi,pseudo-differential" (you don't
>> need to set the =false or w/e, flag properties work based on present/not
>> present). I think that would avoid the awkwardness?
>>
> 
> Yeah, that was also my understanding of your reply... But I think you're also
> mentioning to have this flag together with the single-channel property? 
> 
> I'm a bit confused because it seems to me that single-channel only gets one input
> while we need to select two for pseudo-differential/single-ended. Is this correct
> Dumitru?
>

Yes, that is correct.
 
> FWIW, I think we already have that awkwardness in the current form. If I'm not
> missing anything, what we have in the driver is pretty much:
> 
> if (not diff && single-channel)
> 	// then current channel
> else
> 	// relies on the channel-type stuff
> 
> So, effectively with the above we have
> 
> diff-channels = <1 0>;
> 
> but then wait, not so fast
> 

This comment properly and comically describes the hot mess 
that I've come up with :)))

> adi,channel-type = "single-ended"
> 
> To me the above is equally awkward (not sure if there's any precedence in using diff-
> channels like this though)...
> 
> I would like for this to be explicit... If we have diff-channels, then it's surely
> differential.
> 
> If not we could use the single-channel property and instead of using an extra flag we
> could make the property having either 1 or 2 items. If we have 1, then it's a current
> channel. If we have 2, then it's voltage single-ended/pseudo-differential. 
> 
> David's suggestion is also pretty good (and I like it's more explicit about what's
> going on) so I would likely go with it...
> 
> - Nuno Sá
> 
> 

Yup, as neat as it could be, I'll do it that way.

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[David Lechner]

On 5/29/24 8:38 AM, Ceclan, Dumitru wrote:
> On 28/05/2024 20:52, Conor Dooley wrote:
>> On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
>>> On 27/05/2024 20:48, Conor Dooley wrote:
>>>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>>
>>>>> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
>>>>>
>>>>> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
>>>>> AD4111/AD4112 support current channels, usage is implemented by
>>>>>  specifying channel reg values bigger than 15.
>>>>>
>>>>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>> ---
>>>>>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
>>>>>  1 file changed, 120 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>>> index ea6cfcd0aff4..5b1af382dad3 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>>> @@ -19,7 +19,18 @@ description: |
>>>>>    primarily for measurement of signals close to DC but also delivers
>>>>>    outstanding performance with input bandwidths out to ~10kHz.
>>>>>  
>>>>> +  Analog Devices AD411x ADC's:
>>>>> +  The AD411X family encompasses a series of low power, low noise, 24-bit,
>>>>> +  sigma-delta analog-to-digital converters that offer a versatile range of
>>>>> +  specifications. They integrate an analog front end suitable for processing
>>>>> +  fully differential/single-ended and bipolar voltage inputs.
>>>>> +
>>>>>    Datasheets for supported chips:
>>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>>>>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>>>>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>>>>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>>>>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>>>>> @@ -31,6 +42,11 @@ description: |
>>>>>  properties:
>>>>>    compatible:
>>>>>      enum:
>>>>> +      - adi,ad4111
>>>>> +      - adi,ad4112
>>>>> +      - adi,ad4114
>>>>> +      - adi,ad4115
>>>>> +      - adi,ad4116
>>>>>        - adi,ad7172-2
>>>>>        - adi,ad7172-4
>>>>>        - adi,ad7173-8
>>>>> @@ -129,10 +145,36 @@ patternProperties:
>>>>>          maximum: 15
>>>>>  
>>>>>        diff-channels:
>>>>> +        description: |
>>>>> +          For using current channels specify select the current inputs
>>>>> +           and enable the adi,current-channel property.
>>>>> +
>>>>> +          Family AD411x supports a dedicated VINCOM voltage input.
>>>>> +          To select it set the second channel to 16.
>>>>> +            (VIN2, VINCOM) -> diff-channels = <2 16>
>>>>> +
>>>>> +          There are special values that can be selected besides the voltage
>>>>> +          analog inputs:
>>>>> +            21: REF+
>>>>> +            22: REF−
>>>>> +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>>>>> +            19: ((AVDD1 − AVSS)/5)+
>>>>> +            20: ((AVDD1 − AVSS)/5)−
>>>>> +
>>>>>          items:
>>>>>            minimum: 0
>>>>>            maximum: 31
>>>>>  
>>>>> +      single-channel:
>>>>> +        description: |
>>>>> +          Models AD4111 and AD4112 support single-ended current channels.
>>>>> +          To select the desired current input, specify the desired input pair:
>>>>> +            (IIN2+, IIN2−) -> single-channel = <2>
>>>>> +
>>>>> +        items:
>>>>> +          minimum: 1
>>>>> +          maximum: 16
>>>>> +
>>>>>        adi,reference-select:
>>>>>          description: |
>>>>>            Select the reference source to use when converting on
>>>>> @@ -154,9 +196,26 @@ patternProperties:
>>>>>            - avdd
>>>>>          default: refout-avss
>>>>>  
>>>>> +      adi,current-channel:
>>>>> +        description: |
>>>>> +          Signal that the selected inputs are current channels.
>>>>> +          Only available on AD4111 and AD4112.
>>>>> +        type: boolean
>>>>> +
>>>>> +      adi,channel-type:
>>>>> +        description:
>>>>> +          Used to differentiate between different channel types as the device
>>>>> +           register configurations are the same for all usage types.
>>>>> +          Both pseudo-differential and single-ended channels will use the
>>>>> +           single-ended specifier.
>>>>> +        $ref: /schemas/types.yaml#/definitions/string
>>>>> +        enum:
>>>>> +          - single-ended
>>>>> +          - differential
>>>>> +        default: differential
>>>>
>>>> I dunno if my brain just ain't workin' right today, or if this is not
>>>> sufficiently explained, but why is this property needed? You've got
>>>> diff-channels and single-channels already, why can you not infer the
>>>> information you need from them? What should software do with this
>>>> information?
>>>> Additionally, "pseudo-differential" is not explained in this binding.
>>>
>>> In previous thread we arrived to the conclusion single-ended and
>>> pseudo-differential channels should be marked with the flag
>>> "differential=false" in the IIO channel struct. This cannot
>>> really be inferred as any input pair could be used in that
>>> manner and the only difference would be in external wiring.
>>>
>>> Single-channels cannot be used to define such a channel as
>>> two voltage inputs need to be selected. Also, we are already
>>> using single-channel to define the current channels.
>>
>> If I understand correctly, the property could be simplified to a flag
>> then, since it's only the pseudo differential mode that you cannot be
>> sure of?
>> You know when you're single-ended based on single-channel, so the
>> additional info you need is only in the pseudo-differential case.
>>
> Yes, it could just be a boolean flag. The only thing I have against
> that is the awkwardness of having both diff-channels and
> differential=false within a channel definition.
> 
> No, there is no uncertainty regarding pseudo-differential, it's
> basically single-ended.
> 
> We cannot use single-channel for voltage channels, two voltage
> inputs need to be specified. And again, single-channel will be
> used here for the current channels. 

Instead of using diff-channels for single-ended/pseudo-differential
plus a property that says "actually not differential" could we just
add a second common-mode-channel property to specify the second
input pin that is connected to the common mode voltage source?

Just to make sure I'm understanding, single-ended means common mode
voltage is 0V (or AVSS for this chip, I guess) and pseudo-differential
means the common mode voltage is something else other than that.
In other words, single-ended is just a special case of pseudo-differential.
So effectively, no difference that we need to describe?

So something like this could work?


        /* a fully differential voltage input channel */
        channel@0 {
          reg = <0>;
          bipolar;
          diff-channels = <0 1>; /* VIN0 is +, VIN1 is - */
          adi,reference-select = "vref";
        };

        /* a single-ended voltage input channel */
        channel@1 {
          reg = <1>;
          /* no bipolar since common mode is 0V */
          single-channel = <2>; /* VIN2 is input */
          common-mode-channel = <3>; /* VIN3 connected to 0V */
        };

        /* a pseudo-differential voltage input channel */
        channel@2 {
          reg = <2>;
          bipolar; /* since common mode is not 0V */
          single-channel = <4>; /* VIN4 is input */
          common-mode-channel = <5>; /* VIN5 connected to Vref / 2 */
          adi,reference-select = "vref";
        };

	/* a current input channel */
        channel@3 {
          reg = <3>;
          bipolar;
          /* 0 is not the same pin as channel@0 because of
           * the adi,current-channel flag */
          single-channel = <0>; /* using IIN0+ and IIN0- pins */
          adi,current-channel;
        };


If we really wanted to go all the way, we could also think about
adding a common-mode-supply property in each channel node to with
a common-mode-channel property to describe the voltage source that
the pin is connected to.

Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Nuno Sá]

On Wed, 2024-05-29 at 17:04 -0500, David Lechner wrote:
> On 5/29/24 8:38 AM, Ceclan, Dumitru wrote:
> > On 28/05/2024 20:52, Conor Dooley wrote:
> > > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> > > > On 27/05/2024 20:48, Conor Dooley wrote:
> > > > > On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay
> > > > > wrote:
> > > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > > > > 
> > > > > > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> > > > > > 
> > > > > > AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
> > > > > > AD4111/AD4112 support current channels, usage is implemented by
> > > > > >  specifying channel reg values bigger than 15.
> > > > > > 
> > > > > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122
> > > > > > ++++++++++++++++++++-
> > > > > >  1 file changed, 120 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > index ea6cfcd0aff4..5b1af382dad3 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > @@ -19,7 +19,18 @@ description: |
> > > > > >    primarily for measurement of signals close to DC but also delivers
> > > > > >    outstanding performance with input bandwidths out to ~10kHz.
> > > > > >  
> > > > > > +  Analog Devices AD411x ADC's:
> > > > > > +  The AD411X family encompasses a series of low power, low noise, 24-
> > > > > > bit,
> > > > > > +  sigma-delta analog-to-digital converters that offer a versatile range
> > > > > > of
> > > > > > +  specifications. They integrate an analog front end suitable for
> > > > > > processing
> > > > > > +  fully differential/single-ended and bipolar voltage inputs.
> > > > > > +
> > > > > >    Datasheets for supported chips:
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> > > > > >     
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> > > > > >     
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> > > > > >     
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> > > > > > @@ -31,6 +42,11 @@ description: |
> > > > > >  properties:
> > > > > >    compatible:
> > > > > >      enum:
> > > > > > +      - adi,ad4111
> > > > > > +      - adi,ad4112
> > > > > > +      - adi,ad4114
> > > > > > +      - adi,ad4115
> > > > > > +      - adi,ad4116
> > > > > >        - adi,ad7172-2
> > > > > >        - adi,ad7172-4
> > > > > >        - adi,ad7173-8
> > > > > > @@ -129,10 +145,36 @@ patternProperties:
> > > > > >          maximum: 15
> > > > > >  
> > > > > >        diff-channels:
> > > > > > +        description: |
> > > > > > +          For using current channels specify select the current inputs
> > > > > > +           and enable the adi,current-channel property.
> > > > > > +
> > > > > > +          Family AD411x supports a dedicated VINCOM voltage input.
> > > > > > +          To select it set the second channel to 16.
> > > > > > +            (VIN2, VINCOM) -> diff-channels = <2 16>
> > > > > > +
> > > > > > +          There are special values that can be selected besides the
> > > > > > voltage
> > > > > > +          analog inputs:
> > > > > > +            21: REF+
> > > > > > +            22: REF−
> > > > > > +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8,
> > > > > > AD7177-2:
> > > > > > +            19: ((AVDD1 − AVSS)/5)+
> > > > > > +            20: ((AVDD1 − AVSS)/5)−
> > > > > > +
> > > > > >          items:
> > > > > >            minimum: 0
> > > > > >            maximum: 31
> > > > > >  
> > > > > > +      single-channel:
> > > > > > +        description: |
> > > > > > +          Models AD4111 and AD4112 support single-ended current
> > > > > > channels.
> > > > > > +          To select the desired current input, specify the desired input
> > > > > > pair:
> > > > > > +            (IIN2+, IIN2−) -> single-channel = <2>
> > > > > > +
> > > > > > +        items:
> > > > > > +          minimum: 1
> > > > > > +          maximum: 16
> > > > > > +
> > > > > >        adi,reference-select:
> > > > > >          description: |
> > > > > >            Select the reference source to use when converting on
> > > > > > @@ -154,9 +196,26 @@ patternProperties:
> > > > > >            - avdd
> > > > > >          default: refout-avss
> > > > > >  
> > > > > > +      adi,current-channel:
> > > > > > +        description: |
> > > > > > +          Signal that the selected inputs are current channels.
> > > > > > +          Only available on AD4111 and AD4112.
> > > > > > +        type: boolean
> > > > > > +
> > > > > > +      adi,channel-type:
> > > > > > +        description:
> > > > > > +          Used to differentiate between different channel types as the
> > > > > > device
> > > > > > +           register configurations are the same for all usage types.
> > > > > > +          Both pseudo-differential and single-ended channels will use
> > > > > > the
> > > > > > +           single-ended specifier.
> > > > > > +        $ref: /schemas/types.yaml#/definitions/string
> > > > > > +        enum:
> > > > > > +          - single-ended
> > > > > > +          - differential
> > > > > > +        default: differential
> > > > > 
> > > > > I dunno if my brain just ain't workin' right today, or if this is not
> > > > > sufficiently explained, but why is this property needed? You've got
> > > > > diff-channels and single-channels already, why can you not infer the
> > > > > information you need from them? What should software do with this
> > > > > information?
> > > > > Additionally, "pseudo-differential" is not explained in this binding.
> > > > 
> > > > In previous thread we arrived to the conclusion single-ended and
> > > > pseudo-differential channels should be marked with the flag
> > > > "differential=false" in the IIO channel struct. This cannot
> > > > really be inferred as any input pair could be used in that
> > > > manner and the only difference would be in external wiring.
> > > > 
> > > > Single-channels cannot be used to define such a channel as
> > > > two voltage inputs need to be selected. Also, we are already
> > > > using single-channel to define the current channels.
> > > 
> > > If I understand correctly, the property could be simplified to a flag
> > > then, since it's only the pseudo differential mode that you cannot be
> > > sure of?
> > > You know when you're single-ended based on single-channel, so the
> > > additional info you need is only in the pseudo-differential case.
> > > 
> > Yes, it could just be a boolean flag. The only thing I have against
> > that is the awkwardness of having both diff-channels and
> > differential=false within a channel definition.
> > 
> > No, there is no uncertainty regarding pseudo-differential, it's
> > basically single-ended.
> > 
> > We cannot use single-channel for voltage channels, two voltage
> > inputs need to be specified. And again, single-channel will be
> > used here for the current channels. 
> 
> Instead of using diff-channels for single-ended/pseudo-differential
> plus a property that says "actually not differential" could we just
> add a second common-mode-channel property to specify the second
> input pin that is connected to the common mode voltage source?
> 

Yeah, I definitely don't like having diff-channels and then go "oh, not really a
differential channel". So, if could avoid that, it would be great!

> Just to make sure I'm understanding, single-ended means common mode
> voltage is 0V (or AVSS for this chip, I guess) and pseudo-differential
> means the common mode voltage is something else other than that.
> In other words, single-ended is just a special case of pseudo-differential.
> So effectively, no difference that we need to describe?
> 
> So something like this could work?
> 
> 
>         /* a fully differential voltage input channel */
>         channel@0 {
>           reg = <0>;
>           bipolar;
>           diff-channels = <0 1>; /* VIN0 is +, VIN1 is - */
>           adi,reference-select = "vref";
>         };
> 
>         /* a single-ended voltage input channel */
>         channel@1 {
>           reg = <1>;
>           /* no bipolar since common mode is 0V */
>           single-channel = <2>; /* VIN2 is input */
>           common-mode-channel = <3>; /* VIN3 connected to 0V */
>         };
> 
>         /* a pseudo-differential voltage input channel */
>         channel@2 {
>           reg = <2>;
>           bipolar; /* since common mode is not 0V */
>           single-channel = <4>; /* VIN4 is input */
>           common-mode-channel = <5>; /* VIN5 connected to Vref / 2 */
>           adi,reference-select = "vref";
>         };
> 
> 	/* a current input channel */
>         channel@3 {
>           reg = <3>;
>           bipolar;
>           /* 0 is not the same pin as channel@0 because of
>            * the adi,current-channel flag */
>           single-channel = <0>; /* using IIN0+ and IIN0- pins */
>           adi,current-channel;

Unless I'm missing something, this flag is also not being used anywhere in the
current series?

- Nuno Sá

[PATCH v3 2/6] iio: adc: ad7173: refactor channel configuration parsing

[Dumitru Ceclan via B4 Relay]

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Move configurations regarding number of channels from
*_fw_parse_device_config to *_fw_parse_channel_config.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/all/20240303162148.3ad91aa2@jic23-huawei/
Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index ebacdacf64b9..9e507e2c66f0 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -913,7 +913,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	struct iio_chan_spec *chan_arr, *chan;
 	unsigned int ain[2], chan_index = 0;
-	int ref_sel, ret;
+	int ref_sel, ret, num_channels;
+
+	num_channels = device_get_child_node_count(dev);
+
+	if (st->info->has_temp)
+		num_channels++;
+
+	if (num_channels == 0)
+		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
+
+	if (num_channels > st->info->num_channels)
+		return dev_err_probe(dev, -EINVAL,
+			"Too many channels specified. Maximum is %d, not including temperature channel if supported.\n",
+			st->info->num_channels);
+
+	indio_dev->num_channels = num_channels;
+	st->num_channels = num_channels;
 
 	chan_arr = devm_kcalloc(dev, sizeof(*indio_dev->channels),
 				st->num_channels, GFP_KERNEL);
@@ -1008,7 +1024,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 {
 	struct ad7173_state *st = iio_priv(indio_dev);
 	struct device *dev = indio_dev->dev.parent;
-	unsigned int num_channels;
 	int ret;
 
 	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
@@ -1067,16 +1082,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 
 	ad7173_sigma_delta_info.irq_line = ret;
 
-	num_channels = device_get_child_node_count(dev);
-
-	if (st->info->has_temp)
-		num_channels++;
-
-	if (num_channels == 0)
-		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
-	indio_dev->num_channels = num_channels;
-	st->num_channels = num_channels;
-
 	return ad7173_fw_parse_channel_config(indio_dev);
 }
 

-- 
2.43.0

Re: [PATCH v3 2/6] iio: adc: ad7173: refactor channel configuration parsing

[Nuno Sá]

On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Move configurations regarding number of channels from
> *_fw_parse_device_config to *_fw_parse_channel_config.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: https://lore.kernel.org/all/20240303162148.3ad91aa2@jic23-huawei/
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index ebacdacf64b9..9e507e2c66f0 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -913,7 +913,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev
> *indio_dev)
>  	struct device *dev = indio_dev->dev.parent;
>  	struct iio_chan_spec *chan_arr, *chan;
>  	unsigned int ain[2], chan_index = 0;
> -	int ref_sel, ret;
> +	int ref_sel, ret, num_channels;
> +
> +	num_channels = device_get_child_node_count(dev);
> +
> +	if (st->info->has_temp)
> +		num_channels++;
> +
> +	if (num_channels == 0)
> +		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
> +
> +	if (num_channels > st->info->num_channels)
> +		return dev_err_probe(dev, -EINVAL,
> +			"Too many channels specified. Maximum is %d, not including
> temperature channel if supported.\n",
> +			st->info->num_channels);
> +
> +	indio_dev->num_channels = num_channels;
> +	st->num_channels = num_channels;
>  
>  	chan_arr = devm_kcalloc(dev, sizeof(*indio_dev->channels),
>  				st->num_channels, GFP_KERNEL);
> @@ -1008,7 +1024,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev
> *indio_dev)
>  {
>  	struct ad7173_state *st = iio_priv(indio_dev);
>  	struct device *dev = indio_dev->dev.parent;
> -	unsigned int num_channels;
>  	int ret;
>  
>  	st->regulators[0].supply =
> ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
> @@ -1067,16 +1082,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev
> *indio_dev)
>  
>  	ad7173_sigma_delta_info.irq_line = ret;
>  
> -	num_channels = device_get_child_node_count(dev);
> -
> -	if (st->info->has_temp)
> -		num_channels++;
> -
> -	if (num_channels == 0)
> -		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
> -	indio_dev->num_channels = num_channels;
> -	st->num_channels = num_channels;
> -
>  	return ad7173_fw_parse_channel_config(indio_dev);
>  }
>  
> 

[PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Dumitru Ceclan via B4 Relay]

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Move validation of analog inputs and reference voltage selection to
separate functions to reduce the size of the channel config parsing
function and improve readability.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 62 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 9e507e2c66f0..8a53821c8e58 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -906,6 +906,44 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
 					   &st->int_clk_hw);
 }
 
+static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
+					      unsigned int ain[2])
+{
+	struct device *dev = &st->sd.spi->dev;
+
+	for (int i = 0; i < 2; i++) {
+		if (ain[i] < st->info->num_inputs)
+			continue;
+
+		return dev_err_probe(dev, -EINVAL,
+			"Input pin number out of range for pair (%d %d).\n",
+			ain[0], ain[1]);
+	}
+
+	return 0;
+}
+
+static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
+{
+	struct device *dev = &st->sd.spi->dev;
+	int ret;
+
+	if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
+		return dev_err_probe(dev, -EINVAL,
+			"Internal reference is not available on current model.\n");
+
+	if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
+		return dev_err_probe(dev, -EINVAL,
+			"External reference 2 is not available on current model.\n");
+
+	ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot use reference %u\n",
+				     ref_sel);
+
+	return 0;
+}
+
 static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 {
 	struct ad7173_channel *chans_st_arr, *chan_st_priv;
@@ -966,11 +1004,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		if (ret)
 			return ret;
 
-		if (ain[0] >= st->info->num_inputs ||
-		    ain[1] >= st->info->num_inputs)
-			return dev_err_probe(dev, -EINVAL,
-				"Input pin number out of range for pair (%d %d).\n",
-				ain[0], ain[1]);
+		ret = ad7173_validate_voltage_ain_inputs(st, ain);
+		if (ret)
+			return ret;
 
 		ret = fwnode_property_match_property_string(child,
 							    "adi,reference-select",
@@ -981,19 +1017,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		else
 			ref_sel = ret;
 
-		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
-		    !st->info->has_int_ref)
-			return dev_err_probe(dev, -EINVAL,
-				"Internal reference is not available on current model.\n");
-
-		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
-			return dev_err_probe(dev, -EINVAL,
-				"External reference 2 is not available on current model.\n");
-
-		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Cannot use reference %u\n", ref_sel);
+		ret = ad7173_validate_reference(st, ref_sel);
+		if (ret)
+			return ret;
 
 		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
 			st->adc_mode |= AD7173_ADC_MODE_REF_EN;

-- 
2.43.0

Re: [PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Nuno Sá]

On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Move validation of analog inputs and reference voltage selection to
> separate functions to reduce the size of the channel config parsing
> function and improve readability.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 62 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 9e507e2c66f0..8a53821c8e58 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -906,6 +906,44 @@ static int ad7173_register_clk_provider(struct iio_dev
> *indio_dev)
>  					   &st->int_clk_hw);
>  }
>  
> +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> +					      unsigned int ain[2])
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +
> +	for (int i = 0; i < 2; i++) {
> +		if (ain[i] < st->info->num_inputs)
> +			continue;
> +
> +		return dev_err_probe(dev, -EINVAL,
> +			"Input pin number out of range for pair (%d %d).\n",
> +			ain[0], ain[1]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +	int ret;
> +
> +	if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
> +		return dev_err_probe(dev, -EINVAL,
> +			"Internal reference is not available on current
> model.\n");
> +
> +	if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
> +		return dev_err_probe(dev, -EINVAL,
> +			"External reference 2 is not available on current
> model.\n");
> +
> +	ret = ad7173_get_ref_voltage_milli(st, ref_sel);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> +				     ref_sel);
> +
> +	return 0;

If you need a v4, I would just 'return ad7173_get_ref_voltage_milli(...)'. Any error
log needed should be done inside ad7173_get_ref_voltage_milli(). Anyways:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

- Nuno Sá

Re: [PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Nuno Sá]

On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote:
> On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > 
> > Move validation of analog inputs and reference voltage selection to
> > separate functions to reduce the size of the channel config parsing
> > function and improve readability.
> > 
> > Reviewed-by: David Lechner <dlechner@baylibre.com>
> > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > ---
> >  drivers/iio/adc/ad7173.c | 62 ++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 44 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > index 9e507e2c66f0..8a53821c8e58 100644
> > --- a/drivers/iio/adc/ad7173.c
> > +++ b/drivers/iio/adc/ad7173.c
> > @@ -906,6 +906,44 @@ static int ad7173_register_clk_provider(struct iio_dev
> > *indio_dev)
> >  					   &st->int_clk_hw);
> >  }
> >  
> > +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> > +					      unsigned int ain[2])

Pass the pointer and size of it... Also, it should be made 'const'

> > +{
> > +	struct device *dev = &st->sd.spi->dev;
> > +
> > +	for (int i = 0; i < 2; i++) {

Use the size in here... At the very least, ARRAY_SIZE() if you keep it like this.

> > +		if (ain[i] < st->info->num_inputs)
> > +			continue;
> > +
> > +		return dev_err_probe(dev, -EINVAL,
> > +			"Input pin number out of range for pair (%d %d).\n",
> > +			ain[0], ain[1]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
> > +{
> > +	struct device *dev = &st->sd.spi->dev;
> > +	int ret;
> > +
> > +	if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
> > +		return dev_err_probe(dev, -EINVAL,
> > +			"Internal reference is not available on current
> > model.\n");
> > +
> > +	if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
> > +		return dev_err_probe(dev, -EINVAL,
> > +			"External reference 2 is not available on current
> > model.\n");
> > +
> > +	ret = ad7173_get_ref_voltage_milli(st, ref_sel);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> > +				     ref_sel);
> > +
> > +	return 0;
> 
> If you need a v4, I would just 'return ad7173_get_ref_voltage_milli(...)'. Any
> error
> log needed should be done inside ad7173_get_ref_voltage_milli(). Anyways:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 

In fact, no tag :). Just realized the above in another patch..

- Nuno Sá

Re: [PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Ceclan, Dumitru]

On 29/05/2024 15:49, Nuno Sá wrote:
> On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote:
>> On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>

...

>>> +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
>>> +					      unsigned int ain[2])
> 
> Pass the pointer and size of it... Also, it should be made 'const'
>

I'm learning here: what is the purpose of passing the size of it?
This is a specific case where the size will always be 2

Also, this will make it awkward in the following patch where I'm using
the assumption of size 2 to check if both inputs have a voltage divider
                                                          ain[(i+1) %2]

>>> +{
>>> +	struct device *dev = &st->sd.spi->dev;
>>> +
>>> +	for (int i = 0; i < 2; i++) {
> 
> Use the size in here... At the very least, ARRAY_SIZE() if you keep it like this.
> 

Re: [PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Nuno Sá]

On Thu, 2024-05-30 at 17:45 +0300, Ceclan, Dumitru wrote:
> On 29/05/2024 15:49, Nuno Sá wrote:
> > On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote:
> > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> ...
> 
> > > > +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> > > > +					      unsigned int ain[2])
> > 
> > Pass the pointer and size of it... Also, it should be made 'const'
> > 
> 
> I'm learning here: what is the purpose of passing the size of it?
> This is a specific case where the size will always be 2
> 

Basically readability... Yes, in this case it will be a stretch to assume we'll ever
have anything bigger than 2 (so the scalability argument is not so applicable) so I'm
ok if you don't pass the size. It's just I really dislike (as a practice) to have
raw/magic numbers in the code. In here, it won't be that bad as by the context, one
can easily understand the meaning of 2. Nevertheless, I would, still, at the very
least consider to either use a #define or a better name for the iterator (anything
more meaningful than 'i' so that it looks more understandable than 'i < 2')

- Nuno Sá 
> 

Re: [PATCH v3 3/6] iio: adc: ad7173: refactor ain and vref selection

[Jonathan Cameron]

On Fri, 31 May 2024 09:10:43 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2024-05-30 at 17:45 +0300, Ceclan, Dumitru wrote:
> > On 29/05/2024 15:49, Nuno Sá wrote:  
> > > On Wed, 2024-05-29 at 14:27 +0200, Nuno Sá wrote:  
> > > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:  
> > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>  
> > 
> > ...
> >   
> > > > > +static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> > > > > +					      unsigned int ain[2])  
> > > 
> > > Pass the pointer and size of it... Also, it should be made 'const'
> > >   
> > 
> > I'm learning here: what is the purpose of passing the size of it?
> > This is a specific case where the size will always be 2
> >   
> 
> Basically readability... Yes, in this case it will be a stretch to assume we'll ever
> have anything bigger than 2 (so the scalability argument is not so applicable) so I'm
> ok if you don't pass the size. It's just I really dislike (as a practice) to have
> raw/magic numbers in the code. In here, it won't be that bad as by the context, one
> can easily understand the meaning of 2. Nevertheless, I would, still, at the very
> least consider to either use a #define or a better name for the iterator (anything
> more meaningful than 'i' so that it looks more understandable than 'i < 2')
> 

I'm late to the game, but I'd just split it into two parameters.
Code is shorter as well.

static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
					      unsigned int ain0, unsigned int ain1)
{
	if (ain0 >= st->info->num_inputs ||
	    ain1 >= st->info->num_inputs)
		return dev_err_probe(&st->sd.spi->dev, -EINVAL,
				     "Input pin number out of range for pair (%d %d).\n",
				     ain0, ain1);
	return 0;
}

> - Nuno Sá 
> >   
> 

[PATCH v3 4/6] iio: adc: ad7173: add support for special inputs

[Dumitru Ceclan via B4 Relay]

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

 Add support for selecting REF+ and REF- inputs on all models.
 Add support for selecting ((AVDD1 − AVSS)/5) inputs
  on supported models.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 8a53821c8e58..106a50dbabd4 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -65,6 +65,10 @@
 	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
 #define AD7173_AIN_TEMP_POS	17
 #define AD7173_AIN_TEMP_NEG	18
+#define AD7173_AIN_COM_IN_POS	19
+#define AD7173_AIN_COM_IN_NEG	20
+#define AD7173_AIN_REF_POS	21
+#define AD7173_AIN_REF_NEG	22
 
 #define AD7172_2_ID			0x00d0
 #define AD7175_ID			0x0cd0
@@ -145,6 +149,8 @@ struct ad7173_device_info {
 	unsigned int id;
 	char *name;
 	bool has_temp;
+	/* ((AVDD1 − AVSS)/5) */
+	bool has_common_input;
 	bool has_input_buf;
 	bool has_int_ref;
 	bool has_ref2;
@@ -215,6 +221,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_common_input = true,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -229,6 +236,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = false,
 		.has_input_buf = true,
 		.has_ref2 = true,
+		.has_common_input = true,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -244,6 +252,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_input_buf = true,
 		.has_int_ref = true,
 		.has_ref2 = true,
+		.has_common_input = false,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -258,6 +267,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_common_input = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -273,6 +283,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_input_buf = true,
 		.has_int_ref = true,
 		.has_ref2 = true,
+		.has_common_input = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -287,6 +298,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = false,
 		.has_input_buf = false,
 		.has_int_ref = true,
+		.has_common_input = false,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -301,6 +313,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_common_input = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.odr_start_value = AD7177_ODR_START_VALUE,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
@@ -915,6 +928,14 @@ static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 		if (ain[i] < st->info->num_inputs)
 			continue;
 
+		if (ain[i] == AD7173_AIN_REF_POS || ain[i] == AD7173_AIN_REF_NEG)
+			continue;
+
+		if ((ain[i] == AD7173_AIN_COM_IN_POS ||
+		     ain[i] == AD7173_AIN_COM_IN_NEG) &&
+		    st->info->has_common_input)
+			continue;
+
 		return dev_err_probe(dev, -EINVAL,
 			"Input pin number out of range for pair (%d %d).\n",
 			ain[0], ain[1]);

-- 
2.43.0

Re: [PATCH v3 4/6] iio: adc: ad7173: add support for special inputs

[Nuno Sá]

On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
>  Add support for selecting REF+ and REF- inputs on all models.
>  Add support for selecting ((AVDD1 − AVSS)/5) inputs
>   on supported models.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad7173.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 8a53821c8e58..106a50dbabd4 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -65,6 +65,10 @@
>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
>  #define AD7173_AIN_TEMP_POS	17
>  #define AD7173_AIN_TEMP_NEG	18
> +#define AD7173_AIN_COM_IN_POS	19
> +#define AD7173_AIN_COM_IN_NEG	20
> +#define AD7173_AIN_REF_POS	21
> +#define AD7173_AIN_REF_NEG	22
>  
>  #define AD7172_2_ID			0x00d0
>  #define AD7175_ID			0x0cd0
> @@ -145,6 +149,8 @@ struct ad7173_device_info {
>  	unsigned int id;
>  	char *name;
>  	bool has_temp;
> +	/* ((AVDD1 − AVSS)/5) */
> +	bool has_common_input;
>  	bool has_input_buf;
>  	bool has_int_ref;
>  	bool has_ref2;
> @@ -215,6 +221,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_temp = true,
>  		.has_input_buf = true,
>  		.has_int_ref = true,
> +		.has_common_input = true,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -229,6 +236,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_temp = false,
>  		.has_input_buf = true,
>  		.has_ref2 = true,
> +		.has_common_input = true,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -244,6 +252,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_input_buf = true,
>  		.has_int_ref = true,
>  		.has_ref2 = true,
> +		.has_common_input = false,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -258,6 +267,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_temp = true,
>  		.has_input_buf = true,
>  		.has_int_ref = true,
> +		.has_common_input = true,
>  		.clock = 16 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7175_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
> @@ -273,6 +283,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_input_buf = true,
>  		.has_int_ref = true,
>  		.has_ref2 = true,
> +		.has_common_input = true,
>  		.clock = 16 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7175_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
> @@ -287,6 +298,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_temp = false,
>  		.has_input_buf = false,
>  		.has_int_ref = true,
> +		.has_common_input = false,
>  		.clock = 16 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7175_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
> @@ -301,6 +313,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
>  		.has_temp = true,
>  		.has_input_buf = true,
>  		.has_int_ref = true,
> +		.has_common_input = true,
>  		.clock = 16 * HZ_PER_MHZ,
>  		.odr_start_value = AD7177_ODR_START_VALUE,
>  		.sinc5_data_rates = ad7175_sinc5_data_rates,
> @@ -915,6 +928,14 @@ static int ad7173_validate_voltage_ain_inputs(struct
> ad7173_state *st,
>  		if (ain[i] < st->info->num_inputs)
>  			continue;
>  
> +		if (ain[i] == AD7173_AIN_REF_POS || ain[i] == AD7173_AIN_REF_NEG)
> +			continue;
> +
> +		if ((ain[i] == AD7173_AIN_COM_IN_POS ||
> +		     ain[i] == AD7173_AIN_COM_IN_NEG) &&
> +		    st->info->has_common_input)
> +			continue;
> +
>  		return dev_err_probe(dev, -EINVAL,
>  			"Input pin number out of range for pair (%d %d).\n",
>  			ain[0], ain[1]);
> 

[PATCH v3 5/6] iio: adc: ad7173: Add support for AD411x devices

[Dumitru Ceclan via B4 Relay]

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Add support for AD4111/AD4112/AD4114/AD4115/AD4116.

The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications.

This family of ADCs integrates an analog front end suitable for processing
both fully differential and single-ended, bipolar voltage inputs
addressing a wide array of industrial and instrumentation requirements.

- All ADCs have inputs with a precision voltage divider with a division
  ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
  shunt resistor.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 327 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 297 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 106a50dbabd4..328685ce25e0 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * AD717x family SPI ADC driver
+ * AD717x and AD411x family SPI ADC driver
  *
  * Supported devices:
+ *  AD4111/AD4112/AD4114/AD4115/AD4116
  *  AD7172-2/AD7172-4/AD7173-8/AD7175-2
  *  AD7175-8/AD7176-2/AD7177-2
  *
@@ -75,7 +76,9 @@
 #define AD7176_ID			0x0c90
 #define AD7175_2_ID			0x0cd0
 #define AD7172_4_ID			0x2050
-#define AD7173_ID			0x30d0
+#define AD7173_AD4111_AD4112_AD4114_ID	0x30d0
+#define AD4116_ID			0x34d0
+#define AD4115_ID			0x38d0
 #define AD7175_8_ID			0x3cd0
 #define AD7177_ID			0x4fd0
 #define AD7173_ID_MASK			GENMASK(15, 4)
@@ -106,6 +109,7 @@
 
 #define AD7173_GPO12_DATA(x)	BIT((x) + 0)
 #define AD7173_GPO23_DATA(x)	BIT((x) + 4)
+#define AD4111_GPO01_DATA(x)	BIT((x) + 6)
 #define AD7173_GPO_DATA(x)	((x) < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
 
 #define AD7173_INTERFACE_DATA_STAT	BIT(6)
@@ -124,11 +128,20 @@
 #define AD7173_VOLTAGE_INT_REF_uV	2500000
 #define AD7173_TEMP_SENSIIVITY_uV_per_C	477
 #define AD7177_ODR_START_VALUE		0x07
+#define AD4111_SHUNT_RESISTOR_OHM	50
+#define AD4111_DIVIDER_RATIO		10
+#define AD411X_VCOM_INPUT		0X10
+#define AD4111_CURRENT_CHAN_CUTOFF	16
 
 #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
 #define AD7173_MAX_CONFIGS		8
 
 enum ad7173_ids {
+	ID_AD4111,
+	ID_AD4112,
+	ID_AD4114,
+	ID_AD4115,
+	ID_AD4116,
 	ID_AD7172_2,
 	ID_AD7172_4,
 	ID_AD7173_8,
@@ -138,22 +151,43 @@ enum ad7173_ids {
 	ID_AD7177_2,
 };
 
+enum ad4111_current_channels {
+	AD4111_CURRENT_IN0P_IN0N,
+	AD4111_CURRENT_IN1P_IN1N,
+	AD4111_CURRENT_IN2P_IN2N,
+	AD4111_CURRENT_IN3P_IN3N,
+};
+
+enum ad7173_channel_types {
+	AD7173_CHAN_SINGLE_ENDED,
+	AD7173_CHAN_DIFFERENTIAL,
+};
+
 struct ad7173_device_info {
 	const unsigned int *sinc5_data_rates;
 	unsigned int num_sinc5_data_rates;
 	unsigned int odr_start_value;
+	/*
+	 * AD4116 has both inputs with a volage divider and without.
+	 * These inputs cannot be mixed in the channel configuration.
+	 * Does not include the VCOM input.
+	 */
+	unsigned int num_voltage_inputs_with_divider;
 	unsigned int num_channels;
 	unsigned int num_configs;
-	unsigned int num_inputs;
+	unsigned int num_voltage_inputs;
 	unsigned int clock;
 	unsigned int id;
 	char *name;
+	bool has_current_inputs;
+	bool has_vcom_input;
 	bool has_temp;
 	/* ((AVDD1 − AVSS)/5) */
 	bool has_common_input;
 	bool has_input_buf;
 	bool has_int_ref;
 	bool has_ref2;
+	bool higher_gpio_bits;
 	u8 num_gpios;
 };
 
@@ -195,6 +229,24 @@ struct ad7173_state {
 #endif
 };
 
+static unsigned int ad4115_sinc5_data_rates[] = {
+	24845000, 24845000, 20725000, 20725000,	/*  0-3  */
+	15564000, 13841000, 10390000, 10390000,	/*  4-7  */
+	4994000,  2499000,  1000000,  500000,	/*  8-11 */
+	395500,   200000,   100000,   59890,	/* 12-15 */
+	49920,    20000,    16660,    10000,	/* 16-19 */
+	5000,	  2500,     2500,		/* 20-22 */
+};
+
+static unsigned int ad4116_sinc5_data_rates[] = {
+	12422360, 12422360, 12422360, 12422360,	/*  0-3  */
+	10362690, 10362690, 7782100,  6290530,	/*  4-7  */
+	5194800,  2496900,  1007600,  499900,	/*  8-11 */
+	390600,	  200300,   100000,   59750,	/* 12-15 */
+	49840,	  20000,    16650,    10000,	/* 16-19 */
+	5000,	  2500,	    1250,		/* 20-22 */
+};
+
 static const unsigned int ad7173_sinc5_data_rates[] = {
 	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,	/*  0-7  */
 	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,	/*  8-15 */
@@ -210,14 +262,109 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
 	5000,					/* 20    */
 };
 
+static unsigned int ad4111_current_channel_config[] = {
+	[AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
+	[AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
+	[AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
+	[AD4111_CURRENT_IN3P_IN3N] = 0x18B,
+};
+
 static const struct ad7173_device_info ad7173_device_info[] = {
+	[ID_AD4111] = {
+		.name = "ad4111",
+		.id = AD7173_AD4111_AD4112_AD4114_ID,
+		.num_voltage_inputs_with_divider = 8,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_voltage_inputs = 8,
+		.num_gpios = 2,
+		.higher_gpio_bits = true,
+		.has_temp = true,
+		.has_vcom_input = true,
+		.has_input_buf = true,
+		.has_current_inputs = true,
+		.has_int_ref = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD4112] = {
+		.name = "ad4112",
+		.id = AD7173_AD4111_AD4112_AD4114_ID,
+		.num_voltage_inputs_with_divider = 8,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_voltage_inputs = 8,
+		.num_gpios = 2,
+		.higher_gpio_bits = true,
+		.has_vcom_input = true,
+		.has_temp = true,
+		.has_input_buf = true,
+		.has_current_inputs = true,
+		.has_int_ref = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD4114] = {
+		.name = "ad4114",
+		.id = AD7173_AD4111_AD4112_AD4114_ID,
+		.num_voltage_inputs_with_divider = 16,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_voltage_inputs = 16,
+		.num_gpios = 4,
+		.higher_gpio_bits = true,
+		.has_vcom_input = true,
+		.has_temp = true,
+		.has_input_buf = true,
+		.has_int_ref = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD4115] = {
+		.name = "ad4115",
+		.id = AD4115_ID,
+		.num_voltage_inputs_with_divider = 16,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_voltage_inputs = 16,
+		.num_gpios = 4,
+		.higher_gpio_bits = true,
+		.has_vcom_input = true,
+		.has_temp = true,
+		.has_input_buf = true,
+		.has_int_ref = true,
+		.clock = 8 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad4115_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
+	},
+	[ID_AD4116] = {
+		.name = "ad4116",
+		.id = AD4116_ID,
+		.num_voltage_inputs_with_divider = 11,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_voltage_inputs = 16,
+		.num_gpios = 4,
+		.higher_gpio_bits = true,
+		.has_vcom_input = true,
+		.has_temp = true,
+		.has_input_buf = true,
+		.has_int_ref = true,
+		.clock = 4 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad4116_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
+	},
 	[ID_AD7172_2] = {
 		.name = "ad7172-2",
 		.id = AD7172_2_ID,
-		.num_inputs = 5,
+		.num_voltage_inputs = 5,
 		.num_channels = 4,
 		.num_configs = 4,
 		.num_gpios = 2,
+		.higher_gpio_bits = false,
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
@@ -229,10 +376,11 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 	[ID_AD7172_4] = {
 		.name = "ad7172-4",
 		.id = AD7172_4_ID,
-		.num_inputs = 9,
+		.num_voltage_inputs = 9,
 		.num_channels = 8,
 		.num_configs = 8,
 		.num_gpios = 4,
+		.higher_gpio_bits = false,
 		.has_temp = false,
 		.has_input_buf = true,
 		.has_ref2 = true,
@@ -243,11 +391,12 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 	},
 	[ID_AD7173_8] = {
 		.name = "ad7173-8",
-		.id = AD7173_ID,
-		.num_inputs = 17,
+		.id = AD7173_AD4111_AD4112_AD4114_ID,
+		.num_voltage_inputs = 17,
 		.num_channels = 16,
 		.num_configs = 8,
 		.num_gpios = 4,
+		.higher_gpio_bits = false,
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
@@ -260,10 +409,11 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 	[ID_AD7175_2] = {
 		.name = "ad7175-2",
 		.id = AD7175_2_ID,
-		.num_inputs = 5,
+		.num_voltage_inputs = 5,
 		.num_channels = 4,
 		.num_configs = 4,
 		.num_gpios = 2,
+		.higher_gpio_bits = false,
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
@@ -275,10 +425,11 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 	[ID_AD7175_8] = {
 		.name = "ad7175-8",
 		.id = AD7175_8_ID,
-		.num_inputs = 17,
+		.num_voltage_inputs = 17,
 		.num_channels = 16,
 		.num_configs = 8,
 		.num_gpios = 4,
+		.higher_gpio_bits = false,
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
@@ -291,10 +442,11 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 	[ID_AD7176_2] = {
 		.name = "ad7176-2",
 		.id = AD7176_ID,
-		.num_inputs = 5,
+		.num_voltage_inputs = 5,
 		.num_channels = 4,
 		.num_configs = 4,
 		.num_gpios = 2,
+		.higher_gpio_bits = false,
 		.has_temp = false,
 		.has_input_buf = false,
 		.has_int_ref = true,
@@ -306,10 +458,11 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 	[ID_AD7177_2] = {
 		.name = "ad7177-2",
 		.id = AD7177_ID,
-		.num_inputs = 5,
+		.num_voltage_inputs = 5,
 		.num_channels = 4,
 		.num_configs = 4,
 		.num_gpios = 2,
+		.higher_gpio_bits = false,
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
@@ -328,6 +481,11 @@ static const char *const ad7173_ref_sel_str[] = {
 	[AD7173_SETUP_REF_SEL_AVDD1_AVSS] = "avdd",
 };
 
+static const char *const ad7173_channel_types[] = {
+	[AD7173_CHAN_SINGLE_ENDED] = "single-ended",
+	[AD7173_CHAN_DIFFERENTIAL] = "differential",
+};
+
 static const char *const ad7173_clk_sel[] = {
 	"ext-clk", "xtal"
 };
@@ -360,6 +518,15 @@ static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
 	return 0;
 }
 
+static int ad4111_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
+			     unsigned int offset, unsigned int *reg,
+			     unsigned int *mask)
+{
+	*mask = AD4111_GPO01_DATA(offset);
+	*reg = base;
+	return 0;
+}
+
 static void ad7173_gpio_disable(void *data)
 {
 	struct ad7173_state *st = data;
@@ -392,7 +559,10 @@ static int ad7173_gpio_init(struct ad7173_state *st)
 	gpio_regmap.regmap = st->reg_gpiocon_regmap;
 	gpio_regmap.ngpio = st->info->num_gpios;
 	gpio_regmap.reg_set_base = AD7173_REG_GPIO;
-	gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
+	if (st->info->higher_gpio_bits)
+		gpio_regmap.reg_mask_xlate = ad4111_mask_xlate;
+	else
+		gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
 
 	st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
 	ret = PTR_ERR_OR_ZERO(st->gpio_regmap);
@@ -688,18 +858,33 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		if (chan->type == IIO_TEMP) {
+
+		switch (chan->type) {
+		case IIO_TEMP:
 			temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
 			temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
 			*val = temp;
 			*val2 = chan->scan_type.realbits;
-		} else {
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_VOLTAGE:
 			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
 			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+
+			if (chan->channel < st->info->num_voltage_inputs_with_divider)
+				*val *= AD4111_DIVIDER_RATIO;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_CURRENT:
+			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
+			*val /= AD4111_SHUNT_RESISTOR_OHM;
+			*val2 = chan->scan_type.realbits - (ch->cfg.bipolar ? 1 : 0);
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
 		}
-		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		if (chan->type == IIO_TEMP) {
+
+		switch (chan->type) {
+		case IIO_TEMP:
 			/* 0 Kelvin -> raw sample */
 			temp   = -ABSOLUTE_ZERO_MILLICELSIUS;
 			temp  *= AD7173_TEMP_SENSIIVITY_uV_per_C;
@@ -708,10 +893,14 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 						       AD7173_VOLTAGE_INT_REF_uV *
 						       MILLI);
 			*val   = -temp;
-		} else {
+			return IIO_VAL_INT;
+		case IIO_VOLTAGE:
+		case IIO_CURRENT:
 			*val = -BIT(chan->scan_type.realbits - 1);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
 		}
-		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		reg = st->channels[chan->address].cfg.odr;
 
@@ -919,13 +1108,34 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
 					   &st->int_clk_hw);
 }
 
+static int ad4111_validate_current_ain(struct ad7173_state *st,
+				       unsigned int ain[2])
+{
+	struct device *dev = &st->sd.spi->dev;
+
+	if (!st->info->has_current_inputs)
+		return dev_err_probe(dev, -EINVAL,
+			"Model %s does not support current channels\n",
+			st->info->name);
+
+	if (ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))
+		return dev_err_probe(dev, -EINVAL,
+			"For current channels single-channel must be <[0-3]>\n");
+
+	return 0;
+}
+
 static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 					      unsigned int ain[2])
 {
 	struct device *dev = &st->sd.spi->dev;
+	bool ain_selects_normal_input[] = {
+		ain[0] < st->info->num_voltage_inputs,
+		ain[1] < st->info->num_voltage_inputs
+	};
 
 	for (int i = 0; i < 2; i++) {
-		if (ain[i] < st->info->num_inputs)
+		if (ain_selects_normal_input[i])
 			continue;
 
 		if (ain[i] == AD7173_AIN_REF_POS || ain[i] == AD7173_AIN_REF_NEG)
@@ -936,11 +1146,27 @@ static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 		    st->info->has_common_input)
 			continue;
 
+		if (st->info->has_vcom_input && ain[i] == AD411X_VCOM_INPUT) {
+			if (ain_selects_normal_input[(i + 1) % 2] &&
+			    ain[(i + 1) % 2] >= st->info->num_voltage_inputs_with_divider)
+				return dev_err_probe(dev, -EINVAL,
+					"VCOM must be paired with inputs having divider.\n");
+
+			continue;
+		}
+
 		return dev_err_probe(dev, -EINVAL,
 			"Input pin number out of range for pair (%d %d).\n",
 			ain[0], ain[1]);
 	}
 
+	if ((ain_selects_normal_input[0] && ain_selects_normal_input[1]) &&
+	    ((ain[0] >= st->info->num_voltage_inputs_with_divider) !=
+	     (ain[1] >= st->info->num_voltage_inputs_with_divider)))
+		return dev_err_probe(dev, -EINVAL,
+			"Both inputs must either have a voltage divider or not have: (%d %d).\n",
+			ain[0], ain[1]);
+
 	return 0;
 }
 
@@ -972,7 +1198,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	struct iio_chan_spec *chan_arr, *chan;
 	unsigned int ain[2], chan_index = 0;
-	int ref_sel, ret, num_channels;
+	int ref_sel, ret, is_current_chan, num_channels;
 
 	num_channels = device_get_child_node_count(dev);
 
@@ -1022,12 +1248,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		chan_st_priv = &chans_st_arr[chan_index];
 		ret = fwnode_property_read_u32_array(child, "diff-channels",
 						     ain, ARRAY_SIZE(ain));
-		if (ret)
-			return ret;
+		if (ret) {
+			ret = fwnode_property_read_u32_array(child, "single-channel",
+							     ain, 1);
+			if (ret)
+				return ret;
 
-		ret = ad7173_validate_voltage_ain_inputs(st, ain);
-		if (ret)
-			return ret;
+			ret = ad4111_validate_current_ain(st, ain);
+			if (ret)
+				return ret;
+			is_current_chan = true;
+			ain[1] = 0;
+		} else {
+			ret = ad7173_validate_voltage_ain_inputs(st, ain);
+			if (ret)
+				return ret;
+			is_current_chan = false;
+		}
 
 		ret = fwnode_property_match_property_string(child,
 							    "adi,reference-select",
@@ -1051,17 +1288,34 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		chan->scan_index = chan_index;
 		chan->channel = ain[0];
 		chan->channel2 = ain[1];
-		chan->differential = true;
-
-		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
 		chan_st_priv->chan_reg = chan_index;
 		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
 		chan_st_priv->cfg.odr = 0;
-
 		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
+
 		if (chan_st_priv->cfg.bipolar)
 			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
 
+		ret = fwnode_property_match_property_string(child,
+							    "adi,channel-type",
+							    ad7173_channel_types,
+							    ARRAY_SIZE(ad7173_channel_types));
+		chan->differential = (ret < 0 || ret == AD7173_CHAN_DIFFERENTIAL)
+					? 1 : 0;
+
+
+		if (is_current_chan) {
+			chan->type = IIO_CURRENT;
+			chan->differential = false;
+			ain[1] = FIELD_GET(AD7173_CH_SETUP_AINNEG_MASK,
+					   ad4111_current_channel_config[ain[0]]);
+			ain[0] = FIELD_GET(AD7173_CH_SETUP_AINPOS_MASK,
+					   ad4111_current_channel_config[ain[0]]);
+		} else {
+			chan_st_priv->cfg.input_buf = st->info->has_input_buf;
+		}
+		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+
 		chan_index++;
 	}
 	return 0;
@@ -1188,6 +1442,14 @@ static int ad7173_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id ad7173_of_match[] = {
+	{ .compatible = "ad4111",
+	  .data = &ad7173_device_info[ID_AD4111]},
+	{ .compatible = "ad4112",
+	  .data = &ad7173_device_info[ID_AD4112]},
+	{ .compatible = "ad4114",
+	  .data = &ad7173_device_info[ID_AD4114]},
+	{ .compatible = "ad4115",
+	  .data = &ad7173_device_info[ID_AD4115]},
 	{ .compatible = "adi,ad7172-2",
 	  .data = &ad7173_device_info[ID_AD7172_2]},
 	{ .compatible = "adi,ad7172-4",
@@ -1207,6 +1469,11 @@ static const struct of_device_id ad7173_of_match[] = {
 MODULE_DEVICE_TABLE(of, ad7173_of_match);
 
 static const struct spi_device_id ad7173_id_table[] = {
+	{ "ad4111", (kernel_ulong_t)&ad7173_device_info[ID_AD4111]},
+	{ "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]},
+	{ "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]},
+	{ "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]},
+	{ "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]},
 	{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
 	{ "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]},
 	{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
@@ -1231,5 +1498,5 @@ module_spi_driver(ad7173_driver);
 MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
 MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_DESCRIPTION("Analog Devices AD7173 and similar ADC driver");
 MODULE_LICENSE("GPL");

-- 
2.43.0

Re: [PATCH v3 5/6] iio: adc: ad7173: Add support for AD411x devices

[Nuno Sá]

On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> 
> The AD411X family encompasses a series of low power, low noise, 24-bit,
> sigma-delta analog-to-digital converters that offer a versatile range of
> specifications.
> 
> This family of ADCs integrates an analog front end suitable for processing
> both fully differential and single-ended, bipolar voltage inputs
> addressing a wide array of industrial and instrumentation requirements.
> 
> - All ADCs have inputs with a precision voltage divider with a division
>   ratio of 10.
> - AD4116 has 5 low level inputs without a voltage divider.
> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
>   shunt resistor.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 327 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 297 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 106a50dbabd4..328685ce25e0 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -1,8 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * AD717x family SPI ADC driver
> + * AD717x and AD411x family SPI ADC driver
>   *
>   * Supported devices:
> + *  AD4111/AD4112/AD4114/AD4115/AD4116
>   *  AD7172-2/AD7172-4/AD7173-8/AD7175-2
>   *  AD7175-8/AD7176-2/AD7177-2
>   *
> @@ -75,7 +76,9 @@
>  #define AD7176_ID			0x0c90
>  #define AD7175_2_ID			0x0cd0
>  #define AD7172_4_ID			0x2050
> -#define AD7173_ID			0x30d0
> +#define AD7173_AD4111_AD4112_AD4114_ID	0x30d0

I would definitely rename this :). Would even prefer to have separate defines all
defined by AD7173_ID.

> +#define AD4116_ID			0x34d0
> +#define AD4115_ID			0x38d0
>  #define AD7175_8_ID			0x3cd0
>  #define AD7177_ID			0x4fd0
>  #define AD7173_ID_MASK			GENMASK(15, 4)
> @@ -106,6 +109,7 @@
>  
>  #define AD7173_GPO12_DATA(x)	BIT((x) + 0)
>  #define AD7173_GPO23_DATA(x)	BIT((x) + 4)
> +#define AD4111_GPO01_DATA(x)	BIT((x) + 6)
>  #define AD7173_GPO_DATA(x)	((x) < 2 ? AD7173_GPO12_DATA(x) :
> AD7173_GPO23_DATA(x))
>  
>  #define AD7173_INTERFACE_DATA_STAT	BIT(6)
> @@ -124,11 +128,20 @@
>  #define AD7173_VOLTAGE_INT_REF_uV	2500000
>  #define AD7173_TEMP_SENSIIVITY_uV_per_C	477
>  #define AD7177_ODR_START_VALUE		0x07
> +#define AD4111_SHUNT_RESISTOR_OHM	50
> +#define AD4111_DIVIDER_RATIO		10
> +#define AD411X_VCOM_INPUT		0X10
> +#define AD4111_CURRENT_CHAN_CUTOFF	16
>  
>  #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
>  #define AD7173_MAX_CONFIGS		8
>  
>  enum ad7173_ids {
> +	ID_AD4111,
> +	ID_AD4112,
> +	ID_AD4114,
> +	ID_AD4115,
> +	ID_AD4116,
>  	ID_AD7172_2,
>  	ID_AD7172_4,
>  	ID_AD7173_8,
> @@ -138,22 +151,43 @@ enum ad7173_ids {
>  	ID_AD7177_2,
>  };
>  
> +enum ad4111_current_channels {
> +	AD4111_CURRENT_IN0P_IN0N,
> +	AD4111_CURRENT_IN1P_IN1N,
> +	AD4111_CURRENT_IN2P_IN2N,
> +	AD4111_CURRENT_IN3P_IN3N,
> +};
> +
> +enum ad7173_channel_types {
> +	AD7173_CHAN_SINGLE_ENDED,
> +	AD7173_CHAN_DIFFERENTIAL,
> +};
> +
>  struct ad7173_device_info {
>  	const unsigned int *sinc5_data_rates;
>  	unsigned int num_sinc5_data_rates;
>  	unsigned int odr_start_value;
> +	/*
> +	 * AD4116 has both inputs with a volage divider and without.

s/volage/voltage

> +	 * These inputs cannot be mixed in the channel configuration.
> +	 * Does not include the VCOM input.
> +	 */
> +	unsigned int num_voltage_inputs_with_divider;

nit: maybe num_voltage_in_div?

>  	unsigned int num_channels;
>  	unsigned int num_configs;
> -	unsigned int num_inputs;
> +	unsigned int num_voltage_inputs;

nit: maybe num_voltage_in?

>  	unsigned int clock;
>  	unsigned int id;
>  	char *name;
> +	bool has_current_inputs;
> +	bool has_vcom_input;
>  	bool has_temp;
>  	/* ((AVDD1 − AVSS)/5) */
>  	bool has_common_input;
>  	bool has_input_buf;
>  	bool has_int_ref;
>  	bool has_ref2;
> +	bool higher_gpio_bits;
>  	u8 num_gpios;
>  };
>  
> @@ -195,6 +229,24 @@ struct ad7173_state {
>  #endif
>  };
>  
> +static unsigned int ad4115_sinc5_data_rates[] = {
> +	24845000, 24845000, 20725000, 20725000,	/*  0-3  */
> +	15564000, 13841000, 10390000, 10390000,	/*  4-7  */
> +	4994000,  2499000,  1000000,  500000,	/*  8-11 */
> +	395500,   200000,   100000,   59890,	/* 12-15 */
> +	49920,    20000,    16660,    10000,	/* 16-19 */
> +	5000,	  2500,     2500,		/* 20-22 */
> +};
> +
> +static unsigned int ad4116_sinc5_data_rates[] = {
> +	12422360, 12422360, 12422360, 12422360,	/*  0-3  */
> +	10362690, 10362690, 7782100,  6290530,	/*  4-7  */
> +	5194800,  2496900,  1007600,  499900,	/*  8-11 */
> +	390600,	  200300,   100000,   59750,	/* 12-15 */
> +	49840,	  20000,    16650,    10000,	/* 16-19 */
> +	5000,	  2500,	    1250,		/* 20-22 */
> +};
> +
>  static const unsigned int ad7173_sinc5_data_rates[] = {
>  	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000,
> 4444000,	/*  0-7  */
>  	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500, 
> 59520,	/*  8-15 */
> @@ -210,14 +262,109 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
>  	5000,					/* 20    */
>  };
>  
> +static unsigned int ad4111_current_channel_config[] = {
> +	[AD4111_CURRENT_IN0P_IN0N] = 0x1E8,
> +	[AD4111_CURRENT_IN1P_IN1N] = 0x1C9,
> +	[AD4111_CURRENT_IN2P_IN2N] = 0x1AA,
> +	[AD4111_CURRENT_IN3P_IN3N] = 0x18B,
> +};
> +
>  static const struct ad7173_device_info ad7173_device_info[] = {
> +	[ID_AD4111] = {
> +		.name = "ad4111",
> +		.id = AD7173_AD4111_AD4112_AD4114_ID,
> +		.num_voltage_inputs_with_divider = 8,
> +		.num_channels = 16,
> +		.num_configs = 8,
> +		.num_voltage_inputs = 8,
> +		.num_gpios = 2,
> +		.higher_gpio_bits = true,
> +		.has_temp = true,
> +		.has_vcom_input = true,
> +		.has_input_buf = true,
> +		.has_current_inputs = true,
> +		.has_int_ref = true,
> +		.clock = 2 * HZ_PER_MHZ,
> +		.sinc5_data_rates = ad7173_sinc5_data_rates,
> +		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> +	},

At some point it would be nice to drop the ad7173_device_info array...

...

> 
> @@ -688,18 +858,33 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		if (chan->type == IIO_TEMP) {
> +
> +		switch (chan->type) {
> +		case IIO_TEMP:
>  			temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
>  			temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
>  			*val = temp;
>  			*val2 = chan->scan_type.realbits;
> -		} else {
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_VOLTAGE:
>  			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
>  			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +
> +			if (chan->channel < st->info-
> >num_voltage_inputs_with_divider)
> +				*val *= AD4111_DIVIDER_RATIO;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_CURRENT:
> +			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> +			*val /= AD4111_SHUNT_RESISTOR_OHM;
> +			*val2 = chan->scan_type.realbits - (ch->cfg.bipolar ? 1 :
> 0);

Can bipolar have any other value than 0 or 1? Just subtract it directly...

> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
>  		}
> -		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:
> -		if (chan->type == IIO_TEMP) {
> +
> +		switch (chan->type) {
> +		case IIO_TEMP:
>  			/* 0 Kelvin -> raw sample */
>  			temp   = -ABSOLUTE_ZERO_MILLICELSIUS;
>  			temp  *= AD7173_TEMP_SENSIIVITY_uV_per_C;
> @@ -708,10 +893,14 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
>  						       AD7173_VOLTAGE_INT_REF_uV *
>  						       MILLI);
>  			*val   = -temp;
> -		} else {
> +			return IIO_VAL_INT;
> +		case IIO_VOLTAGE:
> +		case IIO_CURRENT:
>  			*val = -BIT(chan->scan_type.realbits - 1);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
>  		}
> -		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		reg = st->channels[chan->address].cfg.odr;
>  
> @@ -919,13 +1108,34 @@ static int ad7173_register_clk_provider(struct iio_dev
> *indio_dev)
>  					   &st->int_clk_hw);
>  }
>  
> +static int ad4111_validate_current_ain(struct ad7173_state *st,
> +				       unsigned int ain[2])

Hmm, pass by reference... Should also be const AFAICT.

...

>  
> @@ -1022,12 +1248,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev
> *indio_dev)
>  		chan_st_priv = &chans_st_arr[chan_index];
>  		ret = fwnode_property_read_u32_array(child, "diff-channels",
>  						     ain, ARRAY_SIZE(ain));
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			ret = fwnode_property_read_u32_array(child, "single-
> channel",
> +							     ain, 1);
> +			if (ret)
> +				return ret;
>  
> -		ret = ad7173_validate_voltage_ain_inputs(st, ain);
> -		if (ret)
> -			return ret;
> +			ret = ad4111_validate_current_ain(st, ain);
> +			if (ret)
> +				return ret;
> +			is_current_chan = true;
> +			ain[1] = 0;
> +		} else {
> +			ret = ad7173_validate_voltage_ain_inputs(st, ain);
> +			if (ret)
> +				return ret;
> +			is_current_chan = false;
> +		}
>  
>  		ret = fwnode_property_match_property_string(child,
>  							    "adi,reference-
> select",
> @@ -1051,17 +1288,34 @@ static int ad7173_fw_parse_channel_config(struct iio_dev
> *indio_dev)
>  		chan->scan_index = chan_index;
>  		chan->channel = ain[0];
>  		chan->channel2 = ain[1];
> -		chan->differential = true;
> -
> -		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
>  		chan_st_priv->chan_reg = chan_index;
>  		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>  		chan_st_priv->cfg.odr = 0;
> -
>  		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child,
> "bipolar");
> +
>  		if (chan_st_priv->cfg.bipolar)
>  			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
>  
> +		ret = fwnode_property_match_property_string(child,
> +							    "adi,channel-type",
> +							    ad7173_channel_types,
> +							   
> ARRAY_SIZE(ad7173_channel_types));
> +		chan->differential = (ret < 0 || ret == AD7173_CHAN_DIFFERENTIAL)
> +					? 1 : 0;

I don't think we should treat 'ret < 0' has a differential channel. Any reason for
it? For me, it's just an invalid property value given by the user...

- Nuno Sá

Re: [PATCH v3 5/6] iio: adc: ad7173: Add support for AD411x devices

[Ceclan, Dumitru]

On 29/05/2024 15:46, Nuno Sá wrote:
> On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>

...

>>  static const struct ad7173_device_info ad7173_device_info[] = {
>> +	[ID_AD4111] = {
>> +		.name = "ad4111",
>> +		.id = AD7173_AD4111_AD4112_AD4114_ID,
>> +		.num_voltage_inputs_with_divider = 8,
>> +		.num_channels = 16,
>> +		.num_configs = 8,
>> +		.num_voltage_inputs = 8,
>> +		.num_gpios = 2,
>> +		.higher_gpio_bits = true,
>> +		.has_temp = true,
>> +		.has_vcom_input = true,
>> +		.has_input_buf = true,
>> +		.has_current_inputs = true,
>> +		.has_int_ref = true,
>> +		.clock = 2 * HZ_PER_MHZ,
>> +		.sinc5_data_rates = ad7173_sinc5_data_rates,
>> +		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>> +	},
> 
> At some point it would be nice to drop the ad7173_device_info array...
> 
What are good alternatives to this?
...

>> +		ret = fwnode_property_match_property_string(child,
>> +							    "adi,channel-type",
>> +							    ad7173_channel_types,
>> +							   
>> ARRAY_SIZE(ad7173_channel_types));
>> +		chan->differential = (ret < 0 || ret == AD7173_CHAN_DIFFERENTIAL)
>> +					? 1 : 0;
> 
> I don't think we should treat 'ret < 0' has a differential channel. Any reason for
> it? For me, it's just an invalid property value given by the user...
> 
Yes, as that would be the default value if it's missing or invalid

> - Nuno Sá
> 

Re: [PATCH v3 5/6] iio: adc: ad7173: Add support for AD411x devices

[David Lechner]

On 5/29/24 9:03 AM, Ceclan, Dumitru wrote:
> On 29/05/2024 15:46, Nuno Sá wrote:
>> On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> ...
> 
>>>  static const struct ad7173_device_info ad7173_device_info[] = {
>>> +	[ID_AD4111] = {
>>> +		.name = "ad4111",
>>> +		.id = AD7173_AD4111_AD4112_AD4114_ID,
>>> +		.num_voltage_inputs_with_divider = 8,
>>> +		.num_channels = 16,
>>> +		.num_configs = 8,
>>> +		.num_voltage_inputs = 8,
>>> +		.num_gpios = 2,
>>> +		.higher_gpio_bits = true,
>>> +		.has_temp = true,
>>> +		.has_vcom_input = true,
>>> +		.has_input_buf = true,
>>> +		.has_current_inputs = true,
>>> +		.has_int_ref = true,
>>> +		.clock = 2 * HZ_PER_MHZ,
>>> +		.sinc5_data_rates = ad7173_sinc5_data_rates,
>>> +		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>>> +	},
>>
>> At some point it would be nice to drop the ad7173_device_info array...
>>
> What are good alternatives to this?

Drivers like ad7091r8 have individual static struct ad7091r_init_info
instead of putting them all in an array. I like doing it that
way because it makes less code to read compared to having the
array.

It would let us get rid of enum ad7173_ids, have one level less
indent on each static const struct ad7173_device_info and 

{ .compatible = "adi,ad7172-2", .data = &ad7173_device_info },

would now fit on one line since we no longer need the array
index.

Re: [PATCH v3 5/6] iio: adc: ad7173: Add support for AD411x devices

[Nuno Sá]

On Wed, 2024-05-29 at 15:59 -0500, David Lechner wrote:
> On 5/29/24 9:03 AM, Ceclan, Dumitru wrote:
> > On 29/05/2024 15:46, Nuno Sá wrote:
> > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > 
> > ...
> > 
> > > >  static const struct ad7173_device_info ad7173_device_info[] = {
> > > > +	[ID_AD4111] = {
> > > > +		.name = "ad4111",
> > > > +		.id = AD7173_AD4111_AD4112_AD4114_ID,
> > > > +		.num_voltage_inputs_with_divider = 8,
> > > > +		.num_channels = 16,
> > > > +		.num_configs = 8,
> > > > +		.num_voltage_inputs = 8,
> > > > +		.num_gpios = 2,
> > > > +		.higher_gpio_bits = true,
> > > > +		.has_temp = true,
> > > > +		.has_vcom_input = true,
> > > > +		.has_input_buf = true,
> > > > +		.has_current_inputs = true,
> > > > +		.has_int_ref = true,
> > > > +		.clock = 2 * HZ_PER_MHZ,
> > > > +		.sinc5_data_rates = ad7173_sinc5_data_rates,
> > > > +		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> > > > +	},
> > > 
> > > At some point it would be nice to drop the ad7173_device_info array...
> > > 
> > What are good alternatives to this?
> 
> Drivers like ad7091r8 have individual static struct ad7091r_init_info
> instead of putting them all in an array. I like doing it that
> way because it makes less code to read compared to having the
> array.
> 
> It would let us get rid of enum ad7173_ids, have one level less
> indent on each static const struct ad7173_device_info and 
> 
> { .compatible = "adi,ad7172-2", .data = &ad7173_device_info },
> 
> would now fit on one line since we no longer need the array
> index.
> 

Exactly... But up to you to do it now or defer it to a later patch.

- Nuno Sá

Re: [PATCH v3 5/6] iio: adc: ad7173: Add support for AD411x devices

[Jonathan Cameron]

On Thu, 30 May 2024 08:19:57 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2024-05-29 at 15:59 -0500, David Lechner wrote:
> > On 5/29/24 9:03 AM, Ceclan, Dumitru wrote:  
> > > On 29/05/2024 15:46, Nuno Sá wrote:  
> > > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:  
> > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>  
> > > 
> > > ...
> > >   
> > > > >  static const struct ad7173_device_info ad7173_device_info[] = {
> > > > > +	[ID_AD4111] = {
> > > > > +		.name = "ad4111",
> > > > > +		.id = AD7173_AD4111_AD4112_AD4114_ID,
> > > > > +		.num_voltage_inputs_with_divider = 8,
> > > > > +		.num_channels = 16,
> > > > > +		.num_configs = 8,
> > > > > +		.num_voltage_inputs = 8,
> > > > > +		.num_gpios = 2,
> > > > > +		.higher_gpio_bits = true,
> > > > > +		.has_temp = true,
> > > > > +		.has_vcom_input = true,
> > > > > +		.has_input_buf = true,
> > > > > +		.has_current_inputs = true,
> > > > > +		.has_int_ref = true,
> > > > > +		.clock = 2 * HZ_PER_MHZ,
> > > > > +		.sinc5_data_rates = ad7173_sinc5_data_rates,
> > > > > +		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> > > > > +	},  
> > > > 
> > > > At some point it would be nice to drop the ad7173_device_info array...
> > > >   
> > > What are good alternatives to this?  
> > 
> > Drivers like ad7091r8 have individual static struct ad7091r_init_info
> > instead of putting them all in an array. I like doing it that
> > way because it makes less code to read compared to having the
> > array.
> > 
> > It would let us get rid of enum ad7173_ids, have one level less
> > indent on each static const struct ad7173_device_info and 
> > 
> > { .compatible = "adi,ad7172-2", .data = &ad7173_device_info },
> > 
> > would now fit on one line since we no longer need the array
> > index.
> >   
> 
> Exactly... But up to you to do it now or defer it to a later patch.
> 
Agreed - the array pattern for this is not a good idea as it
also encourages people to assign meaning to the enum values leaving
stuff expressed as code that should be a flag or value inside these
device_info structures.

Some of those where mistakes of a younger me ;(

Jonathan

> - Nuno Sá

[PATCH v3 6/6] iio: adc: ad7173: Reduce device info struct size

[Dumitru Ceclan via B4 Relay]

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Reduce the size used by the device info struct by packing the bool
 fields within the same byte. This reduces the struct size from 52 bytes
 to 44 bytes.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 328685ce25e0..e8357a21d513 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -179,15 +179,15 @@ struct ad7173_device_info {
 	unsigned int clock;
 	unsigned int id;
 	char *name;
-	bool has_current_inputs;
-	bool has_vcom_input;
-	bool has_temp;
+	bool has_current_inputs		:1;
+	bool has_vcom_input		:1;
+	bool has_temp			:1;
 	/* ((AVDD1 − AVSS)/5) */
-	bool has_common_input;
-	bool has_input_buf;
-	bool has_int_ref;
-	bool has_ref2;
-	bool higher_gpio_bits;
+	bool has_common_input		:1;
+	bool has_input_buf		:1;
+	bool has_int_ref		:1;
+	bool has_ref2			:1;
+	bool higher_gpio_bits		:1;
 	u8 num_gpios;
 };
 

-- 
2.43.0

Re: [PATCH v3 6/6] iio: adc: ad7173: Reduce device info struct size

[Nuno Sá]

On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Reduce the size used by the device info struct by packing the bool
>  fields within the same byte. This reduces the struct size from 52 bytes
>  to 44 bytes.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 328685ce25e0..e8357a21d513 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -179,15 +179,15 @@ struct ad7173_device_info {
>  	unsigned int clock;
>  	unsigned int id;
>  	char *name;
> -	bool has_current_inputs;
> -	bool has_vcom_input;
> -	bool has_temp;
> +	bool has_current_inputs		:1;
> +	bool has_vcom_input		:1;
> +	bool has_temp			:1;
>  	/* ((AVDD1 − AVSS)/5) */
> -	bool has_common_input;
> -	bool has_input_buf;
> -	bool has_int_ref;
> -	bool has_ref2;
> -	bool higher_gpio_bits;
> +	bool has_common_input		:1;
> +	bool has_input_buf		:1;
> +	bool has_int_ref		:1;
> +	bool has_ref2			:1;
> +	bool higher_gpio_bits		:1;
>  	u8 num_gpios;
>  };
>  
> 

This is really a very micro optimization... I would drop it tbh but no strong
feelings about it.

- Nuno Sá

Re: [PATCH v3 6/6] iio: adc: ad7173: Reduce device info struct size

[David Lechner]

On 5/29/24 7:23 AM, Nuno Sá wrote:
> On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>> Reduce the size used by the device info struct by packing the bool
>>  fields within the same byte. This reduces the struct size from 52 bytes
>>  to 44 bytes.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  drivers/iio/adc/ad7173.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 328685ce25e0..e8357a21d513 100644
>> --- a/drivers/iio/adc/ad7173.c
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -179,15 +179,15 @@ struct ad7173_device_info {
>>  	unsigned int clock;
>>  	unsigned int id;
>>  	char *name;
>> -	bool has_current_inputs;
>> -	bool has_vcom_input;
>> -	bool has_temp;
>> +	bool has_current_inputs		:1;
>> +	bool has_vcom_input		:1;
>> +	bool has_temp			:1;
>>  	/* ((AVDD1 − AVSS)/5) */
>> -	bool has_common_input;
>> -	bool has_input_buf;
>> -	bool has_int_ref;
>> -	bool has_ref2;
>> -	bool higher_gpio_bits;
>> +	bool has_common_input		:1;
>> +	bool has_input_buf		:1;
>> +	bool has_int_ref		:1;
>> +	bool has_ref2			:1;
>> +	bool higher_gpio_bits		:1;
>>  	u8 num_gpios;
>>  };
>>  
>>
> 
> This is really a very micro optimization... I would drop it tbh but no strong
> feelings about it.
> 
> - Nuno Sá

This only considers RAM size and not code size too. At least on ARM arch
every time we read or write to one of these fields, the code is now
implicitly `((field & 0x1) >> bits)` so two extra assembly instructions
for each read and write. This could be bigger than the size saved in
the structs.

Re: [PATCH v3 6/6] iio: adc: ad7173: Reduce device info struct size

[Nuno Sá]

On Wed, 2024-05-29 at 15:32 -0500, David Lechner wrote:
> On 5/29/24 7:23 AM, Nuno Sá wrote:
> > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > 
> > > Reduce the size used by the device info struct by packing the bool
> > >  fields within the same byte. This reduces the struct size from 52 bytes
> > >  to 44 bytes.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > ---
> > >  drivers/iio/adc/ad7173.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 328685ce25e0..e8357a21d513 100644
> > > --- a/drivers/iio/adc/ad7173.c
> > > +++ b/drivers/iio/adc/ad7173.c
> > > @@ -179,15 +179,15 @@ struct ad7173_device_info {
> > >  	unsigned int clock;
> > >  	unsigned int id;
> > >  	char *name;
> > > -	bool has_current_inputs;
> > > -	bool has_vcom_input;
> > > -	bool has_temp;
> > > +	bool has_current_inputs		:1;
> > > +	bool has_vcom_input		:1;
> > > +	bool has_temp			:1;
> > >  	/* ((AVDD1 − AVSS)/5) */
> > > -	bool has_common_input;
> > > -	bool has_input_buf;
> > > -	bool has_int_ref;
> > > -	bool has_ref2;
> > > -	bool higher_gpio_bits;
> > > +	bool has_common_input		:1;
> > > +	bool has_input_buf		:1;
> > > +	bool has_int_ref		:1;
> > > +	bool has_ref2			:1;
> > > +	bool higher_gpio_bits		:1;
> > >  	u8 num_gpios;
> > >  };
> > >  
> > > 
> > 
> > This is really a very micro optimization... I would drop it tbh but no strong
> > feelings about it.
> > 
> > - Nuno Sá
> 
> This only considers RAM size and not code size too. At least on ARM arch
> every time we read or write to one of these fields, the code is now
> implicitly `((field & 0x1) >> bits)` so two extra assembly instructions
> for each read and write. This could be bigger than the size saved in
> the structs.
> 
> 

very good point...

- Nuno Sá