| Message ID | 20240220094344.17556-1-mitrutzceclan@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v13,1/3] dt-bindings: adc: add AD7173 | expand |
On Tue, Feb 20, 2024 at 11:43:38AM +0200, Dumitru Ceclan wrote: > + interrupts: > + minItems: 1 > + description: | > + > + interrupt-names: > + minItems: 1 > + items: > + - const: rdy > + - const: err I noticed that for minItems == 1, the rdy interrupt is required and err is the optional one. With that in mind, you can simplify the interrupts description so that it describes the interrupts separately: interrupts: minItems: items: - description: Ready: multiplexed with SPI data out. While SPI CS is low, can be used to indicate the completion of a conversion. - description: Error: The three error bits in the status register (ADC_ERROR, CRC_ERROR, and REG_ERROR) are OR'ed, inverted, and mapped to the ERROR pin. Therefore, the ERROR pin indicates that an error has occurred. Otherwise, I think everything has been sorted out? Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On Tue, Feb 20, 2024 at 3:43 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > ... > + > + avdd-supply: > + description: Avdd supply, can be used as reference for conversion. > + This supply is referenced to AVSS, voltage specified here > + represens (AVDD - AVSS). The datasheets call this AVDD1, not AVDD. Would be nice to use the correct name to avoid ambiguity. Also check spelling `represents` above and below. > + > + avdd2-supply: > + description: Avdd2 supply, used as the input to the internal voltage regulator. > + This supply is referenced to AVSS, voltage specified here > + represens (AVDD2 - AVSS). > + > + iovdd-supply: > + description: iovdd supply, used for the chip digital interface. > + > + clocks: > + maxItems: 1 > + description: | Don't need `|` here. > + Optional external clock source. Can include one clock source: external > + clock or external crystal. > + > + clock-names: > + enum: > + - ext-clk > + - xtal > + > + '#clock-cells': > + const: 0 > + > +patternProperties: > + "^channel@[0-9a-f]$": > + type: object > + $ref: adc.yaml > + unevaluatedProperties: false > + > + properties: > + reg: > + minimum: 0 > + maximum: 15 Parts ending in -2 only have four channels. > + > + diff-channels: > + items: > + minimum: 0 > + maximum: 31 > + Are we missing `bipolar: true` here? (since we have unevaluatedProperties: false) > + adi,reference-select: > + description: | > + Select the reference source to use when converting on > + the specific channel. Valid values are: > + vref : REF+ /REF− > + vref2 : REF2+ /REF2− > + refout-avss: REFOUT/AVSS (Internal reference) > + avdd : AVDD /AVSS > + > + External reference ref2 only available on ad7173-8. > + If not specified, internal reference used. > + $ref: /schemas/types.yaml#/definitions/string > + enum: > + - vref > + - vref2 > + - refout-avss > + - avdd > + default: refout-avss > + > + required: > + - reg > + - diff-channels > + > +required: > + - compatible > + - reg Aren't the various power supplies supposed to be required? - avdd-supply - avdd2-supply - iovdd-supply
On 2/20/24 22:54, David Lechner wrote: > On Tue, Feb 20, 2024 at 3:43 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: ... >> + clocks: >> + maxItems: 1 >> + description: | > > Don't need `|` here. > The description contains ": ". Without '|' yaml syntax considers the whole string before ':' as another attribute >> + Optional external clock source. Can include one clock source: external >> + clock or external crystal. >> + ... >> + >> + diff-channels: >> + items: >> + minimum: 0 >> + maximum: 31 >> + > > Are we missing `bipolar: true` here? (since we have > unevaluatedProperties: false) > No, since we are referencing the adc schema "$ref: adc.yaml" Which contains: """ bipolar: $ref: /schemas/types.yaml#/definitions/flag description: If provided, the channel is to be used in bipolar mode. """ ... >> + >> +required: >> + - compatible >> + - reg > > Aren't the various power supplies supposed to be required? > > - avdd-supply > - avdd2-supply > - iovdd-supply From my point of view, if someone uses a single supply (avdd == avdd2 == iovdd), and uses only the internal reference then the supplies should not necessarily be required.
> >> + > >> + diff-channels: > >> + items: > >> + minimum: 0 > >> + maximum: 31 > >> + > > > > Are we missing `bipolar: true` here? (since we have > > unevaluatedProperties: false) > > > > No, since we are referencing the adc schema "$ref: adc.yaml" > Which contains: > """ > bipolar: > > $ref: /schemas/types.yaml#/definitions/flag > > description: If provided, the channel is to be used in bipolar mode. > """ FWIW, the difference here is whether or not the binding for the device contains "additionalProperties: false" or "unevaluatedProperties: false". The former requires "bipolar: true", the latter does not. Cheers, Conor.
On Wed, 21 Feb 2024 10:29:30 +0200 Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > On 2/20/24 22:54, David Lechner wrote: > > On Tue, Feb 20, 2024 at 3:43 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > ... > > >> + clocks: > >> + maxItems: 1 > >> + description: | > > > > Don't need `|` here. > > > The description contains ": ". Without '|' yaml syntax considers the > whole string before ':' as another attribute > > >> + Optional external clock source. Can include one clock source: external > >> + clock or external crystal. > >> + > > ... > > >> + > >> + diff-channels: > >> + items: > >> + minimum: 0 > >> + maximum: 31 > >> + > > > > Are we missing `bipolar: true` here? (since we have > > unevaluatedProperties: false) > > > > No, since we are referencing the adc schema "$ref: adc.yaml" > Which contains: > """ > bipolar: > > $ref: /schemas/types.yaml#/definitions/flag > > description: If provided, the channel is to be used in bipolar mode. > """ > > > ... > > >> + > >> +required: > >> + - compatible > >> + - reg > > > > Aren't the various power supplies supposed to be required? > > > > - avdd-supply > > - avdd2-supply > > - iovdd-supply > > From my point of view, if someone uses a single supply (avdd == avdd2 == > iovdd), and uses only the internal reference then the supplies should > not necessarily be required. Convention is that anything that represent a voltage on a pin that is needed for operation should be required. Key here is the difference from optional supplies where the driver does something different. vref is a good example of this. The ones above are always needed I think. Obviously they may all say the same thing if they are connected externally.
On Tue, 20 Feb 2024 18:52:37 +0000 Conor Dooley <conor@kernel.org> wrote: > On Tue, Feb 20, 2024 at 11:43:38AM +0200, Dumitru Ceclan wrote: > > > + interrupts: > > + minItems: 1 > > + description: | > > > + > > + interrupt-names: > > + minItems: 1 > > + items: > > + - const: rdy > > + - const: err > > I noticed that for minItems == 1, the rdy interrupt is required and err > is the optional one. > > With that in mind, you can simplify the interrupts description so that > it describes the interrupts separately: > > interrupts: > minItems: > items: > - description: > Ready: multiplexed with SPI data out. While SPI CS is low, > can be used to indicate the completion of a conversion. > > - description: > Error: The three error bits in the status register (ADC_ERROR, CRC_ERROR, > and REG_ERROR) are OR'ed, inverted, and mapped to the ERROR pin. Therefore, > the ERROR pin indicates that an error has occurred. > > Otherwise, I think everything has been sorted out? > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> This ordering may bite us in the future. Someone will build a board with err as only one wired. But meh, it will be a binding relaxation needed so I'm not that bothered by that. > > Cheers, > Conor.
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml new file mode 100644 index 000000000000..347a0cc0e278 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml @@ -0,0 +1,242 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2023 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD7173 ADC + +maintainers: + - Ceclan Dumitru <dumitru.ceclan@analog.com> + +description: | + Analog Devices AD717x ADC's: + The AD717x family offer a complete integrated Sigma-Delta ADC solution which + can be used in high precision, low noise single channel applications + (Life Science measurements) or higher speed multiplexed applications + (Factory Automation PLC Input modules). The Sigma-Delta ADC is intended + primarily for measurement of signals close to DC but also delivers + outstanding performance with input bandwidths out to ~10kHz. + + Datasheets for supported chips: + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf + +properties: + compatible: + enum: + - adi,ad7172-2 + - adi,ad7173-8 + - adi,ad7175-2 + - adi,ad7176-2 + + reg: + maxItems: 1 + + interrupts: + minItems: 1 + description: | + Ready interrupt: multiplexed with SPI data out. While SPI CS is low, + can be used to indicate the completion of a conversion. + + Error: The three error bits in the status register (ADC_ERROR, CRC_ERROR, + and REG_ERROR) are OR'ed, inverted, and mapped to the ERROR pin. Therefore, + the ERROR pin indicates that an error has occurred. + + interrupt-names: + minItems: 1 + items: + - const: rdy + - const: err + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + spi-max-frequency: + maximum: 20000000 + + gpio-controller: + description: Marks the device node as a GPIO controller. + + '#gpio-cells': + const: 2 + description: + The first cell is the GPIO number and the second cell specifies + GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. + + vref-supply: + description: | + Differential external reference supply used for conversion. The reference + voltage (Vref) specified here must be the voltage difference between the + REF+ and REF- pins: Vref = (REF+) - (REF-). + + vref2-supply: + description: | + Differential external reference supply used for conversion. The reference + voltage (Vref2) specified here must be the voltage difference between the + REF2+ and REF2- pins: Vref2 = (REF2+) - (REF2-). + + avdd-supply: + description: Avdd supply, can be used as reference for conversion. + This supply is referenced to AVSS, voltage specified here + represens (AVDD - AVSS). + + avdd2-supply: + description: Avdd2 supply, used as the input to the internal voltage regulator. + This supply is referenced to AVSS, voltage specified here + represens (AVDD2 - AVSS). + + iovdd-supply: + description: iovdd supply, used for the chip digital interface. + + clocks: + maxItems: 1 + description: | + Optional external clock source. Can include one clock source: external + clock or external crystal. + + clock-names: + enum: + - ext-clk + - xtal + + '#clock-cells': + const: 0 + +patternProperties: + "^channel@[0-9a-f]$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + minimum: 0 + maximum: 15 + + diff-channels: + items: + minimum: 0 + maximum: 31 + + adi,reference-select: + description: | + Select the reference source to use when converting on + the specific channel. Valid values are: + vref : REF+ /REF− + vref2 : REF2+ /REF2− + refout-avss: REFOUT/AVSS (Internal reference) + avdd : AVDD /AVSS + + External reference ref2 only available on ad7173-8. + If not specified, internal reference used. + $ref: /schemas/types.yaml#/definitions/string + enum: + - vref + - vref2 + - refout-avss + - avdd + default: refout-avss + + required: + - reg + - diff-channels + +required: + - compatible + - reg + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + + - if: + properties: + compatible: + not: + contains: + const: adi,ad7173-8 + then: + properties: + vref2-supply: false + patternProperties: + "^channel@[0-9a-f]$": + properties: + adi,reference-select: + enum: + - vref + - refout-avss + - avdd + + - if: + anyOf: + - required: [clock-names] + - required: [clocks] + then: + properties: + '#clock-cells': false + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7173-8"; + reg = <0>; + + #address-cells = <1>; + #size-cells = <0>; + + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; + interrupt-names = "rdy"; + interrupt-parent = <&gpio>; + spi-max-frequency = <5000000>; + gpio-controller; + #gpio-cells = <2>; + #clock-cells = <0>; + + vref-supply = <&dummy_regulator>; + + channel@0 { + reg = <0>; + bipolar; + diff-channels = <0 1>; + adi,reference-select = "vref"; + }; + + channel@1 { + reg = <1>; + diff-channels = <2 3>; + }; + + channel@2 { + reg = <2>; + bipolar; + diff-channels = <4 5>; + }; + + channel@3 { + reg = <3>; + bipolar; + diff-channels = <6 7>; + }; + + channel@4 { + reg = <4>; + diff-channels = <8 9>; + adi,reference-select = "avdd"; + }; + }; + };
The AD7173 family offer a complete integrated Sigma-Delta ADC solution which can be used in high precision, low noise single channel applications or higher speed multiplexed applications. The Sigma-Delta ADC is intended primarily for measurement of signals close to DC but also delivers outstanding performance with input bandwidths out to ~10kHz. Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> --- V12->V13 - Remove adi,clock-select - Update avdd and avdd2 supply descriptions - Update adi,reference-select description to suggest that it is referenced to avss - Make clocks/clock-names and clock-controller mutually exclusive V11->V12 - Drop "binding", describe hardware in binding description - Rename refin and refin2 to vref and vref2 - Add better description to external references to better show that the voltage value that needs to be specified is the difference between the positive and negative reference pins - Add optional clocks properties - Add optional adi,clock-select property - Add option for second interrupt, error - Add description to interrupts V10->V11 - Fix example warning: '#gpio-cells' is a dependency of 'gpio-controller' - Add description to #gpio-cells property V9->V10 - Fix dt_binding_check type warning from adi,reference-select V8->v9 - Add gpio-controller and "#gpio-cells" properties - Add missing avdd2 and iovdd supplies - Add string type to reference-select V7->V8 - include missing fix from V6 V6->V7 <no changes> V5->V6 - Moved global required property to proper placement V4 -> V5 - Use string enum instead of integers for "adi,reference-select" - Fix conditional checking in regards to compatible V3 -> V4 - include supply attributes - add channel attribute for selecting conversion reference V2 -> V3 - remove redundant descriptions - use referenced 'bipolar' property - remove newlines from example V1 -> V2 <no changes> .../bindings/iio/adc/adi,ad7173.yaml | 242 ++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml