Hi Krzysztof, On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote: > On 20/05/2024 21:59, 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. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > 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. > > --- > > .../bindings/gpio/adi,adp5585-gpio.yaml | 36 ++++++ > > .../devicetree/bindings/mfd/adi,adp5585.yaml | 117 ++++++++++++++++++ > > .../bindings/pwm/adi,adp5585-pwm.yaml | 35 ++++++ > > MAINTAINERS | 7 ++ > > 4 files changed, 195 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml > > create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml > > create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml > > > > diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml > > new file mode 100644 > > index 000000000000..210e4d53e764 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml > > @@ -0,0 +1,36 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices ADP5585 GPIO Expander > > + > > +maintainers: > > + - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > + > > +description: | > > + The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child > > + node of the parent MFD device. See > > + Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as > > + well as an example. > > + > > +properties: > > + compatible: > > + const: adi,adp5585-gpio > > + > > + gpio-controller: true > > + > > + '#gpio-cells': > > + const: 2 > > + > > + gpio-reserved-ranges: true > > There are no resources here, so new compatible is not really warranted. > Squash the node into parent. Child nodes seem (to me) to be the standard way to model functions in MFD devices. Looking at mfd_add_device(), for OF-based systems, the function iterates over child nodes. I don't mind going a different routes, could you indicate what you have in mind, perhaps pointing to an existing driver as an example ? > > + > > +required: > > + - compatible > > + - gpio-controller > > + - "#gpio-cells" > > + > > +additionalProperties: false > > + > > +... > > diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml > > new file mode 100644 > > index 000000000000..217c038b2842 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml > > @@ -0,0 +1,117 @@ > > +# 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@xxxxxxxxxxxxxxxx> > > + > > +description: | > > Do not need '|' unless you need to preserve formatting. > > > + 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: > > + $ref: /schemas/gpio/adi,adp5585-gpio.yaml > > + > > + pwm: > > + $ref: /schemas/pwm/adi,adp5585-pwm.yaml > > + > > +required: > > + - compatible > > + - reg > > + - gpio > > + - pwm > > + > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: adi,adp5585-01 > > + then: > > + properties: > > + gpio: > > + properties: > > + gpio-reserved-ranges: false > > This also points to fact your child node is pointless. It does not stand > on its own... That doesn't make the child pointless just for that reason. There are numerous examples of child nodes that don't stand on their own. > > + else: > > + properties: > > + gpio: > > + properties: > > + gpio-reserved-ranges: > > + items: > > + - const: 5 > > + - const: 1 > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mfd@34 { > > + compatible = "adi,adp5585-00", "adi,adp5585"; > > + reg = <0x34>; > > + > > + gpio { > > + compatible = "adi,adp5585-gpio"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + gpio-reserved-ranges = <5 1>; > > + }; > > + > > + pwm { > > + compatible = "adi,adp5585-pwm"; > > + #pwm-cells = <3>; > > + }; > > + }; > > + }; > > + > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mfd@34 { > > + compatible = "adi,adp5585-01", "adi,adp5585"; > > + reg = <0x34>; > > + > > + vdd-supply = <®_3v3>; > > + > > + gpio { > > + compatible = "adi,adp5585-gpio"; > > + gpio-controller; > > + #gpio-cells = <2>; > > Different by one property? So just keep one example, unless there are > more differences. I found the two examples useful during development of the binding to test the gpio-reserved-ranges rule (I got it wrong in the first place, and the dt schema validator told me), but I'm fine dropping one of the two. > > + }; > > + > > + pwm { > > + compatible = "adi,adp5585-pwm"; > > + #pwm-cells = <3>; > > + }; > > + }; > > + }; > > + > > +... > > diff --git a/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml > > new file mode 100644 > > index 000000000000..351a9d5da566 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml > > @@ -0,0 +1,35 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/adi,adp5585-pwm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices ADP5585 PWM Generator > > + > > +maintainers: > > + - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > + > > +description: | > > Do not need '|' unless you need to preserve formatting. > > > + The Analog Devices ADP5585 generates a PWM output with configurable frequency > > + and duty cycle represented by a "pwm" child node of the parent MFD device. > > + See Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further > > + details as well as an example. > > + > > +allOf: > > + - $ref: /schemas/pwm/pwm.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - adi,adp5585-pwm > > + > > + "#pwm-cells": > > + const: 3 > > Also no resources, so this can be part of the parent node. I'll sure follow the same design for the GPIO and PWM functions :-) Let's answer the above question first. -- Regards, Laurent Pinchart
