| Message ID | 20240902103638.686039-8-aardelean@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7606: add support for AD7606C-{16,18} parts | expand |
On Mon, Sep 02, 2024 at 01:36:30PM +0300, Alexandru Ardelean wrote: > reg: > @@ -114,6 +118,25 @@ properties: > assumed that the pins are hardwired to VDD. > type: boolean > > +patternProperties: > + "^channel@([0-7])$": > + type: object > + $ref: adc.yaml > + unevaluatedProperties: false > + > + properties: > + reg: > + description: The channel number. > + minimum: 0 > + maximum: 7 > + > + diff-channels: true Shouldn't this be specific? > + > + bipolar: true > + > + required: > + - reg > + > required: > - compatible > - reg > @@ -202,4 +225,44 @@ examples: > standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; > }; > }; > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad7606c-18"; > + reg = <0>; > + spi-max-frequency = <1000000>; > + spi-cpol; > + spi-cpha; > + > + avcc-supply = <&adc_vref>; > + vdrive-supply = <&vdd_supply>; > + > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > + interrupt-parent = <&gpio>; > + > + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; > + > + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>; > + adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; > + standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; > + > + adi,sw-mode; > + > + channel@1 { > + reg = <1>; > + diff-channel; Where is this property defined (which schema)? Did you test it? Best regards, Krzysztof
On Mon, Sep 2, 2024 at 2:55 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, Sep 02, 2024 at 01:36:30PM +0300, Alexandru Ardelean wrote: > > reg: > > @@ -114,6 +118,25 @@ properties: > > assumed that the pins are hardwired to VDD. > > type: boolean > > > > +patternProperties: > > + "^channel@([0-7])$": > > + type: object > > + $ref: adc.yaml > > + unevaluatedProperties: false > > + > > + properties: > > + reg: > > + description: The channel number. > > + minimum: 0 > > + maximum: 7 > > + > > + diff-channels: true > > Shouldn't this be specific? Umm. Specific how? Like if:then check for certain compatible strings? > > > + > > + bipolar: true > > + > > + required: > > + - reg > > + > > required: > > - compatible > > - reg > > @@ -202,4 +225,44 @@ examples: > > standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; > > }; > > }; > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adc@0 { > > + compatible = "adi,ad7606c-18"; > > + reg = <0>; > > + spi-max-frequency = <1000000>; > > + spi-cpol; > > + spi-cpha; > > + > > + avcc-supply = <&adc_vref>; > > + vdrive-supply = <&vdd_supply>; > > + > > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > > + interrupt-parent = <&gpio>; > > + > > + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; > > + > > + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; > > + reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>; > > + adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; > > + standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; > > + > > + adi,sw-mode; > > + > > + channel@1 { > > + reg = <1>; > > + diff-channel; > > Where is this property defined (which schema)? > > Did you test it? Tested on my board. But forgot to update the DT schema docs. Though, if you're referring to testing it somehow via some make command, I'm a little behind on how all this works now. I'll go re-check the "make dtbs_check" and similar commands. Maybe I sound a bit old (now), but when I last saw these DT bindings going from txt-to-yaml, they seemed relatively simple. Now, they're almost like their own programming language. I'll search for some quick setup guides for these; any pointers are welcome :) > > Best regards, > Krzysztof >
On 9/2/24 1:38 PM, Alexandru Ardelean wrote: > On Mon, Sep 2, 2024 at 2:55 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On Mon, Sep 02, 2024 at 01:36:30PM +0300, Alexandru Ardelean wrote: >>> reg: >>> @@ -114,6 +118,25 @@ properties: >>> assumed that the pins are hardwired to VDD. >>> type: boolean >>> >>> +patternProperties: >>> + "^channel@([0-7])$": >>> + type: object >>> + $ref: adc.yaml >>> + unevaluatedProperties: false >>> + >>> + properties: >>> + reg: >>> + description: The channel number. >>> + minimum: 0 >>> + maximum: 7 >>> + >>> + diff-channels: true >> >> Shouldn't this be specific? > > Umm. > Specific how? > Like if:then check for certain compatible strings? > diff-channels is not a boolean property, it is an array of two integers that specify the positive and negative pins. So this should have e.g. minimum: and maximum: constraints for each item in the array. Even if we only use this like a boolean flag in the driver, we need to make the bindings use the already established definition of diff-channels from adc.yaml. It would look like this in the .dts: diff-channels = <1 1>; The datasheet numbers the pins 1 to 8 instead of 0 to 7, so it would make sense to have the pin number be reg + 1 (or redefine reg to be minimum: 1, maximum: 8). We also recently introduced a single-channel property that can be used when the pin number of of the input doesn't match the reg number. >> >>> + >>> + bipolar: true >>> + >>> + required: >>> + - reg >>> + >>> required: >>> - compatible >>> - reg >>> @@ -202,4 +225,44 @@ examples: >>> standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; >>> }; >>> }; >>> + - | >>> + #include <dt-bindings/gpio/gpio.h> >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + spi { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + adc@0 { >>> + compatible = "adi,ad7606c-18"; >>> + reg = <0>; >>> + spi-max-frequency = <1000000>; >>> + spi-cpol; >>> + spi-cpha; >>> + >>> + avcc-supply = <&adc_vref>; >>> + vdrive-supply = <&vdd_supply>; >>> + >>> + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; >>> + interrupt-parent = <&gpio>; >>> + >>> + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; >>> + >>> + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; >>> + reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>; >>> + adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; >>> + standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; >>> + >>> + adi,sw-mode; >>> + >>> + channel@1 { >>> + reg = <1>; >>> + diff-channel; >> >> Where is this property defined (which schema)? >> >> Did you test it? > > Tested on my board. > But forgot to update the DT schema docs. > Though, if you're referring to testing it somehow via some make > command, I'm a little behind on how all this works now. > I'll go re-check the "make dtbs_check" and similar commands. > > Maybe I sound a bit old (now), but when I last saw these DT bindings > going from txt-to-yaml, they seemed relatively simple. > Now, they're almost like their own programming language. > I'll search for some quick setup guides for these; any pointers are welcome :) I can help you with this.
On 02/09/2024 20:38, Alexandru Ardelean wrote: > On Mon, Sep 2, 2024 at 2:55 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On Mon, Sep 02, 2024 at 01:36:30PM +0300, Alexandru Ardelean wrote: >>> reg: >>> @@ -114,6 +118,25 @@ properties: >>> assumed that the pins are hardwired to VDD. >>> type: boolean >>> >>> +patternProperties: >>> + "^channel@([0-7])$": >>> + type: object >>> + $ref: adc.yaml >>> + unevaluatedProperties: false >>> + >>> + properties: >>> + reg: >>> + description: The channel number. >>> + minimum: 0 >>> + maximum: 7 >>> + >>> + diff-channels: true >> >> Shouldn't this be specific? > > Umm. > Specific how? > Like if:then check for certain compatible strings? Ah, no, list is already constrained to two items. Then just drop it. What's the point of listing it here? > >> >>> + >>> + bipolar: true Same, drop. >>> + >>> + required: >>> + - reg >>> + >>> required: >>> - compatible >>> - reg >>> @@ -202,4 +225,44 @@ examples: >>> standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; >>> }; >>> }; >>> + - | >>> + #include <dt-bindings/gpio/gpio.h> >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + spi { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + adc@0 { >>> + compatible = "adi,ad7606c-18"; >>> + reg = <0>; >>> + spi-max-frequency = <1000000>; >>> + spi-cpol; >>> + spi-cpha; >>> + >>> + avcc-supply = <&adc_vref>; >>> + vdrive-supply = <&vdd_supply>; >>> + >>> + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; >>> + interrupt-parent = <&gpio>; >>> + >>> + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; >>> + >>> + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; >>> + reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>; >>> + adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; >>> + standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; >>> + >>> + adi,sw-mode; >>> + >>> + channel@1 { >>> + reg = <1>; >>> + diff-channel; >> >> Where is this property defined (which schema)? >> >> Did you test it? > > Tested on my board. How can you test a binding on a board? > But forgot to update the DT schema docs. > Though, if you're referring to testing it somehow via some make > command, I'm a little behind on how all this works now. > I'll go re-check the "make dtbs_check" and similar commands. Please read carefully writing-schema and writing-bindings. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml index 69408cae3db9..a1b8bfff76cb 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml @@ -14,6 +14,8 @@ description: | https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf properties: @@ -24,6 +26,8 @@ properties: - adi,ad7606-6 - adi,ad7606-8 # Referred to as AD7606 (without -8) in the datasheet - adi,ad7606b + - adi,ad7606c-16 + - adi,ad7606c-18 - adi,ad7616 reg: @@ -114,6 +118,25 @@ properties: assumed that the pins are hardwired to VDD. type: boolean +patternProperties: + "^channel@([0-7])$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + description: The channel number. + minimum: 0 + maximum: 7 + + diff-channels: true + + bipolar: true + + required: + - reg + required: - compatible - reg @@ -202,4 +225,44 @@ examples: standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; }; }; + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7606c-18"; + reg = <0>; + spi-max-frequency = <1000000>; + spi-cpol; + spi-cpha; + + avcc-supply = <&adc_vref>; + vdrive-supply = <&vdd_supply>; + + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; + + adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>; + adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>; + standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>; + + adi,sw-mode; + + channel@1 { + reg = <1>; + diff-channel; + }; + + channel@3 { + reg = <3>; + bipolar; + }; + }; + }; ...
The driver will support the AD7606C-16 and AD7606C-18. This change adds the compatible strings for these devices. The AD7606C-16,18 channels also support these (individually configurable) types of channels: - bipolar single-ended - unipolar single-ended - bipolar differential Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com> --- .../bindings/iio/adc/adi,ad7606.yaml | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+)