| Message ID | 20240514-ad4111-v2-1-29be6a55efb5@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD411x | expand |
On Tue, May 14, 2024 at 2:23 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 | 118 ++++++++++++++++++++- > 1 file changed, 117 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > index ea6cfcd0aff4..6cc3514f5ed8 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,6 +145,31 @@ 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 VCOM voltage input. Did you mean VINCOM? Searching the datasheets for "VCOM" comes up empty. > + To select it set the second channel to 16. > + (VIN2, VCOM) -> diff-channels = <2 16> Jonathan mentioned this in v1 with regard to the current inputs, but it applies here too. There is a new proposed single-channel property [1] that would be preferred when an input is used as a single-ended or pseudo-differential input (i.e. with VINCOM or ADCIN15). [1]: https://lore.kernel.org/linux-iio/20240514120222.56488-5-alisa.roman@analog.com/ > + > + 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)− > + Supported only by AD4111, AD4112: > + 12: IIN3+ > + 11: IIN3− > + 13: IIN2+ > + 10: IIN2− > + 14: IIN1+ > + 9: IIN1− > + 15: IIN0+ > + 8: IIN0− I just made a late reply on v1 where Jonathan suggested that the current inputs are differential with a similar comment to this: It doesn't seem to me like current inputs are differential if they are only measuring one current. They take 2 pins because you need a way for current to come in and go back out, but the datasheet calls them single-ended inputs. > + > items: > minimum: 0 > maximum: 31 > @@ -154,6 +195,23 @@ 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. > + $ref: /schemas/types.yaml#/definitions/string > + enum: > + - single-ended > + - pseudo-differential > + - differential > + default: differential > + As suggested above, we should soon have diff-channels and single-channel to differentiate between (fully) differential and single-ended. Do we actually need to differentiate between single-ended and pseudo-differential though? I think the AD4116 datasheet is the only one that uses both of those terms. It gives the examples that for "single-ended" ADCIN15 would be connected to AVSS and for "pseudo-differential" ADCIN15 would be connected to REFOUT (AVSS + 2.5 V). So the only difference seems to be if the voltage on ADCIN15 is == 0V or != 0V. To make this like other pseudo-differential chips we have done recently, it seems to me like we should have an adcin15-supply property to describe the ADCIN15 input. Then we could use that property to determine single-ended vs. pseudo-differential (if there actually is a need for that) and we wouldn't need the adi,channel-type property. > required: > - reg > - diff-channels > @@ -166,7 +224,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 +244,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 discussed recently in the the very similar ad719x bindings [2], we may have been misunderstanding this limit so far. 15 is a bit artificially low since input pins can be used more than once in different "channels". But that is really an issues with the existing bindings, not just this patch. [2]: https://lore.kernel.org/linux-iio/20240511122955.2372f56e@jic23-huawei/ > + > + - 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 +298,34 @@ 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: > + adi,current-channel: false > + > - if: > anyOf: > - required: [clock-names] > > -- > 2.43.0 > >
On 16/05/2024 01:37, David Lechner wrote: > On Tue, May 14, 2024 at 2:23 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 | 118 ++++++++++++++++++++- >> 1 file changed, 117 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml >> index ea6cfcd0aff4..6cc3514f5ed8 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,6 +145,31 @@ 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 VCOM voltage input. > > Did you mean VINCOM? Searching the datasheets for "VCOM" comes up empty. > Yep >> + To select it set the second channel to 16. >> + (VIN2, VCOM) -> diff-channels = <2 16> > > Jonathan mentioned this in v1 with regard to the current inputs, but > it applies here too. There is a new proposed single-channel property > [1] that would be preferred when an input is used as a single-ended or > pseudo-differential input (i.e. with VINCOM or ADCIN15). > > [1]: https://lore.kernel.org/linux-iio/20240514120222.56488-5-alisa.roman@analog.com/ > Yet here I thought that it was clear from previous conversations that we are not really dealing with a single-ended/pseudo-differential input, just a differential ADC that can be used in that manner. We do not have here such a clear cut case as with AD7194, where an input is dedicated for single-ended/pseudo usage. Here, the inputs are mix and match and single-ended/pseudo is obtainable with other pins than VINCOM/ ADCIN15. When configuring channels we are *always* specifying two voltage inputs. I strongly disagree using single-channel for voltage channels in these families of ADC's. >> + >> + 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)− >> + Supported only by AD4111, AD4112: >> + 12: IIN3+ >> + 11: IIN3− >> + 13: IIN2+ >> + 10: IIN2− >> + 14: IIN1+ >> + 9: IIN1− >> + 15: IIN0+ >> + 8: IIN0− > > I just made a late reply on v1 where Jonathan suggested that the > current inputs are differential with a similar comment to this: > > It doesn't seem to me like current inputs are differential if they are > only measuring one current. They take 2 pins because you need a way > for current to come in and go back out, but the datasheet calls them > single-ended inputs. > It seemed to me that the conclusion that we arrived to was to expose the precise pins that are used in the conversion and document the selection. Yes, it is a single-ended channel. So revert to the way it was in V1 using single-channel? >> + >> items: >> minimum: 0 >> maximum: 31 >> @@ -154,6 +195,23 @@ 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. >> + $ref: /schemas/types.yaml#/definitions/string >> + enum: >> + - single-ended >> + - pseudo-differential >> + - differential >> + default: differential >> + > > As suggested above, we should soon have diff-channels and > single-channel to differentiate between (fully) differential and > single-ended. Do we actually need to differentiate between > single-ended and pseudo-differential though? > Not really, so just a bool differential flag? (this seems weird as we have diff-channels) > I think the AD4116 datasheet is the only one that uses both of those > terms. It gives the examples that for "single-ended" ADCIN15 would be > connected to AVSS and for "pseudo-differential" ADCIN15 would be > connected to REFOUT (AVSS + 2.5 V). So the only difference seems to be > if the voltage on ADCIN15 is == 0V or != 0V. > In the ad411x yes, over to ad717x it's mixed: https://lore.kernel.org/all/0fef36f8-a7db-40cc-86bd-9449cb4ab46e@gmail.com/ > To make this like other pseudo-differential chips we have done > recently, it seems to me like we should have an adcin15-supply > property to describe the ADCIN15 input. Then we could use that > property to determine single-ended vs. pseudo-differential (if there > actually is a need for that) and we wouldn't need the adi,channel-type > property. > I agree that we do not need to differentiate between those two. But the approach with the supply is too specific, the adi,channel-type property is not only for AD4116-ADCIN15, but for all models compatible. >> required: >> - reg >> - diff-channels >> @@ -166,7 +224,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 +244,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 discussed recently in the the very similar ad719x bindings [2], we > may have been misunderstanding this limit so far. 15 is a bit > artificially low since input pins can be used more than once in > different "channels". But that is really an issues with the existing > bindings, not just this patch. > > [2]: https://lore.kernel.org/linux-iio/20240511122955.2372f56e@jic23-huawei/ > > In this case there is a 1-1 correspondence between this reg limit and the number of channel configuration registers available to the device. Maybe another property then reg? Sure...but this limitation fits the current situation. In addition, the device does not work the same as ad719x. If I understood correctly that documentation, the configuration register needs to be rewritten for every different input combination. This means that the driver is implemented to overwrite the reg for every read. This device, it seems to me, is more in the liking's of write all the channel configs at once, then keep using those. For AD719x yes, it is artificial. Over here we have a clear reason. >> + >> + - 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 +298,34 @@ 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: >> + adi,current-channel: false >> + >> - if: >> anyOf: >> - required: [clock-names] >> >> -- >> 2.43.0 >> >>
On Thu, May 16, 2024 at 10:49 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > > On 16/05/2024 01:37, David Lechner wrote: > > On Tue, May 14, 2024 at 2:23 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 | 118 ++++++++++++++++++++- > >> 1 file changed, 117 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > >> index ea6cfcd0aff4..6cc3514f5ed8 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,6 +145,31 @@ 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 VCOM voltage input. > > > > Did you mean VINCOM? Searching the datasheets for "VCOM" comes up empty. > > > Yep > >> + To select it set the second channel to 16. > >> + (VIN2, VCOM) -> diff-channels = <2 16> > > > > Jonathan mentioned this in v1 with regard to the current inputs, but > > it applies here too. There is a new proposed single-channel property > > [1] that would be preferred when an input is used as a single-ended or > > pseudo-differential input (i.e. with VINCOM or ADCIN15). > > > > [1]: https://lore.kernel.org/linux-iio/20240514120222.56488-5-alisa.roman@analog.com/ > > > Yet here I thought that it was clear from previous conversations that > we are not really dealing with a single-ended/pseudo-differential input, > just a differential ADC that can be used in that manner. > > We do not have here such a clear cut case as with AD7194, where an input > is dedicated for single-ended/pseudo usage. Here, the inputs are mix and > match and single-ended/pseudo is obtainable with other pins than VINCOM/ > ADCIN15. > > When configuring channels we are *always* specifying two voltage inputs. > I strongly disagree using single-channel for voltage channels in these > families of ADC's. Yes, sorry, you are right. I forgot that VINCOM isn't actually electrically different from the other pins (even if the name makes it seem like it would be). > > >> + > >> + 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)− > >> + Supported only by AD4111, AD4112: > >> + 12: IIN3+ > >> + 11: IIN3− > >> + 13: IIN2+ > >> + 10: IIN2− > >> + 14: IIN1+ > >> + 9: IIN1− > >> + 15: IIN0+ > >> + 8: IIN0− > > > > I just made a late reply on v1 where Jonathan suggested that the > > current inputs are differential with a similar comment to this: > > > > It doesn't seem to me like current inputs are differential if they are > > only measuring one current. They take 2 pins because you need a way > > for current to come in and go back out, but the datasheet calls them > > single-ended inputs. > > > It seemed to me that the conclusion that we arrived to was to expose the > precise pins that are used in the conversion and document the selection. > > Yes, it is a single-ended channel. So revert to the way it was in V1 using > single-channel? I'd like to hear Jonathan's opinion on this one. > > >> + > >> items: > >> minimum: 0 > >> maximum: 31 > >> @@ -154,6 +195,23 @@ 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. > >> + $ref: /schemas/types.yaml#/definitions/string > >> + enum: > >> + - single-ended > >> + - pseudo-differential > >> + - differential > >> + default: differential > >> + > > > > As suggested above, we should soon have diff-channels and > > single-channel to differentiate between (fully) differential and > > single-ended. Do we actually need to differentiate between > > single-ended and pseudo-differential though? > > > Not really, so just a bool differential flag? (this seems weird as we have diff-channels) Or we need to change the proposed single-channel property to allow two inputs. I guess we'll see what Johnathan has to say. > > > I think the AD4116 datasheet is the only one that uses both of those > > terms. It gives the examples that for "single-ended" ADCIN15 would be > > connected to AVSS and for "pseudo-differential" ADCIN15 would be > > connected to REFOUT (AVSS + 2.5 V). So the only difference seems to be > > if the voltage on ADCIN15 is == 0V or != 0V. > > > In the ad411x yes, over to ad717x it's mixed: > https://lore.kernel.org/all/0fef36f8-a7db-40cc-86bd-9449cb4ab46e@gmail.com/ > > > To make this like other pseudo-differential chips we have done > > recently, it seems to me like we should have an adcin15-supply > > property to describe the ADCIN15 input. Then we could use that > > property to determine single-ended vs. pseudo-differential (if there > > actually is a need for that) and we wouldn't need the adi,channel-type > > property. > > > > I agree that we do not need to differentiate between those two. > But the approach with the supply is too specific, the adi,channel-type > property is not only for AD4116-ADCIN15, but for all models compatible. > Makes sense, especially given the point above that ADCIN15 isn't really electrically different from other inputs. > >> required: > >> - reg > >> - diff-channels > >> @@ -166,7 +224,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 +244,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 discussed recently in the the very similar ad719x bindings [2], we > > may have been misunderstanding this limit so far. 15 is a bit > > artificially low since input pins can be used more than once in > > different "channels". But that is really an issues with the existing > > bindings, not just this patch. > > > > [2]: https://lore.kernel.org/linux-iio/20240511122955.2372f56e@jic23-huawei/ > > > > > In this case there is a 1-1 correspondence between this reg limit and the number > of channel configuration registers available to the device. Maybe another property > then reg? Sure...but this limitation fits the current situation. > > In addition, the device does not work the same as ad719x. If I understood correctly > that documentation, the configuration register needs to be rewritten for every different > input combination. This means that the driver is implemented to overwrite the reg for > every read. This device, it seems to me, is more in the liking's of write all the channel > configs at once, then keep using those. > > For AD719x yes, it is artificial. Over here we have a clear reason. I thought they worked nearly the same in this regard since they are sharing a lot of code via adc/ad_sigma_delta.h, It looks to me like the channel registers are only set up for a raw read (single channel) or buffered read (only enabled channels), but maybe I didn't look deep enough. Anyway, not a big deal to me. > > >> + > >> + - 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 +298,34 @@ 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: > >> + adi,current-channel: false > >> + > >> - if: > >> anyOf: > >> - required: [clock-names] > >> > >> -- > >> 2.43.0 > >> > >> >
> > > > >> + > > >> + 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)− > > >> + Supported only by AD4111, AD4112: > > >> + 12: IIN3+ > > >> + 11: IIN3− > > >> + 13: IIN2+ > > >> + 10: IIN2− > > >> + 14: IIN1+ > > >> + 9: IIN1− > > >> + 15: IIN0+ > > >> + 8: IIN0− > > > > > > I just made a late reply on v1 where Jonathan suggested that the > > > current inputs are differential with a similar comment to this: > > > > > > It doesn't seem to me like current inputs are differential if they are > > > only measuring one current. They take 2 pins because you need a way > > > for current to come in and go back out, but the datasheet calls them > > > single-ended inputs. > > > > > It seemed to me that the conclusion that we arrived to was to expose the > > precise pins that are used in the conversion and document the selection. > > > > Yes, it is a single-ended channel. So revert to the way it was in V1 using > > single-channel? > > I'd like to hear Jonathan's opinion on this one. Yes. I think we rather went off on a false tangent on this. Any current input using a shunt is of this form and so far we've treated them as single ended :( e.g. pac1934 does this for it's sense inputs where there is an external 'sense resitor'. Similar for the stand alone afe driver for a current sense shunt which is used with a differential voltage input, but presents a single ended current measurement. Sorry for misguiding things :( At the end of this, I don't suppose anyone fancies writing up some notes on how to describe different types of channel? > > > > > >> + > > >> items: > > >> minimum: 0 > > >> maximum: 31 > > >> @@ -154,6 +195,23 @@ 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 I'm lost. Why do we need this one? Is the channel selection not sufficient to tell us this? > > >> + > > >> + adi,channel-type: > > >> + description: > > >> + Used to differentiate between different channel types as the device > > >> + register configurations are the same for all usage types. > > >> + $ref: /schemas/types.yaml#/definitions/string > > >> + enum: > > >> + - single-ended > > >> + - pseudo-differential > > >> + - differential > > >> + default: differential > > >> + > > > > > > As suggested above, we should soon have diff-channels and > > > single-channel to differentiate between (fully) differential and > > > single-ended. Do we actually need to differentiate between > > > single-ended and pseudo-differential though? > > > > > Not really, so just a bool differential flag? (this seems weird as we have diff-channels) > > Or we need to change the proposed single-channel property to allow two > inputs. I guess we'll see what Johnathan has to say. I think single ended fits better for the current channels with just one parameter.
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml index ea6cfcd0aff4..6cc3514f5ed8 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,6 +145,31 @@ 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 VCOM voltage input. + To select it set the second channel to 16. + (VIN2, VCOM) -> 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)− + Supported only by AD4111, AD4112: + 12: IIN3+ + 11: IIN3− + 13: IIN2+ + 10: IIN2− + 14: IIN1+ + 9: IIN1− + 15: IIN0+ + 8: IIN0− + items: minimum: 0 maximum: 31 @@ -154,6 +195,23 @@ 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. + $ref: /schemas/types.yaml#/definitions/string + enum: + - single-ended + - pseudo-differential + - differential + default: differential + required: - reg - diff-channels @@ -166,7 +224,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 +244,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 +298,34 @@ 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: + adi,current-channel: false + - if: anyOf: - required: [clock-names]