Re: [PATCH v4 1/2] Add binding for ti,adc1018. It allows selection of channel as a Device Tree property

[Jonathan Cameron]

On Sun, 09 Jan 2022 14:29:34 +0000
iain@xxxxxxxxxxxxxxxxxxxx wrote:

> On Sunday, 9 January 2022 11:17:18 GMT Jonathan Cameron wrote:
> > On Fri, 31 Dec 2021 13:19:15 +0000
> > 
> > Iain Hunter <drhunter95@xxxxxxxxx> wrote:  
> > > New binding file uses the adc.yaml to define channel selection
> > > 
> > > Signed-off-by: Iain Hunter <drhunter95@xxxxxxxxx>  
> > 
> > Hi Iain,
> > 
> > A few comments in addition to those Rob sent.
> > It's worth noting that there is a lot of 'history' in IIO bindings so
> > sometimes copying stuff from an existing binding is no longer the way
> > things should be done.
> > 
> > Jonathan  
> 
> Hi Jonathan and Rob,
> 
> Thanks for your comments. I'd say my fundamental problem is that I am 
> stumbling about in the dark. To be honest I haven't even worked out the benefit 
> of the yaml bindings.

CI is in many ways the biggest one + formality of definition vs the older
text files where to put it likely things were often rather vague.
yaml has it's 'interesting corners' and a rather steep learning curve but
I'm not aware of anything better and we are stuck with it now anyway!

> 
> I identified the stm32adc binding as the most up to date file to use as a 
> reference. If there is a better one then can you let me know.

It's very much a work in progress so best practice is still evolving.
The stm32-adc ones are now quite old and reflect a very complex bit of hardware.
adc/ti-tsc2076.yaml is fairly similar and up to date.

Problem with bindings vs other code is we can't update everything to the
current view on how to do things because of backwards compatibility. Over time
we'll eventually get there as the parts people use in designs are replaced but
that's a lot longer game than for almost anything else.

Sad truth with bindings is they almost always go through a couple of revisions
as a result. Ideally I'd write some docs on what we consider best practice
for IIO drivers as that could at least be kept up to date, but lots of other
things on the todo list :( 

+ today patches are coming in slightly quicker than I review them. *sigh* :)
Jonathan

> 
> I will work through the comments to try to understand and then implement them.
> Thanks, Iain  
> >   
> > > ---
> > > 
> > >  .../bindings/iio/adc/ti,ads1018.yaml          | 126 ++++++++++++++++++
> > >  1 file changed, 126 insertions(+)
> > >  create mode 100644  
> > >  Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml>   
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml
> > > b/Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml new file mode
> > > 100644
> > > index 000000000000..a65fee9d83dd
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml
> > > @@ -0,0 +1,126 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/ti,ads1018.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI ADS1018 4 channel I2C analog to digital converter
> > > +
> > > +maintainers:
> > > +  - Iain Hunter <iain@xxxxxxxxxxxxxxxxxxxx>
> > > +
> > > +description: |
> > > +  Datasheet at: https://www.ti.com/lit/gpn/ads1018
> > > +  Supports both single ended and differential channels.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ti,ads1018
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +  "#io-channel-cells":
> > > +    const: 1
> > > +
> > > +  spi-max-frequency: true
> > > +  spi-cpol: true
> > > +  spi-cpha: true
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +  - spi-cpha
> > > +
> > > +additionalProperties: false
> > > +
> > > +patternProperties:
> > > +  "^channel@([0-3])$":
> > > +    $ref: "adc.yaml"
> > > +    type: object
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: |
> > > +            Must be 0, actual channel selected in ti,adc-channels for
> > > single ended +            or ti-adc-channels-diff for differential
> > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > +        enum: [0]  
> > 
> > No.  Should be some sort of index value. If I recall correctly, existing use
> > is reg == channel number when single ended and more loosely defined for
> > differential.  In many cases first of the pair, but that's not always
> > guaranteed to be unique (e.g. 0-1 and 0-3 in this case).  
> > > +
> > > +      ti,adc-channels:
> > > +        description: |
> > > +          List of single-ended channels muxed for this ADC. It can have
> > > up to 4 +          channels numbered 0-3  
> > 
> > This is a new binding, so how can we have deprecated properties?
> > Also seems very odd indeed to have a list of channels defined inside a per
> > channel node.  
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +        deprecated: true
> > > +  
> 
> As you can guess, it's because I don't understand it properly :)
> 
> >   
> > > +      ti,adc-diff-channels:  
> > Can this use diff-channels in the standard adc binding:
> > Documentation/devicetree/bindings/iio/adc/adc.yaml
> >   
> > > +        description: |
> > > +          List of differential channels muxed for this ADC between the
> > > pins vinp +          and vinn. The 4 possible options are:
> > > +          vinp=0, vinn=1
> > > +          vinp=0, vinn=3
> > > +          vinp=1, vinn=3
> > > +          vinp=2, vinn=3
> > > +
> > > +          They are listed in a pair <vinp vinn>.
> > > +
> > > +          Note: At least one of "ti,adc-channels" or
> > > "ti,adc-diff-channels" is +          required.
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > +        items:
> > > +          items:
> > > +            - description: |
> > > +                "vinp" indicates positive input number
> > > +              minimum: 0
> > > +              maximum: 2
> > > +            - description: |
> > > +                "vinn" indicates negative input number
> > > +              minimum: 1
> > > +              maximum: 3  
> > 
> > This should be a pair based constraint as not all options possible.
> > Something like oneOf:
> >             - items:
> >                 - const: 0
> >                 - const: 1
> >             - items:
> >                 - enum: [0, 1, 2]
> > 		- const: 3
> >   
> > > +
> > > +
> > > +    required:
> > > +      - reg
> > > +
> > > +examples:
> > > +  - |
> > > +    // example on SPI1 with single ended channel 1
> > > +    spi {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        adc@1 {
> > > +            compatible = "ti,ads1018";
> > > +            reg = <0x0>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            spi-cpha;
> > > +            ti,adc-channels = <1>;  
> > 
> > More recent approach to this is the one you've used for differential
> > channels - 1 child node per channel.
> >   
> > > +        };
> > > +    };
> > > +  - |
> > > +    // example on SPI0 with differential between inputs 0 and 3  
> > 
> > The SPI0 vs 1 is correctly not part of this example, so drop that from
> > the comment.
> >   
> > > +    spi {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        adc@0 {
> > > +            compatible = "ti,ads1018";
> > > +            reg = <0x0>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            spi-cpha;
> > > +            ti,adc-diff-channels = <0 3>;  
> > 
> > This doesn't obey the schema you have above at all. Would looks something
> > like channel@0 {
> >                  diff-channels = <0 3>;
> >                }
> >   
> > > +        };
> > > +    };
> > > +
> > > +...  
> 
> 
> 
>