| Message ID | 20240401-ad4111-v1-1-34618a9cc502@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD411x | expand |
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116. > > AD411x family ADCs support a VCOM pin, dedicated for single-ended usage. > AD4111/AD4112 support current channels, usage is implemented by > specifying channel reg values bigger than 15. > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > --- > .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > index ea6cfcd0aff4..bba2de0a52f3 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > @@ -19,7 +19,18 @@ description: | > primarily for measurement of signals close to DC but also delivers > outstanding performance with input bandwidths out to ~10kHz. > > + Analog Devices AD411x ADC's: > + The AD411X family encompasses a series of low power, low noise, 24-bit, > + sigma-delta analog-to-digital converters that offer a versatile range of > + specifications. They integrate an analog front end suitable for processing > + fully differential/single-ended and bipolar voltage inputs. > + > Datasheets for supported chips: > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf > @@ -31,6 +42,11 @@ description: | > properties: > compatible: > enum: > + - adi,ad4111 > + - adi,ad4112 > + - adi,ad4114 > + - adi,ad4115 > + - adi,ad4116 > - adi,ad7172-2 > - adi,ad7172-4 > - adi,ad7173-8 > @@ -125,10 +141,19 @@ patternProperties: > > properties: > reg: > + description: > + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > minimum: 0 > - maximum: 15 > + maximum: 19 This looks wrong. Isn't reg describing the number of logical channels (# of channel config registers)? After reviewing the driver, I see that > 16 is used as a way of flagging current inputs, but still seems like the wrong way to do it. See suggestion below. > > diff-channels: > + description: > + For using current channels specify only the positive channel. > + (IIN2+, IIN2−) -> diff-channels = <2 0> I find this a bit confusing since 2 is already VIN2 and 0 is already VIN0. I think it would make more sense to assign unique channel numbers individually to the negative and positive current inputs. Also, I think it makes sense to use the same numbers that the registers in the datasheet use (8 - 11 for negative and 12 to 15 for positive). So: (IIN2+, IIN2−) -> diff-channels = <13 10> > + > + Family AD411x supports a dedicated VCOM voltage input. > + To select it set the second channel to 16. > + (VIN2, VCOM) -> diff-channels = <2 16> The 411x datasheets call this pin VINCOM so calling it VCOM here is a bit confusing. Also, do we need to add a vincom-supply to get this voltage? Or is it safe to assume it is always connected to AVSS? The datasheet seems to indicate that the latter is the case. But then it also has this special case (at least for AD4116, didn't check all datasheets) "VIN10, VINCOM (single-ended or differential pair)". If it can be used as part of a fully differential input, we probably need some extra flag to indicate that case. Similarly, do we need special handling for ADCIN15 on AD4116? It has a "(pseudo differential or differential pair)" notation that other inputs don't. In other words, it is more like VINCOM than it is to the other ADCINxx pins. So we probably need an adcin15-supply for this pin to properly get the right channel configuration. I.e. the logic in the IIO driver would be if adcin15-supply is present, any channels that use this input are pseudo-differential, otherwise any channels that use it are fully differential. > items: > minimum: 0 > maximum: 31 > @@ -166,7 +191,6 @@ allOf: > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 > - # Other models have [0-3] channel registers Did you forget to remove reg: maximum: 3 from this if statement that this comment is referring to? > - if: > properties: > compatible: > @@ -187,6 +211,37 @@ allOf: > - vref > - refout-avss > - avdd > + > + - if: > + properties: > + compatible: > + contains: > + enum: > + - adi,ad4114 > + - adi,ad4115 > + - adi,ad4116 > + - adi,ad7173-8 > + - adi,ad7175-8 > + then: > + patternProperties: > + "^channel@[0-9a-f]$": > + properties: > + reg: > + maximum: 15 As with the previous reg comment, this if statement should not be needed since maximum should not be changed to 19. > + > + - if: > + properties: > + compatible: > + contains: > + enum: > + - adi,ad7172-2 > + - adi,ad7175-2 > + - adi,ad7176-2 > + - adi,ad7177-2 > + then: > + patternProperties: > + "^channel@[0-9a-f]$": > + properties: > reg: > maximum: 3 It looks to me like AD7172-4 actually has 8 possible channels rather than 16. So it would need a special condition as well. But that is a bug in the previous bindings and should therefore be fixed in a separate patch.
On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116. > > > > AD411x family ADCs support a VCOM pin, dedicated for single-ended usage. > > AD4111/AD4112 support current channels, usage is implemented by > > specifying channel reg values bigger than 15. > > > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > > --- > > .../devicetree/bindings/iio/adc/adi,ad7173.yaml | 59 +++++++++++++++++++++- > > 1 file changed, 57 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > > index ea6cfcd0aff4..bba2de0a52f3 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml Also, I just noticed that AD411x have only one AVDD input instead of AVDD1 and AVDD2. So we need an if statement that says if properties: compatible: enum: - adi,ad411x, then properties: avdd2-supply: false.
On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116. > > > > AD411x family ADCs support a VCOM pin, dedicated for single-ended usage. > > AD4111/AD4112 support current channels, usage is implemented by > > specifying channel reg values bigger than 15. > > > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > > --- ... > > @@ -125,10 +141,19 @@ patternProperties: > > > > properties: > > reg: > > + description: > > + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > > minimum: 0 > > - maximum: 15 > > + maximum: 19 > > This looks wrong. Isn't reg describing the number of logical channels > (# of channel config registers)? > > After reviewing the driver, I see that > 16 is used as a way of > flagging current inputs, but still seems like the wrong way to do it. > See suggestion below. > > > > > diff-channels: > > + description: > > + For using current channels specify only the positive channel. > > + (IIN2+, IIN2−) -> diff-channels = <2 0> > > I find this a bit confusing since 2 is already VIN2 and 0 is already > VIN0. I think it would make more sense to assign unique channel > numbers individually to the negative and positive current inputs. > Also, I think it makes sense to use the same numbers that the > registers in the datasheet use (8 - 11 for negative and 12 to 15 for > positive). > > So: (IIN2+, IIN2−) -> diff-channels = <13 10> Thinking about this a bit more... Since the current inputs have dedicated pins and aren't mix-and-match with multiple valid wiring configurations like the voltage inputs, do we even need to describe them in the devicetree? In the driver, the current channels would just be hard-coded like the temperature channel since there isn't any application-specific variation.
On 01/04/2024 22:37, David Lechner wrote: > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> ... >> properties: >> reg: >> + description: >> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. >> minimum: 0 >> - maximum: 15 >> + maximum: 19 > > This looks wrong. Isn't reg describing the number of logical channels > (# of channel config registers)? > > After reviewing the driver, I see that > 16 is used as a way of > flagging current inputs, but still seems like the wrong way to do it. > See suggestion below. > This was a suggestion from Jonathan, maybe I implemented it wrong. Other alternative that came to my mind: attribute "adi,current-channel". >> >> diff-channels: >> + description: >> + For using current channels specify only the positive channel. >> + (IIN2+, IIN2−) -> diff-channels = <2 0> > > I find this a bit confusing since 2 is already VIN2 and 0 is already > VIN0. I think it would make more sense to assign unique channel > numbers individually to the negative and positive current inputs. > Also, I think it makes sense to use the same numbers that the > registers in the datasheet use (8 - 11 for negative and 12 to 15 for > positive). > > So: (IIN2+, IIN2−) -> diff-channels = <13 10> > > It would mean for the user to look in the datasheet at the possible channel INPUT configurations values, decode the bit field into two integer values and place it here (0110101010) -> 13 10. This is cumbersome for just choosing current input 2. >> + >> + Family AD411x supports a dedicated VCOM voltage input. >> + To select it set the second channel to 16. >> + (VIN2, VCOM) -> diff-channels = <2 16> > > The 411x datasheets call this pin VINCOM so calling it VCOM here is a > bit confusing. > Sure, I'll rename to VINCOM. > Also, do we need to add a vincom-supply to get this voltage? Or is it > safe to assume it is always connected to AVSS? The datasheet seems to > indicate that the latter is the case. But then it also has this > special case (at least for AD4116, didn't check all datasheets) > "VIN10, VINCOM (single-ended or differential pair)". If it can be used > as part of a fully differential input, we probably need some extra > flag to indicate that case. > I cannot see any configuration options for these use cases. All inputs are routed to the same mux and routed to the differential positive and negative ADC inputs. "VIN10, VINCOM (single-ended or differential pair)" the only difference between these two use cases is if you connected VINCOM to AVSS (with unipolar coding) or not with bipolar encoding. The channel is still measuring the difference between the two selected inputs and comparing to the selected reference. > Similarly, do we need special handling for ADCIN15 on AD4116? It has a > "(pseudo differential or differential pair)" notation that other > inputs don't. In other words, it is more like VINCOM than it is to the > other ADCINxx pins. So we probably need an adcin15-supply for this pin > to properly get the right channel configuration. I.e. the logic in the > IIO driver would be if adcin15-supply is present, any channels that > use this input are pseudo-differential, otherwise any channels that > use it are fully differential. > I cannot seem to understand what would a adcin15-supply be needed for. This input, the same as all others, enters the mux and is routed to either positive or negative input of the ADC. The voltage on the ADCIN15 pin is not important to the user, just the difference in voltage between that pin and the other one selected. >> items: >> minimum: 0 >> maximum: 31 >> @@ -166,7 +191,6 @@ allOf: >> - $ref: /schemas/spi/spi-peripheral-props.yaml# >> >> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 >> - # Other models have [0-3] channel registers > > Did you forget to remove > > reg: > maximum: 3 > > from this if statement that this comment is referring to? > > Other way around, forgot in a previous patch to remove the comment. I'll move this change to a precursor patch. >> - if: >> properties: >> compatible: >> @@ -187,6 +211,37 @@ allOf: >> - vref >> - refout-avss >> - avdd >> + >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - adi,ad4114 >> + - adi,ad4115 >> + - adi,ad4116 >> + - adi,ad7173-8 >> + - adi,ad7175-8 >> + then: >> + patternProperties: >> + "^channel@[0-9a-f]$": >> + properties: >> + reg: >> + maximum: 15 > > As with the previous reg comment, this if statement should not be > needed since maximum should not be changed to 19. > We'll see what is the best approach regarding the current channels, perhaps the one you mentioned in the later reply with always configuring like the temp channel. >> + >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - adi,ad7172-2 >> + - adi,ad7175-2 >> + - adi,ad7176-2 >> + - adi,ad7177-2 >> + then: >> + patternProperties: >> + "^channel@[0-9a-f]$": >> + properties: >> reg: >> maximum: 3 > > It looks to me like AD7172-4 actually has 8 possible channels rather > than 16. So it would need a special condition as well. But that is a > bug in the previous bindings and should therefore be fixed in a > separate patch. It is addressed already in the binding: " - if: properties: compatible: contains: const: adi,ad7172-4 [...] maximum: 7 "
On 01/04/2024 23:22, David Lechner wrote: > On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: >> >> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: ... > > Also, I just noticed that AD411x have only one AVDD input instead of > AVDD1 and AVDD2. So we need an if statement that says if properties: > compatible: enum: - adi,ad411x, then properties: avdd2-supply: false. Already addressed by this: " # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 - if: properties: compatible: not: contains: enum: - adi,ad7172-4 - adi,ad7173-8 - adi,ad7175-8 then: properties: vref2-supply: false patternProperties: "^channel@[0-9a-f]$": properties: adi,reference-select: enum: - vref - refout-avss - avdd "
On 02/04/2024 00:16, David Lechner wrote: > On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: >> >> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >>> >>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >>> ... >>> >>> properties: >>> reg: >>> + description: >>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. >>> minimum: 0 >>> - maximum: 15 >>> + maximum: 19 >> >> This looks wrong. Isn't reg describing the number of logical channels >> (# of channel config registers)? >> >> After reviewing the driver, I see that > 16 is used as a way of >> flagging current inputs, but still seems like the wrong way to do it. >> See suggestion below. >> >>> >>> diff-channels: >>> + description: >>> + For using current channels specify only the positive channel. >>> + (IIN2+, IIN2−) -> diff-channels = <2 0> >> >> I find this a bit confusing since 2 is already VIN2 and 0 is already >> VIN0. I think it would make more sense to assign unique channel >> numbers individually to the negative and positive current inputs. >> Also, I think it makes sense to use the same numbers that the >> registers in the datasheet use (8 - 11 for negative and 12 to 15 for >> positive). >> >> So: (IIN2+, IIN2−) -> diff-channels = <13 10> > > Thinking about this a bit more... > > Since the current inputs have dedicated pins and aren't mix-and-match > with multiple valid wiring configurations like the voltage inputs, do > we even need to describe them in the devicetree? > > In the driver, the current channels would just be hard-coded like the > temperature channel since there isn't any application-specific > variation. Sure, but we still need to offer the user a way to configure which current inputs he wants and if they should use bipolar or unipolar coding. One other issue that arises is the fact that we will reserve 5 (including temp) channels out of the 15 that the user has the option to configure. While the binding will configure only 11 channels for example, the driver probe will error out with the message "Too many channels specified".
On 03/04/2024 10:45, Ceclan, Dumitru wrote: > On 01/04/2024 23:22, David Lechner wrote: >> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: >>> >>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > ... > >> >> Also, I just noticed that AD411x have only one AVDD input instead of >> AVDD1 and AVDD2. So we need an if statement that says if properties: >> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false. > > Already addressed by this: > " > # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 > - if: > properties: > compatible: > not: > contains: > enum: > - adi,ad7172-4 > - adi,ad7173-8 > - adi,ad7175-8 > then: > properties: > vref2-supply: false > patternProperties: > "^channel@[0-9a-f]$": > properties: > adi,reference-select: > enum: > - vref > - refout-avss > - avdd > " Mistaken vref2-supply to avdd2-supply. But still, the presence of avdd2-supply does not influence anything at all. Driver does not use it, you cannot select it for channel conversions. Would a restriction like this really be required?
On Wed, Apr 3, 2024 at 5:08 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > > On 03/04/2024 10:45, Ceclan, Dumitru wrote: > > On 01/04/2024 23:22, David Lechner wrote: > >> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: > >>> > >>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > >>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > > > ... > > > >> > >> Also, I just noticed that AD411x have only one AVDD input instead of > >> AVDD1 and AVDD2. So we need an if statement that says if properties: > >> compatible: enum: - adi,ad411x, then properties: avdd2-supply: false. > > > > Already addressed by this: > > " > > # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 > > - if: > > properties: > > compatible: > > not: > > contains: > > enum: > > - adi,ad7172-4 > > - adi,ad7173-8 > > - adi,ad7175-8 > > then: > > properties: > > vref2-supply: false > > patternProperties: > > "^channel@[0-9a-f]$": > > properties: > > adi,reference-select: > > enum: > > - vref > > - refout-avss > > - avdd > > " > > Mistaken vref2-supply to avdd2-supply. > > But still, the presence of avdd2-supply does not influence anything at all. > Driver does not use it, you cannot select it for channel conversions. > Would a restriction like this really be required? It is true that it is not likely to cause any problems we don't fix this but why would we want the bindings to intentionally be incorrect when there is an easy fix? Driver implementations should not influence leaving something out of the bindings [1]. [1]: https://www.kernel.org/doc/html//v5.10/devicetree/bindings/writing-bindings.html#overall-design
On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > > On 02/04/2024 00:16, David Lechner wrote: > > On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: > >> > >> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > >> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >>> > >>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > >>> > > ... > > >>> > >>> properties: > >>> reg: > >>> + description: > >>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > >>> minimum: 0 > >>> - maximum: 15 > >>> + maximum: 19 > >> > >> This looks wrong. Isn't reg describing the number of logical channels > >> (# of channel config registers)? > >> > >> After reviewing the driver, I see that > 16 is used as a way of > >> flagging current inputs, but still seems like the wrong way to do it. > >> See suggestion below. > >> > >>> > >>> diff-channels: > >>> + description: > >>> + For using current channels specify only the positive channel. > >>> + (IIN2+, IIN2−) -> diff-channels = <2 0> > >> > >> I find this a bit confusing since 2 is already VIN2 and 0 is already > >> VIN0. I think it would make more sense to assign unique channel > >> numbers individually to the negative and positive current inputs. > >> Also, I think it makes sense to use the same numbers that the > >> registers in the datasheet use (8 - 11 for negative and 12 to 15 for > >> positive). > >> > >> So: (IIN2+, IIN2−) -> diff-channels = <13 10> > > > > Thinking about this a bit more... > > > > Since the current inputs have dedicated pins and aren't mix-and-match > > with multiple valid wiring configurations like the voltage inputs, do > > we even need to describe them in the devicetree? > > > > In the driver, the current channels would just be hard-coded like the > > temperature channel since there isn't any application-specific > > variation. > > Sure, but we still need to offer the user a way to configure which > current inputs he wants and if they should use bipolar or unipolar coding. From the datasheet, it looks like only positive current input is allowed so I'm not sure bipolar applies here. But, yes, if there is some other variation in wiring or electrical signal that needs to be describe here, then it makes sense to allow a channel configuration node for it. > > One other issue that arises is the fact that we will reserve 5 > (including temp) channels out of the 15 that the user has the option to > configure. While the binding will configure only 11 channels for > example, the driver probe will error out with the message "Too many > channels specified". > Surely the driver could be changed to fix this, if needed. :-)
On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > > On 01/04/2024 22:37, David Lechner wrote: > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >> > >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > ... > > >> properties: > >> reg: > >> + description: > >> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > >> minimum: 0 > >> - maximum: 15 > >> + maximum: 19 > > > > This looks wrong. Isn't reg describing the number of logical channels > > (# of channel config registers)? > > > > After reviewing the driver, I see that > 16 is used as a way of > > flagging current inputs, but still seems like the wrong way to do it. > > See suggestion below. > > > > This was a suggestion from Jonathan, maybe I implemented it wrong. > Other alternative that came to my mind: attribute "adi,current-channel". Having a boolean flag like this would make more sense to me if we don't agree that the suggestion below is simpler. > >> > >> diff-channels: > >> + description: > >> + For using current channels specify only the positive channel. > >> + (IIN2+, IIN2−) -> diff-channels = <2 0> > > > > I find this a bit confusing since 2 is already VIN2 and 0 is already > > VIN0. I think it would make more sense to assign unique channel > > numbers individually to the negative and positive current inputs. > > Also, I think it makes sense to use the same numbers that the > > registers in the datasheet use (8 - 11 for negative and 12 to 15 for > > positive). > > > > So: (IIN2+, IIN2−) -> diff-channels = <13 10> > > > > > It would mean for the user to look in the datasheet at the possible > channel INPUT configurations values, decode the bit field into two > integer values and place it here (0110101010) -> 13 10. This is > cumbersome for just choosing current input 2. It could be documented in the devicetree bindings, just as it is done in adi,ad4130.yaml so that users of the bindings don't have to decipher the datasheet. > > >> + > >> + Family AD411x supports a dedicated VCOM voltage input. > >> + To select it set the second channel to 16. > >> + (VIN2, VCOM) -> diff-channels = <2 16> > > > > The 411x datasheets call this pin VINCOM so calling it VCOM here is a > > bit confusing. > > > > Sure, I'll rename to VINCOM. > > > Also, do we need to add a vincom-supply to get this voltage? Or is it > > safe to assume it is always connected to AVSS? The datasheet seems to > > indicate that the latter is the case. But then it also has this > > special case (at least for AD4116, didn't check all datasheets) > > "VIN10, VINCOM (single-ended or differential pair)". If it can be used > > as part of a fully differential input, we probably need some extra > > flag to indicate that case. > > > > I cannot see any configuration options for these use cases. All inputs > are routed to the same mux and routed to the differential positive and > negative ADC inputs. > > "VIN10, VINCOM (single-ended or differential pair)" the only difference > between these two use cases is if you connected VINCOM to AVSS (with > unipolar coding) or not with bipolar encoding. The channel is still > measuring the difference between the two selected inputs and comparing > to the selected reference. > > > Similarly, do we need special handling for ADCIN15 on AD4116? It has a > > "(pseudo differential or differential pair)" notation that other > > inputs don't. In other words, it is more like VINCOM than it is to the > > other ADCINxx pins. So we probably need an adcin15-supply for this pin > > to properly get the right channel configuration. I.e. the logic in the > > IIO driver would be if adcin15-supply is present, any channels that > > use this input are pseudo-differential, otherwise any channels that > > use it are fully differential. > > > > I cannot seem to understand what would a adcin15-supply be needed for. > This input, the same as all others, enters the mux and is routed to > either positive or negative input of the ADC. > > The voltage on the ADCIN15 pin is not important to the user, just the > difference in voltage between that pin and the other one selected. > These suggestions come from some recent discussion about pseudo-differential vs. fully differential inputs (e.g. search the IIO mailing list for AD7380). So what I suggested here might be more technically correct according to what I got out of that discussion. But for this specific case, I agree it is good enough to just treat all inputs as always fully-differential to keep things from getting too unwieldy. > >> items: > >> minimum: 0 > >> maximum: 31 > >> @@ -166,7 +191,6 @@ allOf: > >> - $ref: /schemas/spi/spi-peripheral-props.yaml# > >> > >> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 > >> - # Other models have [0-3] channel registers > > > > Did you forget to remove > > > > reg: > > maximum: 3 > > > > from this if statement that this comment is referring to? > > > > > > > Other way around, forgot in a previous patch to remove the comment. > I'll move this change to a precursor patch. > > >> - if: > >> properties: > >> compatible: > >> @@ -187,6 +211,37 @@ allOf: > >> - vref > >> - refout-avss > >> - avdd > >> + > >> + - if: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - adi,ad4114 > >> + - adi,ad4115 > >> + - adi,ad4116 > >> + - adi,ad7173-8 > >> + - adi,ad7175-8 > >> + then: > >> + patternProperties: > >> + "^channel@[0-9a-f]$": > >> + properties: > >> + reg: > >> + maximum: 15 > > > > As with the previous reg comment, this if statement should not be > > needed since maximum should not be changed to 19. > > > > We'll see what is the best approach regarding the current channels, > perhaps the one you mentioned in the later reply with always configuring > like the temp channel. > > >> + > >> + - if: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - adi,ad7172-2 > >> + - adi,ad7175-2 > >> + - adi,ad7176-2 > >> + - adi,ad7177-2 > >> + then: > >> + patternProperties: > >> + "^channel@[0-9a-f]$": > >> + properties: > >> reg: > >> maximum: 3 > > > > It looks to me like AD7172-4 actually has 8 possible channels rather > > than 16. So it would need a special condition as well. But that is a > > bug in the previous bindings and should therefore be fixed in a > > separate patch. > > It is addressed already in the binding: > " > - if: > properties: > compatible: > contains: > const: adi,ad7172-4 > [...] > maximum: 7 > " Ah, I missed it hiding with adi,reference-select overrides.
On 03/04/2024 18:22, David Lechner wrote: > On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: >> On 02/04/2024 00:16, David Lechner wrote: >>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >>>>> >> ... >> >>>>> properties: >>>>> reg: >>>>> + description: >>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. >>>>> minimum: 0 >>>>> - maximum: 15 >>>>> + maximum: 19 >>>> This looks wrong. Isn't reg describing the number of logical channels >>>> (# of channel config registers)? >>>> >>>> After reviewing the driver, I see that > 16 is used as a way of >>>> flagging current inputs, but still seems like the wrong way to do it. >>>> See suggestion below. >>>> >>>>> diff-channels: >>>>> + description: >>>>> + For using current channels specify only the positive channel. >>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0> >>>> I find this a bit confusing since 2 is already VIN2 and 0 is already >>>> VIN0. I think it would make more sense to assign unique channel >>>> numbers individually to the negative and positive current inputs. >>>> Also, I think it makes sense to use the same numbers that the >>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for >>>> positive). >>>> >>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10> >>> Thinking about this a bit more... >>> >>> Since the current inputs have dedicated pins and aren't mix-and-match >>> with multiple valid wiring configurations like the voltage inputs, do >>> we even need to describe them in the devicetree? >>> >>> In the driver, the current channels would just be hard-coded like the >>> temperature channel since there isn't any application-specific >>> variation. >> Sure, but we still need to offer the user a way to configure which >> current inputs he wants and if they should use bipolar or unipolar coding. > From the datasheet, it looks like only positive current input is > allowed so I'm not sure bipolar applies here. But, yes, if there is > some other variation in wiring or electrical signal that needs to be > describe here, then it makes sense to allow a channel configuration > node for it. AD4111 datasheet pg.29: When the ADC is configured for bipolar operation, the output code is offset binary with a negative full-scale voltage resulting in a code of 000 … 000, a zero differential input voltage resulting in a code of 100 … 000, and a positive full-scale input voltage resulting in a code of 111 … 111. The output code for any analog input voltage can be represented as Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1) The output code for any input current is represented as Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1) I would say bipolar applies here, not a great idea because of the limitation on the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option is available.
On Thu, 4 Apr 2024 16:08:56 +0300 "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > On 03/04/2024 18:22, David Lechner wrote: > > On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > >> On 02/04/2024 00:16, David Lechner wrote: > >>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: > >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > >>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > >>>>> > >> ... > >> > >>>>> properties: > >>>>> reg: > >>>>> + description: > >>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > >>>>> minimum: 0 > >>>>> - maximum: 15 > >>>>> + maximum: 19 > >>>> This looks wrong. Isn't reg describing the number of logical channels > >>>> (# of channel config registers)? > >>>> > >>>> After reviewing the driver, I see that > 16 is used as a way of > >>>> flagging current inputs, but still seems like the wrong way to do it. > >>>> See suggestion below. > >>>> > >>>>> diff-channels: > >>>>> + description: > >>>>> + For using current channels specify only the positive channel. > >>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0> > >>>> I find this a bit confusing since 2 is already VIN2 and 0 is already > >>>> VIN0. I think it would make more sense to assign unique channel > >>>> numbers individually to the negative and positive current inputs. > >>>> Also, I think it makes sense to use the same numbers that the > >>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for > >>>> positive). > >>>> > >>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10> > >>> Thinking about this a bit more... > >>> > >>> Since the current inputs have dedicated pins and aren't mix-and-match > >>> with multiple valid wiring configurations like the voltage inputs, do > >>> we even need to describe them in the devicetree? > >>> > >>> In the driver, the current channels would just be hard-coded like the > >>> temperature channel since there isn't any application-specific > >>> variation. > >> Sure, but we still need to offer the user a way to configure which > >> current inputs he wants and if they should use bipolar or unipolar coding. > > From the datasheet, it looks like only positive current input is > > allowed so I'm not sure bipolar applies here. But, yes, if there is > > some other variation in wiring or electrical signal that needs to be > > describe here, then it makes sense to allow a channel configuration > > node for it. > > AD4111 datasheet pg.29: > When the ADC is configured for bipolar operation, the output > code is offset binary with a negative full-scale voltage resulting > in a code of 000 … 000, a zero differential input voltage resulting in > a code of 100 … 000, and a positive full-scale input voltage > resulting in a code of 111 … 111. The output code for any > analog input voltage can be represented as > Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1) > The output code for any input current is represented as > Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1) > > I would say bipolar applies here, not a great idea because of the limitation on > the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option > is available. Just to check I am correct in thinking you 'might' use bipolar if you want to be able to measure small negative currents, but the range is much larger in the positive direction? J >
On Wed, 3 Apr 2024 10:40:39 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > > > > On 01/04/2024 22:37, David Lechner wrote: > > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > > > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > >> > > >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > > ... > > > > >> properties: > > >> reg: > > >> + description: > > >> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > > >> minimum: 0 > > >> - maximum: 15 > > >> + maximum: 19 > > > > > > This looks wrong. Isn't reg describing the number of logical channels > > > (# of channel config registers)? > > > > > > After reviewing the driver, I see that > 16 is used as a way of > > > flagging current inputs, but still seems like the wrong way to do it. > > > See suggestion below. > > > > > > > This was a suggestion from Jonathan, maybe I implemented it wrong. Maybe Jonathan was wrong! I was younger then than now :) However, reg values for child nodes are unique so can't just use a flag these need to be different values. > > Other alternative that came to my mind: attribute "adi,current-channel". > > Having a boolean flag like this would make more sense to me if we > don't agree that the suggestion below is simpler. > > > >> > > >> diff-channels: > > >> + description: > > >> + For using current channels specify only the positive channel. > > >> + (IIN2+, IIN2−) -> diff-channels = <2 0> > > > > > > I find this a bit confusing since 2 is already VIN2 and 0 is already > > > VIN0. I think it would make more sense to assign unique channel > > > numbers individually to the negative and positive current inputs. > > > Also, I think it makes sense to use the same numbers that the > > > registers in the datasheet use (8 - 11 for negative and 12 to 15 for > > > positive). > > > > > > So: (IIN2+, IIN2−) -> diff-channels = <13 10> > > > > > > > > It would mean for the user to look in the datasheet at the possible > > channel INPUT configurations values, decode the bit field into two > > integer values and place it here (0110101010) -> 13 10. This is > > cumbersome for just choosing current input 2. > > It could be documented in the devicetree bindings, just as it is done > in adi,ad4130.yaml so that users of the bindings don't have to > decipher the datasheet. The <13 10> option makes sense to me and avoids suggesting a common negative input. The 'fun' bit here is that diff-channels doesn't actually tell us anything. So we could just not provide it and rely on documentation of reg = 16-19 meaning the current channels? > > > > > >> + > > >> + Family AD411x supports a dedicated VCOM voltage input. > > >> + To select it set the second channel to 16. > > >> + (VIN2, VCOM) -> diff-channels = <2 16> > > > > > > The 411x datasheets call this pin VINCOM so calling it VCOM here is a > > > bit confusing. > > > > > > > Sure, I'll rename to VINCOM. > > > > > Also, do we need to add a vincom-supply to get this voltage? Or is it > > > safe to assume it is always connected to AVSS? The datasheet seems to > > > indicate that the latter is the case. But then it also has this > > > special case (at least for AD4116, didn't check all datasheets) > > > "VIN10, VINCOM (single-ended or differential pair)". If it can be used > > > as part of a fully differential input, we probably need some extra > > > flag to indicate that case. > > > > > > > I cannot see any configuration options for these use cases. All inputs > > are routed to the same mux and routed to the differential positive and > > negative ADC inputs. > > > > "VIN10, VINCOM (single-ended or differential pair)" the only difference > > between these two use cases is if you connected VINCOM to AVSS (with > > unipolar coding) or not with bipolar encoding. The channel is still > > measuring the difference between the two selected inputs and comparing > > to the selected reference. > > > > > Similarly, do we need special handling for ADCIN15 on AD4116? It has a > > > "(pseudo differential or differential pair)" notation that other > > > inputs don't. In other words, it is more like VINCOM than it is to the > > > other ADCINxx pins. So we probably need an adcin15-supply for this pin > > > to properly get the right channel configuration. I.e. the logic in the > > > IIO driver would be if adcin15-supply is present, any channels that > > > use this input are pseudo-differential, otherwise any channels that > > > use it are fully differential. > > > > > > > I cannot seem to understand what would a adcin15-supply be needed for. > > This input, the same as all others, enters the mux and is routed to > > either positive or negative input of the ADC. > > > > The voltage on the ADCIN15 pin is not important to the user, just the > > difference in voltage between that pin and the other one selected. > > > > These suggestions come from some recent discussion about > pseudo-differential vs. fully differential inputs (e.g. search the IIO > mailing list for AD7380). > > So what I suggested here might be more technically correct according > to what I got out of that discussion. But for this specific case, I > agree it is good enough to just treat all inputs as always > fully-differential to keep things from getting too unwieldy. Hmm. That whole approach to pseudo differential does get messy if we have the common line routed through the main MUX rather than an opt in only on the negative side. If I read this right, its almost a trick to support a pseudo differential wiring with simple registers (I guess reflecting MUX limitations). So what could we do? We could assume that VINCOM is used like a conventional pseudo differential negative signal and have supply-vincom + non diffferential channels if that's the configuration wanted. Then for differential channels can support all the VINX VINX+1 and swapped options. For VIN10 it gets fun as non differential and differential options I think map to same actual config. Don't see reason we need to express that in the binding though so let that have VIN10 VINCOM (probably using a magic channel number) and VIN10 pseudo differential. Similar setup for ADCIN15 equivalent usage Code wise this probably won't be particular hard to support in the driver (obviously I haven't tried though :) is it worth the effort to keep it inline with other devices that support pseudo differential channesl. > > > >> items: > > >> minimum: 0 > > >> maximum: 31 > > >> @@ -166,7 +191,6 @@ allOf: > > >> - $ref: /schemas/spi/spi-peripheral-props.yaml# > > >> > > >> # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 > > >> - # Other models have [0-3] channel registers > > > > > > Did you forget to remove > > > > > > reg: > > > maximum: 3 > > > > > > from this if statement that this comment is referring to? > > > > > > > > > > > > Other way around, forgot in a previous patch to remove the comment. > > I'll move this change to a precursor patch. > > > > >> - if: > > >> properties: > > >> compatible: > > >> @@ -187,6 +211,37 @@ allOf: > > >> - vref > > >> - refout-avss > > >> - avdd > > >> + > > >> + - if: > > >> + properties: > > >> + compatible: > > >> + contains: > > >> + enum: > > >> + - adi,ad4114 > > >> + - adi,ad4115 > > >> + - adi,ad4116 > > >> + - adi,ad7173-8 > > >> + - adi,ad7175-8 > > >> + then: > > >> + patternProperties: > > >> + "^channel@[0-9a-f]$": > > >> + properties: > > >> + reg: > > >> + maximum: 15 > > > > > > As with the previous reg comment, this if statement should not be > > > needed since maximum should not be changed to 19. > > > > > > > We'll see what is the best approach regarding the current channels, > > perhaps the one you mentioned in the later reply with always configuring > > like the temp channel. > > That's an option as well. In many early drivers we just provided all the channels. Somewhat depends on whether people buy devices with lots of channels they don't wire. Mostly I suspect they don't as that's money wasted! Jonathan
On 06/04/2024 17:53, Jonathan Cameron wrote: > On Wed, 3 Apr 2024 10:40:39 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: >>> >>> On 01/04/2024 22:37, David Lechner wrote: >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >>>>> >>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >>> >>> ... >>> >>>>> properties: >>>>> reg: >>>>> + description: >>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. >>>>> minimum: 0 >>>>> - maximum: 15 >>>>> + maximum: 19 >>>> >>>> This looks wrong. Isn't reg describing the number of logical channels >>>> (# of channel config registers)? >>>> >>>> After reviewing the driver, I see that > 16 is used as a way of >>>> flagging current inputs, but still seems like the wrong way to do it. >>>> See suggestion below. >>>> >>> >>> This was a suggestion from Jonathan, maybe I implemented it wrong. > > Maybe Jonathan was wrong! I was younger then than now :) > > However, reg values for child nodes are unique so can't just use a flag these > need to be different values. > I do not see where the restriction appears when using just the flag, when defining the channels you would still define unique reg values. >>> Other alternative that came to my mind: attribute "adi,current-channel". >> >> Having a boolean flag like this would make more sense to me if we >> don't agree that the suggestion below is simpler. >> >>>>> >>>>> diff-channels: >>>>> + description: >>>>> + For using current channels specify only the positive channel. >>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0> >>>> >>>> I find this a bit confusing since 2 is already VIN2 and 0 is already >>>> VIN0. I think it would make more sense to assign unique channel >>>> numbers individually to the negative and positive current inputs. >>>> Also, I think it makes sense to use the same numbers that the >>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for >>>> positive). >>>> >>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10> >>>> >>>> >>> It would mean for the user to look in the datasheet at the possible >>> channel INPUT configurations values, decode the bit field into two >>> integer values and place it here (0110101010) -> 13 10. This is >>> cumbersome for just choosing current input 2. >> >> It could be documented in the devicetree bindings, just as it is done >> in adi,ad4130.yaml so that users of the bindings don't have to >> decipher the datasheet. > > The <13 10> option makes sense to me and avoids suggesting a common negative > input. > > The 'fun' bit here is that diff-channels doesn't actually tell us anything. > So we could just not provide it and rely on documentation of reg = 16-19 meaning > the current channels? > So a channel without diff-channels defined and reg=16 means IN0+ IN0-? >> >>> >>>>> + >>>>> + Family AD411x supports a dedicated VCOM voltage input. >>>>> + To select it set the second channel to 16. >>>>> + (VIN2, VCOM) -> diff-channels = <2 16> >>>> >>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a >>>> bit confusing. >>>> >>> >>> Sure, I'll rename to VINCOM. >>> >>>> Also, do we need to add a vincom-supply to get this voltage? Or is it >>>> safe to assume it is always connected to AVSS? The datasheet seems to >>>> indicate that the latter is the case. But then it also has this >>>> special case (at least for AD4116, didn't check all datasheets) >>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used >>>> as part of a fully differential input, we probably need some extra >>>> flag to indicate that case. >>>> >>> >>> I cannot see any configuration options for these use cases. All inputs >>> are routed to the same mux and routed to the differential positive and >>> negative ADC inputs. >>> >>> "VIN10, VINCOM (single-ended or differential pair)" the only difference >>> between these two use cases is if you connected VINCOM to AVSS (with >>> unipolar coding) or not with bipolar encoding. The channel is still >>> measuring the difference between the two selected inputs and comparing >>> to the selected reference. >>> >>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a >>>> "(pseudo differential or differential pair)" notation that other >>>> inputs don't. In other words, it is more like VINCOM than it is to the >>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin >>>> to properly get the right channel configuration. I.e. the logic in the >>>> IIO driver would be if adcin15-supply is present, any channels that >>>> use this input are pseudo-differential, otherwise any channels that >>>> use it are fully differential. >>>> >>> >>> I cannot seem to understand what would a adcin15-supply be needed for. >>> This input, the same as all others, enters the mux and is routed to >>> either positive or negative input of the ADC. >>> >>> The voltage on the ADCIN15 pin is not important to the user, just the >>> difference in voltage between that pin and the other one selected. >>> >> >> These suggestions come from some recent discussion about >> pseudo-differential vs. fully differential inputs (e.g. search the IIO >> mailing list for AD7380). >> >> So what I suggested here might be more technically correct according >> to what I got out of that discussion. But for this specific case, I >> agree it is good enough to just treat all inputs as always >> fully-differential to keep things from getting too unwieldy. > > Hmm. That whole approach to pseudo differential does get messy if > we have the common line routed through the main MUX rather than an opt > in only on the negative side. > > If I read this right, its almost a trick to support a pseudo differential > wiring with simple registers (I guess reflecting MUX limitations). > > So what could we do? > > We could assume that VINCOM is used like a conventional pseudo > differential negative signal and have supply-vincom + non diffferential > channels if that's the configuration wanted. > > Then for differential channels can support all the VINX VINX+1 > and swapped options. > For VIN10 it gets fun as non differential and differential options > I think map to same actual config. Don't see reason we need to express > that in the binding though so let that have VIN10 VINCOM (probably using > a magic channel number) and VIN10 pseudo differential. > > Similar setup for ADCIN15 equivalent usage > > Code wise this probably won't be particular hard to support in the driver > (obviously I haven't tried though :) is it worth the effort to keep > it inline with other devices that support pseudo differential channesl. Then this would need to be done to any fully differential ADC as support for pseudo differential channels is present (connect a fixed non 0 voltage to the negative input). The AD717x family supports pseudo differential channels as well... should this change be applied to them too? It is just the case that the documentation does not mentions this use case. I think that a distinction needs to be made here: - When a device is only pseudo differential, sure, it is in a different category - When a device is fully differential and you are using it as pseudo-differential you are having two inputs compared to one another I would need more clarification is why would supply-vincom be a requirement. The voltage supplied to VINCOM will not be used in any computation within the driver. From the perspective of getting the data it doesn't matter if you are using the channel in a pseudo-differential, single ended or fully differential manner. Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27: "Due to the matching resistors on the analog front end, the differential inputs must be paired together in the following pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and VIN6 and VIN7. If any two voltage inputs are paired in a configuration other than what is described in this data sheet, the accuracy of the device cannot be guaranteed." Tried the device and it works as fully differential when pairing any VINx with VINCOM. Still works when selecting VINCOM as the positive input of the ADC. I really see this as overly complicated and unnecessary. These families of ADCs are fully differential. If you are using it to measure a single ended (Be it compared to 0V or pseudo differential where you are comparing to Vref/2 and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing the common voltage.
On 06/04/2024 17:26, Jonathan Cameron wrote: > On Thu, 4 Apr 2024 16:08:56 +0300 > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > >> On 03/04/2024 18:22, David Lechner wrote: >>> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: >>>> On 02/04/2024 00:16, David Lechner wrote: >>>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote: >>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >>>>>>> >>>> ... >>>> >>>>>>> properties: >>>>>>> reg: >>>>>>> + description: >>>>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. >>>>>>> minimum: 0 >>>>>>> - maximum: 15 >>>>>>> + maximum: 19 >>>>>> This looks wrong. Isn't reg describing the number of logical channels >>>>>> (# of channel config registers)? >>>>>> >>>>>> After reviewing the driver, I see that > 16 is used as a way of >>>>>> flagging current inputs, but still seems like the wrong way to do it. >>>>>> See suggestion below. >>>>>> >>>>>>> diff-channels: >>>>>>> + description: >>>>>>> + For using current channels specify only the positive channel. >>>>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0> >>>>>> I find this a bit confusing since 2 is already VIN2 and 0 is already >>>>>> VIN0. I think it would make more sense to assign unique channel >>>>>> numbers individually to the negative and positive current inputs. >>>>>> Also, I think it makes sense to use the same numbers that the >>>>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for >>>>>> positive). >>>>>> >>>>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10> >>>>> Thinking about this a bit more... >>>>> >>>>> Since the current inputs have dedicated pins and aren't mix-and-match >>>>> with multiple valid wiring configurations like the voltage inputs, do >>>>> we even need to describe them in the devicetree? >>>>> >>>>> In the driver, the current channels would just be hard-coded like the >>>>> temperature channel since there isn't any application-specific >>>>> variation. >>>> Sure, but we still need to offer the user a way to configure which >>>> current inputs he wants and if they should use bipolar or unipolar coding. >>> From the datasheet, it looks like only positive current input is >>> allowed so I'm not sure bipolar applies here. But, yes, if there is >>> some other variation in wiring or electrical signal that needs to be >>> describe here, then it makes sense to allow a channel configuration >>> node for it. >> >> AD4111 datasheet pg.29: >> When the ADC is configured for bipolar operation, the output >> code is offset binary with a negative full-scale voltage resulting >> in a code of 000 … 000, a zero differential input voltage resulting in >> a code of 100 … 000, and a positive full-scale input voltage >> resulting in a code of 111 … 111. The output code for any >> analog input voltage can be represented as >> Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1) >> The output code for any input current is represented as >> Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1) >> >> I would say bipolar applies here, not a great idea because of the limitation on >> the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option >> is available. > Just to check I am correct in thinking you 'might' use bipolar if you want > to be able to measure small negative currents, but the range is much larger > in the positive direction? > > J Yes, exactly
On Tue, 9 Apr 2024 11:08:28 +0300 "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > On 06/04/2024 17:53, Jonathan Cameron wrote: > > On Wed, 3 Apr 2024 10:40:39 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > >> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > >>> > >>> On 01/04/2024 22:37, David Lechner wrote: > >>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > >>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >>>>> > >>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > >>> > >>> ... > >>> > >>>>> properties: > >>>>> reg: > >>>>> + description: > >>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. > >>>>> minimum: 0 > >>>>> - maximum: 15 > >>>>> + maximum: 19 > >>>> > >>>> This looks wrong. Isn't reg describing the number of logical channels > >>>> (# of channel config registers)? > >>>> > >>>> After reviewing the driver, I see that > 16 is used as a way of > >>>> flagging current inputs, but still seems like the wrong way to do it. > >>>> See suggestion below. > >>>> > >>> > >>> This was a suggestion from Jonathan, maybe I implemented it wrong. > > > > Maybe Jonathan was wrong! I was younger then than now :) > > > > However, reg values for child nodes are unique so can't just use a flag these > > need to be different values. > > > > I do not see where the restriction appears when using just the flag, when defining > the channels you would still define unique reg values. Good point. I'd got it into my head we had reg matching the channel index but that doesn't work anyway because those can repeat. Sorry for misdirection! > > >>> Other alternative that came to my mind: attribute "adi,current-channel". > >> > >> Having a boolean flag like this would make more sense to me if we > >> don't agree that the suggestion below is simpler. > >> > >>>>> > >>>>> diff-channels: > >>>>> + description: > >>>>> + For using current channels specify only the positive channel. > >>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0> > >>>> > >>>> I find this a bit confusing since 2 is already VIN2 and 0 is already > >>>> VIN0. I think it would make more sense to assign unique channel > >>>> numbers individually to the negative and positive current inputs. > >>>> Also, I think it makes sense to use the same numbers that the > >>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for > >>>> positive). > >>>> > >>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10> > >>>> > >>>> > >>> It would mean for the user to look in the datasheet at the possible > >>> channel INPUT configurations values, decode the bit field into two > >>> integer values and place it here (0110101010) -> 13 10. This is > >>> cumbersome for just choosing current input 2. > >> > >> It could be documented in the devicetree bindings, just as it is done > >> in adi,ad4130.yaml so that users of the bindings don't have to > >> decipher the datasheet. > > > > The <13 10> option makes sense to me and avoids suggesting a common negative > > input. > > > > The 'fun' bit here is that diff-channels doesn't actually tell us anything. > > So we could just not provide it and rely on documentation of reg = 16-19 meaning > > the current channels? > > > > So a channel without diff-channels defined and reg=16 means IN0+ IN0-? Yes, but with you correcting my error above maybe this isn't as clear cut as I'd falsely recalled. We do directly relate reg to channel numbers in drivers like the ad7292 (where not all channels are differential) I'm not convinced either way on what is best here where reg is currently just an index into a channel specification, not meaningful for which pins are involved. It doesn't seem worth adding an equivalent of diff-channels for a single channel setup but I guess it would be more consistent. > > >> > >>> > >>>>> + > >>>>> + Family AD411x supports a dedicated VCOM voltage input. > >>>>> + To select it set the second channel to 16. > >>>>> + (VIN2, VCOM) -> diff-channels = <2 16> > >>>> > >>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a > >>>> bit confusing. > >>>> > >>> > >>> Sure, I'll rename to VINCOM. > >>> > >>>> Also, do we need to add a vincom-supply to get this voltage? Or is it > >>>> safe to assume it is always connected to AVSS? The datasheet seems to > >>>> indicate that the latter is the case. But then it also has this > >>>> special case (at least for AD4116, didn't check all datasheets) > >>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used > >>>> as part of a fully differential input, we probably need some extra > >>>> flag to indicate that case. > >>>> > >>> > >>> I cannot see any configuration options for these use cases. All inputs > >>> are routed to the same mux and routed to the differential positive and > >>> negative ADC inputs. > >>> > >>> "VIN10, VINCOM (single-ended or differential pair)" the only difference > >>> between these two use cases is if you connected VINCOM to AVSS (with > >>> unipolar coding) or not with bipolar encoding. The channel is still > >>> measuring the difference between the two selected inputs and comparing > >>> to the selected reference. > >>> > >>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a > >>>> "(pseudo differential or differential pair)" notation that other > >>>> inputs don't. In other words, it is more like VINCOM than it is to the > >>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin > >>>> to properly get the right channel configuration. I.e. the logic in the > >>>> IIO driver would be if adcin15-supply is present, any channels that > >>>> use this input are pseudo-differential, otherwise any channels that > >>>> use it are fully differential. > >>>> > >>> > >>> I cannot seem to understand what would a adcin15-supply be needed for. > >>> This input, the same as all others, enters the mux and is routed to > >>> either positive or negative input of the ADC. > >>> > >>> The voltage on the ADCIN15 pin is not important to the user, just the > >>> difference in voltage between that pin and the other one selected. > >>> That statement is the root of disagreement I think. If they are wired for pseudo differential measurement ADCIN15 a reference voltage not a varying signal. It can equally be used as a negative channel of a differential pair. Not different from point of view of hardware config, but potentially different from point of view of how the analog wiring is done and how we may want to present it to userspace. > >> > >> These suggestions come from some recent discussion about > >> pseudo-differential vs. fully differential inputs (e.g. search the IIO > >> mailing list for AD7380). > >> > >> So what I suggested here might be more technically correct according > >> to what I got out of that discussion. But for this specific case, I > >> agree it is good enough to just treat all inputs as always > >> fully-differential to keep things from getting too unwieldy. > > > > Hmm. That whole approach to pseudo differential does get messy if > > we have the common line routed through the main MUX rather than an opt > > in only on the negative side. > > > > If I read this right, its almost a trick to support a pseudo differential > > wiring with simple registers (I guess reflecting MUX limitations). > > > > So what could we do? > > > > We could assume that VINCOM is used like a conventional pseudo > > differential negative signal and have supply-vincom + non diffferential > > channels if that's the configuration wanted. > > > > Then for differential channels can support all the VINX VINX+1 > > and swapped options. > > For VIN10 it gets fun as non differential and differential options > > I think map to same actual config. Don't see reason we need to express > > that in the binding though so let that have VIN10 VINCOM (probably using > > a magic channel number) and VIN10 pseudo differential. > > > > Similar setup for ADCIN15 equivalent usage > > > > Code wise this probably won't be particular hard to support in the driver > > (obviously I haven't tried though :) is it worth the effort to keep > > it inline with other devices that support pseudo differential channesl. > > Then this would need to be done to any fully differential ADC as support > for pseudo differential channels is present (connect a fixed non 0 voltage > to the negative input). Whilst you could argue that, I'd counter that a clearly stated pseudo differential mode with a simple choice of negative input (typically only one pin is used for these modes), is a feature of the ADC, rather than a wiring choice such as tying all negative inputs together and to a reference supply. > > The AD717x family supports pseudo differential channels as well... should > this change be applied to them too? It is just the case that the documentation > does not mentions this use case. Maybe you could argue that if we used the REF- for the negative input. Otherwise I think it falls into the category where there isn't a clearly defined pseudo differential mode. > > I think that a distinction needs to be made here: > - When a device is only pseudo differential, sure, it is in a different category > - When a device is fully differential and you are using it as pseudo-differential > you are having two inputs compared to one another > > I would need more clarification is why would supply-vincom be a requirement. > The voltage supplied to VINCOM will not be used in any computation within > the driver. From the perspective of getting the data it doesn't matter if > you are using the channel in a pseudo-differential, single ended or fully > differential manner. I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog ground so indeed nothing to turn on in this case and no offset to supply (the offset will be 0 so we don't present it). I'll note the datasheet describes the VINCOM as follows. Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured as single-ended. Connect AINCOM to analog ground. The reference to single ended is pretty clear hint to me that this case is not a differential channel. The more complex case is the one David raised of the AD4116 where we have actual pseudo differential inputs. > > Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27: > "Due to the matching resistors on the analog front end, the > differential inputs must be paired together in the following > pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and > VIN6 and VIN7. If any two voltage inputs are paired in a > configuration other than what is described in this data sheet, > the accuracy of the device cannot be guaranteed." OK, but I'll assume no 'good' customer of ADI will do that as any support engineer would grumpily point at that statement if they ever reported a problem :) > > Tried the device and it works as fully differential when pairing any > VINx with VINCOM. Still works when selecting VINCOM as the positive > input of the ADC. > > I really see this as overly complicated and unnecessary. These families > of ADCs are fully differential. If you are using it to measure a single ended > (Be it compared to 0V or pseudo differential where you are comparing to Vref/2 > and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing > the common voltage. For single ended VINCOM should be tied to analog 0V. If the chip docs allowed you to tie it to a different voltage then the single ended mode would be offset wrt to that value. For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because that is not connected to analog 0V. If the device is being used in a pseudo differential mode that provides a fixed offset voltage. So my preference (though I could maybe be convinced it's not worth the effort) is to treat pseudo differential as single ended channels where 'negative' pin is providing a fixed voltage (or 0V if that's relevant). Thus measurements provided to userspace include the information of that offset. We haven't handled pseudo differential channels that well in the past, but the recent discussions have lead to a cleaner overall solution and it would be good to be consistent going forwards. We could deprecate the previous bindings in existing drivers, but that is a job for another day (possibly never happens!) Jonathan > > >
On 13/04/2024 13:49, Jonathan Cameron wrote: > On Tue, 9 Apr 2024 11:08:28 +0300 > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > >> On 06/04/2024 17:53, Jonathan Cameron wrote: >>> On Wed, 3 Apr 2024 10:40:39 -0500 >>> David Lechner <dlechner@baylibre.com> wrote: >>> >>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: >>>>> >>>>> On 01/04/2024 22:37, David Lechner wrote: >>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >>>>>>> >>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >>>>> ... >> >>>>> Other alternative that came to my mind: attribute "adi,current-channel". >>>> >>>> Having a boolean flag like this would make more sense to me if we >>>> don't agree that the suggestion below is simpler. >>>> ... > > We do directly relate reg to channel numbers in drivers like the ad7292 (where not > all channels are differential) I'm not convinced either way on what is best > here where reg is currently just an index into a channel specification, not > meaningful for which pins are involved. > > It doesn't seem worth adding an equivalent of diff-channels for a single channel > setup but I guess it would be more consistent. > Would you agree with the attribute adi,current-channel within the channel and diff-channels set to the correspondent current inputs (13 10 for pair IN2)? >> >>>> >>>>> >>>>>>> + >>>>>>> + Family AD411x supports a dedicated VCOM voltage input. >>>>>>> + To select it set the second channel to 16. >>>>>>> + (VIN2, VCOM) -> diff-channels = <2 16> >>>>>> >>>>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a >>>>>> bit confusing. >>>>>> >>>>> >>>>> Sure, I'll rename to VINCOM. >>>>> >>>>>> Also, do we need to add a vincom-supply to get this voltage? Or is it >>>>>> safe to assume it is always connected to AVSS? The datasheet seems to >>>>>> indicate that the latter is the case. But then it also has this >>>>>> special case (at least for AD4116, didn't check all datasheets) >>>>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used >>>>>> as part of a fully differential input, we probably need some extra >>>>>> flag to indicate that case. >>>>>> >>>>> >>>>> I cannot see any configuration options for these use cases. All inputs >>>>> are routed to the same mux and routed to the differential positive and >>>>> negative ADC inputs. >>>>> >>>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference >>>>> between these two use cases is if you connected VINCOM to AVSS (with >>>>> unipolar coding) or not with bipolar encoding. The channel is still >>>>> measuring the difference between the two selected inputs and comparing >>>>> to the selected reference. >>>>> >>>>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a >>>>>> "(pseudo differential or differential pair)" notation that other >>>>>> inputs don't. In other words, it is more like VINCOM than it is to the >>>>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin >>>>>> to properly get the right channel configuration. I.e. the logic in the >>>>>> IIO driver would be if adcin15-supply is present, any channels that >>>>>> use this input are pseudo-differential, otherwise any channels that >>>>>> use it are fully differential. >>>>>> >>>>> >>>>> I cannot seem to understand what would a adcin15-supply be needed for. >>>>> This input, the same as all others, enters the mux and is routed to >>>>> either positive or negative input of the ADC. >>>>> >>>>> The voltage on the ADCIN15 pin is not important to the user, just the >>>>> difference in voltage between that pin and the other one selected. >>>>> > > That statement is the root of disagreement I think. > If they are wired for pseudo differential measurement ADCIN15 a reference voltage > not a varying signal. It can equally be used as a negative channel of > a differential pair. Not different from point of view of hardware > config, but potentially different from point of view of how the > analog wiring is done and how we may want to present it to userspace. > >>>> >>>> These suggestions come from some recent discussion about >>>> pseudo-differential vs. fully differential inputs (e.g. search the IIO >>>> mailing list for AD7380). >>>> >>>> So what I suggested here might be more technically correct according >>>> to what I got out of that discussion. But for this specific case, I >>>> agree it is good enough to just treat all inputs as always >>>> fully-differential to keep things from getting too unwieldy. >>> >>> Hmm. That whole approach to pseudo differential does get messy if >>> we have the common line routed through the main MUX rather than an opt >>> in only on the negative side. >>> >>> If I read this right, its almost a trick to support a pseudo differential >>> wiring with simple registers (I guess reflecting MUX limitations). >>> >>> So what could we do? >>> >>> We could assume that VINCOM is used like a conventional pseudo >>> differential negative signal and have supply-vincom + non diffferential >>> channels if that's the configuration wanted. >>> >>> Then for differential channels can support all the VINX VINX+1 >>> and swapped options. >>> For VIN10 it gets fun as non differential and differential options >>> I think map to same actual config. Don't see reason we need to express >>> that in the binding though so let that have VIN10 VINCOM (probably using >>> a magic channel number) and VIN10 pseudo differential. >>> >>> Similar setup for ADCIN15 equivalent usage >>> >>> Code wise this probably won't be particular hard to support in the driver >>> (obviously I haven't tried though :) is it worth the effort to keep >>> it inline with other devices that support pseudo differential channesl. >> >> Then this would need to be done to any fully differential ADC as support >> for pseudo differential channels is present (connect a fixed non 0 voltage >> to the negative input). > > Whilst you could argue that, I'd counter that a clearly stated pseudo > differential mode with a simple choice of negative input (typically > only one pin is used for these modes), is a feature of the ADC, rather > than a wiring choice such as tying all negative inputs together and to > a reference supply. > >> >> The AD717x family supports pseudo differential channels as well... should >> this change be applied to them too? It is just the case that the documentation >> does not mentions this use case. > > Maybe you could argue that if we used the REF- for the negative input. > Otherwise I think it falls into the category where there isn't a clearly defined > pseudo differential mode. > While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage: "Pseudo Differential Inputs The user can also choose to measure four different single-ended analog inputs. In this case, each of the analog inputs is converted as being the difference between the single-ended input to be measured and a set analog input common pin. Because there is a crosspoint multiplexer, the user can set any of the analog inputs as the common pin. An example of such a scenario is to connect the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS + 2.5 V) and select this input when configuring the crosspoint multiplexer. When using the AD7176-2 with pseudo differential inputs, the INL specification is degraded." As the crosspoint mux is present on all models it really makes me think that this paragraph applies to all models in the family >> >> I think that a distinction needs to be made here: >> - When a device is only pseudo differential, sure, it is in a different category >> - When a device is fully differential and you are using it as pseudo-differential >> you are having two inputs compared to one another >> >> I would need more clarification is why would supply-vincom be a requirement. >> The voltage supplied to VINCOM will not be used in any computation within >> the driver. From the perspective of getting the data it doesn't matter if >> you are using the channel in a pseudo-differential, single ended or fully >> differential manner. > > I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog > ground so indeed nothing to turn on in this case and no offset to supply > (the offset will be 0 so we don't present it). > > I'll note the datasheet describes the VINCOM as follows. > > Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured > as single-ended. Connect AINCOM to analog ground. > > The reference to single ended is pretty clear hint to me that this case > is not a differential channel. The more complex case is the one David > raised of the AD4116 where we have actual pseudo differential inputs. > Alright, from my perspective they all pass through the same mux but okay, not differential. The only issue would differentiating cases in AD4116 where the pair VIN10 - VINCOM is specified as single-ended or differential pair. Also, AD4116: "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair) 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair) 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair) 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)" Not really sure where the "actual pseudo differential" sits. Would you agree with having device tree flags that specifies how is the channel used: single-ended, pseudo-differential, differential. For the first two, the differential flag will not be set in IIO. >> >> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27: >> "Due to the matching resistors on the analog front end, the >> differential inputs must be paired together in the following >> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and >> VIN6 and VIN7. If any two voltage inputs are paired in a >> configuration other than what is described in this data sheet, >> the accuracy of the device cannot be guaranteed." > > OK, but I'll assume no 'good' customer of ADI will do that as any support > engineer would grumpily point at that statement if they ever reported > a problem :) > >> >> Tried the device and it works as fully differential when pairing any >> VINx with VINCOM. Still works when selecting VINCOM as the positive >> input of the ADC. >> >> I really see this as overly complicated and unnecessary. These families >> of ADCs are fully differential. If you are using it to measure a single ended >> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2 >> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing >> the common voltage. > > For single ended VINCOM should be tied to analog 0V. If the chip docs allowed > you to tie it to a different voltage then the single ended mode would be offset > wrt to that value. > > For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because > that is not connected to analog 0V. If the device is being used in a pseudo differential > mode that provides a fixed offset voltage. > > So my preference (though I could maybe be convinced it's not worth the effort) > is to treat pseudo differential as single ended channels where 'negative' pin is > providing a fixed voltage (or 0V if that's relevant). Thus measurements provided > to userspace include the information of that offset. > What do you mean by offset? I currently understand that the user will have a way of reading the voltage of that specific supply from the driver. If you mean provide a different channel offset value when using it as pseudo-differential then I would disagree > We haven't handled pseudo differential channels that well in the past, but the > recent discussions have lead to a cleaner overall solution and it would be good > to be consistent going forwards. We could deprecate the previous bindings in > existing drivers, but that is a job for another day (possibly never happens!) > I really hope that a clean solution could be obtained for this driver as well :)
On Mon, 15 Apr 2024 21:42:50 +0300 "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > On 13/04/2024 13:49, Jonathan Cameron wrote: > > On Tue, 9 Apr 2024 11:08:28 +0300 > > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > > > >> On 06/04/2024 17:53, Jonathan Cameron wrote: > >>> On Wed, 3 Apr 2024 10:40:39 -0500 > >>> David Lechner <dlechner@baylibre.com> wrote: > >>> > >>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > >>>>> > >>>>> On 01/04/2024 22:37, David Lechner wrote: > >>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > >>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >>>>>>> > >>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > >>>>> > ... > >> > >>>>> Other alternative that came to my mind: attribute "adi,current-channel". > >>>> > >>>> Having a boolean flag like this would make more sense to me if we > >>>> don't agree that the suggestion below is simpler. > >>>> > > ... > > > > > We do directly relate reg to channel numbers in drivers like the ad7292 (where not > > all channels are differential) I'm not convinced either way on what is best > > here where reg is currently just an index into a channel specification, not > > meaningful for which pins are involved. > > > > It doesn't seem worth adding an equivalent of diff-channels for a single channel > > setup but I guess it would be more consistent. > > > > Would you agree with the attribute adi,current-channel within the channel and > diff-channels set to the correspondent current inputs (13 10 for pair IN2)? From another thread today I've concluded we do need a single-channel equivalent of diff-channels, but you are right that here it is a differential channel so <13 10> seems like the best option to me. > > >> > >>>> > >>>>> > >>>>>>> + > >>>>>>> + Family AD411x supports a dedicated VCOM voltage input. > >>>>>>> + To select it set the second channel to 16. > >>>>>>> + (VIN2, VCOM) -> diff-channels = <2 16> > >>>>>> > >>>>>> The 411x datasheets call this pin VINCOM so calling it VCOM here is a > >>>>>> bit confusing. > >>>>>> > >>>>> > >>>>> Sure, I'll rename to VINCOM. > >>>>> > >>>>>> Also, do we need to add a vincom-supply to get this voltage? Or is it > >>>>>> safe to assume it is always connected to AVSS? The datasheet seems to > >>>>>> indicate that the latter is the case. But then it also has this > >>>>>> special case (at least for AD4116, didn't check all datasheets) > >>>>>> "VIN10, VINCOM (single-ended or differential pair)". If it can be used > >>>>>> as part of a fully differential input, we probably need some extra > >>>>>> flag to indicate that case. > >>>>>> > >>>>> > >>>>> I cannot see any configuration options for these use cases. All inputs > >>>>> are routed to the same mux and routed to the differential positive and > >>>>> negative ADC inputs. > >>>>> > >>>>> "VIN10, VINCOM (single-ended or differential pair)" the only difference > >>>>> between these two use cases is if you connected VINCOM to AVSS (with > >>>>> unipolar coding) or not with bipolar encoding. The channel is still > >>>>> measuring the difference between the two selected inputs and comparing > >>>>> to the selected reference. > >>>>> > >>>>>> Similarly, do we need special handling for ADCIN15 on AD4116? It has a > >>>>>> "(pseudo differential or differential pair)" notation that other > >>>>>> inputs don't. In other words, it is more like VINCOM than it is to the > >>>>>> other ADCINxx pins. So we probably need an adcin15-supply for this pin > >>>>>> to properly get the right channel configuration. I.e. the logic in the > >>>>>> IIO driver would be if adcin15-supply is present, any channels that > >>>>>> use this input are pseudo-differential, otherwise any channels that > >>>>>> use it are fully differential. > >>>>>> > >>>>> > >>>>> I cannot seem to understand what would a adcin15-supply be needed for. > >>>>> This input, the same as all others, enters the mux and is routed to > >>>>> either positive or negative input of the ADC. > >>>>> > >>>>> The voltage on the ADCIN15 pin is not important to the user, just the > >>>>> difference in voltage between that pin and the other one selected. > >>>>> > > > > That statement is the root of disagreement I think. > > If they are wired for pseudo differential measurement ADCIN15 a reference voltage > > not a varying signal. It can equally be used as a negative channel of > > a differential pair. Not different from point of view of hardware > > config, but potentially different from point of view of how the > > analog wiring is done and how we may want to present it to userspace. > > > >>>> > >>>> These suggestions come from some recent discussion about > >>>> pseudo-differential vs. fully differential inputs (e.g. search the IIO > >>>> mailing list for AD7380). > >>>> > >>>> So what I suggested here might be more technically correct according > >>>> to what I got out of that discussion. But for this specific case, I > >>>> agree it is good enough to just treat all inputs as always > >>>> fully-differential to keep things from getting too unwieldy. > >>> > >>> Hmm. That whole approach to pseudo differential does get messy if > >>> we have the common line routed through the main MUX rather than an opt > >>> in only on the negative side. > >>> > >>> If I read this right, its almost a trick to support a pseudo differential > >>> wiring with simple registers (I guess reflecting MUX limitations). > >>> > >>> So what could we do? > >>> > >>> We could assume that VINCOM is used like a conventional pseudo > >>> differential negative signal and have supply-vincom + non diffferential > >>> channels if that's the configuration wanted. > >>> > >>> Then for differential channels can support all the VINX VINX+1 > >>> and swapped options. > >>> For VIN10 it gets fun as non differential and differential options > >>> I think map to same actual config. Don't see reason we need to express > >>> that in the binding though so let that have VIN10 VINCOM (probably using > >>> a magic channel number) and VIN10 pseudo differential. > >>> > >>> Similar setup for ADCIN15 equivalent usage > >>> > >>> Code wise this probably won't be particular hard to support in the driver > >>> (obviously I haven't tried though :) is it worth the effort to keep > >>> it inline with other devices that support pseudo differential channesl. > >> > >> Then this would need to be done to any fully differential ADC as support > >> for pseudo differential channels is present (connect a fixed non 0 voltage > >> to the negative input). > > > > Whilst you could argue that, I'd counter that a clearly stated pseudo > > differential mode with a simple choice of negative input (typically > > only one pin is used for these modes), is a feature of the ADC, rather > > than a wiring choice such as tying all negative inputs together and to > > a reference supply. > > > >> > >> The AD717x family supports pseudo differential channels as well... should > >> this change be applied to them too? It is just the case that the documentation > >> does not mentions this use case. > > > > Maybe you could argue that if we used the REF- for the negative input. > > Otherwise I think it falls into the category where there isn't a clearly defined > > pseudo differential mode. > > > > While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage: > "Pseudo Differential Inputs > The user can also choose to measure four different single-ended > analog inputs. In this case, each of the analog inputs is converted > as being the difference between the single-ended input to be > measured and a set analog input common pin. Because there is > a crosspoint multiplexer, the user can set any of the analog inputs > as the common pin. An example of such a scenario is to connect > the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS > + 2.5 V) and select this input when configuring the crosspoint > multiplexer. When using the AD7176-2 with pseudo differential > inputs, the INL specification is degraded." > > As the crosspoint mux is present on all models it really makes me think that this > paragraph applies to all models in the family Interesting indeed. So is your thinking that we need to support this or take that "degraded" comment to imply that we should not bother (at least until someone actually shouts that they want to do this?) > > >> > >> I think that a distinction needs to be made here: > >> - When a device is only pseudo differential, sure, it is in a different category > >> - When a device is fully differential and you are using it as pseudo-differential > >> you are having two inputs compared to one another > >> > >> I would need more clarification is why would supply-vincom be a requirement. > >> The voltage supplied to VINCOM will not be used in any computation within > >> the driver. From the perspective of getting the data it doesn't matter if > >> you are using the channel in a pseudo-differential, single ended or fully > >> differential manner. > > > > I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog > > ground so indeed nothing to turn on in this case and no offset to supply > > (the offset will be 0 so we don't present it). > > > > I'll note the datasheet describes the VINCOM as follows. > > > > Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured > > as single-ended. Connect AINCOM to analog ground. > > > > The reference to single ended is pretty clear hint to me that this case > > is not a differential channel. The more complex case is the one David > > raised of the AD4116 where we have actual pseudo differential inputs. > > > > Alright, from my perspective they all pass through the same mux but okay, > not differential. The only issue would differentiating cases in AD4116 where > the pair VIN10 - VINCOM is specified as single-ended or differential pair. > > Also, AD4116: > "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair) > 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair) > 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair) > 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)" > > Not really sure where the "actual pseudo differential" sits. > > Would you agree with having device tree flags that specifies how is the > channel used: single-ended, pseudo-differential, differential. > For the first two, the differential flag will not be set in IIO. Yes. I think that makes sense - though as you observe in some cases the actual device settings end up the same (the ad4116 note above). If a given channel supports single-ended and pseudo-differential is that really just a low reference change (I assume from an input to the the IO ground)? Or is there more going on? If it's the reference, then can we provide that as the binding control signal? We have other drivers that do that (though we could perhaps make it more generic) e.g. adi,ad7124 with adi,reference-select I don't like that binding because it always ends up have a local enum of values, but can't really think of a better solution. > > >> > >> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27: > >> "Due to the matching resistors on the analog front end, the > >> differential inputs must be paired together in the following > >> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and > >> VIN6 and VIN7. If any two voltage inputs are paired in a > >> configuration other than what is described in this data sheet, > >> the accuracy of the device cannot be guaranteed." > > > > OK, but I'll assume no 'good' customer of ADI will do that as any support > > engineer would grumpily point at that statement if they ever reported > > a problem :) > > > >> > >> Tried the device and it works as fully differential when pairing any > >> VINx with VINCOM. Still works when selecting VINCOM as the positive > >> input of the ADC. > >> > >> I really see this as overly complicated and unnecessary. These families > >> of ADCs are fully differential. If you are using it to measure a single ended > >> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2 > >> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing > >> the common voltage. > > > > For single ended VINCOM should be tied to analog 0V. If the chip docs allowed > > you to tie it to a different voltage then the single ended mode would be offset > > wrt to that value. > > > > For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because > > that is not connected to analog 0V. If the device is being used in a pseudo differential > > mode that provides a fixed offset voltage. > > > > So my preference (though I could maybe be convinced it's not worth the effort) > > is to treat pseudo differential as single ended channels where 'negative' pin is > > providing a fixed voltage (or 0V if that's relevant). Thus measurements provided > > to userspace include the information of that offset. > > > > What do you mean by offset? I currently understand that the user will have > a way of reading the voltage of that specific supply from the driver. How? We could do it that way, but we don't have existing ABI for this that I can think of. > > If you mean provide a different channel offset value when using it as > pseudo-differential then I would disagree Provided to user space as _offset on the channel, userspace can either incorporate it if it wants to compute absolute (relative to some 0V somewhere) value or ignore it if it only wants the difference from the reference value. I'm open to discussion other ABI options, but this is the one we most naturally have available. > > > > We haven't handled pseudo differential channels that well in the past, but the > > recent discussions have lead to a cleaner overall solution and it would be good > > to be consistent going forwards. We could deprecate the previous bindings in > > existing drivers, but that is a job for another day (possibly never happens!) > > > > I really hope that a clean solution could be obtained for this driver as well :) I bet you wish sometimes that you had easier parts to write drivers for! :) These continue to stretch the boundaries which is good, but slow. Jonathan > >
On 20/04/2024 17:33, Jonathan Cameron wrote: > On Mon, 15 Apr 2024 21:42:50 +0300 > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > >> On 13/04/2024 13:49, Jonathan Cameron wrote: >>> On Tue, 9 Apr 2024 11:08:28 +0300 >>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: >>> >>>> On 06/04/2024 17:53, Jonathan Cameron wrote: >>>>> On Wed, 3 Apr 2024 10:40:39 -0500 >>>>> David Lechner <dlechner@baylibre.com> wrote: >>>>> >>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: >>>>>>> >>>>>>> On 01/04/2024 22:37, David Lechner wrote: >>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >>>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >>>>>>>>> >>>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >>>>>>> ... >>>> >>>> The AD717x family supports pseudo differential channels as well... should >>>> this change be applied to them too? It is just the case that the documentation >>>> does not mentions this use case. >>> >>> Maybe you could argue that if we used the REF- for the negative input. >>> Otherwise I think it falls into the category where there isn't a clearly defined >>> pseudo differential mode. >>> >> >> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage: >> "Pseudo Differential Inputs >> The user can also choose to measure four different single-ended >> analog inputs. In this case, each of the analog inputs is converted >> as being the difference between the single-ended input to be >> measured and a set analog input common pin. Because there is >> a crosspoint multiplexer, the user can set any of the analog inputs >> as the common pin. An example of such a scenario is to connect >> the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS >> + 2.5 V) and select this input when configuring the crosspoint >> multiplexer. When using the AD7176-2 with pseudo differential >> inputs, the INL specification is degraded." >> >> As the crosspoint mux is present on all models it really makes me think that this >> paragraph applies to all models in the family > > Interesting indeed. So is your thinking that we need to support this > or take that "degraded" comment to imply that we should not bother > (at least until someone actually shouts that they want to do this?) > My perspective is that support for this is already existent, the chips do not need any special configuration in that use-case. If we want to be correct in how the channel will be presented to the user, besides setting to false the IIO differential flag I do not see what else should be done. >> >>>> >>>> I think that a distinction needs to be made here: >>>> - When a device is only pseudo differential, sure, it is in a different category >>>> - When a device is fully differential and you are using it as pseudo-differential >>>> you are having two inputs compared to one another >>>> >>>> I would need more clarification is why would supply-vincom be a requirement. >>>> The voltage supplied to VINCOM will not be used in any computation within >>>> the driver. From the perspective of getting the data it doesn't matter if >>>> you are using the channel in a pseudo-differential, single ended or fully >>>> differential manner. >>> >>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog >>> ground so indeed nothing to turn on in this case and no offset to supply >>> (the offset will be 0 so we don't present it). >>> >>> I'll note the datasheet describes the VINCOM as follows. >>> >>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured >>> as single-ended. Connect AINCOM to analog ground. >>> >>> The reference to single ended is pretty clear hint to me that this case >>> is not a differential channel. The more complex case is the one David >>> raised of the AD4116 where we have actual pseudo differential inputs. >>> >> >> Alright, from my perspective they all pass through the same mux but okay, >> not differential. The only issue would differentiating cases in AD4116 where >> the pair VIN10 - VINCOM is specified as single-ended or differential pair. >> >> Also, AD4116: >> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair) >> 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair) >> 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair) >> 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)" >> >> Not really sure where the "actual pseudo differential" sits. >> >> Would you agree with having device tree flags that specifies how is the >> channel used: single-ended, pseudo-differential, differential. >> For the first two, the differential flag will not be set in IIO. > > Yes. I think that makes sense - though as you observe in some cases > the actual device settings end up the same (the ad4116 note above). > This precisely why I suggest this approach, because a channel used as single-ended, pseudo or fully differential will have the same register configuration on all models. I do not see any other way to know from the driver this information. > If a given channel supports single-ended and pseudo-differential is > that really just a low reference change (I assume from an input to the > the IO ground)? Or is there more going on? > I'm not sure if I understood what was said here. The reference specified in the channel setup does not need to change. > If it's the reference, then can we provide that as the binding control > signal? We have other drivers that do that (though we could perhaps make > it more generic) e.g. adi,ad7124 with adi,reference-select > We already have adi,reference-select in the binding and driver, I do not see how it could help the driver differentiate between (single, pseudo...) > I don't like that binding because it always ends up have a local enum > of values, but can't really think of a better solution. > >> >>>> >>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27: >>>> "Due to the matching resistors on the analog front end, the >>>> differential inputs must be paired together in the following >>>> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and >>>> VIN6 and VIN7. If any two voltage inputs are paired in a >>>> configuration other than what is described in this data sheet, >>>> the accuracy of the device cannot be guaranteed." >>> >>> OK, but I'll assume no 'good' customer of ADI will do that as any support >>> engineer would grumpily point at that statement if they ever reported >>> a problem :) >>> >>>> >>>> Tried the device and it works as fully differential when pairing any >>>> VINx with VINCOM. Still works when selecting VINCOM as the positive >>>> input of the ADC. >>>> >>>> I really see this as overly complicated and unnecessary. These families >>>> of ADCs are fully differential. If you are using it to measure a single ended >>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2 >>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing >>>> the common voltage. >>> >>> For single ended VINCOM should be tied to analog 0V. If the chip docs allowed >>> you to tie it to a different voltage then the single ended mode would be offset >>> wrt to that value. >>> >>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because >>> that is not connected to analog 0V. If the device is being used in a pseudo differential >>> mode that provides a fixed offset voltage. >>> >>> So my preference (though I could maybe be convinced it's not worth the effort) >>> is to treat pseudo differential as single ended channels where 'negative' pin is >>> providing a fixed voltage (or 0V if that's relevant). Thus measurements provided >>> to userspace include the information of that offset. >>> >> >> What do you mean by offset? I currently understand that the user will have >> a way of reading the voltage of that specific supply from the driver. > > How? We could do it that way, but we don't have existing ABI for this that > I can think of. > Expose a voltage channel which is not reading from the device...but that is too much of a hack to be accepted here >> >> If you mean provide a different channel offset value when using it as >> pseudo-differential then I would disagree > > Provided to user space as _offset on the channel, userspace can either > incorporate it if it wants to compute absolute (relative to some 0V somewhere) value > or ignore it if it only wants the difference from the reference value. > > I'm open to discussion other ABI options, but this is the one we most naturally have > available. _offset is already used when the bipolar coding is enabled on the channel and is computed along datasheet specifications of how data should be processed, this is why I disagree with this. This feels over-engineered, most of the times if a channel is pseudo differential, the relevant measurement will be the differences between those two inputs. If a user needs to know the voltage on the common input, he just needs to also configure a single ended channel with the common input where the negative AIN is connected to AVSS. >> >> >>> We haven't handled pseudo differential channels that well in the past, but the >>> recent discussions have lead to a cleaner overall solution and it would be good >>> to be consistent going forwards. We could deprecate the previous bindings in >>> existing drivers, but that is a job for another day (possibly never happens!) >>> >> >> I really hope that a clean solution could be obtained for this driver as well :) > > I bet you wish sometimes that you had easier parts to write drivers for! :) > These continue to stretch the boundaries which is good, but slow. > > Jonathan Not easier, fewer crammed into the same driver :)
On Tue, 23 Apr 2024 11:18:47 +0300 "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > On 20/04/2024 17:33, Jonathan Cameron wrote: > > On Mon, 15 Apr 2024 21:42:50 +0300 > > "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > > > >> On 13/04/2024 13:49, Jonathan Cameron wrote: > >>> On Tue, 9 Apr 2024 11:08:28 +0300 > >>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote: > >>> > >>>> On 06/04/2024 17:53, Jonathan Cameron wrote: > >>>>> On Wed, 3 Apr 2024 10:40:39 -0500 > >>>>> David Lechner <dlechner@baylibre.com> wrote: > >>>>> > >>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > >>>>>>> > >>>>>>> On 01/04/2024 22:37, David Lechner wrote: > >>>>>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > >>>>>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >>>>>>>>> > >>>>>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > >>>>>>> > > ... > > >>>> > >>>> The AD717x family supports pseudo differential channels as well... should > >>>> this change be applied to them too? It is just the case that the documentation > >>>> does not mentions this use case. > >>> > >>> Maybe you could argue that if we used the REF- for the negative input. > >>> Otherwise I think it falls into the category where there isn't a clearly defined > >>> pseudo differential mode. > >>> > >> > >> While re-reading docs I've noticed that AD7176-2 mentions pseudo differential usage: > >> "Pseudo Differential Inputs > >> The user can also choose to measure four different single-ended > >> analog inputs. In this case, each of the analog inputs is converted > >> as being the difference between the single-ended input to be > >> measured and a set analog input common pin. Because there is > >> a crosspoint multiplexer, the user can set any of the analog inputs > >> as the common pin. An example of such a scenario is to connect > >> the AIN4 pin to AVSS or to the REFOUT voltage (that is, AVSS > >> + 2.5 V) and select this input when configuring the crosspoint > >> multiplexer. When using the AD7176-2 with pseudo differential > >> inputs, the INL specification is degraded." > >> > >> As the crosspoint mux is present on all models it really makes me think that this > >> paragraph applies to all models in the family > > > > Interesting indeed. So is your thinking that we need to support this > > or take that "degraded" comment to imply that we should not bother > > (at least until someone actually shouts that they want to do this?) > > > > My perspective is that support for this is already existent, the chips do not > need any special configuration in that use-case. If we want to be correct in > how the channel will be presented to the user, besides setting to false the IIO > differential flag I do not see what else should be done. ah. The degraded bit bothered me. That wording makes me thing no effort should be applied to support this unless a user shouts that they really want it. If we get it for free or near free than all is good!. > > >> > >>>> > >>>> I think that a distinction needs to be made here: > >>>> - When a device is only pseudo differential, sure, it is in a different category > >>>> - When a device is fully differential and you are using it as pseudo-differential > >>>> you are having two inputs compared to one another > >>>> > >>>> I would need more clarification is why would supply-vincom be a requirement. > >>>> The voltage supplied to VINCOM will not be used in any computation within > >>>> the driver. From the perspective of getting the data it doesn't matter if > >>>> you are using the channel in a pseudo-differential, single ended or fully > >>>> differential manner. > >>> > >>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog > >>> ground so indeed nothing to turn on in this case and no offset to supply > >>> (the offset will be 0 so we don't present it). > >>> > >>> I'll note the datasheet describes the VINCOM as follows. > >>> > >>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured > >>> as single-ended. Connect AINCOM to analog ground. > >>> > >>> The reference to single ended is pretty clear hint to me that this case > >>> is not a differential channel. The more complex case is the one David > >>> raised of the AD4116 where we have actual pseudo differential inputs. > >>> > >> > >> Alright, from my perspective they all pass through the same mux but okay, > >> not differential. The only issue would differentiating cases in AD4116 where > >> the pair VIN10 - VINCOM is specified as single-ended or differential pair. > >> > >> Also, AD4116: > >> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair) > >> 0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair) > >> 0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair) > >> 0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)" > >> > >> Not really sure where the "actual pseudo differential" sits. > >> > >> Would you agree with having device tree flags that specifies how is the > >> channel used: single-ended, pseudo-differential, differential. > >> For the first two, the differential flag will not be set in IIO. > > > > Yes. I think that makes sense - though as you observe in some cases > > the actual device settings end up the same (the ad4116 note above). > > > This precisely why I suggest this approach, because a channel used as > single-ended, pseudo or fully differential will have the same register > configuration on all models. I do not see any other way to know from > the driver this information. > > > If a given channel supports single-ended and pseudo-differential is > > that really just a low reference change (I assume from an input to the > > the IO ground)? Or is there more going on? > > > I'm not sure if I understood what was said here. The reference specified > in the channel setup does not need to change. So what is the effective difference? My assumption was that single-ended means reference to 0V in all cases. Pseudo differential means reference to an input that is common across multiple channels, but not necessarily 0V? > > > If it's the reference, then can we provide that as the binding control > > signal? We have other drivers that do that (though we could perhaps make > > it more generic) e.g. adi,ad7124 with adi,reference-select > > We already have adi,reference-select in the binding and driver, I do not > see how it could help the driver differentiate between (single, pseudo...) Indeed that doesn't work. Problem in this discussion is I've normally forgotten the earlier discussion when I come back to it :( > > > I don't like that binding because it always ends up have a local enum > > of values, but can't really think of a better solution. > > > > >> > >>>> > >>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27: > >>>> "Due to the matching resistors on the analog front end, the > >>>> differential inputs must be paired together in the following > >>>> pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and > >>>> VIN6 and VIN7. If any two voltage inputs are paired in a > >>>> configuration other than what is described in this data sheet, > >>>> the accuracy of the device cannot be guaranteed." > >>> > >>> OK, but I'll assume no 'good' customer of ADI will do that as any support > >>> engineer would grumpily point at that statement if they ever reported > >>> a problem :) > >>> > >>>> > >>>> Tried the device and it works as fully differential when pairing any > >>>> VINx with VINCOM. Still works when selecting VINCOM as the positive > >>>> input of the ADC. > >>>> > >>>> I really see this as overly complicated and unnecessary. These families > >>>> of ADCs are fully differential. If you are using it to measure a single ended > >>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2 > >>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing > >>>> the common voltage. > >>> > >>> For single ended VINCOM should be tied to analog 0V. If the chip docs allowed > >>> you to tie it to a different voltage then the single ended mode would be offset > >>> wrt to that value. > >>> > >>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because > >>> that is not connected to analog 0V. If the device is being used in a pseudo differential > >>> mode that provides a fixed offset voltage. > >>> > >>> So my preference (though I could maybe be convinced it's not worth the effort) > >>> is to treat pseudo differential as single ended channels where 'negative' pin is > >>> providing a fixed voltage (or 0V if that's relevant). Thus measurements provided > >>> to userspace include the information of that offset. > >>> > >> > >> What do you mean by offset? I currently understand that the user will have > >> a way of reading the voltage of that specific supply from the driver. > > > > How? We could do it that way, but we don't have existing ABI for this that > > I can think of. > > > Expose a voltage channel which is not reading from the device...but that is > too much of a hack to be accepted here We have done things like that for a few corner cases where we were really stuck but it is indeed nasty and hard to comprehend. Also so far they've been 'out' channels I think which doesn't make sense here. > >> > >> If you mean provide a different channel offset value when using it as > >> pseudo-differential then I would disagree > > > > Provided to user space as _offset on the channel, userspace can either > > incorporate it if it wants to compute absolute (relative to some 0V somewhere) value > > or ignore it if it only wants the difference from the reference value. > > > > I'm open to discussion other ABI options, but this is the one we most naturally have > > available. > _offset is already used when the bipolar coding is enabled on the channel > and is computed along datasheet specifications of how data should be processed, > this is why I disagree with this. OK. It would be easy enough to apply an offset to that, but it would complicate the driver and could seem a little odd. > > This feels over-engineered, most of the times if a channel is pseudo > differential, the relevant measurement will be the differences between > those two inputs. > > If a user needs to know the voltage on the common input, he just needs to > also configure a single ended channel with the common input where the > negative AIN is connected to AVSS. OK. I'm somewhat convinced that this is enough of a pain to describe that we should just rely on them having some other way to get that offset if they are deliberately using it to shift the range. We can revisit if it ever becomes a problem. So, I think the conclusion is just don't represent AIN-COMM (or similar) as an explicit voltage we can measure. > >> > >> > >>> We haven't handled pseudo differential channels that well in the past, but the > >>> recent discussions have lead to a cleaner overall solution and it would be good > >>> to be consistent going forwards. We could deprecate the previous bindings in > >>> existing drivers, but that is a job for another day (possibly never happens!) > >>> > >> > >> I really hope that a clean solution could be obtained for this driver as well :) > > > > I bet you wish sometimes that you had easier parts to write drivers for! :) > > These continue to stretch the boundaries which is good, but slow. > > > > Jonathan > > Not easier, fewer crammed into the same driver :) I sympathise! It's been an annoyingly busy kernel cycle in the day job. I was hoping to get back to you sooner so that more of this was fresh(ish) in my mind :( My gut feeling is that this is a case for documentation / really detailed cover letter for the next version to make sure we have come to at least a (mostly) consistent conclusion. Jonathan
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml index ea6cfcd0aff4..bba2de0a52f3 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml @@ -19,7 +19,18 @@ description: | primarily for measurement of signals close to DC but also delivers outstanding performance with input bandwidths out to ~10kHz. + Analog Devices AD411x ADC's: + The AD411X family encompasses a series of low power, low noise, 24-bit, + sigma-delta analog-to-digital converters that offer a versatile range of + specifications. They integrate an analog front end suitable for processing + fully differential/single-ended and bipolar voltage inputs. + Datasheets for supported chips: + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf @@ -31,6 +42,11 @@ description: | properties: compatible: enum: + - adi,ad4111 + - adi,ad4112 + - adi,ad4114 + - adi,ad4115 + - adi,ad4116 - adi,ad7172-2 - adi,ad7172-4 - adi,ad7173-8 @@ -125,10 +141,19 @@ patternProperties: properties: reg: + description: + Reg values 16-19 are only permitted for ad4111/ad4112 current channels. minimum: 0 - maximum: 15 + maximum: 19 diff-channels: + description: + For using current channels specify only the positive channel. + (IIN2+, IIN2−) -> diff-channels = <2 0> + + Family AD411x supports a dedicated VCOM voltage input. + To select it set the second channel to 16. + (VIN2, VCOM) -> diff-channels = <2 16> items: minimum: 0 maximum: 31 @@ -166,7 +191,6 @@ allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# # Only ad7172-4, ad7173-8 and ad7175-8 support vref2 - # Other models have [0-3] channel registers - if: properties: compatible: @@ -187,6 +211,37 @@ allOf: - vref - refout-avss - avdd + + - if: + properties: + compatible: + contains: + enum: + - adi,ad4114 + - adi,ad4115 + - adi,ad4116 + - adi,ad7173-8 + - adi,ad7175-8 + then: + patternProperties: + "^channel@[0-9a-f]$": + properties: + reg: + maximum: 15 + + - if: + properties: + compatible: + contains: + enum: + - adi,ad7172-2 + - adi,ad7175-2 + - adi,ad7176-2 + - adi,ad7177-2 + then: + patternProperties: + "^channel@[0-9a-f]$": + properties: reg: maximum: 3