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

["Ceclan, Dumitru" <mitrutzceclan@gmail.com>]

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.