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

[David Lechner <dlechner@baylibre.com>]

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.