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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 3 Jun 2024 12:46:10 +0300
"Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:

> On 01/06/2024 21:35, Jonathan Cameron wrote:
> > On Fri, 31 May 2024 22:42:27 +0300
> > Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >   
> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>
> >> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> >>
> >> AD411x family ADCs support a VCOM pin. The purpose of this pin is to
> >> offer a dedicated common-mode voltage input for single-ended channels.
> >> This pin is specified as supporting a differential channel with VIN10 on
> >> model AD4116.
> >>
> >> AD4111/AD4112 support current channels. Support is implemented using
> >> single-channel and "adi,current-channel".
> >>
> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>  
> > I like the common-mode-channel solution to the pseudo differential
> > description. It makes things explicit whilst avoiding an ugly differential
> > but not differential mess.
> > 
> > However, it feels like a general thing to me not a vendor specific one.
> > Perhaps makes sense to put in adc.yaml?
> >   
> 
> Sure
> 
> > One other question that is more me being curious and failing to understand
> > the datasheet than a request to change anything.  
> >> ---
> >>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 192 ++++++++++++++++++++-
> >>  1 file changed, 190 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..d8474eee553e 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,54 @@ patternProperties:
> >>          maximum: 15
> >>  
> >>        diff-channels:
> >> +        description: |
> >> +          This property is used for defining the inputs of a differential
> >> +          voltage channel. The first value is the positive input and the second
> >> +          value is the negative input of the channel.
> >> +
> >> +          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)−  
> > 
> > That's what it says on the datasheet (so fine to copy that here) but I'm curious, what does
> > that mean in practice?  How can we have negative and postive signals of the difference
> > between two power supply voltages where I'm fairly sure AVDD1 always greater than AVSS.
> >  
> 
> I have not tested that as I do not have a model that supports this wired up.
> If I had to guess they are the same signal but one should be connected to the
> positive input, one to the negative input...but I could be wrong.

If they are, then as far as I we are concerned is this one channel with two
representations depending on whether it is 1st or 2nd in the list?
Can we use one number and hide that detail in the driver?

Seems odd though if that is the case.

I guess if we find out later this is the case we can tighten the binding to
enforce the right one instead of squashing them to one value, but that
is a bit ugly.  Any chance of digging out the info?  If not we can go ahead
but ideally answering things like this make a our life easier in the long run.

Jonathan


>  
> > Anyhow, that's a problem for the person reading the datasheet to figure out :)
> >    
> >> +
> >>          items:
> >>            minimum: 0
> >>            maximum: 31
> >>  
> >> +      single-channel:
> >> +        description: |
> >> +          This property is used for defining a current channel or the positive
> >> +          input of a voltage channel (single-ended or pseudo-differential).
> >> +
> >> +          Models AD4111 and AD4112 support current channels.
> >> +            Example: (IIN2+, IIN2−) -> single-channel = <2>
> >> +          To correctly configure a current channel set the "adi,current-channel"
> >> +          property to true.
> >> +
> >> +          To configure a single-ended/pseudo-differential channel set the
> >> +          "adi,common-mode-channel" property to the desired negative voltage input.
> >> +
> >> +          When used as a voltage channel, special inputs are valid as well.
> >> +        minimum: 0
> >> +        maximum: 31
> >> +
> >> +      adi,common-mode-channel:
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        description:
> >> +          This property is used for defining the negative input of a
> >> +          single-ended or pseudo-differential voltage channel.
> >> +
> >> +          Special inputs are valid as well.
> >> +        minimum: 0
> >> +        maximum: 31
> >> +  
> >   
>