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@xxxxxxxxxx> wrote: > > > > Add device tree documentation for AD7091R-8. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> > > --- > > .../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@xxxxxxxxxx> > > + > > +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 > > > >
