> On Thu, 7 Jul 2022 08:31:26 +0000 > Ibrahim Tilki <Ibrahim.Tilki@xxxxxxxxxx> wrote: > > > Adding devicetree binding documentation for max11410 adc. > > > > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@xxxxxxxxxx> > > Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@xxxxxxxxxx> > > Hi. > > A few questions inline. Mostly stuff I couldn't figure out from a quick > scan through the datasheet. > > > --- > > .../bindings/iio/adc/adi,max11410.yaml | 168 ++++++++++++++++++ > > 1 file changed, 168 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml > > new file mode 100644 > > index 000000000..f28d29fb2 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml > > @@ -0,0 +1,168 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +# Copyright 2022 Analog Devices Inc. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/adc/adi,max11410.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices MAX11410 ADC device driver > > + > > +maintainers: > > + - Ibrahim Tilki <ibrahim.tilki@xxxxxxxxxx> > > + > > +description: | > > + Bindings for the Analog Devices MAX11410 ADC device. Datasheet can be > > + found here: > > + https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf > > + > > +properties: > > + compatible: > > + enum: > > + - adi,max11410 > > + > > + reg: > > + description: SPI chip select number for the device > > Description not needed as same for all SPI devices. > Removed in v3. > > + maxItems: 1 > > + > > + interrupts: > > + description: IRQ line for the ADC > The description doesn't tell us anything so drop it. > There is no need to provide description lines for self documenting > items like this. Removed in v3. > > + maxItems: 1 > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > + avdd-supply: > > + description: avdd supply can be used as reference for conversion. > > Mention it's also a necessary power supply. As mentioned in driver review > I'd suggest you actually treat this as 'no explicit reference supplied'. > That simplifies the meaning of the adi,reference below. > Please see my comments to this in section "adi,reference". > > + > > + vref0p-supply: > > + description: vref0p supply can be used as reference for conversion. > > + > > + vref1p-supply: > > + description: vref1p supply can be used as reference for conversion. > > + > > + vref2p-supply: > > + description: vref2p supply can be used as reference for conversion. > > + > > + vref0n-supply: > > + description: vref0n supply can be used as reference for conversion. > > + > > + vref1n-supply: > > + description: vref1n supply can be used as reference for conversion. > > + > > + vref2n-supply: > > + description: vref2n supply can be used as reference for conversion. > > + > > + spi-max-frequency: > > + maximum: 8000000 > > + > > +required: > > + - compatible > > + - reg > > + - avdd-supply > > + > > +patternProperties: > > + "^channel(@[0-9a-f]+)?$": > > + $ref: "adc.yaml" > > + type: object > > + description: Represents the external channels which are connected to the ADC. > > + > > + properties: > > + reg: > > + description: The channel number in single-ended mode. > > + minimum: 0 > > + maximum: 10 > > + > > + adi,reference: > > + description: | > > + Select the reference source to use when converting on > > + the specific channel. Valid values are: > > + 0: REF0P/REF0N > > VREF0P etc to match namign above. > Corrected in v3. > > + 1: REF1P/REF1N > > + 2: REF2P/REF2N > > + 3: AVDD/AGND > > + 4: REF0P/AGND > > + 5: REF1P/AGND > > + 6: REF2P/AGND > > Is it valid to use REF0P/AGND for a differential channel? If not > I would reduce this list to 0-2 only. If it is valid (so actually > useful to do so) then we are stuck with this. That does make me wonder > if there is a difference between 3 and 7? If not, just don't list 7 > Yes, it is valid. Option 3 and 7 are the same, 7 is removed in v3. I think we should list 4-6 here and removing 3 would cause more confusion to users in that case. This enum reflects the register field directly, maybe we can do some mapping for convenience. AVDD is already treated as default reference and user is free to specify it explicitly. Maybe we can split reference description into two parts: "adi,reference" = <0>; // [0-2], AVDD if not specified. "adi,reference-n-agnd"; // boolean for connecting negative input to agnd. But I think it would be more confusing. The most straightforward way is the enum IMO. I cannot think of a better way to describe reference than what we already have. > > + 7: AVDD/AGND > > + If this field is left empty, AVDD/AGND is selected. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > > + default: 7 ...
