| Message ID | 20240719203946.22909-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
On Fri, 19 Jul 2024 23:39:43 +0300, Laurent Pinchart wrote: > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > These bindings model the device as an MFD, and support the GPIO expander > and PWM functions. > > These bindings support the GPIO and PWM functions. > > Drop the existing adi,adp5585 and adi,adp5585-02 compatible strings from > trivial-devices.yaml. They have been added there by mistake as the > driver that was submitted at the same time used different compatible > strings. We can take them over safely. > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > I've limited the bindings to GPIO and PWM as I lack hardware to design, > implement and test the rest of the features the chip supports. > > Changes since v4: > > - Drop the right comment in trivial-devices.yaml > > Changes since v3: > > - Fix prefix and drop redundant text in subject line > - Rename node in example from mfd@ to io-expander@ > > Changes since v2: > > - Drop gpio property from required > - Drop second example > > Changes since v1: > > - Squash "dt-bindings: trivial-devices: Drop adi,adp5585 and > adi,adp5585-02" into this patch > - Merge child nodes into parent node > --- > .../devicetree/bindings/mfd/adi,adp5585.yaml | 90 +++++++++++++++++++ > .../devicetree/bindings/trivial-devices.yaml | 4 - > MAINTAINERS | 7 ++ > 3 files changed, 97 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: io-expander@34: gpio-reserved-ranges:0: 5 was expected from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: io-expander@34: gpio-reserved-ranges: [[5, 1]] is too short from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240719203946.22909-2-laurent.pinchart@ideasonboard.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 19/07/2024 22:39, Laurent Pinchart wrote: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + vdd-supply: true > + > + gpio-controller: true > + > + '#gpio-cells': > + const: 2 > + > + gpio-reserved-ranges: true > + > + "#pwm-cells": > + const: 3 > + > +required: > + - compatible > + - reg > + - gpio-controller > + - "#gpio-cells" > + - "#pwm-cells" > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: adi,adp5585-01 > + then: > + properties: > + gpio-reserved-ranges: false > + else: > + properties: > + gpio-reserved-ranges: > + items: > + - const: 5 > + - const: 1 Why reserved ranges are fixed? If they pins are *always* not accessible, then these are not GPIOs. This really looks incorrect. Anyway, testing reports failures which *must* be addressed, one way or another. Best regards, Krzysztof
Hi Krzysztof, On Sun, Jul 21, 2024 at 11:23:12AM +0200, Krzysztof Kozlowski wrote: > On 19/07/2024 22:39, Laurent Pinchart wrote: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + vdd-supply: true > > + > > + gpio-controller: true > > + > > + '#gpio-cells': > > + const: 2 > > + > > + gpio-reserved-ranges: true > > + > > + "#pwm-cells": > > + const: 3 > > + > > +required: > > + - compatible > > + - reg > > + - gpio-controller > > + - "#gpio-cells" > > + - "#pwm-cells" > > + > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: adi,adp5585-01 > > + then: > > + properties: > > + gpio-reserved-ranges: false > > + else: > > + properties: > > + gpio-reserved-ranges: > > + items: > > + - const: 5 > > + - const: 1 > > Why reserved ranges are fixed? If they pins are *always* not accessible, > then these are not GPIOs. This really looks incorrect. It's model-dependent. The ADP5585 has 11 pins that can be used as GPIOs. They are named GPIO 1 to GPIO 11 in the datasheet. The -01 variant uses the pin associated with GPIO 6 for a different purpose, so GPIO 6 is not usable. That maps to index 5 as GPIO numbers in DT bindings are 0-based. I've decided to handle that as a reserved GPIO range to keep the GPIO 7 to GPIO 11 indices the same across all ADP5585 variants. > Anyway, testing reports failures which *must* be addressed, one way or > another. Yes I'll fix that.
On 21/07/2024 11:45, Laurent Pinchart wrote: > Hi Krzysztof, > > On Sun, Jul 21, 2024 at 11:23:12AM +0200, Krzysztof Kozlowski wrote: >> On 19/07/2024 22:39, Laurent Pinchart wrote: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + vdd-supply: true >>> + >>> + gpio-controller: true >>> + >>> + '#gpio-cells': >>> + const: 2 >>> + >>> + gpio-reserved-ranges: true >>> + >>> + "#pwm-cells": >>> + const: 3 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - gpio-controller >>> + - "#gpio-cells" >>> + - "#pwm-cells" >>> + >>> +allOf: >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: adi,adp5585-01 >>> + then: >>> + properties: >>> + gpio-reserved-ranges: false >>> + else: >>> + properties: >>> + gpio-reserved-ranges: >>> + items: >>> + - const: 5 >>> + - const: 1 >> >> Why reserved ranges are fixed? If they pins are *always* not accessible, >> then these are not GPIOs. This really looks incorrect. > > It's model-dependent. The ADP5585 has 11 pins that can be used as GPIOs. > They are named GPIO 1 to GPIO 11 in the datasheet. The -01 variant uses > the pin associated with GPIO 6 for a different purpose, so GPIO 6 is not > usable. That maps to index 5 as GPIO numbers in DT bindings are 0-based. > I've decided to handle that as a reserved GPIO range to keep the GPIO 7 > to GPIO 11 indices the same across all ADP5585 variants. Ah, I missed the fact that gpio-reserved-ranges are not required, so some of variants can just skip it. It's fine. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml new file mode 100644 index 000000000000..46487b9144f6 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml @@ -0,0 +1,90 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices ADP5585 Keypad Decoder and I/O Expansion + +maintainers: + - Laurent Pinchart <laurent.pinchart@ideasonboard.com> + +description: + The ADP5585 is a 10/11 input/output port expander with a built in keypad + matrix decoder, programmable logic, reset generator, and PWM generator. + +properties: + compatible: + items: + - enum: + - adi,adp5585-00 # Default + - adi,adp5585-01 # 11 GPIOs + - adi,adp5585-02 # No pull-up resistors by default on special pins + - adi,adp5585-03 # Alternate I2C address + - adi,adp5585-04 # Pull-down resistors on all pins by default + - const: adi,adp5585 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + vdd-supply: true + + gpio-controller: true + + '#gpio-cells': + const: 2 + + gpio-reserved-ranges: true + + "#pwm-cells": + const: 3 + +required: + - compatible + - reg + - gpio-controller + - "#gpio-cells" + - "#pwm-cells" + +allOf: + - if: + properties: + compatible: + contains: + const: adi,adp5585-01 + then: + properties: + gpio-reserved-ranges: false + else: + properties: + gpio-reserved-ranges: + items: + - const: 5 + - const: 1 + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + io-expander@34 { + compatible = "adi,adp5585-00", "adi,adp5585"; + reg = <0x34>; + + vdd-supply = <®_3v3>; + + gpio-controller; + #gpio-cells = <2>; + gpio-reserved-ranges = <5 1>; + + #pwm-cells = <3>; + }; + }; + +... diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index 0a419453d183..8a6056323545 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -38,10 +38,6 @@ properties: - ad,adm9240 # AD5110 - Nonvolatile Digital Potentiometer - adi,ad5110 - # Analog Devices ADP5585 Keypad Decoder and I/O Expansion - - adi,adp5585 - # Analog Devices ADP5585 Keypad Decoder and I/O Expansion with support for Row5 - - adi,adp5585-02 # Analog Devices ADP5589 Keypad Decoder and I/O Expansion - adi,adp5589 # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher diff --git a/MAINTAINERS b/MAINTAINERS index 958e935449e5..4fe8bd8752a5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -526,6 +526,13 @@ F: drivers/leds/leds-adp5520.c F: drivers/mfd/adp5520.c F: drivers/video/backlight/adp5520_bl.c +ADP5585 GPIO EXPANDER, PWM AND KEYPAD CONTROLLER DRIVER +M: Laurent Pinchart <laurent.pinchart@ideasonboard.com> +L: linux-gpio@vger.kernel.org +L: linux-pwm@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/*/adi,adp5585*.yaml + ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587) M: Michael Hennerich <michael.hennerich@analog.com> S: Supported