Re: [PATCH v2 3/3] dt-bindings: iio: adc: add adi,max11410.yaml

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 19 Jul 2022 14:59:30 +0000
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> > On Thu, 7 Jul 2022 08:31:26 +0000
> > Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:
> >   
> > > Adding devicetree binding documentation for max11410 adc.
> > > 
> > > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > > Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>  
> > 
> > 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@analog.com>
> > > +
> > > +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.

Fair enough.  Sometimes we can't do better than something opaque :(

Jonathan

> 
> > > +          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  
> 
> ...