| Message ID | 53d55f3195b15bd8d47387e296036730ea270770.1701971344.git.marcelo.schmitt1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD7091R-2/-4/-8 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > Add device tree documentation for AD7091R-8. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > .../bindings/iio/adc/adi,ad7091r8.yaml | 99 +++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > new file mode 100644 > index 000000000000..02320778f225 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > @@ -0,0 +1,99 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD7091R8 8-Channel 12-Bit ADC > + > +maintainers: > + - Marcelo Schmitt <marcelo.schmitt@analog.com> > + > +description: | > + Analog Devices AD7091R-8 8-Channel 12-Bit ADC > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf > + > +properties: > + compatible: > + enum: > + - adi,ad7091r2 > + - adi,ad7091r4 > + - adi,ad7091r8 > + > + reg: > + maxItems: 1 > + Missing other supplies? Like vdd-supply and vdrive-supply? > + vref-supply: true refin-supply might be a better name to match the datasheet pin name. > + > + adi,conversion-start-gpios: gpios usually don't get a vendor prefix do they? convst-gpios could be a better name to match the pin name on the datasheet. > + description: > + GPIO connected to the CONVST pin. > + This logic input is used to initiate conversions on the analog > + input channels. > + maxItems: 1 > + > + reset-gpios: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 A description of what the interrupt is attached to (ALERT/BUSY/GPO0 pin) would be helpful. > + > +patternProperties: > + "^channel@[0-7]$": > + $ref: adc.yaml > + type: object > + description: Represents the external channels which are connected to the ADC. > + > + properties: > + reg: > + minimum: 0 > + maximum: 7 Shouldn't this be: items: - minimum: 0 maximum: 7 > + > + required: > + - reg Missing `unevaluatedProperties: false` for channels? Bigger picture: since no other properties besides `reg` are included here, do we actually need channel nodes? > + > +required: > + - compatible > + - reg > + - adi,conversion-start-gpios > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > + # AD7091R-2 does not have ALERT/BUSY/GPO pin > + - if: > + properties: > + compatible: > + contains: > + enum: > + - adi,ad7091r4 > + - adi,ad7091r8 > + then: > + properties: > + interrupts: true Interrupts is already true. Maybe better to only match chips without interrupts and set false? > + else: > + properties: > + interrupts: false > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/gpio/gpio.h> > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad7091r8"; > + reg = <0x0>; > + spi-max-frequency = <45454545>; > + vref-supply = <&adc_vref>; > + adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>; > + reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; > + interrupts = <22 IRQ_TYPE_EDGE_FALLING>; > + interrupt-parent = <&gpio>; > + }; > + }; > +... > -- > 2.42.0 > >
On 07/12/2023 19:42, Marcelo Schmitt wrote: > Add device tree documentation for AD7091R-8. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Except what David said also: > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/gpio/gpio.h> > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad7091r8"; Use 4 spaces for example indentation. Best regards, Krzysztof
Hi David, thank you for your suggestions. Comments inline. On 12/07, David Lechner wrote: > On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt > <marcelo.schmitt@analog.com> wrote: > > > > Add device tree documentation for AD7091R-8. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > --- > > .../bindings/iio/adc/adi,ad7091r8.yaml | 99 +++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > new file mode 100644 > > index 000000000000..02320778f225 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices AD7091R8 8-Channel 12-Bit ADC > > + > > +maintainers: > > + - Marcelo Schmitt <marcelo.schmitt@analog.com> > > + > > +description: | > > + Analog Devices AD7091R-8 8-Channel 12-Bit ADC > > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ad7091r2 > > + - adi,ad7091r4 > > + - adi,ad7091r8 > > + > > + reg: > > + maxItems: 1 > > + > > Missing other supplies? Like vdd-supply and vdrive-supply? > I used the name that would work with ad7091r-base.c. If I'm not misinterpreting the datasheet, vdd-supply and vdrive-supply are for powering the ADC and setting SPI lanes logic level, respectively. They don't have any impact on ADC readings. By the way, should maybe I extend ad7091r5 dt doc instead of creating this new one? > > + vref-supply: true > > refin-supply might be a better name to match the datasheet pin name. > Agree, though I guess changing the name now would break users of ad7091r5 if they happen to update the driver without updating their device tree. > > + > > + adi,conversion-start-gpios: > > gpios usually don't get a vendor prefix do they? > > convst-gpios could be a better name to match the pin name on the datasheet. Ack, will do for v4. > > > + description: > > + GPIO connected to the CONVST pin. > > + This logic input is used to initiate conversions on the analog > > + input channels. > > + maxItems: 1 > > + > > + reset-gpios: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > A description of what the interrupt is attached to (ALERT/BUSY/GPO0 > pin) would be helpful. > Ack, will do for v4. > > + > > +patternProperties: > > + "^channel@[0-7]$": > > + $ref: adc.yaml > > + type: object > > + description: Represents the external channels which are connected to the ADC. > > + > > + properties: > > + reg: > > + minimum: 0 > > + maximum: 7 > > Shouldn't this be: > > items: > - minimum: 0 > maximum: 7 > Ack > > + > > + required: > > + - reg > > Missing `unevaluatedProperties: false` for channels? > > Bigger picture: since no other properties besides `reg` are included > here, do we actually need channel nodes? > The channel nodes are not used by the drivers so we can remove them if we want. I thought they would be required as documentation even if they were not used in drivers. Looks like they're not required so will remove them in v4. > > + > > +required: > > + - compatible > > + - reg > > + - adi,conversion-start-gpios > > + > > +allOf: > > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + > > + # AD7091R-2 does not have ALERT/BUSY/GPO pin > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - adi,ad7091r4 > > + - adi,ad7091r8 > > + then: > > + properties: > > + interrupts: true > > Interrupts is already true. Maybe better to only match chips without > interrupts and set false? > Agree, that should simplify the constrain logic. Will do for v4. > > + else: > > + properties: > > + interrupts: false > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/gpio/gpio.h> > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adc@0 { > > + compatible = "adi,ad7091r8"; > > + reg = <0x0>; > > + spi-max-frequency = <45454545>; > > + vref-supply = <&adc_vref>; > > + adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>; > > + reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; > > + interrupts = <22 IRQ_TYPE_EDGE_FALLING>; > > + interrupt-parent = <&gpio>; > > + }; > > + }; > > +... > > -- > > 2.42.0 > > > >
On 12/08, Krzysztof Kozlowski wrote: > On 07/12/2023 19:42, Marcelo Schmitt wrote: > > Add device tree documentation for AD7091R-8. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > Except what David said also: > > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/gpio/gpio.h> > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adc@0 { > > + compatible = "adi,ad7091r8"; > > Use 4 spaces for example indentation. Ack, will do for v4. Thanks > > Best regards, > Krzysztof >
On Fri, Dec 8, 2023 at 7:28 AM Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > Hi David, thank you for your suggestions. > Comments inline. > > On 12/07, David Lechner wrote: > > On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt > > <marcelo.schmitt@analog.com> wrote: > > > > > > Add device tree documentation for AD7091R-8. > > > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > > --- > > > .../bindings/iio/adc/adi,ad7091r8.yaml | 99 +++++++++++++++++++ > > > 1 file changed, 99 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > > new file mode 100644 > > > index 000000000000..02320778f225 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml > > > @@ -0,0 +1,99 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Analog Devices AD7091R8 8-Channel 12-Bit ADC > > > + > > > +maintainers: > > > + - Marcelo Schmitt <marcelo.schmitt@analog.com> > > > + > > > +description: | > > > + Analog Devices AD7091R-8 8-Channel 12-Bit ADC > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - adi,ad7091r2 > > > + - adi,ad7091r4 > > > + - adi,ad7091r8 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > > Missing other supplies? Like vdd-supply and vdrive-supply? > > > > I used the name that would work with ad7091r-base.c. > If I'm not misinterpreting the datasheet, vdd-supply and vdrive-supply are > for powering the ADC and setting SPI lanes logic level, respectively. > They don't have any impact on ADC readings. The guidelines [1] say that bindings should be complete even if the feature is not used. In the most recent bindings I have submitted, Jonathan specifically called out making sure all supplies were included in the bindings. So I would assume the same applies here. [1]: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html > By the way, should maybe I extend ad7091r5 dt doc instead of creating this > new one? If it is pin-compatible or 90% the same, then perhaps.
On Fri, 8 Dec 2023 10:28:25 -0300 > > > + > > > + required: > > > + - reg > > > > Missing `unevaluatedProperties: false` for channels? > > > > Bigger picture: since no other properties besides `reg` are included > > here, do we actually need channel nodes? > > > > The channel nodes are not used by the drivers so we can remove them if we want. > I thought they would be required as documentation even if they were not used > in drivers. > Looks like they're not required so will remove them in v4. A lot of drivers assume that if you paid for a device with N channels you probably want N channels. Of course there are always boards that wire a subset but it's optional whether a driver cares about that. We have drivers where not channel nodes being supplied means they are all on so this is extensible if we later decide that fine grained information about what is routed where is needed or need to add per channel controls. So fine to drop this. Jonathan
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file mode 100644 index 000000000000..02320778f225 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD7091R8 8-Channel 12-Bit ADC + +maintainers: + - Marcelo Schmitt <marcelo.schmitt@analog.com> + +description: | + Analog Devices AD7091R-8 8-Channel 12-Bit ADC + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf + +properties: + compatible: + enum: + - adi,ad7091r2 + - adi,ad7091r4 + - adi,ad7091r8 + + reg: + maxItems: 1 + + vref-supply: true + + adi,conversion-start-gpios: + description: + GPIO connected to the CONVST pin. + This logic input is used to initiate conversions on the analog + input channels. + maxItems: 1 + + reset-gpios: + maxItems: 1 + + interrupts: + maxItems: 1 + +patternProperties: + "^channel@[0-7]$": + $ref: adc.yaml + type: object + description: Represents the external channels which are connected to the ADC. + + properties: + reg: + minimum: 0 + maximum: 7 + + required: + - reg + +required: + - compatible + - reg + - adi,conversion-start-gpios + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + + # AD7091R-2 does not have ALERT/BUSY/GPO pin + - if: + properties: + compatible: + contains: + enum: + - adi,ad7091r4 + - adi,ad7091r8 + then: + properties: + interrupts: true + else: + properties: + interrupts: false + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7091r8"; + reg = <0x0>; + spi-max-frequency = <45454545>; + vref-supply = <&adc_vref>; + adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>; + reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; + interrupts = <22 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + }; + }; +...
Add device tree documentation for AD7091R-8. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- .../bindings/iio/adc/adi,ad7091r8.yaml | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml