Message-ID: <CAMknhBHeKAQ45=5-dL1T1tv-mZcPN+bNo3vxWJYgWpEPE+8p3Q@mail.gmail.com>
Date: Mon, 1 Apr 2024 14:37:50 -0500
From: David Lechner <dlechner@...libre.com>
To: dumitru.ceclan@...log.com
Cc: 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>, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Dumitru Ceclan <mitrutzceclan@...il.com>
Subject: Re: [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x

On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
<devnull+dumitru.ceclan.analog.com@...nel.org> 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    | 59 +++++++++++++++++++++-
>  1 file changed, 57 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..bba2de0a52f3 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
> @@ -125,10 +141,19 @@ patternProperties:
>
>      properties:
>        reg:
> +        description:
> +          Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>          minimum: 0
> -        maximum: 15
> +        maximum: 19

This looks wrong. Isn't reg describing the number of logical channels
(# of channel config registers)?

After reviewing the driver, I see that > 16 is used as a way of
flagging current inputs, but still seems like the wrong way to do it.
See suggestion below.

>
>        diff-channels:
> +        description:
> +          For using current channels specify only the positive channel.
> +            (IIN2+, IIN2−) -> diff-channels = <2 0>

I find this a bit confusing since 2 is already VIN2 and 0 is already
VIN0. I think it would make more sense to assign unique channel
numbers individually to the negative and positive current inputs.
Also, I think it makes sense to use the same numbers that the
registers in the datasheet use (8 - 11 for negative and 12 to 15 for
positive).

So: (IIN2+, IIN2−) -> diff-channels = <13 10>


> +
> +          Family AD411x supports a dedicated VCOM voltage input.
> +          To select it set the second channel to 16.
> +            (VIN2, VCOM) -> diff-channels = <2 16>

The 411x datasheets call this pin VINCOM so calling it VCOM here is a
bit confusing.

Also, do we need to add a vincom-supply to get this voltage? Or is it
safe to assume it is always connected to AVSS? The datasheet seems to
indicate that the latter is the case. But then it also has this
special case (at least for AD4116, didn't check all datasheets)
"VIN10, VINCOM (single-ended or differential pair)". If it can be used
as part of a fully differential input, we probably need some extra
flag to indicate that case.

Similarly, do we need special handling for ADCIN15 on AD4116? It has a
"(pseudo differential or differential pair)" notation that other
inputs don't. In other words, it is more like VINCOM than it is to the
other ADCINxx pins. So we probably need an adcin15-supply for this pin
to properly get the right channel configuration. I.e. the logic in the
IIO driver would be if adcin15-supply is present, any channels that
use this input are pseudo-differential, otherwise any channels that
use it are fully differential.

>          items:
>            minimum: 0
>            maximum: 31
> @@ -166,7 +191,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

Did you forget to remove

            reg:
              maximum: 3

from this if statement that this comment is referring to?


>    - if:
>        properties:
>          compatible:
> @@ -187,6 +211,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

As with the previous reg comment, this if statement should not be
needed since maximum should not be changed to 19.

> +
> +  - 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

It looks to me like AD7172-4 actually has 8 possible channels rather
than 16. So it would need a special condition as well. But that is a
bug in the previous bindings and should therefore be fixed in a
separate patch.

Powered by blists - more mailing lists