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. 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@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>; > } > > > + }; > > + }; > > + > > +...
