diff mbox series

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

Message ID 20211231131951.1245508-1-drhunter95@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [v4,1/2] Add binding for ti,adc1018. It allows selection of channel as a Device Tree property | expand

Commit Message

Iain Hunter Dec. 31, 2021, 1:19 p.m. UTC
New binding file uses the adc.yaml to define channel selection 

Signed-off-by: Iain Hunter <drhunter95@gmail.com>
---
 .../bindings/iio/adc/ti,ads1018.yaml          | 126 ++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml

Comments

Rob Herring Jan. 1, 2022, 10:01 p.m. UTC | #1
On Fri, 31 Dec 2021 13:19:15 +0000, Iain Hunter wrote:
> New binding file uses the adc.yaml to define channel selection
> 
> Signed-off-by: Iain Hunter <drhunter95@gmail.com>
> ---
>  .../bindings/iio/adc/ti,ads1018.yaml          | 126 ++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/iio/adc/ti,ads1018.example.dts:24.19-31.15: Warning (spi_bus_reg): /example-0/spi/adc@1: SPI bus unit address format error, expected "0"
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ti,ads1018.example.dt.yaml: adc@1: 'ti,adc-channels' does not match any of the regexes: '^channel@([0-3])$', 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ti,ads1018.example.dt.yaml: adc@0: 'ti,adc-diff-channels' does not match any of the regexes: '^channel@([0-3])$', 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1574373

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring Jan. 4, 2022, 3:53 p.m. UTC | #2
On Fri, Dec 31, 2021 at 01:19:15PM +0000, Iain Hunter wrote:
> New binding file uses the adc.yaml to define channel selection 

For the subject, follow the conventions used by other files in the 
subsystem. Run 'git log --oneline directory/for/file' and it should be 
evident.

> 
> Signed-off-by: Iain Hunter <drhunter95@gmail.com>
> ---
>  .../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@hunterembedded.co.uk>
> +
> +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])$":

0-3 doesn't match the allowed 'reg' values.

> +    $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]
> +
> +      ti,adc-channels:
> +        description: |
> +          List of single-ended channels muxed for this ADC. It can have up to 4
> +          channels numbered 0-3
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        deprecated: true
> +
> +      ti,adc-diff-channels:
> +        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:

No bounds for the number of tuples?

> +          items:
> +            - description: |
> +                "vinp" indicates positive input number
> +              minimum: 0
> +              maximum: 2
> +            - description: |
> +                "vinn" indicates negative input number
> +              minimum: 1
> +              maximum: 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>;
> +        };
> +    };
> +  - |
> +    // example on SPI0 with differential between inputs 0 and 3
> +    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>;
> +        };
> +    };
> +
> +...
> -- 
> 2.25.1
> 
>
Jonathan Cameron Jan. 9, 2022, 11:17 a.m. UTC | #3
On Fri, 31 Dec 2021 13:19:15 +0000
Iain Hunter <drhunter95@gmail.com> wrote:

> New binding file uses the adc.yaml to define channel selection 
> 
> Signed-off-by: Iain Hunter <drhunter95@gmail.com>
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

> ---
>  .../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@hunterembedded.co.uk>
> +
> +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
> +
> +      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>;
               }

> +        };
> +    };
> +
> +...
iain@hunterembedded.co.uk Jan. 9, 2022, 2:29 p.m. UTC | #4
On Sunday, 9 January 2022 11:17:18 GMT Jonathan Cameron wrote:
> On Fri, 31 Dec 2021 13:19:15 +0000
> 
> Iain Hunter <drhunter95@gmail.com> wrote:
> > New binding file uses the adc.yaml to define channel selection
> > 
> > Signed-off-by: Iain Hunter <drhunter95@gmail.com>
> 
> 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.

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.

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@hunterembedded.co.uk>
> > +
> > +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>;
>                }
> 
> > +        };
> > +    };
> > +
> > +...
Jonathan Cameron Jan. 9, 2022, 4:49 p.m. UTC | #5
On Sun, 09 Jan 2022 14:29:34 +0000
iain@hunterembedded.co.uk 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@gmail.com> wrote:  
> > > New binding file uses the adc.yaml to define channel selection
> > > 
> > > Signed-off-by: Iain Hunter <drhunter95@gmail.com>  
> > 
> > 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@hunterembedded.co.uk>
> > > +
> > > +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>;
> >                }
> >   
> > > +        };
> > > +    };
> > > +
> > > +...  
> 
> 
> 
>
diff mbox series

Patch

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@hunterembedded.co.uk>
+
+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]
+
+      ti,adc-channels:
+        description: |
+          List of single-ended channels muxed for this ADC. It can have up to 4
+          channels numbered 0-3
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        deprecated: true
+
+      ti,adc-diff-channels:
+        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
+
+
+    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>;
+        };
+    };
+  - |
+    // example on SPI0 with differential between inputs 0 and 3
+    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>;
+        };
+    };
+
+...