| Message ID | 20240118125001.12809-1-mitrutzceclan@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v12,1/2] dt-bindings: adc: add AD7173 | expand |
On 1/18/24 17:23, Conor Dooley wrote: > On Thu, Jan 18, 2024 at 02:49:22PM +0200, Dumitru Ceclan wrote: ... >> + adi,clock-select: >> + description: | >> + Select the ADC clock source. Valid values are: >> + int : Internal oscillator >> + int-out : Internal oscillator with output on XTAL2 pin >> + ext-clk : External clock input on XTAL2 pin >> + xtal : External crystal on XTAL1 and XTAL2 pins >> + >> + $ref: /schemas/types.yaml#/definitions/string >> + enum: >> + - int >> + - int-out >> + - ext-clk >> + - xtal >> + default: int > I am not a fan of properties like this one, that in my view reimplement > things that are supported by the regular clocks properties. I've got > some questions for you so I can understand whether or not this custom > property is required. > > Whether or not the ext-clk or xtal is used is known based on > clock-names - why is the custom property required to determine that? > If neither of those clocks are present, then the internal clock would be > used. Choosing to use the internal clock if an external one is provided > sounds to me like a software policy decision made by the operating > system. If there was no int-out, sure. I considered that the choice between int and int-out could be made here. So better for driver to choose int/int-out? > > Finally, if the ADC has a clock output, why can that not be represented > by making the ADC a clock-controller? > Was not familiar with this/did not cross my mind. So if xtal/ext-clk is present, the driver should detect it and disable the option for clock output? (Common pin XTAL2)
On Thu, Jan 18, 2024 at 6:50 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> > --- > > 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 > > 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..4d0870cc014c > --- /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. I think it would be helpful to have a description similar to vref-supply here. This is the voltage between AVDD and AVSS. So in both cases AVDD=5V, AVSS=0V and AVDD=+2.5V, AVSS=-2.5V, this supply should report 5V. > + > + avdd2-supply: > + description: avdd2 supply, used as the input to the internal voltage regulator. This supply is also referenced to 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 > + > + adi,clock-select: > + description: | > + Select the ADC clock source. Valid values are: > + int : Internal oscillator > + int-out : Internal oscillator with output on XTAL2 pin > + ext-clk : External clock input on XTAL2 pin > + xtal : External crystal on XTAL1 and XTAL2 pins > + > + $ref: /schemas/types.yaml#/definitions/string > + enum: > + - int > + - int-out > + - ext-clk > + - xtal > + default: int > + > +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 Could write this as AVDD/AVSS to be consistent with the other 3 options. (Or if this is really AVDD to 0V, we may need to reconsider some of our other decisions.) > + > + 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 > + > +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>; > + > + 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"; > + }; > + }; > + }; > -- > 2.42.0 >
On Thu, 18 Jan 2024 16:06:29 +0000 Conor Dooley <conor@kernel.org> wrote: > On Thu, Jan 18, 2024 at 05:51:20PM +0200, Ceclan Dumitru wrote: > > > > > > On 1/18/24 17:23, Conor Dooley wrote: > > > On Thu, Jan 18, 2024 at 02:49:22PM +0200, Dumitru Ceclan wrote: > > > > ... > > > > >> + adi,clock-select: > > >> + description: | > > >> + Select the ADC clock source. Valid values are: > > >> + int : Internal oscillator > > >> + int-out : Internal oscillator with output on XTAL2 pin > > >> + ext-clk : External clock input on XTAL2 pin > > >> + xtal : External crystal on XTAL1 and XTAL2 pins > > >> + > > >> + $ref: /schemas/types.yaml#/definitions/string > > >> + enum: > > >> + - int > > >> + - int-out > > >> + - ext-clk > > >> + - xtal > > >> + default: int > > > I am not a fan of properties like this one, that in my view reimplement > > > things that are supported by the regular clocks properties. I've got > > > some questions for you so I can understand whether or not this custom > > > property is required. > > > > > > Whether or not the ext-clk or xtal is used is known based on > > > clock-names - why is the custom property required to determine that? > > > > If neither of those clocks are present, then the internal clock would be > > > used. Choosing to use the internal clock if an external one is provided > > > sounds to me like a software policy decision made by the operating > > > system. > > > > If there was no int-out, sure. I considered that the choice between int > > and int-out could be made here. So better for driver to choose int/int-out? > > This part of my comments was specifically about choosing between use of > the internal clock when ext-clk or xtal are provided, which I think > excludes the possibility of using int-out, since the XTAL2 pin is an > input. > > There's 3 situations: > - no external clock provided > - ext-clk provided > - xtal provided > > For the former, you know you're in that state when no "clocks" property > is present. The latter two you can differentiate based on "clock-names". > > Choosing to use the internal clock if an external clock is provided > seems to be a software policy decision, unless I am mistaken. Agreed, though it rarely makes sense as if someone put down a precision clock they normally wanted you to use it! So as a general rule we don't both providing policy controls beyond if there is extra hardware (external clock source) then use that. If someone has a good reason to want to do something else then we can probably figure out a reasonable way to control it. > > > > > > > Finally, if the ADC has a clock output, why can that not be represented > > > by making the ADC a clock-controller? > > > > > > > Was not familiar with this/did not cross my mind. So if xtal/ext-clk is > > present, the driver should detect it and disable the option for clock > > output? (Common pin XTAL2) > > Yeah, if those clocks are provided you would not register as a clock > controller. If there is a user of the output clock, it should have its > own "clocks" property that references the ADC's output. > > Your dt-binding could also make clocks/clock-names & clock-controller > mutually exclusive. That would indeed be the nicest solution. How this has been done in drivers has somewhat 'evolved' over time, but this is the nicest option from point of view of standard bindings and clarity over what is going on. > > Cheers, > Conor.
On Sun, Jan 21, 2024 at 03:41:18PM +0000, Jonathan Cameron wrote: > On Thu, 18 Jan 2024 16:06:29 +0000 > Conor Dooley <conor@kernel.org> wrote: > > On Thu, Jan 18, 2024 at 05:51:20PM +0200, Ceclan Dumitru wrote: > > > On 1/18/24 17:23, Conor Dooley wrote: > > > > On Thu, Jan 18, 2024 at 02:49:22PM +0200, Dumitru Ceclan wrote: > > > >> + adi,clock-select: > > > >> + description: | > > > >> + Select the ADC clock source. Valid values are: > > > >> + int : Internal oscillator > > > >> + int-out : Internal oscillator with output on XTAL2 pin > > > >> + ext-clk : External clock input on XTAL2 pin > > > >> + xtal : External crystal on XTAL1 and XTAL2 pins > > > >> + > > > >> + $ref: /schemas/types.yaml#/definitions/string > > > >> + enum: > > > >> + - int > > > >> + - int-out > > > >> + - ext-clk > > > >> + - xtal > > > >> + default: int > > > > I am not a fan of properties like this one, that in my view reimplement > > > > things that are supported by the regular clocks properties. I've got > > > > some questions for you so I can understand whether or not this custom > > > > property is required. > > > > > > > > Whether or not the ext-clk or xtal is used is known based on > > > > clock-names - why is the custom property required to determine that? > > > > > > If neither of those clocks are present, then the internal clock would be > > > > used. Choosing to use the internal clock if an external one is provided > > > > sounds to me like a software policy decision made by the operating > > > > system. > > > > > > If there was no int-out, sure. I considered that the choice between int > > > and int-out could be made here. So better for driver to choose int/int-out? > > > > This part of my comments was specifically about choosing between use of > > the internal clock when ext-clk or xtal are provided, which I think > > excludes the possibility of using int-out, since the XTAL2 pin is an > > input. > > > > There's 3 situations: > > - no external clock provided > > - ext-clk provided > > - xtal provided > > > > For the former, you know you're in that state when no "clocks" property > > is present. The latter two you can differentiate based on "clock-names". > > > > Choosing to use the internal clock if an external clock is provided > > seems to be a software policy decision, unless I am mistaken. > > Agreed, though it rarely makes sense as if someone put down a precision > clock they normally wanted you to use it! > > So as a general rule we don't both providing policy controls beyond if > there is extra hardware (external clock source) then use that. > > If someone has a good reason to want to do something else then we can > probably figure out a reasonable way to control it. Yah, I figured there'd be few situations, outside of maybe debugging hardware issues, where that internal clock is more desirable to use. > > > > Finally, if the ADC has a clock output, why can that not be represented > > > > by making the ADC a clock-controller? > > > > > > > > > > Was not familiar with this/did not cross my mind. So if xtal/ext-clk is > > > present, the driver should detect it and disable the option for clock > > > output? (Common pin XTAL2) > > > > Yeah, if those clocks are provided you would not register as a clock > > controller. If there is a user of the output clock, it should have its > > own "clocks" property that references the ADC's output. > > > > Your dt-binding could also make clocks/clock-names & clock-controller > > mutually exclusive. > > That would indeed be the nicest solution. How this has been done > in drivers has somewhat 'evolved' over time, but this is the nicest > option from point of view of standard bindings and clarity over what > is going on. Yeah, I know that this has not really been normal thing to do in some corners of the kernel (ethernet PHYs in particular I think have been rolling their own clock controller stuff) and I've been trying to push back on these kinds of things for new devices. Thanks, 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..4d0870cc014c --- /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. + + 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. + + clocks: + maxItems: 1 + description: | + Optional external clock source. Can include one clock source: external + clock or external crystal. + + clock-names: + enum: + - ext-clk + - xtal + + adi,clock-select: + description: | + Select the ADC clock source. Valid values are: + int : Internal oscillator + int-out : Internal oscillator with output on XTAL2 pin + ext-clk : External clock input on XTAL2 pin + xtal : External crystal on XTAL1 and XTAL2 pins + + $ref: /schemas/types.yaml#/definitions/string + enum: + - int + - int-out + - ext-clk + - xtal + default: int + +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 + + 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 + +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>; + + 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> --- 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