Message-ID: <20240528-filtrate-cloning-b9152322a3da@spud>
Date: Tue, 28 May 2024 18:52:46 +0100
From: Conor Dooley <conor@...nel.org>
To: "Ceclan, Dumitru" <mitrutzceclan@...il.com>
Cc: dumitru.ceclan@...log.com, Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	David Lechner <dlechner@...libre.com>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

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@...log.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@...log.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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists