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

From: Dumitru Ceclan <[email protected]>

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 <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7173.yaml | 122 ++++++++++++++++++++-
1 file changed, 120 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
index ea6cfcd0aff4..5b1af382dad3 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -19,7 +19,18 @@ description: |
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.

+ Analog Devices AD411x ADC's:
+ The AD411X family encompasses a series of low power, low noise, 24-bit,
+ sigma-delta analog-to-digital converters that offer a versatile range of
+ specifications. They integrate an analog front end suitable for processing
+ fully differential/single-ended and bipolar voltage inputs.
+
Datasheets for supported chips:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
@@ -31,6 +42,11 @@ description: |
properties:
compatible:
enum:
+ - adi,ad4111
+ - adi,ad4112
+ - adi,ad4114
+ - adi,ad4115
+ - adi,ad4116
- adi,ad7172-2
- adi,ad7172-4
- adi,ad7173-8
@@ -129,10 +145,36 @@ patternProperties:
maximum: 15

diff-channels:
+ description: |
+ For using current channels specify select the current inputs
+ and enable the adi,current-channel property.
+
+ Family AD411x supports a dedicated VINCOM voltage input.
+ To select it set the second channel to 16.
+ (VIN2, VINCOM) -> diff-channels = <2 16>
+
+ There are special values that can be selected besides the voltage
+ analog inputs:
+ 21: REF+
+ 22: REF−
+ Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
+ 19: ((AVDD1 − AVSS)/5)+
+ 20: ((AVDD1 − AVSS)/5)−
+
items:
minimum: 0
maximum: 31

+ single-channel:
+ description: |
+ Models AD4111 and AD4112 support single-ended current channels.
+ To select the desired current input, specify the desired input pair:
+ (IIN2+, IIN2−) -> single-channel = <2>
+
+ items:
+ minimum: 1
+ maximum: 16
+
adi,reference-select:
description: |
Select the reference source to use when converting on
@@ -154,9 +196,26 @@ patternProperties:
- avdd
default: refout-avss

+ adi,current-channel:
+ description: |
+ Signal that the selected inputs are current channels.
+ Only available on AD4111 and AD4112.
+ type: boolean
+
+ adi,channel-type:
+ description:
+ Used to differentiate between different channel types as the device
+ register configurations are the same for all usage types.
+ Both pseudo-differential and single-ended channels will use the
+ single-ended specifier.
+ $ref: /schemas/types.yaml#/definitions/string
+ enum:
+ - single-ended
+ - differential
+ default: differential
+
required:
- reg
- - diff-channels

required:
- compatible
@@ -166,7 +225,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 +245,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

@@ -210,6 +299,35 @@ allOf:
required:
- adi,reference-select

+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,ad4111
+ - adi,ad4112
+ - adi,ad4114
+ - adi,ad4115
+ - adi,ad4116
+ then:
+ properties:
+ avdd2-supply: false
+
+ - if:
+ properties:
+ compatible:
+ not:
+ contains:
+ enum:
+ - adi,ad4111
+ - adi,ad4112
+ then:
+ patternProperties:
+ "^channel@[0-9a-f]$":
+ properties:
+ single-channel: false
+ adi,current-channel: false
+
- if:
anyOf:
- required: [clock-names]

--
2.43.0




2024-05-27 18:06:27

by Conor Dooley

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

On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <[email protected]>
>
> 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 <[email protected]>
> ---
> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 122 ++++++++++++++++++++-
> 1 file changed, 120 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> index ea6cfcd0aff4..5b1af382dad3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -19,7 +19,18 @@ description: |
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
>
> + Analog Devices AD411x ADC's:
> + The AD411X family encompasses a series of low power, low noise, 24-bit,
> + sigma-delta analog-to-digital converters that offer a versatile range of
> + specifications. They integrate an analog front end suitable for processing
> + fully differential/single-ended and bipolar voltage inputs.
> +
> Datasheets for supported chips:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> @@ -31,6 +42,11 @@ description: |
> properties:
> compatible:
> enum:
> + - adi,ad4111
> + - adi,ad4112
> + - adi,ad4114
> + - adi,ad4115
> + - adi,ad4116
> - adi,ad7172-2
> - adi,ad7172-4
> - adi,ad7173-8
> @@ -129,10 +145,36 @@ patternProperties:
> maximum: 15
>
> diff-channels:
> + description: |
> + For using current channels specify select the current inputs
> + and enable the adi,current-channel property.
> +
> + Family AD411x supports a dedicated VINCOM voltage input.
> + To select it set the second channel to 16.
> + (VIN2, VINCOM) -> diff-channels = <2 16>
> +
> + There are special values that can be selected besides the voltage
> + analog inputs:
> + 21: REF+
> + 22: REF−
> + Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
> + 19: ((AVDD1 − AVSS)/5)+
> + 20: ((AVDD1 − AVSS)/5)−
> +
> items:
> minimum: 0
> maximum: 31
>
> + single-channel:
> + description: |
> + Models AD4111 and AD4112 support single-ended current channels.
> + To select the desired current input, specify the desired input pair:
> + (IIN2+, IIN2−) -> single-channel = <2>
> +
> + items:
> + minimum: 1
> + maximum: 16
> +
> adi,reference-select:
> description: |
> Select the reference source to use when converting on
> @@ -154,9 +196,26 @@ patternProperties:
> - avdd
> default: refout-avss
>
> + adi,current-channel:
> + description: |
> + Signal that the selected inputs are current channels.
> + Only available on AD4111 and AD4112.
> + type: boolean
> +
> + adi,channel-type:
> + description:
> + Used to differentiate between different channel types as the device
> + register configurations are the same for all usage types.
> + Both pseudo-differential and single-ended channels will use the
> + single-ended specifier.
> + $ref: /schemas/types.yaml#/definitions/string
> + enum:
> + - single-ended
> + - differential
> + default: differential

I dunno if my brain just ain't workin' right today, or if this is not
sufficiently explained, but why is this property needed? You've got
diff-channels and single-channels already, why can you not infer the
information you need from them? What should software do with this
information?
Additionally, "pseudo-differential" is not explained in this binding.

Also, what does "the device register configurations are the same for
all uses types" mean? The description here implies that you'd be reading
the registers to determine the configuration, but as far as I understand
it's the job of drivers to actually configure devices.
The only way I could interpret this that makes sense to me is that you're
trying to say that the device doesn't have registers that allow you to
do runtime configuration detection - but that's the norm and I would not
call it out here.

Thanks,
Conor.


Attachments:
(No filename) (5.37 kB)
signature.asc (235.00 B)
Download all attachments

2024-05-28 12:38:06

by Ceclan, Dumitru

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

On 27/05/2024 20:48, Conor Dooley wrote:
> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 122 ++++++++++++++++++++-
>> 1 file changed, 120 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>> index ea6cfcd0aff4..5b1af382dad3 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>> @@ -19,7 +19,18 @@ description: |
>> primarily for measurement of signals close to DC but also delivers
>> outstanding performance with input bandwidths out to ~10kHz.
>>
>> + Analog Devices AD411x ADC's:
>> + The AD411X family encompasses a series of low power, low noise, 24-bit,
>> + sigma-delta analog-to-digital converters that offer a versatile range of
>> + specifications. They integrate an analog front end suitable for processing
>> + fully differential/single-ended and bipolar voltage inputs.
>> +
>> Datasheets for supported chips:
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>> @@ -31,6 +42,11 @@ description: |
>> properties:
>> compatible:
>> enum:
>> + - adi,ad4111
>> + - adi,ad4112
>> + - adi,ad4114
>> + - adi,ad4115
>> + - adi,ad4116
>> - adi,ad7172-2
>> - adi,ad7172-4
>> - adi,ad7173-8
>> @@ -129,10 +145,36 @@ patternProperties:
>> maximum: 15
>>
>> diff-channels:
>> + description: |
>> + For using current channels specify select the current inputs
>> + and enable the adi,current-channel property.
>> +
>> + Family AD411x supports a dedicated VINCOM voltage input.
>> + To select it set the second channel to 16.
>> + (VIN2, VINCOM) -> diff-channels = <2 16>
>> +
>> + There are special values that can be selected besides the voltage
>> + analog inputs:
>> + 21: REF+
>> + 22: REF−
>> + Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>> + 19: ((AVDD1 − AVSS)/5)+
>> + 20: ((AVDD1 − AVSS)/5)−
>> +
>> items:
>> minimum: 0
>> maximum: 31
>>
>> + single-channel:
>> + description: |
>> + Models AD4111 and AD4112 support single-ended current channels.
>> + To select the desired current input, specify the desired input pair:
>> + (IIN2+, IIN2−) -> single-channel = <2>
>> +
>> + items:
>> + minimum: 1
>> + maximum: 16
>> +
>> adi,reference-select:
>> description: |
>> Select the reference source to use when converting on
>> @@ -154,9 +196,26 @@ patternProperties:
>> - avdd
>> default: refout-avss
>>
>> + adi,current-channel:
>> + description: |
>> + Signal that the selected inputs are current channels.
>> + Only available on AD4111 and AD4112.
>> + type: boolean
>> +
>> + adi,channel-type:
>> + description:
>> + Used to differentiate between different channel types as the device
>> + register configurations are the same for all usage types.
>> + Both pseudo-differential and single-ended channels will use the
>> + single-ended specifier.
>> + $ref: /schemas/types.yaml#/definitions/string
>> + enum:
>> + - single-ended
>> + - differential
>> + default: differential
>
> I dunno if my brain just ain't workin' right today, or if this is not
> sufficiently explained, but why is this property needed? You've got
> diff-channels and single-channels already, why can you not infer the
> information you need from them? What should software do with this
> information?
> Additionally, "pseudo-differential" is not explained in this binding.
> In previous thread we arrived to the conclusion single-ended and
pseudo-differential channels should be marked with the flag
"differential=false" in the IIO channel struct. This cannot
really be inferred as any input pair could be used in that
manner and the only difference would be in external wiring.

Single-channels cannot be used to define such a channel as
two voltage inputs need to be selected. Also, we are already
using single-channel to define the current channels.

As for explaining the pseudo-differential, should it be explained?
A voltage channel within the context of these families is actually
differential(as there are always two inputs selected).
The single-ended and pseudo-diff use case is actually wiring up a
constant voltage to the selected negative input.

I did not consider that this should be described, as there is no
need for an attribute to describe it.

> Also, what does "the device register configurations are the same for
> all uses types" mean? The description here implies that you'd be reading
> the registers to determine the configuration, but as far as I understand
> it's the job of drivers to actually configure devices.
> The only way I could interpret this that makes sense to me is that you're
> trying to say that the device doesn't have registers that allow you to
> do runtime configuration detection - but that's the norm and I would not
> call it out here.

No, I meant that the same register configuration will be set for
both fully differential and single-ended.

The user will set diff-channels = <0, 1>, bipolar(or not) and
then they can wire whatever to those pins:
- a differential signal
- AVSS to 1 and a single-ended signal to 0
- AVSS+offset to 1 and a single-ended signal to 0
(which is called pseudo-differential in some datasheets)

All these cases will look the same in terms of configuration

>
> Thanks,
> Conor.


2024-05-28 17:53:01

by Conor Dooley

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

On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> On 27/05/2024 20:48, Conor Dooley wrote:
> > On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> >> From: Dumitru Ceclan <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 122 ++++++++++++++++++++-
> >> 1 file changed, 120 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> >> index ea6cfcd0aff4..5b1af382dad3 100644
> >> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> >> @@ -19,7 +19,18 @@ description: |
> >> primarily for measurement of signals close to DC but also delivers
> >> outstanding performance with input bandwidths out to ~10kHz.
> >>
> >> + Analog Devices AD411x ADC's:
> >> + The AD411X family encompasses a series of low power, low noise, 24-bit,
> >> + sigma-delta analog-to-digital converters that offer a versatile range of
> >> + specifications. They integrate an analog front end suitable for processing
> >> + fully differential/single-ended and bipolar voltage inputs.
> >> +
> >> Datasheets for supported chips:
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> >> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> >> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> >> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> >> @@ -31,6 +42,11 @@ description: |
> >> properties:
> >> compatible:
> >> enum:
> >> + - adi,ad4111
> >> + - adi,ad4112
> >> + - adi,ad4114
> >> + - adi,ad4115
> >> + - adi,ad4116
> >> - adi,ad7172-2
> >> - adi,ad7172-4
> >> - adi,ad7173-8
> >> @@ -129,10 +145,36 @@ patternProperties:
> >> maximum: 15
> >>
> >> diff-channels:
> >> + description: |
> >> + For using current channels specify select the current inputs
> >> + and enable the adi,current-channel property.
> >> +
> >> + Family AD411x supports a dedicated VINCOM voltage input.
> >> + To select it set the second channel to 16.
> >> + (VIN2, VINCOM) -> diff-channels = <2 16>
> >> +
> >> + There are special values that can be selected besides the voltage
> >> + analog inputs:
> >> + 21: REF+
> >> + 22: REF−
> >> + Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
> >> + 19: ((AVDD1 − AVSS)/5)+
> >> + 20: ((AVDD1 − AVSS)/5)−
> >> +
> >> items:
> >> minimum: 0
> >> maximum: 31
> >>
> >> + single-channel:
> >> + description: |
> >> + Models AD4111 and AD4112 support single-ended current channels.
> >> + To select the desired current input, specify the desired input pair:
> >> + (IIN2+, IIN2−) -> single-channel = <2>
> >> +
> >> + items:
> >> + minimum: 1
> >> + maximum: 16
> >> +
> >> adi,reference-select:
> >> description: |
> >> Select the reference source to use when converting on
> >> @@ -154,9 +196,26 @@ patternProperties:
> >> - avdd
> >> default: refout-avss
> >>
> >> + adi,current-channel:
> >> + description: |
> >> + Signal that the selected inputs are current channels.
> >> + Only available on AD4111 and AD4112.
> >> + type: boolean
> >> +
> >> + adi,channel-type:
> >> + description:
> >> + Used to differentiate between different channel types as the device
> >> + register configurations are the same for all usage types.
> >> + Both pseudo-differential and single-ended channels will use the
> >> + single-ended specifier.
> >> + $ref: /schemas/types.yaml#/definitions/string
> >> + enum:
> >> + - single-ended
> >> + - differential
> >> + default: differential
> >
> > I dunno if my brain just ain't workin' right today, or if this is not
> > sufficiently explained, but why is this property needed? You've got
> > diff-channels and single-channels already, why can you not infer the
> > information you need from them? What should software do with this
> > information?
> > Additionally, "pseudo-differential" is not explained in this binding.
>
> In previous thread we arrived to the conclusion single-ended and
> pseudo-differential channels should be marked with the flag
> "differential=false" in the IIO channel struct. This cannot
> really be inferred as any input pair could be used in that
> manner and the only difference would be in external wiring.
>
> Single-channels cannot be used to define such a channel as
> two voltage inputs need to be selected. Also, we are already
> using single-channel to define the current channels.

If I understand correctly, the property could be simplified to a flag
then, since it's only the pseudo differential mode that you cannot be
sure of?
You know when you're single-ended based on single-channel, so the
additional info you need is only in the pseudo-differential case.

> As for explaining the pseudo-differential, should it be explained?
> A voltage channel within the context of these families is actually
> differential(as there are always two inputs selected).
> The single-ended and pseudo-diff use case is actually wiring up a
> constant voltage to the selected negative input.
>
> I did not consider that this should be described, as there is no
> need for an attribute to describe it.

I dunno, adding an explanation of it in the text for the channel type
seems trivial to do. "Both pseudo-differential mode (where the
one of differential inputs is connected to a constant voltage) and
single-ended channels will..."

> > Also, what does "the device register configurations are the same for
> > all uses types" mean? The description here implies that you'd be reading
> > the registers to determine the configuration, but as far as I understand
> > it's the job of drivers to actually configure devices.
> > The only way I could interpret this that makes sense to me is that you're
> > trying to say that the device doesn't have registers that allow you to
> > do runtime configuration detection - but that's the norm and I would not
> > call it out here.
>
> No, I meant that the same register configuration will be set for
> both fully differential and single-ended.
>
> The user will set diff-channels = <0, 1>, bipolar(or not) and
> then they can wire whatever to those pins:
> - a differential signal
> - AVSS to 1 and a single-ended signal to 0
> - AVSS+offset to 1 and a single-ended signal to 0
> (which is called pseudo-differential in some datasheets)
>
> All these cases will look the same in terms of configuration

In that case, I'd just remove this sentence from the description then.
How you configure the registers to use the device doesn't really have
anything to do with describing the configuration of the hardware.
Given it isn't related to configuration detection at runtime, what
you've got written here just makes it seem like the property is
redundant because the register settings do not change.

Instead, use the description to talk about when the property should be
used and what software should use it to determine, e.g. "Software can
use vendor,channel-type to determine whether or not the measured voltage
is absolute or relative". I pulled that outta my ass, it might not
be what you're actually doing, but I figure you just want to know if
you're measuring from the origin or either side of it.

Cheers,
Conor.


Attachments:
(No filename) (8.55 kB)
signature.asc (235.00 B)
Download all attachments

2024-05-29 13:42:09

by Ceclan, Dumitru

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

On 28/05/2024 20:52, Conor Dooley wrote:
> On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
>> On 27/05/2024 20:48, Conor Dooley wrote:
>>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
>>>> From: Dumitru Ceclan <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 122 ++++++++++++++++++++-
>>>> 1 file changed, 120 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>> index ea6cfcd0aff4..5b1af382dad3 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>> @@ -19,7 +19,18 @@ description: |
>>>> primarily for measurement of signals close to DC but also delivers
>>>> outstanding performance with input bandwidths out to ~10kHz.
>>>>
>>>> + Analog Devices AD411x ADC's:
>>>> + The AD411X family encompasses a series of low power, low noise, 24-bit,
>>>> + sigma-delta analog-to-digital converters that offer a versatile range of
>>>> + specifications. They integrate an analog front end suitable for processing
>>>> + fully differential/single-ended and bipolar voltage inputs.
>>>> +
>>>> Datasheets for supported chips:
>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>>>> @@ -31,6 +42,11 @@ description: |
>>>> properties:
>>>> compatible:
>>>> enum:
>>>> + - adi,ad4111
>>>> + - adi,ad4112
>>>> + - adi,ad4114
>>>> + - adi,ad4115
>>>> + - adi,ad4116
>>>> - adi,ad7172-2
>>>> - adi,ad7172-4
>>>> - adi,ad7173-8
>>>> @@ -129,10 +145,36 @@ patternProperties:
>>>> maximum: 15
>>>>
>>>> diff-channels:
>>>> + description: |
>>>> + For using current channels specify select the current inputs
>>>> + and enable the adi,current-channel property.
>>>> +
>>>> + Family AD411x supports a dedicated VINCOM voltage input.
>>>> + To select it set the second channel to 16.
>>>> + (VIN2, VINCOM) -> diff-channels = <2 16>
>>>> +
>>>> + There are special values that can be selected besides the voltage
>>>> + analog inputs:
>>>> + 21: REF+
>>>> + 22: REF−
>>>> + Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>>>> + 19: ((AVDD1 − AVSS)/5)+
>>>> + 20: ((AVDD1 − AVSS)/5)−
>>>> +
>>>> items:
>>>> minimum: 0
>>>> maximum: 31
>>>>
>>>> + single-channel:
>>>> + description: |
>>>> + Models AD4111 and AD4112 support single-ended current channels.
>>>> + To select the desired current input, specify the desired input pair:
>>>> + (IIN2+, IIN2−) -> single-channel = <2>
>>>> +
>>>> + items:
>>>> + minimum: 1
>>>> + maximum: 16
>>>> +
>>>> adi,reference-select:
>>>> description: |
>>>> Select the reference source to use when converting on
>>>> @@ -154,9 +196,26 @@ patternProperties:
>>>> - avdd
>>>> default: refout-avss
>>>>
>>>> + adi,current-channel:
>>>> + description: |
>>>> + Signal that the selected inputs are current channels.
>>>> + Only available on AD4111 and AD4112.
>>>> + type: boolean
>>>> +
>>>> + adi,channel-type:
>>>> + description:
>>>> + Used to differentiate between different channel types as the device
>>>> + register configurations are the same for all usage types.
>>>> + Both pseudo-differential and single-ended channels will use the
>>>> + single-ended specifier.
>>>> + $ref: /schemas/types.yaml#/definitions/string
>>>> + enum:
>>>> + - single-ended
>>>> + - differential
>>>> + default: differential
>>>
>>> I dunno if my brain just ain't workin' right today, or if this is not
>>> sufficiently explained, but why is this property needed? You've got
>>> diff-channels and single-channels already, why can you not infer the
>>> information you need from them? What should software do with this
>>> information?
>>> Additionally, "pseudo-differential" is not explained in this binding.
>>
>> In previous thread we arrived to the conclusion single-ended and
>> pseudo-differential channels should be marked with the flag
>> "differential=false" in the IIO channel struct. This cannot
>> really be inferred as any input pair could be used in that
>> manner and the only difference would be in external wiring.
>>
>> Single-channels cannot be used to define such a channel as
>> two voltage inputs need to be selected. Also, we are already
>> using single-channel to define the current channels.
>
> If I understand correctly, the property could be simplified to a flag
> then, since it's only the pseudo differential mode that you cannot be
> sure of?
> You know when you're single-ended based on single-channel, so the
> additional info you need is only in the pseudo-differential case.
>
Yes, it could just be a boolean flag. The only thing I have against
that is the awkwardness of having both diff-channels and
differential=false within a channel definition.

No, there is no uncertainty regarding pseudo-differential, it's
basically single-ended.

We cannot use single-channel for voltage channels, two voltage
inputs need to be specified. And again, single-channel will be
used here for the current channels.

>> As for explaining the pseudo-differential, should it be explained?
>> A voltage channel within the context of these families is actually
>> differential(as there are always two inputs selected).
>> The single-ended and pseudo-diff use case is actually wiring up a
>> constant voltage to the selected negative input.
>>
>> I did not consider that this should be described, as there is no
>> need for an attribute to describe it.
>
> I dunno, adding an explanation of it in the text for the channel type
> seems trivial to do. "Both pseudo-differential mode (where the
> one of differential inputs is connected to a constant voltage) and
> single-ended channels will..."
>
>>> Also, what does "the device register configurations are the same for
>>> all uses types" mean? The description here implies that you'd be reading
>>> the registers to determine the configuration, but as far as I understand
>>> it's the job of drivers to actually configure devices.
>>> The only way I could interpret this that makes sense to me is that you're
>>> trying to say that the device doesn't have registers that allow you to
>>> do runtime configuration detection - but that's the norm and I would not
>>> call it out here.
>>
>> No, I meant that the same register configuration will be set for
>> both fully differential and single-ended.
>>
>> The user will set diff-channels = <0, 1>, bipolar(or not) and
>> then they can wire whatever to those pins:
>> - a differential signal
>> - AVSS to 1 and a single-ended signal to 0
>> - AVSS+offset to 1 and a single-ended signal to 0
>> (which is called pseudo-differential in some datasheets)
>>
>> All these cases will look the same in terms of configuration
>
> In that case, I'd just remove this sentence from the description then.
> How you configure the registers to use the device doesn't really have
> anything to do with describing the configuration of the hardware.
> Given it isn't related to configuration detection at runtime, what
> you've got written here just makes it seem like the property is
> redundant because the register settings do not change.
>
> Instead, use the description to talk about when the property should be
> used and what software should use it to determine, e.g. "Software can
> use vendor,channel-type to determine whether or not the measured voltage
> is absolute or relative". I pulled that outta my ass, it might not
> be what you're actually doing, but I figure you just want to know if
> you're measuring from the origin or either side of it.
>It's more to the "software can this property to correctly mark the channel
as differential or not". Hope this is acceptable. But got it, thanks.

> Cheers,
> Conor.


2024-05-29 16:04:57

by Conor Dooley

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

On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
> On 28/05/2024 20:52, Conor Dooley wrote:
> > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> >> On 27/05/2024 20:48, Conor Dooley wrote:
> >>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> >>>> From: Dumitru Ceclan <[email protected]>
> >>>> + adi,channel-type:
> >>>> + description:
> >>>> + Used to differentiate between different channel types as the device
> >>>> + register configurations are the same for all usage types.
> >>>> + Both pseudo-differential and single-ended channels will use the
> >>>> + single-ended specifier.
> >>>> + $ref: /schemas/types.yaml#/definitions/string
> >>>> + enum:
> >>>> + - single-ended
> >>>> + - differential
> >>>> + default: differential
> >>>
> >>> I dunno if my brain just ain't workin' right today, or if this is not
> >>> sufficiently explained, but why is this property needed? You've got
> >>> diff-channels and single-channels already, why can you not infer the
> >>> information you need from them? What should software do with this
> >>> information?
> >>> Additionally, "pseudo-differential" is not explained in this binding.
> >>
> >> In previous thread we arrived to the conclusion single-ended and
> >> pseudo-differential channels should be marked with the flag
> >> "differential=false" in the IIO channel struct. This cannot
> >> really be inferred as any input pair could be used in that
> >> manner and the only difference would be in external wiring.
> >>
> >> Single-channels cannot be used to define such a channel as
> >> two voltage inputs need to be selected. Also, we are already
> >> using single-channel to define the current channels.
> >
> > If I understand correctly, the property could be simplified to a flag
> > then, since it's only the pseudo differential mode that you cannot be
> > sure of?
> > You know when you're single-ended based on single-channel, so the
> > additional info you need is only in the pseudo-differential case.
> >
> Yes, it could just be a boolean flag. The only thing I have against
> that is the awkwardness of having both diff-channels and
> differential=false within a channel definition.

What I was suggesting was more like "adi,pseudo-differential" (you don't
need to set the =false or w/e, flag properties work based on present/not
present). I think that would avoid the awkwardness?

> >> As for explaining the pseudo-differential, should it be explained?
> >> A voltage channel within the context of these families is actually
> >> differential(as there are always two inputs selected).
> >> The single-ended and pseudo-diff use case is actually wiring up a
> >> constant voltage to the selected negative input.
> >>
> >> I did not consider that this should be described, as there is no
> >> need for an attribute to describe it.
> >
> > I dunno, adding an explanation of it in the text for the channel type
> > seems trivial to do. "Both pseudo-differential mode (where the
> > one of differential inputs is connected to a constant voltage) and
> > single-ended channels will..."
> >
> >>> Also, what does "the device register configurations are the same for
> >>> all uses types" mean? The description here implies that you'd be reading
> >>> the registers to determine the configuration, but as far as I understand
> >>> it's the job of drivers to actually configure devices.
> >>> The only way I could interpret this that makes sense to me is that you're
> >>> trying to say that the device doesn't have registers that allow you to
> >>> do runtime configuration detection - but that's the norm and I would not
> >>> call it out here.
> >>
> >> No, I meant that the same register configuration will be set for
> >> both fully differential and single-ended.
> >>
> >> The user will set diff-channels = <0, 1>, bipolar(or not) and
> >> then they can wire whatever to those pins:
> >> - a differential signal
> >> - AVSS to 1 and a single-ended signal to 0
> >> - AVSS+offset to 1 and a single-ended signal to 0
> >> (which is called pseudo-differential in some datasheets)
> >>
> >> All these cases will look the same in terms of configuration
> >
> > In that case, I'd just remove this sentence from the description then.
> > How you configure the registers to use the device doesn't really have
> > anything to do with describing the configuration of the hardware.
> > Given it isn't related to configuration detection at runtime, what
> > you've got written here just makes it seem like the property is
> > redundant because the register settings do not change.
> >
> > Instead, use the description to talk about when the property should be
> > used and what software should use it to determine, e.g. "Software can
> > use vendor,channel-type to determine whether or not the measured voltage
> > is absolute or relative". I pulled that outta my ass, it might not
> > be what you're actually doing, but I figure you just want to know if
> > you're measuring from the origin or either side of it.

> >It's more to the "software can this property to correctly mark the channel

Your quoting is scuffed here, I didn't write this!

> as differential or not". Hope this is acceptable. But got it, thanks.

As long as you've got a description that tells the OS what the property
actually represents, I'm happy.


Attachments:
(No filename) (5.42 kB)
signature.asc (235.00 B)
Download all attachments

2024-05-29 22:11:48

by David Lechner

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

On 5/29/24 8:38 AM, Ceclan, Dumitru wrote:
> On 28/05/2024 20:52, Conor Dooley wrote:
>> On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
>>> On 27/05/2024 20:48, Conor Dooley wrote:
>>>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 122 ++++++++++++++++++++-
>>>>> 1 file changed, 120 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>>> index ea6cfcd0aff4..5b1af382dad3 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>>>>> @@ -19,7 +19,18 @@ description: |
>>>>> primarily for measurement of signals close to DC but also delivers
>>>>> outstanding performance with input bandwidths out to ~10kHz.
>>>>>
>>>>> + Analog Devices AD411x ADC's:
>>>>> + The AD411X family encompasses a series of low power, low noise, 24-bit,
>>>>> + sigma-delta analog-to-digital converters that offer a versatile range of
>>>>> + specifications. They integrate an analog front end suitable for processing
>>>>> + fully differential/single-ended and bipolar voltage inputs.
>>>>> +
>>>>> Datasheets for supported chips:
>>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>>>>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>>>>> @@ -31,6 +42,11 @@ description: |
>>>>> properties:
>>>>> compatible:
>>>>> enum:
>>>>> + - adi,ad4111
>>>>> + - adi,ad4112
>>>>> + - adi,ad4114
>>>>> + - adi,ad4115
>>>>> + - adi,ad4116
>>>>> - adi,ad7172-2
>>>>> - adi,ad7172-4
>>>>> - adi,ad7173-8
>>>>> @@ -129,10 +145,36 @@ patternProperties:
>>>>> maximum: 15
>>>>>
>>>>> diff-channels:
>>>>> + description: |
>>>>> + For using current channels specify select the current inputs
>>>>> + and enable the adi,current-channel property.
>>>>> +
>>>>> + Family AD411x supports a dedicated VINCOM voltage input.
>>>>> + To select it set the second channel to 16.
>>>>> + (VIN2, VINCOM) -> diff-channels = <2 16>
>>>>> +
>>>>> + There are special values that can be selected besides the voltage
>>>>> + analog inputs:
>>>>> + 21: REF+
>>>>> + 22: REF−
>>>>> + Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>>>>> + 19: ((AVDD1 − AVSS)/5)+
>>>>> + 20: ((AVDD1 − AVSS)/5)−
>>>>> +
>>>>> items:
>>>>> minimum: 0
>>>>> maximum: 31
>>>>>
>>>>> + single-channel:
>>>>> + description: |
>>>>> + Models AD4111 and AD4112 support single-ended current channels.
>>>>> + To select the desired current input, specify the desired input pair:
>>>>> + (IIN2+, IIN2−) -> single-channel = <2>
>>>>> +
>>>>> + items:
>>>>> + minimum: 1
>>>>> + maximum: 16
>>>>> +
>>>>> adi,reference-select:
>>>>> description: |
>>>>> Select the reference source to use when converting on
>>>>> @@ -154,9 +196,26 @@ patternProperties:
>>>>> - avdd
>>>>> default: refout-avss
>>>>>
>>>>> + adi,current-channel:
>>>>> + description: |
>>>>> + Signal that the selected inputs are current channels.
>>>>> + Only available on AD4111 and AD4112.
>>>>> + type: boolean
>>>>> +
>>>>> + adi,channel-type:
>>>>> + description:
>>>>> + Used to differentiate between different channel types as the device
>>>>> + register configurations are the same for all usage types.
>>>>> + Both pseudo-differential and single-ended channels will use the
>>>>> + single-ended specifier.
>>>>> + $ref: /schemas/types.yaml#/definitions/string
>>>>> + enum:
>>>>> + - single-ended
>>>>> + - differential
>>>>> + default: differential
>>>>
>>>> I dunno if my brain just ain't workin' right today, or if this is not
>>>> sufficiently explained, but why is this property needed? You've got
>>>> diff-channels and single-channels already, why can you not infer the
>>>> information you need from them? What should software do with this
>>>> information?
>>>> Additionally, "pseudo-differential" is not explained in this binding.
>>>
>>> In previous thread we arrived to the conclusion single-ended and
>>> pseudo-differential channels should be marked with the flag
>>> "differential=false" in the IIO channel struct. This cannot
>>> really be inferred as any input pair could be used in that
>>> manner and the only difference would be in external wiring.
>>>
>>> Single-channels cannot be used to define such a channel as
>>> two voltage inputs need to be selected. Also, we are already
>>> using single-channel to define the current channels.
>>
>> If I understand correctly, the property could be simplified to a flag
>> then, since it's only the pseudo differential mode that you cannot be
>> sure of?
>> You know when you're single-ended based on single-channel, so the
>> additional info you need is only in the pseudo-differential case.
>>
> Yes, it could just be a boolean flag. The only thing I have against
> that is the awkwardness of having both diff-channels and
> differential=false within a channel definition.
>
> No, there is no uncertainty regarding pseudo-differential, it's
> basically single-ended.
>
> We cannot use single-channel for voltage channels, two voltage
> inputs need to be specified. And again, single-channel will be
> used here for the current channels.

Instead of using diff-channels for single-ended/pseudo-differential
plus a property that says "actually not differential" could we just
add a second common-mode-channel property to specify the second
input pin that is connected to the common mode voltage source?

Just to make sure I'm understanding, single-ended means common mode
voltage is 0V (or AVSS for this chip, I guess) and pseudo-differential
means the common mode voltage is something else other than that.
In other words, single-ended is just a special case of pseudo-differential.
So effectively, no difference that we need to describe?

So something like this could work?


/* a fully differential voltage input channel */
channel@0 {
reg = <0>;
bipolar;
diff-channels = <0 1>; /* VIN0 is +, VIN1 is - */
adi,reference-select = "vref";
};

/* a single-ended voltage input channel */
channel@1 {
reg = <1>;
/* no bipolar since common mode is 0V */
single-channel = <2>; /* VIN2 is input */
common-mode-channel = <3>; /* VIN3 connected to 0V */
};

/* a pseudo-differential voltage input channel */
channel@2 {
reg = <2>;
bipolar; /* since common mode is not 0V */
single-channel = <4>; /* VIN4 is input */
common-mode-channel = <5>; /* VIN5 connected to Vref / 2 */
adi,reference-select = "vref";
};

/* a current input channel */
channel@3 {
reg = <3>;
bipolar;
/* 0 is not the same pin as channel@0 because of
* the adi,current-channel flag */
single-channel = <0>; /* using IIN0+ and IIN0- pins */
adi,current-channel;
};


If we really wanted to go all the way, we could also think about
adding a common-mode-supply property in each channel node to with
a common-mode-channel property to describe the voltage source that
the pin is connected to.


2024-05-30 07:12:47

by Nuno Sá

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

On Wed, 2024-05-29 at 17:04 -0500, David Lechner wrote:
> On 5/29/24 8:38 AM, Ceclan, Dumitru wrote:
> > On 28/05/2024 20:52, Conor Dooley wrote:
> > > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> > > > On 27/05/2024 20:48, Conor Dooley wrote:
> > > > > On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay
> > > > > wrote:
> > > > > > From: Dumitru Ceclan <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > > ---
> > > > > >  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122
> > > > > > ++++++++++++++++++++-
> > > > > >  1 file changed, 120 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > index ea6cfcd0aff4..5b1af382dad3 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > @@ -19,7 +19,18 @@ description: |
> > > > > >    primarily for measurement of signals close to DC but also delivers
> > > > > >    outstanding performance with input bandwidths out to ~10kHz.
> > > > > >  
> > > > > > +  Analog Devices AD411x ADC's:
> > > > > > +  The AD411X family encompasses a series of low power, low noise, 24-
> > > > > > bit,
> > > > > > +  sigma-delta analog-to-digital converters that offer a versatile range
> > > > > > of
> > > > > > +  specifications. They integrate an analog front end suitable for
> > > > > > processing
> > > > > > +  fully differential/single-ended and bipolar voltage inputs.
> > > > > > +
> > > > > >    Datasheets for supported chips:
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
> > > > > > +   
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
> > > > > >     
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> > > > > >     
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
> > > > > >     
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> > > > > > @@ -31,6 +42,11 @@ description: |
> > > > > >  properties:
> > > > > >    compatible:
> > > > > >      enum:
> > > > > > +      - adi,ad4111
> > > > > > +      - adi,ad4112
> > > > > > +      - adi,ad4114
> > > > > > +      - adi,ad4115
> > > > > > +      - adi,ad4116
> > > > > >        - adi,ad7172-2
> > > > > >        - adi,ad7172-4
> > > > > >        - adi,ad7173-8
> > > > > > @@ -129,10 +145,36 @@ patternProperties:
> > > > > >          maximum: 15
> > > > > >  
> > > > > >        diff-channels:
> > > > > > +        description: |
> > > > > > +          For using current channels specify select the current inputs
> > > > > > +           and enable the adi,current-channel property.
> > > > > > +
> > > > > > +          Family AD411x supports a dedicated VINCOM voltage input.
> > > > > > +          To select it set the second channel to 16.
> > > > > > +            (VIN2, VINCOM) -> diff-channels = <2 16>
> > > > > > +
> > > > > > +          There are special values that can be selected besides the
> > > > > > voltage
> > > > > > +          analog inputs:
> > > > > > +            21: REF+
> > > > > > +            22: REF−
> > > > > > +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8,
> > > > > > AD7177-2:
> > > > > > +            19: ((AVDD1 − AVSS)/5)+
> > > > > > +            20: ((AVDD1 − AVSS)/5)−
> > > > > > +
> > > > > >          items:
> > > > > >            minimum: 0
> > > > > >            maximum: 31
> > > > > >  
> > > > > > +      single-channel:
> > > > > > +        description: |
> > > > > > +          Models AD4111 and AD4112 support single-ended current
> > > > > > channels.
> > > > > > +          To select the desired current input, specify the desired input
> > > > > > pair:
> > > > > > +            (IIN2+, IIN2−) -> single-channel = <2>
> > > > > > +
> > > > > > +        items:
> > > > > > +          minimum: 1
> > > > > > +          maximum: 16
> > > > > > +
> > > > > >        adi,reference-select:
> > > > > >          description: |
> > > > > >            Select the reference source to use when converting on
> > > > > > @@ -154,9 +196,26 @@ patternProperties:
> > > > > >            - avdd
> > > > > >          default: refout-avss
> > > > > >  
> > > > > > +      adi,current-channel:
> > > > > > +        description: |
> > > > > > +          Signal that the selected inputs are current channels.
> > > > > > +          Only available on AD4111 and AD4112.
> > > > > > +        type: boolean
> > > > > > +
> > > > > > +      adi,channel-type:
> > > > > > +        description:
> > > > > > +          Used to differentiate between different channel types as the
> > > > > > device
> > > > > > +           register configurations are the same for all usage types.
> > > > > > +          Both pseudo-differential and single-ended channels will use
> > > > > > the
> > > > > > +           single-ended specifier.
> > > > > > +        $ref: /schemas/types.yaml#/definitions/string
> > > > > > +        enum:
> > > > > > +          - single-ended
> > > > > > +          - differential
> > > > > > +        default: differential
> > > > >
> > > > > I dunno if my brain just ain't workin' right today, or if this is not
> > > > > sufficiently explained, but why is this property needed? You've got
> > > > > diff-channels and single-channels already, why can you not infer the
> > > > > information you need from them? What should software do with this
> > > > > information?
> > > > > Additionally, "pseudo-differential" is not explained in this binding.
> > > >
> > > > In previous thread we arrived to the conclusion single-ended and
> > > > pseudo-differential channels should be marked with the flag
> > > > "differential=false" in the IIO channel struct. This cannot
> > > > really be inferred as any input pair could be used in that
> > > > manner and the only difference would be in external wiring.
> > > >
> > > > Single-channels cannot be used to define such a channel as
> > > > two voltage inputs need to be selected. Also, we are already
> > > > using single-channel to define the current channels.
> > >
> > > If I understand correctly, the property could be simplified to a flag
> > > then, since it's only the pseudo differential mode that you cannot be
> > > sure of?
> > > You know when you're single-ended based on single-channel, so the
> > > additional info you need is only in the pseudo-differential case.
> > >
> > Yes, it could just be a boolean flag. The only thing I have against
> > that is the awkwardness of having both diff-channels and
> > differential=false within a channel definition.
> >
> > No, there is no uncertainty regarding pseudo-differential, it's
> > basically single-ended.
> >
> > We cannot use single-channel for voltage channels, two voltage
> > inputs need to be specified. And again, single-channel will be
> > used here for the current channels.
>
> Instead of using diff-channels for single-ended/pseudo-differential
> plus a property that says "actually not differential" could we just
> add a second common-mode-channel property to specify the second
> input pin that is connected to the common mode voltage source?
>

Yeah, I definitely don't like having diff-channels and then go "oh, not really a
differential channel". So, if could avoid that, it would be great!

> Just to make sure I'm understanding, single-ended means common mode
> voltage is 0V (or AVSS for this chip, I guess) and pseudo-differential
> means the common mode voltage is something else other than that.
> In other words, single-ended is just a special case of pseudo-differential.
> So effectively, no difference that we need to describe?
>
> So something like this could work?
>
>
>         /* a fully differential voltage input channel */
>         channel@0 {
>           reg = <0>;
>           bipolar;
>           diff-channels = <0 1>; /* VIN0 is +, VIN1 is - */
>           adi,reference-select = "vref";
>         };
>
>         /* a single-ended voltage input channel */
>         channel@1 {
>           reg = <1>;
>           /* no bipolar since common mode is 0V */
>           single-channel = <2>; /* VIN2 is input */
>           common-mode-channel = <3>; /* VIN3 connected to 0V */
>         };
>
>         /* a pseudo-differential voltage input channel */
>         channel@2 {
>           reg = <2>;
>           bipolar; /* since common mode is not 0V */
>           single-channel = <4>; /* VIN4 is input */
>           common-mode-channel = <5>; /* VIN5 connected to Vref / 2 */
>           adi,reference-select = "vref";
>         };
>
> /* a current input channel */
>         channel@3 {
>           reg = <3>;
>           bipolar;
>           /* 0 is not the same pin as channel@0 because of
>            * the adi,current-channel flag */
>           single-channel = <0>; /* using IIN0+ and IIN0- pins */
>           adi,current-channel;

Unless I'm missing something, this flag is also not being used anywhere in the
current series?

- Nuno Sá



2024-05-30 07:51:06

by Nuno Sá

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

On Wed, 2024-05-29 at 17:04 +0100, Conor Dooley wrote:
> On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
> > On 28/05/2024 20:52, Conor Dooley wrote:
> > > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> > > > On 27/05/2024 20:48, Conor Dooley wrote:
> > > > > On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay
> > > > > wrote:
> > > > > > From: Dumitru Ceclan <[email protected]>
> > > > > > +      adi,channel-type:
> > > > > > +        description:
> > > > > > +          Used to differentiate between different channel types as the
> > > > > > device
> > > > > > +           register configurations are the same for all usage types.
> > > > > > +          Both pseudo-differential and single-ended channels will use
> > > > > > the
> > > > > > +           single-ended specifier.
> > > > > > +        $ref: /schemas/types.yaml#/definitions/string
> > > > > > +        enum:
> > > > > > +          - single-ended
> > > > > > +          - differential
> > > > > > +        default: differential
> > > > >
> > > > > I dunno if my brain just ain't workin' right today, or if this is not
> > > > > sufficiently explained, but why is this property needed? You've got
> > > > > diff-channels and single-channels already, why can you not infer the
> > > > > information you need from them? What should software do with this
> > > > > information?
> > > > > Additionally, "pseudo-differential" is not explained in this binding.
> > > >
> > > > In previous thread we arrived to the conclusion single-ended and
> > > > pseudo-differential channels should be marked with the flag
> > > > "differential=false" in the IIO channel struct. This cannot
> > > > really be inferred as any input pair could be used in that
> > > > manner and the only difference would be in external wiring.
> > > >
> > > > Single-channels cannot be used to define such a channel as
> > > > two voltage inputs need to be selected. Also, we are already
> > > > using single-channel to define the current channels.
> > >
> > > If I understand correctly, the property could be simplified to a flag
> > > then, since it's only the pseudo differential mode that you cannot be
> > > sure of?
> > > You know when you're single-ended based on single-channel, so the
> > > additional info you need is only in the pseudo-differential case.
> > >
> > Yes, it could just be a boolean flag. The only thing I have against
> > that is the awkwardness of having both diff-channels and
> > differential=false within a channel definition.
>
> What I was suggesting was more like "adi,pseudo-differential" (you don't
> need to set the =false or w/e, flag properties work based on present/not
> present). I think that would avoid the awkwardness?
>

Yeah, that was also my understanding of your reply... But I think you're also
mentioning to have this flag together with the single-channel property? 

I'm a bit confused because it seems to me that single-channel only gets one input
while we need to select two for pseudo-differential/single-ended. Is this correct
Dumitru?

FWIW, I think we already have that awkwardness in the current form. If I'm not
missing anything, what we have in the driver is pretty much:

if (not diff && single-channel)
// then current channel
else
// relies on the channel-type stuff

So, effectively with the above we have

diff-channels = <1 0>;

but then wait, not so fast

adi,channel-type = "single-ended"

To me the above is equally awkward (not sure if there's any precedence in using diff-
channels like this though)...

I would like for this to be explicit... If we have diff-channels, then it's surely
differential.

If not we could use the single-channel property and instead of using an extra flag we
could make the property having either 1 or 2 items. If we have 1, then it's a current
channel. If we have 2, then it's voltage single-ended/pseudo-differential.

David's suggestion is also pretty good (and I like it's more explicit about what's
going on) so I would likely go with it...

- Nuno Sá


> > > >


2024-05-30 11:55:42

by Ceclan, Dumitru

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

On 30/05/2024 10:50, Nuno Sá wrote:
> On Wed, 2024-05-29 at 17:04 +0100, Conor Dooley wrote:
>> On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
>>> On 28/05/2024 20:52, Conor Dooley wrote:
>>>> On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
>>>>> On 27/05/2024 20:48, Conor Dooley wrote:
>>>>>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay
>>>>>> wrote:
>>>>>>> From: Dumitru Ceclan <[email protected]>
>>>>>>> +      adi,channel-type:
>>>>>>> +        description:
>>>>>>> +          Used to differentiate between different channel types as the
>>>>>>> device
>>>>>>> +           register configurations are the same for all usage types.
>>>>>>> +          Both pseudo-differential and single-ended channels will use
>>>>>>> the
>>>>>>> +           single-ended specifier.
>>>>>>> +        $ref: /schemas/types.yaml#/definitions/string
>>>>>>> +        enum:
>>>>>>> +          - single-ended
>>>>>>> +          - differential
>>>>>>> +        default: differential
>>>>>>
>>>>>> I dunno if my brain just ain't workin' right today, or if this is not
>>>>>> sufficiently explained, but why is this property needed? You've got
>>>>>> diff-channels and single-channels already, why can you not infer the
>>>>>> information you need from them? What should software do with this
>>>>>> information?
>>>>>> Additionally, "pseudo-differential" is not explained in this binding.
>>>>>
>>>>> In previous thread we arrived to the conclusion single-ended and
>>>>> pseudo-differential channels should be marked with the flag
>>>>> "differential=false" in the IIO channel struct. This cannot
>>>>> really be inferred as any input pair could be used in that
>>>>> manner and the only difference would be in external wiring.
>>>>>
>>>>> Single-channels cannot be used to define such a channel as
>>>>> two voltage inputs need to be selected. Also, we are already
>>>>> using single-channel to define the current channels.
>>>>
>>>> If I understand correctly, the property could be simplified to a flag
>>>> then, since it's only the pseudo differential mode that you cannot be
>>>> sure of?
>>>> You know when you're single-ended based on single-channel, so the
>>>> additional info you need is only in the pseudo-differential case.
>>>>
>>> Yes, it could just be a boolean flag. The only thing I have against
>>> that is the awkwardness of having both diff-channels and
>>> differential=false within a channel definition.
>>
>> What I was suggesting was more like "adi,pseudo-differential" (you don't
>> need to set the =false or w/e, flag properties work based on present/not
>> present). I think that would avoid the awkwardness?
>>
>
> Yeah, that was also my understanding of your reply... But I think you're also
> mentioning to have this flag together with the single-channel property? 
>
> I'm a bit confused because it seems to me that single-channel only gets one input
> while we need to select two for pseudo-differential/single-ended. Is this correct
> Dumitru?
>

Yes, that is correct.

> FWIW, I think we already have that awkwardness in the current form. If I'm not
> missing anything, what we have in the driver is pretty much:
>
> if (not diff && single-channel)
> // then current channel
> else
> // relies on the channel-type stuff
>
> So, effectively with the above we have
>
> diff-channels = <1 0>;
>
> but then wait, not so fast
>

This comment properly and comically describes the hot mess
that I've come up with :)))

> adi,channel-type = "single-ended"
>
> To me the above is equally awkward (not sure if there's any precedence in using diff-
> channels like this though)...
>
> I would like for this to be explicit... If we have diff-channels, then it's surely
> differential.
>
> If not we could use the single-channel property and instead of using an extra flag we
> could make the property having either 1 or 2 items. If we have 1, then it's a current
> channel. If we have 2, then it's voltage single-ended/pseudo-differential.
>
> David's suggestion is also pretty good (and I like it's more explicit about what's
> going on) so I would likely go with it...
>
> - Nuno Sá
>
>

Yup, as neat as it could be, I'll do it that way.