| Message ID | 20231220104810.3179-1-mitrutzceclan@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v11,1/2] dt-bindings: adc: add AD7173 | expand |
On 20/12/2023 11:48, Dumitru Ceclan wrote: > 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> > --- > V10->V11 > - Fix example warning: '#gpio-cells' is a dependency of 'gpio-controller' > - Add description to #gpio-cells property > V9->V10 None of your previous version were tested before sending, so I have really doubts that this version was. > - Fix dt_binding_check type warning from adi,reference-select > V8->v9 ... > +# 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: | > + Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips: If there is going to be resend: Drop "Bindings for" and instead describe shortly the hardware. Also wrap above according to Linux coding style, so at 80. > + 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 > + Rest looks good, so please tell did you test it? Best regards, Krzysztof
On 12/21/23 19:45, Krzysztof Kozlowski wrote: > On 20/12/2023 11:48, Dumitru Ceclan wrote: >> 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> >> --- >> V10->V11 >> - Fix example warning: '#gpio-cells' is a dependency of 'gpio-controller' >> - Add description to #gpio-cells property >> V9->V10 > > None of your previous version were tested before sending, so I have > really doubts that this version was. > >> - Fix dt_binding_check type warning from adi,reference-select >> V8->v9 > > ... > >> +# 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: | >> + Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips: > > If there is going to be resend: > Drop "Bindings for" and instead describe shortly the hardware. Also wrap > above according to Linux coding style, so at 80. > >> + 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 >> + > > Rest looks good, so please tell did you test it? Yes.
On Wed, 20 Dec 2023 12:48:04 +0200, Dumitru Ceclan wrote: > 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> > --- > 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 | 188 ++++++++++++++++++ > 1 file changed, 188 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > 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> > --- Sorry for the late reply as I see this has been applied already but... (I've been going over the datasheets for these and other related parts (AD411x) in detail today so please CC me on future updates to the bindings/driver for these if you don't mind) > .../bindings/iio/adc/adi,ad7173.yaml | 188 ++++++++++++++++++ > 1 file changed, 188 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > > 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..7c8caef76528 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > @@ -0,0 +1,188 @@ > +# 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: | > + Bindings for the Analog Devices AD717X ADC's. 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: > + maxItems: 1 As discussed in v8 [1] it is not clear what signal this is. Based on that discussion, I'm assuming the RDY signal, but how would bindings consumers know that without a description since it is not the only digital output signal of the chip? And why the ERROR signal was omitted here was never addressed AFAICT. [1]: https://lore.kernel.org/linux-iio/20231217135007.3e5d959a@jic23-huawei/ > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + spi-max-frequency: > + maximum: 20000000 > + According to the timing diagram in the datasheet, SCLK is high during idle, so don't we need `spi-cpol: true` here? Likewise, data is valid on the trailing SCLK edge, so don't we need `spi-cpha: true` here? > + 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>. > + > + refin-supply: > + description: external reference supply, can be used as reference for conversion. If I'm understanding correctly, this represents both voltage inputs REF+ and REF-, correct? The datasheet says "Reference Input Negative Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they should be separate supplies in case REF- is non-zero. Otherwise, how can we know what voltage it is? (same comment applies to refin2.) > + > + refin2-supply: > + description: external reference supply, can be used as reference for conversion. > + > + avdd-supply: > + description: avdd supply, can be used as reference for conversion. > + > + avdd2-supply: > + description: avdd2 supply, used as the input to the internal voltage regulator. > + > + iovdd-supply: > + description: iovdd supply, used for the chip digital interface. > + I overlooked this before, but these chips also have an optional external clock input so it seems like they should have an optional clocks property as well. > +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: > + refin : REFIN(+)/REFIN(−). > + refin2 : REFIN2(+)/REFIN2(−) > + refout-avss: REFOUT/AVSS (Internal reference) > + avdd : AVDD > + > + External reference refin2 only available on ad7173-8. > + If not specified, internal reference used. > + $ref: /schemas/types.yaml#/definitions/string > + enum: > + - refin > + - refin2 > + - refout-avss > + - avdd > + default: refout-avss > + > + required: > + - reg > + - diff-channels > + > +required: > + - compatible > + - reg > + - interrupts Why are interrupts required? What if the pin is not connected? > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > + - if: > + properties: > + compatible: > + not: > + contains: > + const: adi,ad7173-8 > + then: > + properties: > + refin2-supply: false > + patternProperties: > + "^channel@[0-9a-f]$": > + properties: > + adi,reference-select: > + enum: > + - refin > + - refout-avss > + - avdd > + > +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-parent = <&gpio>; > + spi-max-frequency = <5000000>; > + gpio-controller; > + #gpio-cells = <2>; > + > + refin-supply = <&dummy_regulator>; > + > + channel@0 { > + reg = <0>; > + bipolar; > + diff-channels = <0 1>; > + adi,reference-select = "refin"; > + }; > + > + 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"; > + }; > + }; > + }; > -- > 2.42.0 > >
On Mon, 15 Jan 2024 15:53:39 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > > > 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> > > --- > > Sorry for the late reply as I see this has been applied already but... We have plenty of time. Rather than dropping the ad7173 from my tree, I'd prefer to see additional patches on top to tidy up whatever makes sense from David's feedback. > > (I've been going over the datasheets for these and other related parts > (AD411x) in detail today so please CC me on future updates to the > bindings/driver for these if you don't mind) > > > .../bindings/iio/adc/adi,ad7173.yaml | 188 ++++++++++++++++++ > > 1 file changed, 188 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > > > > 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..7c8caef76528 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > > @@ -0,0 +1,188 @@ > > +# 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: | > > + Bindings for the Analog Devices AD717X ADC's. 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: > > + maxItems: 1 > > As discussed in v8 [1] it is not clear what signal this is. Based on > that discussion, I'm assuming the RDY signal, but how would bindings > consumers know that without a description since it is not the only > digital output signal of the chip? And why the ERROR signal was > omitted here was never addressed AFAICT. > > [1]: https://lore.kernel.org/linux-iio/20231217135007.3e5d959a@jic23-huawei/ I'd forgotten about that. Adding interrupt-names would be the easiest way to resolve this. > > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > + spi-max-frequency: > > + maximum: 20000000 > > + > > According to the timing diagram in the datasheet, SCLK is high during > idle, so don't we need `spi-cpol: true` here? > > Likewise, data is valid on the trailing SCLK edge, so don't we need > `spi-cpha: true` here? > > > > + 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>. > > + > > + refin-supply: > > + description: external reference supply, can be used as reference for conversion. > > If I'm understanding correctly, this represents both voltage inputs > REF+ and REF-, correct? The datasheet says "Reference Input Negative > Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they > should be separate supplies in case REF- is non-zero. Otherwise, how > can we know what voltage it is? (same comment applies to refin2.) Agreed, in this case these are directly used as references (we recently had another driver that could take a wide range of negative and positive inputs but in that case an internal reference was generated that didn't made it not matter exactly what was being supplied. Not true here though! > > > + > > + refin2-supply: > > + description: external reference supply, can be used as reference for conversion. > > + > > + avdd-supply: > > + description: avdd supply, can be used as reference for conversion. > > + > > + avdd2-supply: > > + description: avdd2 supply, used as the input to the internal voltage regulator. > > + > > + iovdd-supply: > > + description: iovdd supply, used for the chip digital interface. > > + > > I overlooked this before, but these chips also have an optional > external clock input so it seems like they should have an optional > clocks property as well. > > > +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: > > + refin : REFIN(+)/REFIN(−). > > + refin2 : REFIN2(+)/REFIN2(−) > > + refout-avss: REFOUT/AVSS (Internal reference) > > + avdd : AVDD > > + > > + External reference refin2 only available on ad7173-8. > > + If not specified, internal reference used. > > + $ref: /schemas/types.yaml#/definitions/string > > + enum: > > + - refin > > + - refin2 > > + - refout-avss > > + - avdd > > + default: refout-avss > > + > > + required: > > + - reg > > + - diff-channels > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > Why are interrupts required? What if the pin is not connected? > Ah. I clearly failed to review this one closely enough. Absolutely agree that interrupts should never be required. No need for the driver to work if they aren't, but the binding shouldn't require them! Jonathan
On 1/15/24 23:53, David Lechner wrote: > On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: ... > > According to the timing diagram in the datasheet, SCLK is high during > idle, so don't we need `spi-cpol: true` here? > > Likewise, data is valid on the trailing SCLK edge, so don't we need > `spi-cpha: true` here? > > V1 Rob Herring suggested that if device is not configurable, driver should set the spi mode >> + 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>. >> + >> + refin-supply: >> + description: external reference supply, can be used as reference for conversion. > > If I'm understanding correctly, this represents both voltage inputs > REF+ and REF-, correct? The datasheet says "Reference Input Negative > Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they > should be separate supplies in case REF- is non-zero. Otherwise, how > can we know what voltage it is? (same comment applies to refin2.) > Yes, but in that case, the value of the referenced supply should reflect that and be equal to (REF+)-(REF-). I'll add to the description this. ... >> +required: >> + - compatible >> + - reg >> + - interrupts > > Why are interrupts required? What if the pin is not connected? > From the datasheet, the reading of the conversions seem to be only interrupt based: "As soon as the next conversion is complete, the data register is updated; therefore, the period in which to read the conversion is limited." this paragraph suggests to me that interrupts are required
On 1/16/24 18:30, Jonathan Cameron wrote: > On Mon, 15 Jan 2024 15:53:39 -0600 > David Lechner <dlechner@baylibre.com> wrote: > >> On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: ... >> Sorry for the late reply as I see this has been applied already but... > We have plenty of time. Rather than dropping the ad7173 from my tree, > I'd prefer to see additional patches on top to tidy up whatever > makes sense from David's feedback. > Alright then. ... >> >> As discussed in v8 [1] it is not clear what signal this is. Based on >> that discussion, I'm assuming the RDY signal, but how would bindings >> consumers know that without a description since it is not the only >> digital output signal of the chip? And why the ERROR signal was >> omitted here was never addressed AFAICT. >> >> [1]: https://lore.kernel.org/linux-iio/20231217135007.3e5d959a@jic23-huawei/ > > I'd forgotten about that. Adding interrupt-names would be the easiest > way to resolve this. > I'll add this, but my curiosity for the long run is: How should differences between what bindings include and what drivers support should be managed and documented? ... >>> + >>> + refin-supply: >>> + description: external reference supply, can be used as reference for conversion. >> >> If I'm understanding correctly, this represents both voltage inputs >> REF+ and REF-, correct? The datasheet says "Reference Input Negative >> Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they >> should be separate supplies in case REF- is non-zero. Otherwise, how >> can we know what voltage it is? (same comment applies to refin2.) > > Agreed, in this case these are directly used as references (we recently > had another driver that could take a wide range of negative and positive > inputs but in that case an internal reference was generated that didn't > made it not matter exactly what was being supplied. Not true here though! > Wouldn't it be alright to specify that the voltage specified here should be the actual difference (REF+)-(REF-)? ... >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >> >> Why are interrupts required? What if the pin is not connected? >> > Ah. I clearly failed to review this one closely enough. > > Absolutely agree that interrupts should never be required. > No need for the driver to work if they aren't, but the binding > shouldn't require them! > > Jonathan > To make sure that I understand, the driver will not probe without interrupts, but it is alright to make then optional in the bindings? This is in the case that someone will want to use this binding and implement reading with polling?
On Wed, 17 Jan 2024 14:43:21 +0200 Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > On 1/16/24 18:30, Jonathan Cameron wrote: > > On Mon, 15 Jan 2024 15:53:39 -0600 > > David Lechner <dlechner@baylibre.com> wrote: > > > >> On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > ... > > >> Sorry for the late reply as I see this has been applied already but... > > We have plenty of time. Rather than dropping the ad7173 from my tree, > > I'd prefer to see additional patches on top to tidy up whatever > > makes sense from David's feedback. > > > Alright then. > > ... > > >> > >> As discussed in v8 [1] it is not clear what signal this is. Based on > >> that discussion, I'm assuming the RDY signal, but how would bindings > >> consumers know that without a description since it is not the only > >> digital output signal of the chip? And why the ERROR signal was > >> omitted here was never addressed AFAICT. > >> > >> [1]: https://lore.kernel.org/linux-iio/20231217135007.3e5d959a@jic23-huawei/ > > > > I'd forgotten about that. Adding interrupt-names would be the easiest > > way to resolve this. > > > > I'll add this, but my curiosity for the long run is: How should > differences between what bindings include and what drivers support > should be managed and documented? Drivers almost always support a subset of functionality of the device. This isn't much different. The driver 'should' use interrupt-names but it doesn't need to support all the things that the binding says should be in there. Sometimes we document things in a driver, but there isn't any obligation to do so and those docs are often out of date. > > ... > > >>> + > >>> + refin-supply: > >>> + description: external reference supply, can be used as reference for conversion. > >> > >> If I'm understanding correctly, this represents both voltage inputs > >> REF+ and REF-, correct? The datasheet says "Reference Input Negative > >> Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they > >> should be separate supplies in case REF- is non-zero. Otherwise, how > >> can we know what voltage it is? (same comment applies to refin2.) > > > > Agreed, in this case these are directly used as references (we recently > > had another driver that could take a wide range of negative and positive > > inputs but in that case an internal reference was generated that didn't > > made it not matter exactly what was being supplied. Not true here though! > > > Wouldn't it be alright to specify that the voltage specified here should > be the actual difference (REF+)-(REF-)? How do you establish the offset to apply to single ended channels if you don't know the value of REF- (relative to local ground)? So no - as the device supports single ended channels the difference isn't enough information. It would probably be fine to do as you say if it were a device with only differential channels where all that matters is the scaling. > > ... > > >>> +required: > >>> + - compatible > >>> + - reg > >>> + - interrupts > >> > >> Why are interrupts required? What if the pin is not connected? > >> > > Ah. I clearly failed to review this one closely enough. > > > > Absolutely agree that interrupts should never be required. > > No need for the driver to work if they aren't, but the binding > > shouldn't require them! > > > > Jonathan > > > > To make sure that I understand, the driver will not probe without > interrupts, but it is alright to make then optional in the bindings? Yes - it is fine for a driver to only support a subset of functionality and fail to probe if that subset isn't what the hardware enables. > > This is in the case that someone will want to use this binding and > implement reading with polling? Yes. J
On Wed, 17 Jan 2024 14:30:33 +0200 Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > On 1/15/24 23:53, David Lechner wrote: > > On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > ... > > > > According to the timing diagram in the datasheet, SCLK is high during > > idle, so don't we need `spi-cpol: true` here? > > > > Likewise, data is valid on the trailing SCLK edge, so don't we need > > `spi-cpha: true` here? > > > > > V1 Rob Herring suggested that if device is not configurable, driver > should set the spi mode I'm not sure on that given it's fairly common to do evil things with Not Gates to implement cheap level converters. That tends to break doing this entirely in driver. However, I'm fine with whatever the current convention on this is. We can always deal with inverters when hardware turns up that is doing that. > >> + 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>. > >> + > >> + refin-supply: > >> + description: external reference supply, can be used as reference for conversion. > > > > If I'm understanding correctly, this represents both voltage inputs > > REF+ and REF-, correct? The datasheet says "Reference Input Negative > > Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they > > should be separate supplies in case REF- is non-zero. Otherwise, how > > can we know what voltage it is? (same comment applies to refin2.) > > Yes, but in that case, the value of the referenced supply should reflect > that and be equal to (REF+)-(REF-). I'll add to the description this. See other thread - I don't think that works for single ended where we should probably provide an offset. > > ... > > >> +required: > >> + - compatible > >> + - reg > >> + - interrupts > > > > Why are interrupts required? What if the pin is not connected? > > > From the datasheet, the reading of the conversions seem to be only > interrupt based: "As soon as the next conversion is complete, > the data register is updated; therefore, the period in which to > read the conversion is limited." this paragraph suggests to me that > interrupts are required Nasty. However, a valid use case would be to use a single channel at a time, which case that statement doesn't matter. We have other devices that only support onehot channel sequencing - that would work here. >
On 1/17/24 18:37, Jonathan Cameron wrote: > On Wed, 17 Jan 2024 14:43:21 +0200 > Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: >> On 1/16/24 18:30, Jonathan Cameron wrote: >>> On Mon, 15 Jan 2024 15:53:39 -0600 >>> David Lechner <dlechner@baylibre.com> wrote: >>>> On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: ... >>>>> + >>>>> + refin-supply: >>>>> + description: external reference supply, can be used as reference for conversion. >>>> If I'm understanding correctly, this represents both voltage inputs >>>> REF+ and REF-, correct? The datasheet says "Reference Input Negative >>>> Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they >>>> should be separate supplies in case REF- is non-zero. Otherwise, how >>>> can we know what voltage it is? (same comment applies to refin2.) >>> Agreed, in this case these are directly used as references (we recently >>> had another driver that could take a wide range of negative and positive >>> inputs but in that case an internal reference was generated that didn't >>> made it not matter exactly what was being supplied. Not true here though! >>> >> Wouldn't it be alright to specify that the voltage specified here should >> be the actual difference (REF+)-(REF-)? > How do you establish the offset to apply to single ended channels if you don't > know the value of REF- (relative to local ground)? > > So no - as the device supports single ended channels the difference isn't > enough information. It would probably be fine to do as you say if it > were a device with only differential channels where all that matters is > the scaling. I suppose that you are referring to the first page presentation: "Cross point multiplexer; 8 full differential or 16 single-ended channels". I consider this to be a bit misleading as all channels are actually fully differential (must select positive and negative source, AVSS is not one of them). Even more, the datasheet specifies that when using "single-ended" inputs you need to select which of the pins is the common one and connect it to the desired GND (be it AVSS, REF-): "Because there is a cross point mux, the user can set any of the analog inputs as the common pin. An example of such a scenario is to connect the AIN16 pin to AVSS or to the REFOUT voltage (that is, AVSS + 2.5 V)" (ad7173-8 page 27) For me this is 100% the case that this is a fully differential ADC in which the datasheet presents a way to use it single-ended. Let's say that we are using EXT_REF, and REF- is non zero. If someone connects AVSS to the desired common pin, the ADC will still measure correctly the difference of voltage between AIN_POS and AIN_NEG and compare it to the EXT_REF.
On Thu, 18 Jan 2024 10:10:46 +0200 Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > On 1/17/24 18:37, Jonathan Cameron wrote: > > On Wed, 17 Jan 2024 14:43:21 +0200 > > Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > >> On 1/16/24 18:30, Jonathan Cameron wrote: > >>> On Mon, 15 Jan 2024 15:53:39 -0600 > >>> David Lechner <dlechner@baylibre.com> wrote: > >>>> On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > ... > > >>>>> + > >>>>> + refin-supply: > >>>>> + description: external reference supply, can be used as reference for conversion. > >>>> If I'm understanding correctly, this represents both voltage inputs > >>>> REF+ and REF-, correct? The datasheet says "Reference Input Negative > >>>> Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they > >>>> should be separate supplies in case REF- is non-zero. Otherwise, how > >>>> can we know what voltage it is? (same comment applies to refin2.) > >>> Agreed, in this case these are directly used as references (we recently > >>> had another driver that could take a wide range of negative and positive > >>> inputs but in that case an internal reference was generated that didn't > >>> made it not matter exactly what was being supplied. Not true here though! > >>> > >> Wouldn't it be alright to specify that the voltage specified here should > >> be the actual difference (REF+)-(REF-)? > > How do you establish the offset to apply to single ended channels if you don't > > know the value of REF- (relative to local ground)? > > > > So no - as the device supports single ended channels the difference isn't > > enough information. It would probably be fine to do as you say if it > > were a device with only differential channels where all that matters is > > the scaling. > > I suppose that you are referring to the first page presentation: "Cross > point multiplexer; 8 full differential or 16 single-ended channels". I > consider this to be a bit misleading as all channels are actually fully > differential (must select positive and negative source, AVSS is not one > of them). > > > Even more, the datasheet specifies that when using "single-ended" > inputs you need to select which of the pins is the common one and > connect it to the desired GND (be it AVSS, REF-): > > "Because there is a cross point mux, the user can set any of the analog > inputs as the common pin. An example of such a scenario is to connect > the AIN16 pin to AVSS or to the REFOUT voltage (that is, AVSS + > 2.5 V)" (ad7173-8 page 27) > > For me this is 100% the case that this is a fully differential ADC in > which the datasheet presents a way to use it single-ended. Let's say > that we are using EXT_REF, and REF- is non zero. If someone connects > AVSS to the desired common pin, the ADC will still measure correctly the > difference of voltage between AIN_POS and AIN_NEG and compare it to the > EXT_REF. Thanks for the explanation. I indeed was mislead by the introduction! Seems that the difference in the reference inputs is sufficient for this device. Jonathan
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..7c8caef76528 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml @@ -0,0 +1,188 @@ +# 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: | + Bindings for the Analog Devices AD717X ADC's. 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: + maxItems: 1 + + '#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>. + + refin-supply: + description: external reference supply, can be used as reference for conversion. + + refin2-supply: + description: external reference supply, can be used as reference for conversion. + + avdd-supply: + description: avdd supply, can be used as reference for conversion. + + avdd2-supply: + description: avdd2 supply, used as the input to the internal voltage regulator. + + iovdd-supply: + description: iovdd supply, used for the chip digital interface. + +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: + refin : REFIN(+)/REFIN(−). + refin2 : REFIN2(+)/REFIN2(−) + refout-avss: REFOUT/AVSS (Internal reference) + avdd : AVDD + + External reference refin2 only available on ad7173-8. + If not specified, internal reference used. + $ref: /schemas/types.yaml#/definitions/string + enum: + - refin + - refin2 + - refout-avss + - avdd + default: refout-avss + + required: + - reg + - diff-channels + +required: + - compatible + - reg + - interrupts + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + + - if: + properties: + compatible: + not: + contains: + const: adi,ad7173-8 + then: + properties: + refin2-supply: false + patternProperties: + "^channel@[0-9a-f]$": + properties: + adi,reference-select: + enum: + - refin + - refout-avss + - avdd + +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-parent = <&gpio>; + spi-max-frequency = <5000000>; + gpio-controller; + #gpio-cells = <2>; + + refin-supply = <&dummy_regulator>; + + channel@0 { + reg = <0>; + bipolar; + diff-channels = <0 1>; + adi,reference-select = "refin"; + }; + + 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> --- 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 | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml