| Message ID | 20240208172459.280189-5-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7192: Add support for AD7194 | expand |
Hey, On Thu, Feb 08, 2024 at 07:24:58PM +0200, Alisa-Dariana Roman wrote: > +patternProperties: > + "^channel@([0-7a-f])$": > + type: object > + $ref: adc.yaml > + unevaluatedProperties: false > + > + properties: > + reg: > + description: The channel index. > + minimum: 0 > + maximum: 7 There are only 8 possible channels, at indices 0 to 7, so why is the pattern property more permissive than that? Shouldn't "^channel@[0-7]$" suffice? > + > + diff-channels: > + description: | > + The differential channel pair for Ad7194 configurable channels. The > + first channel is the positive input, the second channel is the > + negative input. This duplicates the description in adc.yaml > + items: > + minimum: 1 > + maximum: 16 Hmm, this makes me wonder: why doesn't this match the number of channels available and why is 0 not a valid channel for differential measurements? Thanks, Conor.
On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman <alisadariana@gmail.com> wrote: > > Unlike the other AD719Xs, AD7194 has configurable differential > channels. The default configuration for these channels can be changed > from the devicetree. > > Also add an example for AD7194 devicetree. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > --- > .../bindings/iio/adc/adi,ad7192.yaml | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > index 16def2985ab4..169bdd1dd0e1 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > @@ -21,8 +21,15 @@ properties: > - adi,ad7190 > - adi,ad7192 > - adi,ad7193 > + - adi,ad7194 > - adi,ad7195 > > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > reg: > maxItems: 1 > > @@ -96,8 +103,44 @@ required: > - spi-cpol > - spi-cpha > > +patternProperties: > + "^channel@([0-7a-f])$": > + type: object > + $ref: adc.yaml > + unevaluatedProperties: false > + > + properties: > + reg: > + description: The channel index. > + minimum: 0 > + maximum: 7 Should the max be 16 since in pseudo-differential mode there can be up to 16 inputs? > + > + diff-channels: > + description: | > + The differential channel pair for Ad7194 configurable channels. The all caps: AD7194 > + first channel is the positive input, the second channel is the > + negative input. > + items: > + minimum: 1 > + maximum: 16 As someone who would need to write a .dts based on these bindings, the information I would find helpful in the description here is that this is specifying how the logical channels are mapped to the 16 possible input pins. It seems safe to assume that the values 1 to 16 correspond to the pins AIN1 to AIN16, but it would be nice to say this explicitly. And what do we do when using pseudo-differential inputs where AINCOM is used as the negative input? Use 0? > + > + required: > + - reg > + - diff-channels > + > allOf: > - $ref: /schemas/spi/spi-peripheral-props.yaml# > + - if: > + properties: > + compatible: > + enum: > + - adi,ad7190 > + - adi,ad7192 > + - adi,ad7193 > + - adi,ad7195 > + then: > + patternProperties: > + "^channel@([0-7a-f])$": false > > unevaluatedProperties: false > > @@ -127,3 +170,35 @@ examples: > adi,burnout-currents-enable; > }; > }; > + - | > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "adi,ad7194"; > + reg = <0>; > + spi-max-frequency = <1000000>; > + spi-cpol; > + spi-cpha; > + clocks = <&ad7192_mclk>; > + clock-names = "mclk"; > + interrupts = <25 0x2>; > + interrupt-parent = <&gpio>; > + dvdd-supply = <&dvdd>; > + avdd-supply = <&avdd>; > + vref-supply = <&vref>; > + > + channel@0 { > + reg = <0>; > + diff-channels = <1 6>; > + }; > + > + channel@1 { > + reg = <1>; > + diff-channels = <2 5>; > + }; > + }; > + }; > -- > 2.34.1 >
On 08.02.2024 20:03, Conor Dooley wrote: > Hey, > > On Thu, Feb 08, 2024 at 07:24:58PM +0200, Alisa-Dariana Roman wrote: > >> +patternProperties: >> + "^channel@([0-7a-f])$": >> + type: object >> + $ref: adc.yaml >> + unevaluatedProperties: false >> + >> + properties: >> + reg: >> + description: The channel index. >> + minimum: 0 >> + maximum: 7 > > There are only 8 possible channels, at indices 0 to 7, so why is the > pattern property more permissive than that? Shouldn't "^channel@[0-7]$" > suffice? > >> + >> + diff-channels: > >> + description: | >> + The differential channel pair for Ad7194 configurable channels. The >> + first channel is the positive input, the second channel is the >> + negative input. > > This duplicates the description in adc.yaml > >> + items: >> + minimum: 1 >> + maximum: 16 > > Hmm, this makes me wonder: why doesn't this match the number of channels > available and why is 0 not a valid channel for differential measurements? > > Thanks, > Conor. Hello and thank you for the feedback! I will change the pattern property and the description. Regarding the channels, I followed the existing style of the driver for the AD7194 channels: one iio channel for each pseudo-differential input channel(AINx - AINCOM), summing up to 16 channels; and one iio channel for each differential channel (AINx - AINy), summing up to 8 channels. For the diff-channels, I thought the possible values should be 1->16 corresponding to AIN1->AIN16 (I will add this to the description as suggested by David). Kind regards, Alisa-Dariana Roman
On Thu, Feb 15, 2024 at 02:13:38PM +0200, Alisa-Dariana Roman wrote: > On 08.02.2024 20:03, Conor Dooley wrote: > > Hey, > > > > On Thu, Feb 08, 2024 at 07:24:58PM +0200, Alisa-Dariana Roman wrote: > > > > > +patternProperties: > > > + "^channel@([0-7a-f])$": > > > + type: object > > > + $ref: adc.yaml > > > + unevaluatedProperties: false > > > + > > > + properties: > > > + reg: > > > + description: The channel index. > > > + minimum: 0 > > > + maximum: 7 > > > > There are only 8 possible channels, at indices 0 to 7, so why is the > > pattern property more permissive than that? Shouldn't "^channel@[0-7]$" > > suffice? > > > > > + > > > + diff-channels: > > > > > + description: | > > > + The differential channel pair for Ad7194 configurable channels. The > > > + first channel is the positive input, the second channel is the > > > + negative input. > > > > This duplicates the description in adc.yaml > > > > > + items: > > > + minimum: 1 > > > + maximum: 16 > > > > Hmm, this makes me wonder: why doesn't this match the number of channels > > available and why is 0 not a valid channel for differential measurements? > > > > Thanks, > > Conor. > > Hello and thank you for the feedback! > > I will change the pattern property and the description. > > Regarding the channels, I followed the existing style of the driver for the > AD7194 channels: one iio channel for each pseudo-differential input > channel(AINx - AINCOM), summing up to 16 channels; and one iio channel for > each differential channel (AINx - AINy), summing up to 8 channels. I don't know what question this is answering. Everything here is about channels so it is hard for me to relate it back. Please reply inline & not at the end of the message to everything :) Was it meant to answer the following? > > > + properties: > > > + reg: > > > + description: The channel index. > > > + minimum: 0 > > > + maximum: 7 > > > > There are only 8 possible channels, at indices 0 to 7, so why is the > > pattern property more permissive than that? Shouldn't "^channel@[0-7]$" > > suffice? If it was a response to this, the reg property only allows 8 channels so the regex should only allow 8 too. The number after @ must match the number in reg. If using each of the 16 "pseudo-differential" inputs in isolation is thing you want to be able to do, your reg property does not allow it. > For the > diff-channels, I thought the possible values should be 1->16 corresponding > to AIN1->AIN16 (I will add this to the description as suggested by David). With a description, this should be fine.
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml index 16def2985ab4..169bdd1dd0e1 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml @@ -21,8 +21,15 @@ properties: - adi,ad7190 - adi,ad7192 - adi,ad7193 + - adi,ad7194 - adi,ad7195 + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + reg: maxItems: 1 @@ -96,8 +103,44 @@ required: - spi-cpol - spi-cpha +patternProperties: + "^channel@([0-7a-f])$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + description: The channel index. + minimum: 0 + maximum: 7 + + diff-channels: + description: | + The differential channel pair for Ad7194 configurable channels. The + first channel is the positive input, the second channel is the + negative input. + items: + minimum: 1 + maximum: 16 + + required: + - reg + - diff-channels + allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# + - if: + properties: + compatible: + enum: + - adi,ad7190 + - adi,ad7192 + - adi,ad7193 + - adi,ad7195 + then: + patternProperties: + "^channel@([0-7a-f])$": false unevaluatedProperties: false @@ -127,3 +170,35 @@ examples: adi,burnout-currents-enable; }; }; + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "adi,ad7194"; + reg = <0>; + spi-max-frequency = <1000000>; + spi-cpol; + spi-cpha; + clocks = <&ad7192_mclk>; + clock-names = "mclk"; + interrupts = <25 0x2>; + interrupt-parent = <&gpio>; + dvdd-supply = <&dvdd>; + avdd-supply = <&avdd>; + vref-supply = <&vref>; + + channel@0 { + reg = <0>; + diff-channels = <1 6>; + }; + + channel@1 { + reg = <1>; + diff-channels = <2 5>; + }; + }; + };
Unlike the other AD719Xs, AD7194 has configurable differential channels. The default configuration for these channels can be changed from the devicetree. Also add an example for AD7194 devicetree. Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> --- .../bindings/iio/adc/adi,ad7192.yaml | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+)