On Thu, May 16, 2024 at 10:49 AM Ceclan, Dumitru <mitrutzceclan@xxxxxxxxx> 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@xxxxxxxxxx> wrote: > >> > >> From: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx> > >> > >> 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@xxxxxxxxxx> > >> --- > >> .../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@xxxxxxxxxx/ > > > 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@xxxxxxxxx/ > > > 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 > >> > >> >
