diff mbox series

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

Message ID 20240401-ad4111-v1-1-34618a9cc502@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for AD411x | expand

Commit Message

Dumitru Ceclan via B4 Relay April 1, 2024, 3:32 p.m. UTC
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    | 59 +++++++++++++++++++++-
 1 file changed, 57 insertions(+), 2 deletions(-)

Comments

David Lechner April 1, 2024, 7:37 p.m. UTC | #1
On Mon, Apr 1, 2024 at 10:10 AM 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, 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    | 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.
David Lechner April 1, 2024, 8:22 p.m. UTC | #2
On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Mon, Apr 1, 2024 at 10:10 AM 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, 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    | 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

Also, I just noticed that AD411x have only one AVDD input instead of
AVDD1 and AVDD2. So we need an if statement that says if properties:
compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
David Lechner April 1, 2024, 9:16 p.m. UTC | #3
On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Mon, Apr 1, 2024 at 10:10 AM 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, 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>
> > ---

...

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

Thinking about this a bit more...

Since the current inputs have dedicated pins and aren't mix-and-match
with multiple valid wiring configurations like the voltage inputs, do
we even need to describe them in the devicetree?

In the driver, the current channels would just be hard-coded like the
temperature channel since there isn't any application-specific
variation.
Ceclan, Dumitru April 3, 2024, 7:43 a.m. UTC | #4
On 01/04/2024 22:37, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>

...

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

This was a suggestion from Jonathan, maybe I implemented it wrong.
Other alternative that came to my mind: attribute "adi,current-channel".
>>
>>        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>
> 
> 
It would mean for the user to look in the datasheet at the possible
channel INPUT configurations values, decode the bit field into two
integer values and place it here (0110101010) -> 13 10. This is
cumbersome for just choosing current input 2.

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

Sure, I'll rename to VINCOM.

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

I cannot see any configuration options for these use cases. All inputs
are routed to the same mux and routed to the differential positive and
negative ADC inputs.

"VIN10, VINCOM (single-ended or differential pair)" the only difference
between these two use cases is if you connected VINCOM to AVSS (with
unipolar coding) or not with bipolar encoding. The channel is still
measuring the difference between the two selected inputs and comparing
to the selected reference.

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

I cannot seem to understand what would a adcin15-supply be needed for.
This input, the same as all others, enters the mux and is routed to
either positive or negative input of the ADC.

The voltage on the ADCIN15 pin is not important to the user, just the
difference in voltage between that pin and the other one selected.

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


Other way around, forgot in a previous patch to remove the comment.
I'll move this change to a precursor patch.

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

We'll see what is the best approach regarding the current channels,
perhaps the one you mentioned in the later reply with always configuring
like the temp channel.

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

It is addressed already in the binding:
"
  - if:
      properties:
        compatible:
          contains:
            const: adi,ad7172-4
[...]
              maximum: 7
"
Ceclan, Dumitru April 3, 2024, 7:45 a.m. UTC | #5
On 01/04/2024 23:22, David Lechner wrote:
> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>>
>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:

...

> 
> Also, I just noticed that AD411x have only one AVDD input instead of
> AVDD1 and AVDD2. So we need an if statement that says if properties:
> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.

Already addressed by this:
"
  # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
  - if:
      properties:
        compatible:
          not:
            contains:
              enum:
                - adi,ad7172-4
                - adi,ad7173-8
                - adi,ad7175-8
    then:
      properties:
        vref2-supply: false
      patternProperties:
        "^channel@[0-9a-f]$":
          properties:
            adi,reference-select:
              enum:
                - vref
                - refout-avss
                - avdd
"
Ceclan, Dumitru April 3, 2024, 7:50 a.m. UTC | #6
On 02/04/2024 00:16, David Lechner wrote:
> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>>
>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>>
>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>

...

>>>
>>>      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>
> 
> Thinking about this a bit more...
> 
> Since the current inputs have dedicated pins and aren't mix-and-match
> with multiple valid wiring configurations like the voltage inputs, do
> we even need to describe them in the devicetree?
> 
> In the driver, the current channels would just be hard-coded like the
> temperature channel since there isn't any application-specific
> variation.

 Sure, but we still need to offer the user a way to configure which
current inputs he wants and if they should use bipolar or unipolar coding.

 One other issue that arises is the fact that we will reserve 5
(including temp) channels out of the 15 that the user has the option to
configure. While the binding will configure only 11 channels for
example, the driver probe will error out with the message "Too many
channels specified".
Ceclan, Dumitru April 3, 2024, 10:08 a.m. UTC | #7
On 03/04/2024 10:45, Ceclan, Dumitru wrote:
> On 01/04/2024 23:22, David Lechner wrote:
>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>>>
>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> 
> ...
> 
>>
>> Also, I just noticed that AD411x have only one AVDD input instead of
>> AVDD1 and AVDD2. So we need an if statement that says if properties:
>> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
> 
> Already addressed by this:
> "
>   # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
>   - if:
>       properties:
>         compatible:
>           not:
>             contains:
>               enum:
>                 - adi,ad7172-4
>                 - adi,ad7173-8
>                 - adi,ad7175-8
>     then:
>       properties:
>         vref2-supply: false
>       patternProperties:
>         "^channel@[0-9a-f]$":
>           properties:
>             adi,reference-select:
>               enum:
>                 - vref
>                 - refout-avss
>                 - avdd
> "

Mistaken vref2-supply to avdd2-supply.

But still, the presence of avdd2-supply does not influence anything at all.
Driver does not use it, you cannot select it for channel conversions.
Would a restriction like this really be required?
David Lechner April 3, 2024, 3:14 p.m. UTC | #8
On Wed, Apr 3, 2024 at 5:08 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>
> On 03/04/2024 10:45, Ceclan, Dumitru wrote:
> > On 01/04/2024 23:22, David Lechner wrote:
> >> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
> >>>
> >>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >
> > ...
> >
> >>
> >> Also, I just noticed that AD411x have only one AVDD input instead of
> >> AVDD1 and AVDD2. So we need an if statement that says if properties:
> >> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
> >
> > Already addressed by this:
> > "
> >   # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
> >   - if:
> >       properties:
> >         compatible:
> >           not:
> >             contains:
> >               enum:
> >                 - adi,ad7172-4
> >                 - adi,ad7173-8
> >                 - adi,ad7175-8
> >     then:
> >       properties:
> >         vref2-supply: false
> >       patternProperties:
> >         "^channel@[0-9a-f]$":
> >           properties:
> >             adi,reference-select:
> >               enum:
> >                 - vref
> >                 - refout-avss
> >                 - avdd
> > "
>
> Mistaken vref2-supply to avdd2-supply.
>
> But still, the presence of avdd2-supply does not influence anything at all.
> Driver does not use it, you cannot select it for channel conversions.
> Would a restriction like this really be required?

It is true that it is not likely to cause any problems we don't fix
this but why would we want the bindings to intentionally be incorrect
when there is an easy fix? Driver implementations should not influence
leaving something out of the bindings [1].

[1]: https://www.kernel.org/doc/html//v5.10/devicetree/bindings/writing-bindings.html#overall-design
David Lechner April 3, 2024, 3:22 p.m. UTC | #9
On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>
> On 02/04/2024 00:16, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
> >>
> >> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >>>
> >>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>>
>
> ...
>
> >>>
> >>>      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>
> >
> > Thinking about this a bit more...
> >
> > Since the current inputs have dedicated pins and aren't mix-and-match
> > with multiple valid wiring configurations like the voltage inputs, do
> > we even need to describe them in the devicetree?
> >
> > In the driver, the current channels would just be hard-coded like the
> > temperature channel since there isn't any application-specific
> > variation.
>
>  Sure, but we still need to offer the user a way to configure which
> current inputs he wants and if they should use bipolar or unipolar coding.

From the datasheet, it looks like only positive current input is
allowed so I'm not sure bipolar applies here. But, yes, if there is
some other variation in wiring or electrical signal that needs to be
describe here, then it makes sense to allow a channel configuration
node for it.

>
>  One other issue that arises is the fact that we will reserve 5
> (including temp) channels out of the 15 that the user has the option to
> configure. While the binding will configure only 11 channels for
> example, the driver probe will error out with the message "Too many
> channels specified".
>

Surely the driver could be changed to fix this, if needed. :-)
David Lechner April 3, 2024, 3:40 p.m. UTC | #10
On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>
> On 01/04/2024 22:37, David Lechner wrote:
> > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> >>
> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>
> ...
>
> >>      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.
> >
>
> This was a suggestion from Jonathan, maybe I implemented it wrong.
> Other alternative that came to my mind: attribute "adi,current-channel".

Having a boolean flag like this would make more sense to me if we
don't agree that the suggestion below is simpler.

> >>
> >>        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>
> >
> >
> It would mean for the user to look in the datasheet at the possible
> channel INPUT configurations values, decode the bit field into two
> integer values and place it here (0110101010) -> 13 10. This is
> cumbersome for just choosing current input 2.

It could be documented in the devicetree bindings, just as it is done
in adi,ad4130.yaml so that users of the bindings don't have to
decipher the datasheet.

>
> >> +
> >> +          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.
> >
>
> Sure, I'll rename to VINCOM.
>
> > 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.
> >
>
> I cannot see any configuration options for these use cases. All inputs
> are routed to the same mux and routed to the differential positive and
> negative ADC inputs.
>
> "VIN10, VINCOM (single-ended or differential pair)" the only difference
> between these two use cases is if you connected VINCOM to AVSS (with
> unipolar coding) or not with bipolar encoding. The channel is still
> measuring the difference between the two selected inputs and comparing
> to the selected reference.
>
> > 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.
> >
>
> I cannot seem to understand what would a adcin15-supply be needed for.
> This input, the same as all others, enters the mux and is routed to
> either positive or negative input of the ADC.
>
> The voltage on the ADCIN15 pin is not important to the user, just the
> difference in voltage between that pin and the other one selected.
>

These suggestions come from some recent discussion about
pseudo-differential vs. fully differential inputs (e.g. search the IIO
mailing list for AD7380).

So what I suggested here might be more technically correct according
to what I got out of that discussion. But for this specific case, I
agree it is good enough to just treat all inputs as always
fully-differential to keep things from getting too unwieldy.

> >>          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?
> >
> >
>
>
> Other way around, forgot in a previous patch to remove the comment.
> I'll move this change to a precursor patch.
>
> >>    - 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.
> >
>
> We'll see what is the best approach regarding the current channels,
> perhaps the one you mentioned in the later reply with always configuring
> like the temp channel.
>
> >> +
> >> +  - 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.
>
> It is addressed already in the binding:
> "
>   - if:
>       properties:
>         compatible:
>           contains:
>             const: adi,ad7172-4
> [...]
>               maximum: 7
> "

Ah, I missed it hiding with adi,reference-select overrides.
Ceclan, Dumitru April 4, 2024, 1:08 p.m. UTC | #11
On 03/04/2024 18:22, David Lechner wrote:
> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>> On 02/04/2024 00:16, David Lechner wrote:
>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>>
>> ...
>>
>>>>>      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>
>>> Thinking about this a bit more...
>>>
>>> Since the current inputs have dedicated pins and aren't mix-and-match
>>> with multiple valid wiring configurations like the voltage inputs, do
>>> we even need to describe them in the devicetree?
>>>
>>> In the driver, the current channels would just be hard-coded like the
>>> temperature channel since there isn't any application-specific
>>> variation.
>>  Sure, but we still need to offer the user a way to configure which
>> current inputs he wants and if they should use bipolar or unipolar coding.
> From the datasheet, it looks like only positive current input is
> allowed so I'm not sure bipolar applies here. But, yes, if there is
> some other variation in wiring or electrical signal that needs to be
> describe here, then it makes sense to allow a channel configuration
> node for it.

AD4111 datasheet pg.29:
When the ADC is configured for bipolar operation, the output
code is offset binary with a negative full-scale voltage resulting
in a code of 000 … 000, a zero differential input voltage resulting in
a code of 100 … 000, and a positive full-scale input voltage
resulting in a code of 111 … 111. The output code for any
analog input voltage can be represented as
Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
The output code for any input current is represented as
Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)

I would say bipolar applies here, not a great idea because of the limitation on
 the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
 is available.
Jonathan Cameron April 6, 2024, 2:26 p.m. UTC | #12
On Thu, 4 Apr 2024 16:08:56 +0300
"Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:

> On 03/04/2024 18:22, David Lechner wrote:
> > On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:  
> >> On 02/04/2024 00:16, David Lechner wrote:  
> >>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:  
> >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:  
> >>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>>>>  
> >> ...
> >>  
> >>>>>      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>  
> >>> Thinking about this a bit more...
> >>>
> >>> Since the current inputs have dedicated pins and aren't mix-and-match
> >>> with multiple valid wiring configurations like the voltage inputs, do
> >>> we even need to describe them in the devicetree?
> >>>
> >>> In the driver, the current channels would just be hard-coded like the
> >>> temperature channel since there isn't any application-specific
> >>> variation.  
> >>  Sure, but we still need to offer the user a way to configure which
> >> current inputs he wants and if they should use bipolar or unipolar coding.  
> > From the datasheet, it looks like only positive current input is
> > allowed so I'm not sure bipolar applies here. But, yes, if there is
> > some other variation in wiring or electrical signal that needs to be
> > describe here, then it makes sense to allow a channel configuration
> > node for it.  
> 
> AD4111 datasheet pg.29:
> When the ADC is configured for bipolar operation, the output
> code is offset binary with a negative full-scale voltage resulting
> in a code of 000 … 000, a zero differential input voltage resulting in
> a code of 100 … 000, and a positive full-scale input voltage
> resulting in a code of 111 … 111. The output code for any
> analog input voltage can be represented as
> Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
> The output code for any input current is represented as
> Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
> 
> I would say bipolar applies here, not a great idea because of the limitation on
>  the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
>  is available.
Just to check I am correct in thinking you 'might' use bipolar if you want
to be able to measure small negative currents, but the range is much larger
in the positive direction?

J
>
Jonathan Cameron April 6, 2024, 2:53 p.m. UTC | #13
On Wed, 3 Apr 2024 10:40:39 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
> >
> > On 01/04/2024 22:37, David Lechner wrote:  
> > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:  
> > >>
> > >> From: Dumitru Ceclan <dumitru.ceclan@analog.com>  
> >
> > ...
> >  
> > >>      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.
> > >  
> >
> > This was a suggestion from Jonathan, maybe I implemented it wrong.

Maybe Jonathan was wrong!  I was younger then than now :)

However, reg values for child nodes are unique so can't just use a flag these
need to be different values.

> > Other alternative that came to my mind: attribute "adi,current-channel".  
> 
> Having a boolean flag like this would make more sense to me if we
> don't agree that the suggestion below is simpler.
> 
> > >>
> > >>        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>
> > >
> > >  
> > It would mean for the user to look in the datasheet at the possible
> > channel INPUT configurations values, decode the bit field into two
> > integer values and place it here (0110101010) -> 13 10. This is
> > cumbersome for just choosing current input 2.  
> 
> It could be documented in the devicetree bindings, just as it is done
> in adi,ad4130.yaml so that users of the bindings don't have to
> decipher the datasheet.

The <13 10> option makes sense to me and avoids suggesting a common negative
input.

The 'fun' bit here is that diff-channels doesn't actually tell us anything.
So we could just not provide it and rely on documentation of reg = 16-19 meaning
the current channels?

> 
> >  
> > >> +
> > >> +          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.
> > >  
> >
> > Sure, I'll rename to VINCOM.
> >  
> > > 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.
> > >  
> >
> > I cannot see any configuration options for these use cases. All inputs
> > are routed to the same mux and routed to the differential positive and
> > negative ADC inputs.
> >
> > "VIN10, VINCOM (single-ended or differential pair)" the only difference
> > between these two use cases is if you connected VINCOM to AVSS (with
> > unipolar coding) or not with bipolar encoding. The channel is still
> > measuring the difference between the two selected inputs and comparing
> > to the selected reference.
> >  
> > > 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.
> > >  
> >
> > I cannot seem to understand what would a adcin15-supply be needed for.
> > This input, the same as all others, enters the mux and is routed to
> > either positive or negative input of the ADC.
> >
> > The voltage on the ADCIN15 pin is not important to the user, just the
> > difference in voltage between that pin and the other one selected.
> >  
> 
> These suggestions come from some recent discussion about
> pseudo-differential vs. fully differential inputs (e.g. search the IIO
> mailing list for AD7380).
> 
> So what I suggested here might be more technically correct according
> to what I got out of that discussion. But for this specific case, I
> agree it is good enough to just treat all inputs as always
> fully-differential to keep things from getting too unwieldy.

Hmm.  That whole approach to pseudo differential does get messy if
we have the common line routed through the main MUX rather than an opt
in only on the negative side.  

If I read this right, its almost a trick to support a pseudo differential
wiring with simple registers (I guess reflecting MUX limitations).

So what could we do?

We could assume that VINCOM is used like a conventional pseudo
differential negative signal and have supply-vincom + non diffferential
channels if that's the configuration wanted.

Then for differential channels can support all the VINX VINX+1
and swapped options.
For VIN10 it gets fun as non differential and differential options
I think map to same actual config.   Don't see reason we need to express
that in the binding though so let that have VIN10 VINCOM (probably using
a magic channel number) and  VIN10 pseudo differential.

Similar setup for ADCIN15 equivalent usage

Code wise this probably won't be particular hard to support in the driver
(obviously I haven't tried though :) is it worth the effort to keep
it inline with other devices that support pseudo differential channesl.


> 
> > >>          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?
> > >
> > >  
> >
> >
> > Other way around, forgot in a previous patch to remove the comment.
> > I'll move this change to a precursor patch.
> >  
> > >>    - 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.
> > >  
> >
> > We'll see what is the best approach regarding the current channels,
> > perhaps the one you mentioned in the later reply with always configuring
> > like the temp channel.
> >  
That's an option as well.  In many early drivers we just provided all the
channels. Somewhat depends on whether people buy devices with lots of
channels they don't wire.  Mostly I suspect they don't as that's money
wasted!

Jonathan
Ceclan, Dumitru April 9, 2024, 8:08 a.m. UTC | #14
On 06/04/2024 17:53, Jonathan Cameron wrote:
> On Wed, 3 Apr 2024 10:40:39 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>>>
>>> On 01/04/2024 22:37, David Lechner wrote:  
>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:  
>>>>>
>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>  
>>>
>>> ...
>>>  
>>>>>      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.
>>>>  
>>>
>>> This was a suggestion from Jonathan, maybe I implemented it wrong.
> 
> Maybe Jonathan was wrong!  I was younger then than now :)
> 
> However, reg values for child nodes are unique so can't just use a flag these
> need to be different values.
> 

I do not see where the restriction appears when using just the flag, when defining
the channels you would still define unique reg values.

>>> Other alternative that came to my mind: attribute "adi,current-channel".  
>>
>> Having a boolean flag like this would make more sense to me if we
>> don't agree that the suggestion below is simpler.
>>
>>>>>
>>>>>        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>
>>>>
>>>>  
>>> It would mean for the user to look in the datasheet at the possible
>>> channel INPUT configurations values, decode the bit field into two
>>> integer values and place it here (0110101010) -> 13 10. This is
>>> cumbersome for just choosing current input 2.  
>>
>> It could be documented in the devicetree bindings, just as it is done
>> in adi,ad4130.yaml so that users of the bindings don't have to
>> decipher the datasheet.
> 
> The <13 10> option makes sense to me and avoids suggesting a common negative
> input.
> 
> The 'fun' bit here is that diff-channels doesn't actually tell us anything.
> So we could just not provide it and rely on documentation of reg = 16-19 meaning
> the current channels?
> 

So a channel without diff-channels defined and reg=16 means IN0+ IN0-?

>>
>>>  
>>>>> +
>>>>> +          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.
>>>>  
>>>
>>> Sure, I'll rename to VINCOM.
>>>  
>>>> 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.
>>>>  
>>>
>>> I cannot see any configuration options for these use cases. All inputs
>>> are routed to the same mux and routed to the differential positive and
>>> negative ADC inputs.
>>>
>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
>>> between these two use cases is if you connected VINCOM to AVSS (with
>>> unipolar coding) or not with bipolar encoding. The channel is still
>>> measuring the difference between the two selected inputs and comparing
>>> to the selected reference.
>>>  
>>>> 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.
>>>>  
>>>
>>> I cannot seem to understand what would a adcin15-supply be needed for.
>>> This input, the same as all others, enters the mux and is routed to
>>> either positive or negative input of the ADC.
>>>
>>> The voltage on the ADCIN15 pin is not important to the user, just the
>>> difference in voltage between that pin and the other one selected.
>>>  
>>
>> These suggestions come from some recent discussion about
>> pseudo-differential vs. fully differential inputs (e.g. search the IIO
>> mailing list for AD7380).
>>
>> So what I suggested here might be more technically correct according
>> to what I got out of that discussion. But for this specific case, I
>> agree it is good enough to just treat all inputs as always
>> fully-differential to keep things from getting too unwieldy.
> 
> Hmm.  That whole approach to pseudo differential does get messy if
> we have the common line routed through the main MUX rather than an opt
> in only on the negative side.  
> 
> If I read this right, its almost a trick to support a pseudo differential
> wiring with simple registers (I guess reflecting MUX limitations).
> 
> So what could we do?
> 
> We could assume that VINCOM is used like a conventional pseudo
> differential negative signal and have supply-vincom + non diffferential
> channels if that's the configuration wanted.
> 
> Then for differential channels can support all the VINX VINX+1
> and swapped options.
> For VIN10 it gets fun as non differential and differential options
> I think map to same actual config.   Don't see reason we need to express
> that in the binding though so let that have VIN10 VINCOM (probably using
> a magic channel number) and  VIN10 pseudo differential.
> 
> Similar setup for ADCIN15 equivalent usage
> 
> Code wise this probably won't be particular hard to support in the driver
> (obviously I haven't tried though :) is it worth the effort to keep
> it inline with other devices that support pseudo differential channesl.

Then this would need to be done to any fully differential ADC as support
for pseudo differential channels is present (connect a fixed non 0 voltage
to the negative input).

The AD717x family supports pseudo differential channels as well... should
this change be applied to them too? It is just the case that the documentation
does not mentions this use case.

I think that a distinction needs to be made here:
- When a device is only pseudo differential, sure, it is in a different category
- When a device is fully differential and you are using it as pseudo-differential
  you are having two inputs compared to one another

I would need more clarification is why would supply-vincom be a requirement.
The voltage supplied to VINCOM will not be used in any computation within 
the driver. From the perspective of getting the data it doesn't matter if 
you are using the channel in a pseudo-differential, single ended or fully
differential manner.

Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
"Due to the matching resistors on the analog front end, the
 differential inputs must be paired together in the following
 pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
 VIN6 and VIN7. If any two voltage inputs are paired in a
 configuration other than what is described in this data sheet,
 the accuracy of the device cannot be guaranteed."

Tried the device and it works as fully differential when pairing any
VINx with VINCOM. Still works when selecting VINCOM as the positive
input of the ADC.

I really see this as overly complicated and unnecessary. These families
of ADCs are fully differential. If you are using it to measure a single ended
(Be it compared to 0V or pseudo differential where you are comparing to Vref/2
and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
the common voltage.
Ceclan, Dumitru April 9, 2024, 8:10 a.m. UTC | #15
On 06/04/2024 17:26, Jonathan Cameron wrote:
> On Thu, 4 Apr 2024 16:08:56 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> 
>> On 03/04/2024 18:22, David Lechner wrote:
>>> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:  
>>>> On 02/04/2024 00:16, David Lechner wrote:  
>>>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:  
>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:  
>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>>>>  
>>>> ...
>>>>  
>>>>>>>      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>  
>>>>> Thinking about this a bit more...
>>>>>
>>>>> Since the current inputs have dedicated pins and aren't mix-and-match
>>>>> with multiple valid wiring configurations like the voltage inputs, do
>>>>> we even need to describe them in the devicetree?
>>>>>
>>>>> In the driver, the current channels would just be hard-coded like the
>>>>> temperature channel since there isn't any application-specific
>>>>> variation.  
>>>>  Sure, but we still need to offer the user a way to configure which
>>>> current inputs he wants and if they should use bipolar or unipolar coding.  
>>> From the datasheet, it looks like only positive current input is
>>> allowed so I'm not sure bipolar applies here. But, yes, if there is
>>> some other variation in wiring or electrical signal that needs to be
>>> describe here, then it makes sense to allow a channel configuration
>>> node for it.  
>>
>> AD4111 datasheet pg.29:
>> When the ADC is configured for bipolar operation, the output
>> code is offset binary with a negative full-scale voltage resulting
>> in a code of 000 … 000, a zero differential input voltage resulting in
>> a code of 100 … 000, and a positive full-scale input voltage
>> resulting in a code of 111 … 111. The output code for any
>> analog input voltage can be represented as
>> Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
>> The output code for any input current is represented as
>> Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
>>
>> I would say bipolar applies here, not a great idea because of the limitation on
>>  the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
>>  is available.
> Just to check I am correct in thinking you 'might' use bipolar if you want
> to be able to measure small negative currents, but the range is much larger
> in the positive direction?
> 
> J

Yes, exactly
Jonathan Cameron April 13, 2024, 10:49 a.m. UTC | #16
On Tue, 9 Apr 2024 11:08:28 +0300
"Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:

> On 06/04/2024 17:53, Jonathan Cameron wrote:
> > On Wed, 3 Apr 2024 10:40:39 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:  
> >>>
> >>> On 01/04/2024 22:37, David Lechner wrote:    
> >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:    
> >>>>>
> >>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>    
> >>>
> >>> ...
> >>>    
> >>>>>      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.
> >>>>    
> >>>
> >>> This was a suggestion from Jonathan, maybe I implemented it wrong.  
> > 
> > Maybe Jonathan was wrong!  I was younger then than now :)
> > 
> > However, reg values for child nodes are unique so can't just use a flag these
> > need to be different values.
> >   
> 
> I do not see where the restriction appears when using just the flag, when defining
> the channels you would still define unique reg values.

Good point. I'd got it into my head we had reg matching the channel index
but that doesn't work anyway because those can repeat.   Sorry for misdirection!

> 
> >>> Other alternative that came to my mind: attribute "adi,current-channel".    
> >>
> >> Having a boolean flag like this would make more sense to me if we
> >> don't agree that the suggestion below is simpler.
> >>  
> >>>>>
> >>>>>        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>
> >>>>
> >>>>    
> >>> It would mean for the user to look in the datasheet at the possible
> >>> channel INPUT configurations values, decode the bit field into two
> >>> integer values and place it here (0110101010) -> 13 10. This is
> >>> cumbersome for just choosing current input 2.    
> >>
> >> It could be documented in the devicetree bindings, just as it is done
> >> in adi,ad4130.yaml so that users of the bindings don't have to
> >> decipher the datasheet.  
> > 
> > The <13 10> option makes sense to me and avoids suggesting a common negative
> > input.
> > 
> > The 'fun' bit here is that diff-channels doesn't actually tell us anything.
> > So we could just not provide it and rely on documentation of reg = 16-19 meaning
> > the current channels?
> >   
> 
> So a channel without diff-channels defined and reg=16 means IN0+ IN0-?

Yes, but with you correcting my error above maybe this isn't as clear cut as
I'd falsely recalled.

We do directly relate reg to channel numbers in drivers like the ad7292 (where not
all channels are differential)  I'm not convinced either way on what is best
here where reg is currently just an index into a channel specification, not
meaningful for which pins are involved.

It doesn't seem worth adding an equivalent of diff-channels for a single channel
setup but I guess it would be more consistent.

> 
> >>  
> >>>    
> >>>>> +
> >>>>> +          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.
> >>>>    
> >>>
> >>> Sure, I'll rename to VINCOM.
> >>>    
> >>>> 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.
> >>>>    
> >>>
> >>> I cannot see any configuration options for these use cases. All inputs
> >>> are routed to the same mux and routed to the differential positive and
> >>> negative ADC inputs.
> >>>
> >>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
> >>> between these two use cases is if you connected VINCOM to AVSS (with
> >>> unipolar coding) or not with bipolar encoding. The channel is still
> >>> measuring the difference between the two selected inputs and comparing
> >>> to the selected reference.
> >>>    
> >>>> 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.
> >>>>    
> >>>
> >>> I cannot seem to understand what would a adcin15-supply be needed for.
> >>> This input, the same as all others, enters the mux and is routed to
> >>> either positive or negative input of the ADC.
> >>>
> >>> The voltage on the ADCIN15 pin is not important to the user, just the
> >>> difference in voltage between that pin and the other one selected.
> >>> 

That statement is the root of disagreement I think.
If they are wired for pseudo differential measurement ADCIN15 a reference voltage
not a varying signal. It can equally be used as a negative channel of
a differential pair.   Not different from point of view of hardware
config, but potentially different from point of view of how the
analog wiring is done and how we may want to present it to userspace.

> >>
> >> These suggestions come from some recent discussion about
> >> pseudo-differential vs. fully differential inputs (e.g. search the IIO
> >> mailing list for AD7380).
> >>
> >> So what I suggested here might be more technically correct according
> >> to what I got out of that discussion. But for this specific case, I
> >> agree it is good enough to just treat all inputs as always
> >> fully-differential to keep things from getting too unwieldy.  
> > 
> > Hmm.  That whole approach to pseudo differential does get messy if
> > we have the common line routed through the main MUX rather than an opt
> > in only on the negative side.  
> > 
> > If I read this right, its almost a trick to support a pseudo differential
> > wiring with simple registers (I guess reflecting MUX limitations).
> > 
> > So what could we do?
> > 
> > We could assume that VINCOM is used like a conventional pseudo
> > differential negative signal and have supply-vincom + non diffferential
> > channels if that's the configuration wanted.
> > 
> > Then for differential channels can support all the VINX VINX+1
> > and swapped options.
> > For VIN10 it gets fun as non differential and differential options
> > I think map to same actual config.   Don't see reason we need to express
> > that in the binding though so let that have VIN10 VINCOM (probably using
> > a magic channel number) and  VIN10 pseudo differential.
> > 
> > Similar setup for ADCIN15 equivalent usage
> > 
> > Code wise this probably won't be particular hard to support in the driver
> > (obviously I haven't tried though :) is it worth the effort to keep
> > it inline with other devices that support pseudo differential channesl.  
> 
> Then this would need to be done to any fully differential ADC as support
> for pseudo differential channels is present (connect a fixed non 0 voltage
> to the negative input).

Whilst you could argue that, I'd counter that a clearly stated pseudo
differential mode with a simple choice of negative input (typically
only one pin is used for these modes), is a feature of the ADC, rather
than a wiring choice such as tying all negative inputs together and to
a reference supply.

> 
> The AD717x family supports pseudo differential channels as well... should
> this change be applied to them too? It is just the case that the documentation
> does not mentions this use case.

Maybe you could argue that if we used the REF- for the negative input.
Otherwise I think it falls into the category where there isn't a clearly defined
pseudo differential mode.

> 
> I think that a distinction needs to be made here:
> - When a device is only pseudo differential, sure, it is in a different category
> - When a device is fully differential and you are using it as pseudo-differential
>   you are having two inputs compared to one another
> 
> I would need more clarification is why would supply-vincom be a requirement.
> The voltage supplied to VINCOM will not be used in any computation within 
> the driver. From the perspective of getting the data it doesn't matter if 
> you are using the channel in a pseudo-differential, single ended or fully
> differential manner.

I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
ground so indeed nothing to turn on in this case and no offset to supply
(the offset will be 0 so we don't present it).

I'll note the datasheet describes the VINCOM as follows.

Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
as single-ended.  Connect AINCOM to analog ground.

The reference to single ended is pretty clear hint to me that this case
is not a differential channel. The more complex case is the one David
raised of the AD4116 where we have actual pseudo differential inputs.

> 
> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
> "Due to the matching resistors on the analog front end, the
>  differential inputs must be paired together in the following
>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>  VIN6 and VIN7. If any two voltage inputs are paired in a
>  configuration other than what is described in this data sheet,
>  the accuracy of the device cannot be guaranteed."

OK, but I'll assume no 'good' customer of ADI will do that as any support
engineer would grumpily point at that statement if they ever reported
a problem :)

> 
> Tried the device and it works as fully differential when pairing any
> VINx with VINCOM. Still works when selecting VINCOM as the positive
> input of the ADC.
> 
> I really see this as overly complicated and unnecessary. These families
> of ADCs are fully differential. If you are using it to measure a single ended
> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
> the common voltage.

For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
you to tie it to a different voltage then the single ended mode would be offset
wrt to that value.

For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
that is not connected to analog 0V.  If the device is being used in a pseudo differential
mode that provides a fixed offset voltage.

So my preference (though I could maybe be convinced it's not worth the effort)
is to treat pseudo differential as single ended channels where 'negative' pin is
providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
to userspace include the information of that offset.

We haven't handled pseudo differential channels that well in the past, but the
recent discussions have lead to a cleaner overall solution and it would be good
to be consistent going forwards.  We could deprecate the previous bindings in
existing drivers, but that is a job for another day  (possibly never happens!)

Jonathan



> 
> 
>
Ceclan, Dumitru April 15, 2024, 6:42 p.m. UTC | #17
On 13/04/2024 13:49, Jonathan Cameron wrote:
> On Tue, 9 Apr 2024 11:08:28 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> 
>> On 06/04/2024 17:53, Jonathan Cameron wrote:
>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>> David Lechner <dlechner@baylibre.com> wrote:
>>>   
>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:  
>>>>>
>>>>> On 01/04/2024 22:37, David Lechner wrote:    
>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:    
>>>>>>>
>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>    
>>>>>
...
>>
>>>>> Other alternative that came to my mind: attribute "adi,current-channel".    
>>>>
>>>> Having a boolean flag like this would make more sense to me if we
>>>> don't agree that the suggestion below is simpler.
>>>>  

...

> 
> We do directly relate reg to channel numbers in drivers like the ad7292 (where not
> all channels are differential)  I'm not convinced either way on what is best
> here where reg is currently just an index into a channel specification, not
> meaningful for which pins are involved.
> 
> It doesn't seem worth adding an equivalent of diff-channels for a single channel
> setup but I guess it would be more consistent.
> 

Would you agree with the attribute adi,current-channel within the channel and
 diff-channels set to the correspondent current inputs (13 10 for pair IN2)?

>>
>>>>  
>>>>>    
>>>>>>> +
>>>>>>> +          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.
>>>>>>    
>>>>>
>>>>> Sure, I'll rename to VINCOM.
>>>>>    
>>>>>> 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.
>>>>>>    
>>>>>
>>>>> I cannot see any configuration options for these use cases. All inputs
>>>>> are routed to the same mux and routed to the differential positive and
>>>>> negative ADC inputs.
>>>>>
>>>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
>>>>> between these two use cases is if you connected VINCOM to AVSS (with
>>>>> unipolar coding) or not with bipolar encoding. The channel is still
>>>>> measuring the difference between the two selected inputs and comparing
>>>>> to the selected reference.
>>>>>    
>>>>>> 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.
>>>>>>    
>>>>>
>>>>> I cannot seem to understand what would a adcin15-supply be needed for.
>>>>> This input, the same as all others, enters the mux and is routed to
>>>>> either positive or negative input of the ADC.
>>>>>
>>>>> The voltage on the ADCIN15 pin is not important to the user, just the
>>>>> difference in voltage between that pin and the other one selected.
>>>>>
> 
> That statement is the root of disagreement I think.
> If they are wired for pseudo differential measurement ADCIN15 a reference voltage
> not a varying signal. It can equally be used as a negative channel of
> a differential pair.   Not different from point of view of hardware
> config, but potentially different from point of view of how the
> analog wiring is done and how we may want to present it to userspace.
> 
>>>>
>>>> These suggestions come from some recent discussion about
>>>> pseudo-differential vs. fully differential inputs (e.g. search the IIO
>>>> mailing list for AD7380).
>>>>
>>>> So what I suggested here might be more technically correct according
>>>> to what I got out of that discussion. But for this specific case, I
>>>> agree it is good enough to just treat all inputs as always
>>>> fully-differential to keep things from getting too unwieldy.  
>>>
>>> Hmm.  That whole approach to pseudo differential does get messy if
>>> we have the common line routed through the main MUX rather than an opt
>>> in only on the negative side.  
>>>
>>> If I read this right, its almost a trick to support a pseudo differential
>>> wiring with simple registers (I guess reflecting MUX limitations).
>>>
>>> So what could we do?
>>>
>>> We could assume that VINCOM is used like a conventional pseudo
>>> differential negative signal and have supply-vincom + non diffferential
>>> channels if that's the configuration wanted.
>>>
>>> Then for differential channels can support all the VINX VINX+1
>>> and swapped options.
>>> For VIN10 it gets fun as non differential and differential options
>>> I think map to same actual config.   Don't see reason we need to express
>>> that in the binding though so let that have VIN10 VINCOM (probably using
>>> a magic channel number) and  VIN10 pseudo differential.
>>>
>>> Similar setup for ADCIN15 equivalent usage
>>>
>>> Code wise this probably won't be particular hard to support in the driver
>>> (obviously I haven't tried though :) is it worth the effort to keep
>>> it inline with other devices that support pseudo differential channesl.  
>>
>> Then this would need to be done to any fully differential ADC as support
>> for pseudo differential channels is present (connect a fixed non 0 voltage
>> to the negative input).
> 
> Whilst you could argue that, I'd counter that a clearly stated pseudo
> differential mode with a simple choice of negative input (typically
> only one pin is used for these modes), is a feature of the ADC, rather
> than a wiring choice such as tying all negative inputs together and to
> a reference supply.
> 
>>
>> The AD717x family supports pseudo differential channels as well... should
>> this change be applied to them too? It is just the case that the documentation
>> does not mentions this use case.
> 
> Maybe you could argue that if we used the REF- for the negative input.
> Otherwise I think it falls into the category where there isn't a clearly defined
> pseudo differential mode.
> 

While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
"Pseudo Differential Inputs
 The user can also choose to measure four different single-ended
 analog inputs. In this case, each of the analog inputs is converted
 as being the difference between the single-ended input to be
 measured and a set analog input common pin. Because there is
 a crosspoint multiplexer, the user can set any of the analog inputs
 as the common pin. An example of such a scenario is to connect
 the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
 + 2.5 V) and select this input when configuring the crosspoint
 multiplexer. When using the AD7176-2 with pseudo differential
 inputs, the INL specification is degraded."

As the crosspoint mux is present on all models it really makes me think that this
paragraph applies to all models in the family

>>
>> I think that a distinction needs to be made here:
>> - When a device is only pseudo differential, sure, it is in a different category
>> - When a device is fully differential and you are using it as pseudo-differential
>>   you are having two inputs compared to one another
>>
>> I would need more clarification is why would supply-vincom be a requirement.
>> The voltage supplied to VINCOM will not be used in any computation within 
>> the driver. From the perspective of getting the data it doesn't matter if 
>> you are using the channel in a pseudo-differential, single ended or fully
>> differential manner.
> 
> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
> ground so indeed nothing to turn on in this case and no offset to supply
> (the offset will be 0 so we don't present it).
> 
> I'll note the datasheet describes the VINCOM as follows.
> 
> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
> as single-ended.  Connect AINCOM to analog ground.
> 
> The reference to single ended is pretty clear hint to me that this case
> is not a differential channel. The more complex case is the one David
> raised of the AD4116 where we have actual pseudo differential inputs.
> 

Alright, from my perspective they all pass through the same mux but okay,
not differential. The only issue would differentiating cases in AD4116 where
the pair VIN10 - VINCOM is specified as single-ended or differential pair.

Also, AD4116:
"0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"

Not really sure where the "actual pseudo differential" sits.

Would you agree with having device tree flags that specifies how is the
channel used: single-ended, pseudo-differential, differential.
For the first two, the differential flag will not be set in IIO.

>>
>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
>> "Due to the matching resistors on the analog front end, the
>>  differential inputs must be paired together in the following
>>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>>  VIN6 and VIN7. If any two voltage inputs are paired in a
>>  configuration other than what is described in this data sheet,
>>  the accuracy of the device cannot be guaranteed."
> 
> OK, but I'll assume no 'good' customer of ADI will do that as any support
> engineer would grumpily point at that statement if they ever reported
> a problem :)
> 
>>
>> Tried the device and it works as fully differential when pairing any
>> VINx with VINCOM. Still works when selecting VINCOM as the positive
>> input of the ADC.
>>
>> I really see this as overly complicated and unnecessary. These families
>> of ADCs are fully differential. If you are using it to measure a single ended
>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
>> the common voltage.
> 
> For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
> you to tie it to a different voltage then the single ended mode would be offset
> wrt to that value.
> 
> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
> that is not connected to analog 0V.  If the device is being used in a pseudo differential
> mode that provides a fixed offset voltage.
> 
> So my preference (though I could maybe be convinced it's not worth the effort)
> is to treat pseudo differential as single ended channels where 'negative' pin is
> providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
> to userspace include the information of that offset.
> 

What do you mean by offset? I currently understand that the user will have
a way of reading the voltage of that specific supply from the driver. 

If you mean provide a different channel offset value when using it as
pseudo-differential then I would disagree


> We haven't handled pseudo differential channels that well in the past, but the
> recent discussions have lead to a cleaner overall solution and it would be good
> to be consistent going forwards.  We could deprecate the previous bindings in
> existing drivers, but that is a job for another day  (possibly never happens!)
> 

I really hope that a clean solution could be obtained for this driver as well :)
Jonathan Cameron April 20, 2024, 2:33 p.m. UTC | #18
On Mon, 15 Apr 2024 21:42:50 +0300
"Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:

> On 13/04/2024 13:49, Jonathan Cameron wrote:
> > On Tue, 9 Apr 2024 11:08:28 +0300
> > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> >   
> >> On 06/04/2024 17:53, Jonathan Cameron wrote:  
> >>> On Wed, 3 Apr 2024 10:40:39 -0500
> >>> David Lechner <dlechner@baylibre.com> wrote:
> >>>     
> >>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:    
> >>>>>
> >>>>> On 01/04/2024 22:37, David Lechner wrote:      
> >>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:      
> >>>>>>>
> >>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>      
> >>>>>  
> ...
> >>  
> >>>>> Other alternative that came to my mind: attribute "adi,current-channel".      
> >>>>
> >>>> Having a boolean flag like this would make more sense to me if we
> >>>> don't agree that the suggestion below is simpler.
> >>>>    
> 
> ...
> 
> > 
> > We do directly relate reg to channel numbers in drivers like the ad7292 (where not
> > all channels are differential)  I'm not convinced either way on what is best
> > here where reg is currently just an index into a channel specification, not
> > meaningful for which pins are involved.
> > 
> > It doesn't seem worth adding an equivalent of diff-channels for a single channel
> > setup but I guess it would be more consistent.
> >   
> 
> Would you agree with the attribute adi,current-channel within the channel and
>  diff-channels set to the correspondent current inputs (13 10 for pair IN2)?

From another thread today I've concluded we do need a single-channel
equivalent of diff-channels, but you are right that here it is a differential
channel so <13 10> seems like the best option to me.

> 
> >>  
> >>>>    
> >>>>>      
> >>>>>>> +
> >>>>>>> +          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.
> >>>>>>      
> >>>>>
> >>>>> Sure, I'll rename to VINCOM.
> >>>>>      
> >>>>>> 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.
> >>>>>>      
> >>>>>
> >>>>> I cannot see any configuration options for these use cases. All inputs
> >>>>> are routed to the same mux and routed to the differential positive and
> >>>>> negative ADC inputs.
> >>>>>
> >>>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference
> >>>>> between these two use cases is if you connected VINCOM to AVSS (with
> >>>>> unipolar coding) or not with bipolar encoding. The channel is still
> >>>>> measuring the difference between the two selected inputs and comparing
> >>>>> to the selected reference.
> >>>>>      
> >>>>>> 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.
> >>>>>>      
> >>>>>
> >>>>> I cannot seem to understand what would a adcin15-supply be needed for.
> >>>>> This input, the same as all others, enters the mux and is routed to
> >>>>> either positive or negative input of the ADC.
> >>>>>
> >>>>> The voltage on the ADCIN15 pin is not important to the user, just the
> >>>>> difference in voltage between that pin and the other one selected.
> >>>>>  
> > 
> > That statement is the root of disagreement I think.
> > If they are wired for pseudo differential measurement ADCIN15 a reference voltage
> > not a varying signal. It can equally be used as a negative channel of
> > a differential pair.   Not different from point of view of hardware
> > config, but potentially different from point of view of how the
> > analog wiring is done and how we may want to present it to userspace.
> >   
> >>>>
> >>>> These suggestions come from some recent discussion about
> >>>> pseudo-differential vs. fully differential inputs (e.g. search the IIO
> >>>> mailing list for AD7380).
> >>>>
> >>>> So what I suggested here might be more technically correct according
> >>>> to what I got out of that discussion. But for this specific case, I
> >>>> agree it is good enough to just treat all inputs as always
> >>>> fully-differential to keep things from getting too unwieldy.    
> >>>
> >>> Hmm.  That whole approach to pseudo differential does get messy if
> >>> we have the common line routed through the main MUX rather than an opt
> >>> in only on the negative side.  
> >>>
> >>> If I read this right, its almost a trick to support a pseudo differential
> >>> wiring with simple registers (I guess reflecting MUX limitations).
> >>>
> >>> So what could we do?
> >>>
> >>> We could assume that VINCOM is used like a conventional pseudo
> >>> differential negative signal and have supply-vincom + non diffferential
> >>> channels if that's the configuration wanted.
> >>>
> >>> Then for differential channels can support all the VINX VINX+1
> >>> and swapped options.
> >>> For VIN10 it gets fun as non differential and differential options
> >>> I think map to same actual config.   Don't see reason we need to express
> >>> that in the binding though so let that have VIN10 VINCOM (probably using
> >>> a magic channel number) and  VIN10 pseudo differential.
> >>>
> >>> Similar setup for ADCIN15 equivalent usage
> >>>
> >>> Code wise this probably won't be particular hard to support in the driver
> >>> (obviously I haven't tried though :) is it worth the effort to keep
> >>> it inline with other devices that support pseudo differential channesl.    
> >>
> >> Then this would need to be done to any fully differential ADC as support
> >> for pseudo differential channels is present (connect a fixed non 0 voltage
> >> to the negative input).  
> > 
> > Whilst you could argue that, I'd counter that a clearly stated pseudo
> > differential mode with a simple choice of negative input (typically
> > only one pin is used for these modes), is a feature of the ADC, rather
> > than a wiring choice such as tying all negative inputs together and to
> > a reference supply.
> >   
> >>
> >> The AD717x family supports pseudo differential channels as well... should
> >> this change be applied to them too? It is just the case that the documentation
> >> does not mentions this use case.  
> > 
> > Maybe you could argue that if we used the REF- for the negative input.
> > Otherwise I think it falls into the category where there isn't a clearly defined
> > pseudo differential mode.
> >   
> 
> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
> "Pseudo Differential Inputs
>  The user can also choose to measure four different single-ended
>  analog inputs. In this case, each of the analog inputs is converted
>  as being the difference between the single-ended input to be
>  measured and a set analog input common pin. Because there is
>  a crosspoint multiplexer, the user can set any of the analog inputs
>  as the common pin. An example of such a scenario is to connect
>  the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
>  + 2.5 V) and select this input when configuring the crosspoint
>  multiplexer. When using the AD7176-2 with pseudo differential
>  inputs, the INL specification is degraded."
> 
> As the crosspoint mux is present on all models it really makes me think that this
> paragraph applies to all models in the family

Interesting indeed.  So is your thinking that we need to support this
or take that "degraded" comment to imply that we should not bother
(at least until someone actually shouts that they want to do this?)

> 
> >>
> >> I think that a distinction needs to be made here:
> >> - When a device is only pseudo differential, sure, it is in a different category
> >> - When a device is fully differential and you are using it as pseudo-differential
> >>   you are having two inputs compared to one another
> >>
> >> I would need more clarification is why would supply-vincom be a requirement.
> >> The voltage supplied to VINCOM will not be used in any computation within 
> >> the driver. From the perspective of getting the data it doesn't matter if 
> >> you are using the channel in a pseudo-differential, single ended or fully
> >> differential manner.  
> > 
> > I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
> > ground so indeed nothing to turn on in this case and no offset to supply
> > (the offset will be 0 so we don't present it).
> > 
> > I'll note the datasheet describes the VINCOM as follows.
> > 
> > Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
> > as single-ended.  Connect AINCOM to analog ground.
> > 
> > The reference to single ended is pretty clear hint to me that this case
> > is not a differential channel. The more complex case is the one David
> > raised of the AD4116 where we have actual pseudo differential inputs.
> >   
> 
> Alright, from my perspective they all pass through the same mux but okay,
> not differential. The only issue would differentiating cases in AD4116 where
> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
> 
> Also, AD4116:
> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
>  0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
>  0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
>  0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
> 
> Not really sure where the "actual pseudo differential" sits.
> 
> Would you agree with having device tree flags that specifies how is the
> channel used: single-ended, pseudo-differential, differential.
> For the first two, the differential flag will not be set in IIO.

Yes. I think that makes sense - though as you observe in some cases
the actual device settings end up the same (the ad4116 note above).

If a given channel supports single-ended and pseudo-differential is
that really just a low reference change (I assume from an input to the
the IO ground)? Or is there more going on?

If it's the reference, then can we provide that as the binding control
signal?  We have other drivers that do that (though we could perhaps make
it more generic) e.g. adi,ad7124 with adi,reference-select

I don't like that binding because it always ends up have a local enum
of values, but can't really think of a better solution.

> 
> >>
> >> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
> >> "Due to the matching resistors on the analog front end, the
> >>  differential inputs must be paired together in the following
> >>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
> >>  VIN6 and VIN7. If any two voltage inputs are paired in a
> >>  configuration other than what is described in this data sheet,
> >>  the accuracy of the device cannot be guaranteed."  
> > 
> > OK, but I'll assume no 'good' customer of ADI will do that as any support
> > engineer would grumpily point at that statement if they ever reported
> > a problem :)
> >   
> >>
> >> Tried the device and it works as fully differential when pairing any
> >> VINx with VINCOM. Still works when selecting VINCOM as the positive
> >> input of the ADC.
> >>
> >> I really see this as overly complicated and unnecessary. These families
> >> of ADCs are fully differential. If you are using it to measure a single ended
> >> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
> >> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
> >> the common voltage.  
> > 
> > For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
> > you to tie it to a different voltage then the single ended mode would be offset
> > wrt to that value.
> > 
> > For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
> > that is not connected to analog 0V.  If the device is being used in a pseudo differential
> > mode that provides a fixed offset voltage.
> > 
> > So my preference (though I could maybe be convinced it's not worth the effort)
> > is to treat pseudo differential as single ended channels where 'negative' pin is
> > providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
> > to userspace include the information of that offset.
> >   
> 
> What do you mean by offset? I currently understand that the user will have
> a way of reading the voltage of that specific supply from the driver. 

How?  We could do it that way, but we don't have existing ABI for this that
I can think of.

> 
> If you mean provide a different channel offset value when using it as
> pseudo-differential then I would disagree

Provided to user space as _offset on the channel, userspace can either
incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
or ignore it if it only wants the difference from the reference value.

I'm open to discussion other ABI options, but this is the one we most naturally have
available.
> 
> 
> > We haven't handled pseudo differential channels that well in the past, but the
> > recent discussions have lead to a cleaner overall solution and it would be good
> > to be consistent going forwards.  We could deprecate the previous bindings in
> > existing drivers, but that is a job for another day  (possibly never happens!)
> >   
> 
> I really hope that a clean solution could be obtained for this driver as well :) 

I bet you wish sometimes that you had easier parts to write drivers for! :)
These continue to stretch the boundaries which is good, but slow.

Jonathan

> 
>
Ceclan, Dumitru April 23, 2024, 8:18 a.m. UTC | #19
On 20/04/2024 17:33, Jonathan Cameron wrote:
> On Mon, 15 Apr 2024 21:42:50 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> 
>> On 13/04/2024 13:49, Jonathan Cameron wrote:
>>> On Tue, 9 Apr 2024 11:08:28 +0300
>>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>>   
>>>> On 06/04/2024 17:53, Jonathan Cameron wrote:  
>>>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>>>> David Lechner <dlechner@baylibre.com> wrote:
>>>>>     
>>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:    
>>>>>>>
>>>>>>> On 01/04/2024 22:37, David Lechner wrote:      
>>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:      
>>>>>>>>>
>>>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>      
>>>>>>>  

...
 
>>>>
>>>> The AD717x family supports pseudo differential channels as well... should
>>>> this change be applied to them too? It is just the case that the documentation
>>>> does not mentions this use case.  
>>>
>>> Maybe you could argue that if we used the REF- for the negative input.
>>> Otherwise I think it falls into the category where there isn't a clearly defined
>>> pseudo differential mode.
>>>   
>>
>> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
>> "Pseudo Differential Inputs
>>  The user can also choose to measure four different single-ended
>>  analog inputs. In this case, each of the analog inputs is converted
>>  as being the difference between the single-ended input to be
>>  measured and a set analog input common pin. Because there is
>>  a crosspoint multiplexer, the user can set any of the analog inputs
>>  as the common pin. An example of such a scenario is to connect
>>  the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
>>  + 2.5 V) and select this input when configuring the crosspoint
>>  multiplexer. When using the AD7176-2 with pseudo differential
>>  inputs, the INL specification is degraded."
>>
>> As the crosspoint mux is present on all models it really makes me think that this
>> paragraph applies to all models in the family
> 
> Interesting indeed.  So is your thinking that we need to support this
> or take that "degraded" comment to imply that we should not bother
> (at least until someone actually shouts that they want to do this?)
> 

 My perspective is that support for this is already existent, the chips do not
need any special configuration in that use-case. If we want to be correct in
how the channel will be presented to the user, besides setting to false the IIO
differential flag I do not see what else should be done.

>>
>>>>
>>>> I think that a distinction needs to be made here:
>>>> - When a device is only pseudo differential, sure, it is in a different category
>>>> - When a device is fully differential and you are using it as pseudo-differential
>>>>   you are having two inputs compared to one another
>>>>
>>>> I would need more clarification is why would supply-vincom be a requirement.
>>>> The voltage supplied to VINCOM will not be used in any computation within 
>>>> the driver. From the perspective of getting the data it doesn't matter if 
>>>> you are using the channel in a pseudo-differential, single ended or fully
>>>> differential manner.  
>>>
>>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
>>> ground so indeed nothing to turn on in this case and no offset to supply
>>> (the offset will be 0 so we don't present it).
>>>
>>> I'll note the datasheet describes the VINCOM as follows.
>>>
>>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
>>> as single-ended.  Connect AINCOM to analog ground.
>>>
>>> The reference to single ended is pretty clear hint to me that this case
>>> is not a differential channel. The more complex case is the one David
>>> raised of the AD4116 where we have actual pseudo differential inputs.
>>>   
>>
>> Alright, from my perspective they all pass through the same mux but okay,
>> not differential. The only issue would differentiating cases in AD4116 where
>> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
>>
>> Also, AD4116:
>> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
>>  0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
>>  0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
>>  0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
>>
>> Not really sure where the "actual pseudo differential" sits.
>>
>> Would you agree with having device tree flags that specifies how is the
>> channel used: single-ended, pseudo-differential, differential.
>> For the first two, the differential flag will not be set in IIO.
> 
> Yes. I think that makes sense - though as you observe in some cases
> the actual device settings end up the same (the ad4116 note above).
>
 This precisely why I suggest this approach, because a channel used as
single-ended, pseudo or fully differential will have the same register
configuration on all models. I do not see any other way to know from
the driver this information.

> If a given channel supports single-ended and pseudo-differential is
> that really just a low reference change (I assume from an input to the
> the IO ground)? Or is there more going on?
> 
 I'm not sure if I understood what was said here. The reference specified
in the channel setup does not need to change.

> If it's the reference, then can we provide that as the binding control
> signal?  We have other drivers that do that (though we could perhaps make
> it more generic) e.g. adi,ad7124 with adi,reference-select
>  We already have adi,reference-select in the binding and driver, I do not
see how it could help the driver differentiate between (single, pseudo...)

> I don't like that binding because it always ends up have a local enum
> of values, but can't really think of a better solution.
>

>>
>>>>
>>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
>>>> "Due to the matching resistors on the analog front end, the
>>>>  differential inputs must be paired together in the following
>>>>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>>>>  VIN6 and VIN7. If any two voltage inputs are paired in a
>>>>  configuration other than what is described in this data sheet,
>>>>  the accuracy of the device cannot be guaranteed."  
>>>
>>> OK, but I'll assume no 'good' customer of ADI will do that as any support
>>> engineer would grumpily point at that statement if they ever reported
>>> a problem :)
>>>   
>>>>
>>>> Tried the device and it works as fully differential when pairing any
>>>> VINx with VINCOM. Still works when selecting VINCOM as the positive
>>>> input of the ADC.
>>>>
>>>> I really see this as overly complicated and unnecessary. These families
>>>> of ADCs are fully differential. If you are using it to measure a single ended
>>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
>>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
>>>> the common voltage.  
>>>
>>> For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
>>> you to tie it to a different voltage then the single ended mode would be offset
>>> wrt to that value.
>>>
>>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
>>> that is not connected to analog 0V.  If the device is being used in a pseudo differential
>>> mode that provides a fixed offset voltage.
>>>
>>> So my preference (though I could maybe be convinced it's not worth the effort)
>>> is to treat pseudo differential as single ended channels where 'negative' pin is
>>> providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
>>> to userspace include the information of that offset.
>>>   
>>
>> What do you mean by offset? I currently understand that the user will have
>> a way of reading the voltage of that specific supply from the driver. 
> 
> How?  We could do it that way, but we don't have existing ABI for this that
> I can think of.
> 
Expose a voltage channel which is not reading from the device...but that is
too much of a hack to be accepted here
>>
>> If you mean provide a different channel offset value when using it as
>> pseudo-differential then I would disagree
> 
> Provided to user space as _offset on the channel, userspace can either
> incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
> or ignore it if it only wants the difference from the reference value.
> 
> I'm open to discussion other ABI options, but this is the one we most naturally have
> available.
_offset is already used when the bipolar coding is enabled on the channel
and is computed along datasheet specifications of how data should be processed,
this is why I disagree with this.

This feels over-engineered, most of the times if a channel is pseudo
differential, the relevant measurement will be the differences between
those two inputs.

If a user needs to know the voltage on the common input, he just needs to
also configure a single ended channel with the common input where the
negative AIN is connected to AVSS.
>>
>>
>>> We haven't handled pseudo differential channels that well in the past, but the
>>> recent discussions have lead to a cleaner overall solution and it would be good
>>> to be consistent going forwards.  We could deprecate the previous bindings in
>>> existing drivers, but that is a job for another day  (possibly never happens!)
>>>   
>>
>> I really hope that a clean solution could be obtained for this driver as well :) 
> 
> I bet you wish sometimes that you had easier parts to write drivers for! :)
> These continue to stretch the boundaries which is good, but slow.
> 
> Jonathan

Not easier, fewer crammed into the same driver :)
Jonathan Cameron April 28, 2024, 5:13 p.m. UTC | #20
On Tue, 23 Apr 2024 11:18:47 +0300
"Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:

> On 20/04/2024 17:33, Jonathan Cameron wrote:
> > On Mon, 15 Apr 2024 21:42:50 +0300
> > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> >   
> >> On 13/04/2024 13:49, Jonathan Cameron wrote:  
> >>> On Tue, 9 Apr 2024 11:08:28 +0300
> >>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> >>>     
> >>>> On 06/04/2024 17:53, Jonathan Cameron wrote:    
> >>>>> On Wed, 3 Apr 2024 10:40:39 -0500
> >>>>> David Lechner <dlechner@baylibre.com> wrote:
> >>>>>       
> >>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:      
> >>>>>>>
> >>>>>>> On 01/04/2024 22:37, David Lechner wrote:        
> >>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> >>>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:        
> >>>>>>>>>
> >>>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>        
> >>>>>>>    
> 
> ...
>  
> >>>>
> >>>> The AD717x family supports pseudo differential channels as well... should
> >>>> this change be applied to them too? It is just the case that the documentation
> >>>> does not mentions this use case.    
> >>>
> >>> Maybe you could argue that if we used the REF- for the negative input.
> >>> Otherwise I think it falls into the category where there isn't a clearly defined
> >>> pseudo differential mode.
> >>>     
> >>
> >> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage:
> >> "Pseudo Differential Inputs
> >>  The user can also choose to measure four different single-ended
> >>  analog inputs. In this case, each of the analog inputs is converted
> >>  as being the difference between the single-ended input to be
> >>  measured and a set analog input common pin. Because there is
> >>  a crosspoint multiplexer, the user can set any of the analog inputs
> >>  as the common pin. An example of such a scenario is to connect
> >>  the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS
> >>  + 2.5 V) and select this input when configuring the crosspoint
> >>  multiplexer. When using the AD7176-2 with pseudo differential
> >>  inputs, the INL specification is degraded."
> >>
> >> As the crosspoint mux is present on all models it really makes me think that this
> >> paragraph applies to all models in the family  
> > 
> > Interesting indeed.  So is your thinking that we need to support this
> > or take that "degraded" comment to imply that we should not bother
> > (at least until someone actually shouts that they want to do this?)
> >   
> 
>  My perspective is that support for this is already existent, the chips do not
> need any special configuration in that use-case. If we want to be correct in
> how the channel will be presented to the user, besides setting to false the IIO
> differential flag I do not see what else should be done.

ah. The degraded bit bothered me.  That wording makes me thing no effort
should be applied to support this unless a user shouts that they really want it.
If we get it for free or near free than all is good!.

> 
> >>  
> >>>>
> >>>> I think that a distinction needs to be made here:
> >>>> - When a device is only pseudo differential, sure, it is in a different category
> >>>> - When a device is fully differential and you are using it as pseudo-differential
> >>>>   you are having two inputs compared to one another
> >>>>
> >>>> I would need more clarification is why would supply-vincom be a requirement.
> >>>> The voltage supplied to VINCOM will not be used in any computation within 
> >>>> the driver. From the perspective of getting the data it doesn't matter if 
> >>>> you are using the channel in a pseudo-differential, single ended or fully
> >>>> differential manner.    
> >>>
> >>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
> >>> ground so indeed nothing to turn on in this case and no offset to supply
> >>> (the offset will be 0 so we don't present it).
> >>>
> >>> I'll note the datasheet describes the VINCOM as follows.
> >>>
> >>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
> >>> as single-ended.  Connect AINCOM to analog ground.
> >>>
> >>> The reference to single ended is pretty clear hint to me that this case
> >>> is not a differential channel. The more complex case is the one David
> >>> raised of the AD4116 where we have actual pseudo differential inputs.
> >>>     
> >>
> >> Alright, from my perspective they all pass through the same mux but okay,
> >> not differential. The only issue would differentiating cases in AD4116 where
> >> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
> >>
> >> Also, AD4116:
> >> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
> >>  0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
> >>  0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
> >>  0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
> >>
> >> Not really sure where the "actual pseudo differential" sits.
> >>
> >> Would you agree with having device tree flags that specifies how is the
> >> channel used: single-ended, pseudo-differential, differential.
> >> For the first two, the differential flag will not be set in IIO.  
> > 
> > Yes. I think that makes sense - though as you observe in some cases
> > the actual device settings end up the same (the ad4116 note above).
> >  
>  This precisely why I suggest this approach, because a channel used as
> single-ended, pseudo or fully differential will have the same register
> configuration on all models. I do not see any other way to know from
> the driver this information.
> 
> > If a given channel supports single-ended and pseudo-differential is
> > that really just a low reference change (I assume from an input to the
> > the IO ground)? Or is there more going on?
> >   
>  I'm not sure if I understood what was said here. The reference specified
> in the channel setup does not need to change.

So what is the effective difference?  My assumption was that single-ended
means reference to 0V in all cases.  Pseudo differential means reference
to an input that is common across multiple channels, but not necessarily 0V?

> 
> > If it's the reference, then can we provide that as the binding control
> > signal?  We have other drivers that do that (though we could perhaps make
> > it more generic) e.g. adi,ad7124 with adi,reference-select
> >  We already have adi,reference-select in the binding and driver, I do not  
> see how it could help the driver differentiate between (single, pseudo...)

Indeed that doesn't work.  Problem in this discussion is I've normally forgotten
the earlier discussion when I come back to it :( 
> 
> > I don't like that binding because it always ends up have a local enum
> > of values, but can't really think of a better solution.
> >  
> 
> >>  
> >>>>
> >>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
> >>>> "Due to the matching resistors on the analog front end, the
> >>>>  differential inputs must be paired together in the following
> >>>>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
> >>>>  VIN6 and VIN7. If any two voltage inputs are paired in a
> >>>>  configuration other than what is described in this data sheet,
> >>>>  the accuracy of the device cannot be guaranteed."    
> >>>
> >>> OK, but I'll assume no 'good' customer of ADI will do that as any support
> >>> engineer would grumpily point at that statement if they ever reported
> >>> a problem :)
> >>>     
> >>>>
> >>>> Tried the device and it works as fully differential when pairing any
> >>>> VINx with VINCOM. Still works when selecting VINCOM as the positive
> >>>> input of the ADC.
> >>>>
> >>>> I really see this as overly complicated and unnecessary. These families
> >>>> of ADCs are fully differential. If you are using it to measure a single ended
> >>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
> >>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
> >>>> the common voltage.    
> >>>
> >>> For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
> >>> you to tie it to a different voltage then the single ended mode would be offset
> >>> wrt to that value.
> >>>
> >>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
> >>> that is not connected to analog 0V.  If the device is being used in a pseudo differential
> >>> mode that provides a fixed offset voltage.
> >>>
> >>> So my preference (though I could maybe be convinced it's not worth the effort)
> >>> is to treat pseudo differential as single ended channels where 'negative' pin is
> >>> providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
> >>> to userspace include the information of that offset.
> >>>     
> >>
> >> What do you mean by offset? I currently understand that the user will have
> >> a way of reading the voltage of that specific supply from the driver.   
> > 
> > How?  We could do it that way, but we don't have existing ABI for this that
> > I can think of.
> >   
> Expose a voltage channel which is not reading from the device...but that is
> too much of a hack to be accepted here

We have done things like that for a few corner cases where we were really stuck
but it is indeed nasty and hard to comprehend. Also so far they've been 'out'
channels I think which doesn't make sense here.

> >>
> >> If you mean provide a different channel offset value when using it as
> >> pseudo-differential then I would disagree  
> > 
> > Provided to user space as _offset on the channel, userspace can either
> > incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
> > or ignore it if it only wants the difference from the reference value.
> > 
> > I'm open to discussion other ABI options, but this is the one we most naturally have
> > available.  
> _offset is already used when the bipolar coding is enabled on the channel
> and is computed along datasheet specifications of how data should be processed,
> this is why I disagree with this.

OK.  It would be easy enough to apply an offset to that, but it would
complicate the driver and could seem a little odd.

> 
> This feels over-engineered, most of the times if a channel is pseudo
> differential, the relevant measurement will be the differences between
> those two inputs.
> 
> If a user needs to know the voltage on the common input, he just needs to
> also configure a single ended channel with the common input where the
> negative AIN is connected to AVSS.

OK.  I'm somewhat convinced that this is enough of a pain to describe that
we should just rely on them having some other way to get that offset if they
are deliberately using it to shift the range. We can revisit if it ever
becomes a problem.

So, I think the conclusion is just don't represent AIN-COMM (or similar)
as an explicit voltage we can measure.

> >>
> >>  
> >>> We haven't handled pseudo differential channels that well in the past, but the
> >>> recent discussions have lead to a cleaner overall solution and it would be good
> >>> to be consistent going forwards.  We could deprecate the previous bindings in
> >>> existing drivers, but that is a job for another day  (possibly never happens!)
> >>>     
> >>
> >> I really hope that a clean solution could be obtained for this driver as well :)   
> > 
> > I bet you wish sometimes that you had easier parts to write drivers for! :)
> > These continue to stretch the boundaries which is good, but slow.
> > 
> > Jonathan  
> 
> Not easier, fewer crammed into the same driver :)

I sympathise! It's been an annoyingly busy kernel cycle in the day job. I was hoping to
get back to you sooner so that more of this was fresh(ish) in my mind :(

My gut feeling is that this is a case for documentation / really detailed cover
letter for the next version to make sure we have come to at least a (mostly)
consistent conclusion.

Jonathan
Ceclan, Dumitru May 9, 2024, 1:48 p.m. UTC | #21
On 28/04/2024 20:13, Jonathan Cameron wrote:
> On Tue, 23 Apr 2024 11:18:47 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> 
>> On 20/04/2024 17:33, Jonathan Cameron wrote:
>>> On Mon, 15 Apr 2024 21:42:50 +0300
>>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>>   
>>>> On 13/04/2024 13:49, Jonathan Cameron wrote:  
>>>>> On Tue, 9 Apr 2024 11:08:28 +0300
>>>>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>>>>     
>>>>>> On 06/04/2024 17:53, Jonathan Cameron wrote:    
>>>>>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>>>>>> David Lechner <dlechner@baylibre.com> wrote:
>>>>>>>       
>>>>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:      
>>>>>>>>>
>>>>>>>>> On 01/04/2024 22:37, David Lechner wrote:        
>>>>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:        
>>>>>>>>>>>
>>>>>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>        
>>>>>>>>>    
...
>>
>>>>  
>>>>>>
>>>>>> I think that a distinction needs to be made here:
>>>>>> - When a device is only pseudo differential, sure, it is in a different category
>>>>>> - When a device is fully differential and you are using it as pseudo-differential
>>>>>>   you are having two inputs compared to one another
>>>>>>
>>>>>> I would need more clarification is why would supply-vincom be a requirement.
>>>>>> The voltage supplied to VINCOM will not be used in any computation within 
>>>>>> the driver. From the perspective of getting the data it doesn't matter if 
>>>>>> you are using the channel in a pseudo-differential, single ended or fully
>>>>>> differential manner.    
>>>>>
>>>>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
>>>>> ground so indeed nothing to turn on in this case and no offset to supply
>>>>> (the offset will be 0 so we don't present it).
>>>>>
>>>>> I'll note the datasheet describes the VINCOM as follows.
>>>>>
>>>>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
>>>>> as single-ended.  Connect AINCOM to analog ground.
>>>>>
>>>>> The reference to single ended is pretty clear hint to me that this case
>>>>> is not a differential channel. The more complex case is the one David
>>>>> raised of the AD4116 where we have actual pseudo differential inputs.
>>>>>     
>>>>
>>>> Alright, from my perspective they all pass through the same mux but okay,
>>>> not differential. The only issue would differentiating cases in AD4116 where
>>>> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
>>>>
>>>> Also, AD4116:
>>>> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
>>>>  0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
>>>>  0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
>>>>  0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
>>>>
>>>> Not really sure where the "actual pseudo differential" sits.
>>>>
>>>> Would you agree with having device tree flags that specifies how is the
>>>> channel used: single-ended, pseudo-differential, differential.
>>>> For the first two, the differential flag will not be set in IIO.  
>>>
>>> Yes. I think that makes sense - though as you observe in some cases
>>> the actual device settings end up the same (the ad4116 note above).
>>>  
>>  This precisely why I suggest this approach, because a channel used as
>> single-ended, pseudo or fully differential will have the same register
>> configuration on all models. I do not see any other way to know from
>> the driver this information.
>>
>>> If a given channel supports single-ended and pseudo-differential is
>>> that really just a low reference change (I assume from an input to the
>>> the IO ground)? Or is there more going on?
>>>   
>>  I'm not sure if I understood what was said here. The reference specified
>> in the channel setup does not need to change.
> 
> So what is the effective difference?  My assumption was that single-ended
> means reference to 0V in all cases.  Pseudo differential means reference
> to an input that is common across multiple channels, but not necessarily 0V?
> 

I do not have a clear answer...

This ADI page for example, defines pseudo-differential precisely what we
assumed (non 0V on IN-):
	"On the negative input IN- you apply a DC voltage to shift your signal."
https://ez.analog.com/data_converters/precision_adcs/w/documents/2875/difference-between-pseudo-differential-and-single-ended-mode-of-an-adc

While this page defines what we call single-ended as "pseudo-differential unipolar".
https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html

These two use-cases are not clearly differentiated from one another, what we are
referring to as pseudo-differential is called single-ended in some datasheets.



I've made a summary of how each datasheet defines these cases:

AD411x (1, 2, 4, 5) only mention single-ended as IN- selecting VINCOM.
                                                         (connected to AVSS)

AD4116 mentions both single-ended and pseudo 
       (pseudo specified as IN- connected to a non 0V reference)

AD717x (2-2, 3-8) 
	Does not contain pseudo differential references
	Single-ended is exemplified as IN- connected to 0V or a non 0V reference.

AD717x (5-2, 5-8, 7-2)
	Mentions at the start that pseudo differential is supported.
	Single-ended is exemplified as both IN- connected to 0V and a non 0V reference.

AD7172-4 only mentions single-ended with the example IN- connected to AVSS.
                                 (This model does not have the internal reference)

AD7176-2 is special, as has only the pseudo differential section but
calls them single-ended:
"Pseudo Differential Inputs
 The user can also choose to measure four different single-ended
 analog inputs. In this case, each of the analog inputs is converted
 as being the difference between the single-ended input to be
 measured and a set analog input common pin."

>>
>>> If it's the reference, then can we provide that as the binding control
>>> signal?  We have other drivers that do that (though we could perhaps make
>>> it more generic) e.g. adi,ad7124 with adi,reference-select
>>>  We already have adi,reference-select in the binding and driver, I do not  
>> see how it could help the driver differentiate between (single, pseudo...)
> 
> Indeed that doesn't work.  Problem in this discussion is I've normally forgotten
> the earlier discussion when I come back to it :( 
>>
>>> I don't like that binding because it always ends up have a local enum
>>> of values, but can't really think of a better solution.
>>>  
>>
>>>>  
>>>>>>
>>>>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
>>>>>> "Due to the matching resistors on the analog front end, the
>>>>>>  differential inputs must be paired together in the following
>>>>>>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>>>>>>  VIN6 and VIN7. If any two voltage inputs are paired in a
>>>>>>  configuration other than what is described in this data sheet,
>>>>>>  the accuracy of the device cannot be guaranteed."    
>>>>>
>>>>> OK, but I'll assume no 'good' customer of ADI will do that as any support
>>>>> engineer would grumpily point at that statement if they ever reported
>>>>> a problem :)
>>>>>     
>>>>>>
>>>>>> Tried the device and it works as fully differential when pairing any
>>>>>> VINx with VINCOM. Still works when selecting VINCOM as the positive
>>>>>> input of the ADC.
>>>>>>
>>>>>> I really see this as overly complicated and unnecessary. These families
>>>>>> of ADCs are fully differential. If you are using it to measure a single ended
>>>>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
>>>>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
>>>>>> the common voltage.    
>>>>>
>>>>> For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
>>>>> you to tie it to a different voltage then the single ended mode would be offset
>>>>> wrt to that value.
>>>>>
>>>>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
>>>>> that is not connected to analog 0V.  If the device is being used in a pseudo differential
>>>>> mode that provides a fixed offset voltage.
>>>>>
>>>>> So my preference (though I could maybe be convinced it's not worth the effort)
>>>>> is to treat pseudo differential as single ended channels where 'negative' pin is
>>>>> providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
>>>>> to userspace include the information of that offset.
>>>>>     
>>>>
>>>> What do you mean by offset? I currently understand that the user will have
>>>> a way of reading the voltage of that specific supply from the driver.   
>>>
>>> How?  We could do it that way, but we don't have existing ABI for this that
>>> I can think of.
>>>   
>> Expose a voltage channel which is not reading from the device...but that is
>> too much of a hack to be accepted here
> 
> We have done things like that for a few corner cases where we were really stuck
> but it is indeed nasty and hard to comprehend. Also so far they've been 'out'
> channels I think which doesn't make sense here.
> 
>>>>
>>>> If you mean provide a different channel offset value when using it as
>>>> pseudo-differential then I would disagree  
>>>
>>> Provided to user space as _offset on the channel, userspace can either
>>> incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
>>> or ignore it if it only wants the difference from the reference value.
>>>
>>> I'm open to discussion other ABI options, but this is the one we most naturally have
>>> available.  
>> _offset is already used when the bipolar coding is enabled on the channel
>> and is computed along datasheet specifications of how data should be processed,
>> this is why I disagree with this.
> 
> OK.  It would be easy enough to apply an offset to that, but it would
> complicate the driver and could seem a little odd.
> 
>>
>> This feels over-engineered, most of the times if a channel is pseudo
>> differential, the relevant measurement will be the differences between
>> those two inputs.
>>
>> If a user needs to know the voltage on the common input, he just needs to
>> also configure a single ended channel with the common input where the
>> negative AIN is connected to AVSS.
> 
> OK.  I'm somewhat convinced that this is enough of a pain to describe that
> we should just rely on them having some other way to get that offset if they
> are deliberately using it to shift the range. We can revisit if it ever
> becomes a problem.
> 
> So, I think the conclusion is just don't represent AIN-COMM (or similar)
> as an explicit voltage we can measure.
> 
Perfect. I'll keep the adi,channel-type attribute in the next version 
(that is there just to control the differential flag from the iio
channel struct) and we'll see if it's alright.

>>>>  
>>>>> We haven't handled pseudo differential channels that well in the past, but the
>>>>> recent discussions have lead to a cleaner overall solution and it would be good
>>>>> to be consistent going forwards.  We could deprecate the previous bindings in
>>>>> existing drivers, but that is a job for another day  (possibly never happens!)
>>>>>     
>>>>
>>>> I really hope that a clean solution could be obtained for this driver as well :)   
>>>
>>> I bet you wish sometimes that you had easier parts to write drivers for! :)
>>> These continue to stretch the boundaries which is good, but slow.
>>>
>>> Jonathan  
>>
>> Not easier, fewer crammed into the same driver :)
> 
> I sympathise! It's been an annoyingly busy kernel cycle in the day job. I was hoping to
> get back to you sooner so that more of this was fresh(ish) in my mind :(
> 
> My gut feeling is that this is a case for documentation / really detailed cover
> letter for the next version to make sure we have come to at least a (mostly)
> consistent conclusion.
> 
> Jonathan
>
David Lechner May 15, 2024, 9:42 p.m. UTC | #22
On Sat, Apr 20, 2024 at 9:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 15 Apr 2024 21:42:50 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>
> > On 13/04/2024 13:49, Jonathan Cameron wrote:
> > > On Tue, 9 Apr 2024 11:08:28 +0300
> > > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> > >
> > >> On 06/04/2024 17:53, Jonathan Cameron wrote:
> > >>> On Wed, 3 Apr 2024 10:40:39 -0500
> > >>> David Lechner <dlechner@baylibre.com> wrote:
> > >>>
> > >>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
> > >>>>>
> > >>>>> On 01/04/2024 22:37, David Lechner wrote:
> > >>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> > >>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
> > >>>>>>>
> > >>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > >>>>>
> > ...
> > >>
> > >>>>> Other alternative that came to my mind: attribute "adi,current-channel".
> > >>>>
> > >>>> Having a boolean flag like this would make more sense to me if we
> > >>>> don't agree that the suggestion below is simpler.
> > >>>>
> >
> > ...
> >
> > >
> > > We do directly relate reg to channel numbers in drivers like the ad7292 (where not
> > > all channels are differential)  I'm not convinced either way on what is best
> > > here where reg is currently just an index into a channel specification, not
> > > meaningful for which pins are involved.
> > >
> > > It doesn't seem worth adding an equivalent of diff-channels for a single channel
> > > setup but I guess it would be more consistent.
> > >
> >
> > Would you agree with the attribute adi,current-channel within the channel and
> >  diff-channels set to the correspondent current inputs (13 10 for pair IN2)?
>
> From another thread today I've concluded we do need a single-channel
> equivalent of diff-channels, but you are right that here it is a differential
> channel so <13 10> seems like the best option to me.
>

Current inputs are differential? It seems like we would need 4 input
pins for that.
Ceclan, Dumitru May 16, 2024, 8:18 a.m. UTC | #23
On 16/05/2024 00:42, David Lechner wrote:
> On Sat, Apr 20, 2024 at 9:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> On Mon, 15 Apr 2024 21:42:50 +0300
>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>
>>> On 13/04/2024 13:49, Jonathan Cameron wrote:
>>>> On Tue, 9 Apr 2024 11:08:28 +0300
>>>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>>>
>>>>> On 06/04/2024 17:53, Jonathan Cameron wrote:
>>>>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>>>>> David Lechner <dlechner@baylibre.com> wrote:
>>>>>>
>>>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 01/04/2024 22:37, David Lechner wrote:
>>>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>>>>>
>>> ...
>>>>>
>>>>>>>> Other alternative that came to my mind: attribute "adi,current-channel".
>>>>>>>
>>>>>>> Having a boolean flag like this would make more sense to me if we
>>>>>>> don't agree that the suggestion below is simpler.
>>>>>>>
>>>
>>> ...
>>>
>>>>
>>>> We do directly relate reg to channel numbers in drivers like the ad7292 (where not
>>>> all channels are differential)  I'm not convinced either way on what is best
>>>> here where reg is currently just an index into a channel specification, not
>>>> meaningful for which pins are involved.
>>>>
>>>> It doesn't seem worth adding an equivalent of diff-channels for a single channel
>>>> setup but I guess it would be more consistent.
>>>>
>>>
>>> Would you agree with the attribute adi,current-channel within the channel and
>>>  diff-channels set to the correspondent current inputs (13 10 for pair IN2)?
>>
>> From another thread today I've concluded we do need a single-channel
>> equivalent of diff-channels, but you are right that here it is a differential
>> channel so <13 10> seems like the best option to me.
>>
> 
> Current inputs are differential? It seems like we would need 4 input
> pins for that.
We cannot measure differential current channels.
We are measuring a voltage differential channel of the voltage drop across
the shunt resistor. Differential in the sense that inputs 13 and 10 are
routed to the ADC.
diff mbox series

Patch

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
 
       diff-channels:
+        description:
+          For using current channels specify only the positive channel.
+            (IIN2+, IIN2−) -> diff-channels = <2 0>
+
+          Family AD411x supports a dedicated VCOM voltage input.
+          To select it set the second channel to 16.
+            (VIN2, VCOM) -> diff-channels = <2 16>
         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
   - 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
+
+  - 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