| Message ID | 20240520195942.11582-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
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@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. > --- > .../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@ideasonboard.com> > + > +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. > + > +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@ideasonboard.com> > + > +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... > + 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. > + }; > + > + 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@ideasonboard.com> > + > +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. Best regards, Krzysztof
On 21/05/2024 21:43, Laurent Pinchart wrote: > 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@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. >>> --- >>> .../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@ideasonboard.com> >>> + >>> +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 Only to assign of node, which could be skipped as well. > routes, could you indicate what you have in mind, perhaps pointing to an > existing driver as an example ? Most of them? OK, let's take the last added driver in MFD directory: cirrus,cs42l43 It has three children and only two nodes, because only these two devices actually need/use/benefit the subnodes. > >>> + >>> +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@ideasonboard.com> >>> + >>> +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. No, your if-then must be in the schema defining it. This is just unmaintianable code. It proves that child's compatible means nothing. If you cannot use child's compatible to make any meaningful choices, then it is useless. Best regards, Krzysztof
On 22/05/2024 08:57, Krzysztof Kozlowski wrote: > On 21/05/2024 21:43, Laurent Pinchart wrote: >> 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@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. >>>> --- >>>> .../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@ideasonboard.com> >>>> + >>>> +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 > > Only to assign of node, which could be skipped as well. > >> routes, could you indicate what you have in mind, perhaps pointing to an >> existing driver as an example ? > > Most of them? OK, let's take the last added driver in MFD directory: > cirrus,cs42l43 > It has three children and only two nodes, because only these two devices > actually need/use/benefit the subnodes. BTW, if these devices have their own I2C addresses or are reusable blocks, then of course their existence as child nodes is warranted. Similarly if these devices had additional subnodes, like pinctrl or SPI controller. Best regards, Krzysztof
On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: > On 21/05/2024 21:43, Laurent Pinchart wrote: > > 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@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. > >>> --- > >>> .../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@ideasonboard.com> > >>> + > >>> +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 > > Only to assign of node, which could be skipped as well. It has to be assigned somehow, otherwise the GPIO and PWM lookups won't work. That doesn't have to be done in mfd_add_device() though, it can also be done manually by the driver. Looking at the example you gave, cs42l43_pin_probe() handles that assignment. I would have considered that a bit of a hack, but if that's your preferred approach, I'm fine with it. Could you confirm you're OK with that ? > > routes, could you indicate what you have in mind, perhaps pointing to an > > existing driver as an example ? > > Most of them? OK, let's take the last added driver in MFD directory: > cirrus,cs42l43 > It has three children and only two nodes, because only these two devices > actually need/use/benefit the subnodes. Still trying to understand what bothers you here, is it the child nodes, or the fact that they have a compatible string and are documented in a separate binding ? Looking at the cirrus,cs42l43 bindings and the corresponding drivers, the pinctrl child node serves the purpose of grouping properties related to the pinctrl function, and allows referencing pinctrl entries from other DT nodes. All those properties could have been placed in the parent node. Are you fine with the adi,adp5585 having gpio and pwm child nodes, as long as they don't have compatible strings, and are documented in a single binding ? > >>> + > >>> +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@ideasonboard.com> > >>> + > >>> +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. > > No, your if-then must be in the schema defining it. This is just > unmaintianable code. It proves that child's compatible means nothing. If > you cannot use child's compatible to make any meaningful choices, then > it is useless. The compatible string may not be very useful. The child nodes have a use.
On 22/05/2024 09:22, Laurent Pinchart wrote: > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: >> On 21/05/2024 21:43, Laurent Pinchart wrote: >>> 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@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. >>>>> --- >>>>> .../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@ideasonboard.com> >>>>> + >>>>> +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 >> >> Only to assign of node, which could be skipped as well. > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't > work. That doesn't have to be done in mfd_add_device() though, it can > also be done manually by the driver. Looking at the example you gave, > cs42l43_pin_probe() handles that assignment. I would have considered > that a bit of a hack, but if that's your preferred approach, I'm fine > with it. Could you confirm you're OK with that ? I am fine with the drivers doing that. It's not a hack, for all sub-devices (e.g. also auxiliary bus) you won't have automatic of_node assignment. > >>> routes, could you indicate what you have in mind, perhaps pointing to an >>> existing driver as an example ? >> >> Most of them? OK, let's take the last added driver in MFD directory: >> cirrus,cs42l43 >> It has three children and only two nodes, because only these two devices >> actually need/use/benefit the subnodes. > > Still trying to understand what bothers you here, is it the child nodes, > or the fact that they have a compatible string and are documented in a > separate binding ? Looking at the cirrus,cs42l43 bindings and the What bothers me (and as expressed in many reviews by us) is representing driver structure directly in DT. People model DT based how their Linux drivers are represented. I don't care about driver stuff here, but DT/DTS. > corresponding drivers, the pinctrl child node serves the purpose of > grouping properties related to the pinctrl function, and allows > referencing pinctrl entries from other DT nodes. All those properties If you have sub-subnodes, it warrants for me such child. Why? Because it makes DTS easier to read. > could have been placed in the parent node. Are you fine with the > adi,adp5585 having gpio and pwm child nodes, as long as they don't have > compatible strings, and are documented in a single binding ? As well not, because then you have even less reasons to have them as separate nodes. With compatible, one could at least try to argue that sub-devices are re-usable across families. >>>>> +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. >> >> No, your if-then must be in the schema defining it. This is just >> unmaintianable code. It proves that child's compatible means nothing. If >> you cannot use child's compatible to make any meaningful choices, then >> it is useless. > > The compatible string may not be very useful. The child nodes have a > use. What is their use? Grouping few properties? As mentioned above - grouping subnodes like pinctrl does, is argument on its own for code readability. Grouping few properties, which in many other devices are in top-node (see last 100 reviews of new drivers doing exactly the same), is not that argument. OTOH, my first, main argument was: They do not have any resources on their own. Otherwise please point me - which property represents their resource, like clock, reset, gpio, suppy, IO address? Best regards, Krzysztof
Hi Krzysztof, (There's a question for the GPIO and PWM maintainers below) On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote: > On 22/05/2024 09:22, Laurent Pinchart wrote: > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: > >> On 21/05/2024 21:43, Laurent Pinchart wrote: > >>> 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@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. > >>>>> --- > >>>>> .../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@ideasonboard.com> > >>>>> + > >>>>> +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 > >> > >> Only to assign of node, which could be skipped as well. > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't > > work. That doesn't have to be done in mfd_add_device() though, it can > > also be done manually by the driver. Looking at the example you gave, > > cs42l43_pin_probe() handles that assignment. I would have considered > > that a bit of a hack, but if that's your preferred approach, I'm fine > > with it. Could you confirm you're OK with that ? > > I am fine with the drivers doing that. It's not a hack, for all > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node > assignment. I gave this a try, and here's what I came up with to drop the compatible string. Please ignore for a moment the fact that the child nodes are still there, that's an orthogonal question which I can address separately. What I would like is feedback on how the OF nodes are handled. diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c index 9696a4cdcfc1..8480ceef05ce 100644 --- a/drivers/gpio/gpio-adp5585.c +++ b/drivers/gpio/gpio-adp5585.c @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev) struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); struct adp5585_gpio_dev *adp5585_gpio; struct device *dev = &pdev->dev; + struct device_node *node; struct gpio_chip *gc; int ret; @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev) mutex_init(&adp5585_gpio->lock); + node = of_get_child_by_name(dev->parent->of_node, "gpio"); + if (!node) + return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n"); + + dev->of_node = node; + dev->fwnode = &node->fwnode; + gc = &adp5585_gpio->gpio_chip; gc->parent = dev; gc->direction_input = adp5585_gpio_direction_input; @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev) ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip, adp5585_gpio); if (ret) { + of_node_put(dev->of_node); + dev->of_node = NULL; + dev->fwnode = NULL; mutex_destroy(&adp5585_gpio->lock); return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n"); } @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev) { struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev); + of_node_put(pdev->dev.of_node); + pdev->dev.of_node = NULL; + pdev->dev.fwnode = NULL; + mutex_destroy(&adp5585_gpio->lock); } diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c index e39a6ea5f794..3b190567ea0b 100644 --- a/drivers/pwm/pwm-adp5585.c +++ b/drivers/pwm/pwm-adp5585.c @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = { static int adp5585_pwm_probe(struct platform_device *pdev) { struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); + struct device *dev = &pdev->dev; + struct device_node *node; struct pwm_chip *chip; int ret; @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev) if (IS_ERR(chip)) return PTR_ERR(chip); + node = of_get_child_by_name(dev->parent->of_node, "pwm"); + if (!node) + return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n"); + + dev->of_node = node; + dev->fwnode = &node->fwnode; + pwmchip_set_drvdata(chip, adp5585->regmap); chip->ops = &adp5585_pwm_ops; ret = devm_pwmchip_add(&pdev->dev, chip); - if (ret) + if (ret) { + of_node_put(dev->of_node); + dev->of_node = NULL; + dev->fwnode = NULL; return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); + } return 0; } +static void adp5585_pwm_remove(struct platform_device *pdev) +{ + of_node_put(pdev->dev.of_node); + pdev->dev.of_node = NULL; + pdev->dev.fwnode = NULL; +} + static struct platform_driver adp5585_pwm_driver = { .driver = { .name = "adp5585-pwm", Is this acceptable ? I'm a bit concerned about poking the internals of struct device directly from drivers. I have also refrained from setting fnode->dev to point back to the device as fone by cs42l43_pin_probe(), as a comment in struct fwnode_handle indicates that the dev field is for device links only and shouldn't be touched by anything else. I'm not sure if I should set it. > >>> routes, could you indicate what you have in mind, perhaps pointing to an > >>> existing driver as an example ? > >> > >> Most of them? OK, let's take the last added driver in MFD directory: > >> cirrus,cs42l43 > >> It has three children and only two nodes, because only these two devices > >> actually need/use/benefit the subnodes. > > > > Still trying to understand what bothers you here, is it the child nodes, > > or the fact that they have a compatible string and are documented in a > > separate binding ? Looking at the cirrus,cs42l43 bindings and the > > What bothers me (and as expressed in many reviews by us) is representing > driver structure directly in DT. People model DT based how their Linux > drivers are represented. I don't care about driver stuff here, but DT/DTS. DT models the hardware as seen from a software point of view. It shouldn't reflect the structure of Linux drivers, but it has to be usable by drivers. > > corresponding drivers, the pinctrl child node serves the purpose of > > grouping properties related to the pinctrl function, and allows > > referencing pinctrl entries from other DT nodes. All those properties > > If you have sub-subnodes, it warrants for me such child. Why? Because it > makes DTS easier to read. > > > could have been placed in the parent node. Are you fine with the > > adi,adp5585 having gpio and pwm child nodes, as long as they don't have > > compatible strings, and are documented in a single binding ? > > As well not, because then you have even less reasons to have them as > separate nodes. With compatible, one could at least try to argue that > sub-devices are re-usable across families. I'll reuse your argument, I think the child nodes make the DTS easier to read :-) > >>>>> +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. > >> > >> No, your if-then must be in the schema defining it. This is just > >> unmaintianable code. It proves that child's compatible means nothing. If > >> you cannot use child's compatible to make any meaningful choices, then > >> it is useless. > > > > The compatible string may not be very useful. The child nodes have a > > use. > > What is their use? Grouping few properties? As mentioned above - > grouping subnodes like pinctrl does, is argument on its own for code > readability. Grouping few properties, which in many other devices are in > top-node (see last 100 reviews of new drivers doing exactly the same), > is not that argument. > > OTOH, my first, main argument was: > > They do not have any resources on their own. Otherwise please point me - > which property represents their resource, like clock, reset, gpio, > suppy, IO address?
On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote: > Hi Krzysztof, > > (There's a question for the GPIO and PWM maintainers below) > > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote: > > On 22/05/2024 09:22, Laurent Pinchart wrote: > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: > > >> On 21/05/2024 21:43, Laurent Pinchart wrote: > > >>> 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@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. > > >>>>> --- > > >>>>> .../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@ideasonboard.com> > > >>>>> + > > >>>>> +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 > > >> > > >> Only to assign of node, which could be skipped as well. > > > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't > > > work. That doesn't have to be done in mfd_add_device() though, it can > > > also be done manually by the driver. Looking at the example you gave, > > > cs42l43_pin_probe() handles that assignment. I would have considered > > > that a bit of a hack, but if that's your preferred approach, I'm fine > > > with it. Could you confirm you're OK with that ? > > > > I am fine with the drivers doing that. It's not a hack, for all > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node > > assignment. > > I gave this a try, and here's what I came up with to drop the compatible > string. Please ignore for a moment the fact that the child nodes are > still there, that's an orthogonal question which I can address > separately. What I would like is feedback on how the OF nodes are > handled. > > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c > index 9696a4cdcfc1..8480ceef05ce 100644 > --- a/drivers/gpio/gpio-adp5585.c > +++ b/drivers/gpio/gpio-adp5585.c > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > struct adp5585_gpio_dev *adp5585_gpio; > struct device *dev = &pdev->dev; > + struct device_node *node; > struct gpio_chip *gc; > int ret; > > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > mutex_init(&adp5585_gpio->lock); > > + node = of_get_child_by_name(dev->parent->of_node, "gpio"); > + if (!node) > + return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n"); > + > + dev->of_node = node; > + dev->fwnode = &node->fwnode; Use device_set_of_node_from_dev(). > + > gc = &adp5585_gpio->gpio_chip; > gc->parent = dev; > gc->direction_input = adp5585_gpio_direction_input; > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip, > adp5585_gpio); > if (ret) { > + of_node_put(dev->of_node); > + dev->of_node = NULL; > + dev->fwnode = NULL; > mutex_destroy(&adp5585_gpio->lock); > return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n"); > } > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev) > { > struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev); > > + of_node_put(pdev->dev.of_node); > + pdev->dev.of_node = NULL; > + pdev->dev.fwnode = NULL; > + > mutex_destroy(&adp5585_gpio->lock); > } > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > index e39a6ea5f794..3b190567ea0b 100644 > --- a/drivers/pwm/pwm-adp5585.c > +++ b/drivers/pwm/pwm-adp5585.c > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = { > static int adp5585_pwm_probe(struct platform_device *pdev) > { > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > + struct device *dev = &pdev->dev; > + struct device_node *node; > struct pwm_chip *chip; > int ret; > > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev) > if (IS_ERR(chip)) > return PTR_ERR(chip); > > + node = of_get_child_by_name(dev->parent->of_node, "pwm"); > + if (!node) > + return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n"); > + > + dev->of_node = node; > + dev->fwnode = &node->fwnode; > + > pwmchip_set_drvdata(chip, adp5585->regmap); > chip->ops = &adp5585_pwm_ops; > > ret = devm_pwmchip_add(&pdev->dev, chip); > - if (ret) > + if (ret) { > + of_node_put(dev->of_node); > + dev->of_node = NULL; > + dev->fwnode = NULL; > return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > + } > > return 0; > } > > +static void adp5585_pwm_remove(struct platform_device *pdev) > +{ > + of_node_put(pdev->dev.of_node); Wouldn't the driver core do this already? It's not going to know how or when of_node was set, so should be doing a put regardless. > + pdev->dev.of_node = NULL; > + pdev->dev.fwnode = NULL; > +} > + > static struct platform_driver adp5585_pwm_driver = { > .driver = { > .name = "adp5585-pwm", > > Is this acceptable ? I'm a bit concerned about poking the internals of > struct device directly from drivers. > > I have also refrained from setting fnode->dev to point back to the > device as fone by cs42l43_pin_probe(), as a comment in struct > fwnode_handle indicates that the dev field is for device links only and > shouldn't be touched by anything else. I'm not sure if I should set it. I think no, but best for Saravana to comment. > > > >>> routes, could you indicate what you have in mind, perhaps pointing to an > > >>> existing driver as an example ? > > >> > > >> Most of them? OK, let's take the last added driver in MFD directory: > > >> cirrus,cs42l43 > > >> It has three children and only two nodes, because only these two devices > > >> actually need/use/benefit the subnodes. > > > > > > Still trying to understand what bothers you here, is it the child nodes, > > > or the fact that they have a compatible string and are documented in a > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the > > > > What bothers me (and as expressed in many reviews by us) is representing > > driver structure directly in DT. People model DT based how their Linux > > drivers are represented. I don't care about driver stuff here, but DT/DTS. > > DT models the hardware as seen from a software point of view. True, but it's for all software's PoV, not some specific s/w. > It > shouldn't reflect the structure of Linux drivers, but it has to be > usable by drivers. Either way is usable. Rob
Hi Rob, On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote: > On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote: > > Hi Krzysztof, > > > > (There's a question for the GPIO and PWM maintainers below) > > > > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote: > > > On 22/05/2024 09:22, Laurent Pinchart wrote: > > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: > > > >> On 21/05/2024 21:43, Laurent Pinchart wrote: > > > >>> 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@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. > > > >>>>> --- > > > >>>>> .../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@ideasonboard.com> > > > >>>>> + > > > >>>>> +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 > > > >> > > > >> Only to assign of node, which could be skipped as well. > > > > > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't > > > > work. That doesn't have to be done in mfd_add_device() though, it can > > > > also be done manually by the driver. Looking at the example you gave, > > > > cs42l43_pin_probe() handles that assignment. I would have considered > > > > that a bit of a hack, but if that's your preferred approach, I'm fine > > > > with it. Could you confirm you're OK with that ? > > > > > > I am fine with the drivers doing that. It's not a hack, for all > > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node > > > assignment. > > > > I gave this a try, and here's what I came up with to drop the compatible > > string. Please ignore for a moment the fact that the child nodes are > > still there, that's an orthogonal question which I can address > > separately. What I would like is feedback on how the OF nodes are > > handled. > > > > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c > > index 9696a4cdcfc1..8480ceef05ce 100644 > > --- a/drivers/gpio/gpio-adp5585.c > > +++ b/drivers/gpio/gpio-adp5585.c > > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > struct adp5585_gpio_dev *adp5585_gpio; > > struct device *dev = &pdev->dev; > > + struct device_node *node; > > struct gpio_chip *gc; > > int ret; > > > > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > > mutex_init(&adp5585_gpio->lock); > > > > + node = of_get_child_by_name(dev->parent->of_node, "gpio"); > > + if (!node) > > + return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n"); > > + > > + dev->of_node = node; > > + dev->fwnode = &node->fwnode; > > Use device_set_of_node_from_dev(). That only works without child nodes in DT. Here I'm assigning the gpio child node, not the node of the parent device. > > + > > gc = &adp5585_gpio->gpio_chip; > > gc->parent = dev; > > gc->direction_input = adp5585_gpio_direction_input; > > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip, > > adp5585_gpio); > > if (ret) { > > + of_node_put(dev->of_node); > > + dev->of_node = NULL; > > + dev->fwnode = NULL; > > mutex_destroy(&adp5585_gpio->lock); > > return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n"); > > } > > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev) > > { > > struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev); > > > > + of_node_put(pdev->dev.of_node); > > + pdev->dev.of_node = NULL; > > + pdev->dev.fwnode = NULL; > > + > > mutex_destroy(&adp5585_gpio->lock); > > } > > > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > > index e39a6ea5f794..3b190567ea0b 100644 > > --- a/drivers/pwm/pwm-adp5585.c > > +++ b/drivers/pwm/pwm-adp5585.c > > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = { > > static int adp5585_pwm_probe(struct platform_device *pdev) > > { > > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > + struct device *dev = &pdev->dev; > > + struct device_node *node; > > struct pwm_chip *chip; > > int ret; > > > > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev) > > if (IS_ERR(chip)) > > return PTR_ERR(chip); > > > > + node = of_get_child_by_name(dev->parent->of_node, "pwm"); > > + if (!node) > > + return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n"); > > + > > + dev->of_node = node; > > + dev->fwnode = &node->fwnode; > > + > > pwmchip_set_drvdata(chip, adp5585->regmap); > > chip->ops = &adp5585_pwm_ops; > > > > ret = devm_pwmchip_add(&pdev->dev, chip); > > - if (ret) > > + if (ret) { > > + of_node_put(dev->of_node); > > + dev->of_node = NULL; > > + dev->fwnode = NULL; > > return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > > + } > > > > return 0; > > } > > > > +static void adp5585_pwm_remove(struct platform_device *pdev) > > +{ > > + of_node_put(pdev->dev.of_node); > > Wouldn't the driver core do this already? It's not going to know how or > when of_node was set, so should be doing a put regardless. It does, but only when the struct device is being destroyed. Unbinding and rebinding would leak a reference. Using device_set_of_node_from_dev() solves that problem, but doesn't work with child nodes. I'm going to send a v2 that squashes everything in a single DT node, which allows usage of device_set_of_node_from_dev(). Let's see if it will be more palatable. > > + pdev->dev.of_node = NULL; > > + pdev->dev.fwnode = NULL; > > +} > > + > > static struct platform_driver adp5585_pwm_driver = { > > .driver = { > > .name = "adp5585-pwm", > > > > Is this acceptable ? I'm a bit concerned about poking the internals of > > struct device directly from drivers. > > > > I have also refrained from setting fnode->dev to point back to the > > device as fone by cs42l43_pin_probe(), as a comment in struct > > fwnode_handle indicates that the dev field is for device links only and > > shouldn't be touched by anything else. I'm not sure if I should set it. > > I think no, but best for Saravana to comment. > > > > >>> routes, could you indicate what you have in mind, perhaps pointing to an > > > >>> existing driver as an example ? > > > >> > > > >> Most of them? OK, let's take the last added driver in MFD directory: > > > >> cirrus,cs42l43 > > > >> It has three children and only two nodes, because only these two devices > > > >> actually need/use/benefit the subnodes. > > > > > > > > Still trying to understand what bothers you here, is it the child nodes, > > > > or the fact that they have a compatible string and are documented in a > > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the > > > > > > What bothers me (and as expressed in many reviews by us) is representing > > > driver structure directly in DT. People model DT based how their Linux > > > drivers are represented. I don't care about driver stuff here, but DT/DTS. > > > > DT models the hardware as seen from a software point of view. > > True, but it's for all software's PoV, not some specific s/w. Agreed. > > It > > shouldn't reflect the structure of Linux drivers, but it has to be > > usable by drivers. > > Either way is usable.
On Tue, May 28, 2024 at 8:08 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Rob, > > On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote: > > On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote: > > > Hi Krzysztof, > > > > > > (There's a question for the GPIO and PWM maintainers below) > > > > > > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote: > > > > On 22/05/2024 09:22, Laurent Pinchart wrote: > > > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: > > > > >> On 21/05/2024 21:43, Laurent Pinchart wrote: > > > > >>> 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@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. > > > > >>>>> --- > > > > >>>>> .../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@ideasonboard.com> > > > > >>>>> + > > > > >>>>> +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 > > > > >> > > > > >> Only to assign of node, which could be skipped as well. > > > > > > > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't > > > > > work. That doesn't have to be done in mfd_add_device() though, it can > > > > > also be done manually by the driver. Looking at the example you gave, > > > > > cs42l43_pin_probe() handles that assignment. I would have considered > > > > > that a bit of a hack, but if that's your preferred approach, I'm fine > > > > > with it. Could you confirm you're OK with that ? > > > > > > > > I am fine with the drivers doing that. It's not a hack, for all > > > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node > > > > assignment. > > > > > > I gave this a try, and here's what I came up with to drop the compatible > > > string. Please ignore for a moment the fact that the child nodes are > > > still there, that's an orthogonal question which I can address > > > separately. What I would like is feedback on how the OF nodes are > > > handled. > > > > > > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c > > > index 9696a4cdcfc1..8480ceef05ce 100644 > > > --- a/drivers/gpio/gpio-adp5585.c > > > +++ b/drivers/gpio/gpio-adp5585.c > > > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > struct adp5585_gpio_dev *adp5585_gpio; > > > struct device *dev = &pdev->dev; > > > + struct device_node *node; > > > struct gpio_chip *gc; > > > int ret; > > > > > > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > > > > mutex_init(&adp5585_gpio->lock); > > > > > > + node = of_get_child_by_name(dev->parent->of_node, "gpio"); > > > + if (!node) > > > + return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n"); > > > + > > > + dev->of_node = node; > > > + dev->fwnode = &node->fwnode; > > > > Use device_set_of_node_from_dev(). > > That only works without child nodes in DT. Here I'm assigning the gpio > child node, not the node of the parent device. > > > > + > > > gc = &adp5585_gpio->gpio_chip; > > > gc->parent = dev; > > > gc->direction_input = adp5585_gpio_direction_input; > > > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip, > > > adp5585_gpio); > > > if (ret) { > > > + of_node_put(dev->of_node); > > > + dev->of_node = NULL; > > > + dev->fwnode = NULL; > > > mutex_destroy(&adp5585_gpio->lock); > > > return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n"); > > > } > > > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev) > > > { > > > struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev); > > > > > > + of_node_put(pdev->dev.of_node); > > > + pdev->dev.of_node = NULL; > > > + pdev->dev.fwnode = NULL; > > > + > > > mutex_destroy(&adp5585_gpio->lock); > > > } > > > > > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > > > index e39a6ea5f794..3b190567ea0b 100644 > > > --- a/drivers/pwm/pwm-adp5585.c > > > +++ b/drivers/pwm/pwm-adp5585.c > > > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = { > > > static int adp5585_pwm_probe(struct platform_device *pdev) > > > { > > > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > + struct device *dev = &pdev->dev; > > > + struct device_node *node; > > > struct pwm_chip *chip; > > > int ret; > > > > > > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev) > > > if (IS_ERR(chip)) > > > return PTR_ERR(chip); > > > > > > + node = of_get_child_by_name(dev->parent->of_node, "pwm"); > > > + if (!node) > > > + return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n"); > > > + > > > + dev->of_node = node; > > > + dev->fwnode = &node->fwnode; > > > + > > > pwmchip_set_drvdata(chip, adp5585->regmap); > > > chip->ops = &adp5585_pwm_ops; > > > > > > ret = devm_pwmchip_add(&pdev->dev, chip); > > > - if (ret) > > > + if (ret) { > > > + of_node_put(dev->of_node); > > > + dev->of_node = NULL; > > > + dev->fwnode = NULL; > > > return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > > > + } > > > > > > return 0; > > > } > > > > > > +static void adp5585_pwm_remove(struct platform_device *pdev) > > > +{ > > > + of_node_put(pdev->dev.of_node); > > > > Wouldn't the driver core do this already? It's not going to know how or > > when of_node was set, so should be doing a put regardless. > > It does, but only when the struct device is being destroyed. Unbinding > and rebinding would leak a reference. Using > device_set_of_node_from_dev() solves that problem, but doesn't work with > child nodes. > > I'm going to send a v2 that squashes everything in a single DT node, > which allows usage of device_set_of_node_from_dev(). Let's see if it > will be more palatable. > > > > + pdev->dev.of_node = NULL; > > > + pdev->dev.fwnode = NULL; > > > +} > > > + > > > static struct platform_driver adp5585_pwm_driver = { > > > .driver = { > > > .name = "adp5585-pwm", > > > > > > Is this acceptable ? I'm a bit concerned about poking the internals of > > > struct device directly from drivers. > > > > > > I have also refrained from setting fnode->dev to point back to the > > > device as fone by cs42l43_pin_probe(), as a comment in struct > > > fwnode_handle indicates that the dev field is for device links only and > > > shouldn't be touched by anything else. I'm not sure if I should set it. > > > > I think no, but best for Saravana to comment. Don't set fwnode->dev. But I'd actually go even further and say don't set dev->fwnode to NULL. If it ever needs to be set to NULL the driver core will take care of it. And when it does that is when it'll set fwnode->dev to NULL too. So, you setting dev->fwnode to NULL is actually not good once you add the device. -Saravana > > > > > > >>> routes, could you indicate what you have in mind, perhaps pointing to an > > > > >>> existing driver as an example ? > > > > >> > > > > >> Most of them? OK, let's take the last added driver in MFD directory: > > > > >> cirrus,cs42l43 > > > > >> It has three children and only two nodes, because only these two devices > > > > >> actually need/use/benefit the subnodes. > > > > > > > > > > Still trying to understand what bothers you here, is it the child nodes, > > > > > or the fact that they have a compatible string and are documented in a > > > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the > > > > > > > > What bothers me (and as expressed in many reviews by us) is representing > > > > driver structure directly in DT. People model DT based how their Linux > > > > drivers are represented. I don't care about driver stuff here, but DT/DTS. > > > > > > DT models the hardware as seen from a software point of view. > > > > True, but it's for all software's PoV, not some specific s/w. > > Agreed. > > > > It > > > shouldn't reflect the structure of Linux drivers, but it has to be > > > usable by drivers. > > > > Either way is usable. > > -- > Regards, > > Laurent Pinchart
On Wed, May 29, 2024 at 12:02:39AM +0200, Saravana Kannan wrote: > On Tue, May 28, 2024 at 8:08 PM Laurent Pinchart wrote: > > On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote: > > > On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote: > > > > Hi Krzysztof, > > > > > > > > (There's a question for the GPIO and PWM maintainers below) > > > > > > > > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote: > > > > > On 22/05/2024 09:22, Laurent Pinchart wrote: > > > > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote: > > > > > >> On 21/05/2024 21:43, Laurent Pinchart wrote: > > > > > >>> 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@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. > > > > > >>>>> --- > > > > > >>>>> .../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@ideasonboard.com> > > > > > >>>>> + > > > > > >>>>> +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 > > > > > >> > > > > > >> Only to assign of node, which could be skipped as well. > > > > > > > > > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't > > > > > > work. That doesn't have to be done in mfd_add_device() though, it can > > > > > > also be done manually by the driver. Looking at the example you gave, > > > > > > cs42l43_pin_probe() handles that assignment. I would have considered > > > > > > that a bit of a hack, but if that's your preferred approach, I'm fine > > > > > > with it. Could you confirm you're OK with that ? > > > > > > > > > > I am fine with the drivers doing that. It's not a hack, for all > > > > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node > > > > > assignment. > > > > > > > > I gave this a try, and here's what I came up with to drop the compatible > > > > string. Please ignore for a moment the fact that the child nodes are > > > > still there, that's an orthogonal question which I can address > > > > separately. What I would like is feedback on how the OF nodes are > > > > handled. > > > > > > > > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c > > > > index 9696a4cdcfc1..8480ceef05ce 100644 > > > > --- a/drivers/gpio/gpio-adp5585.c > > > > +++ b/drivers/gpio/gpio-adp5585.c > > > > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > > struct adp5585_gpio_dev *adp5585_gpio; > > > > struct device *dev = &pdev->dev; > > > > + struct device_node *node; > > > > struct gpio_chip *gc; > > > > int ret; > > > > > > > > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > > > > > > mutex_init(&adp5585_gpio->lock); > > > > > > > > + node = of_get_child_by_name(dev->parent->of_node, "gpio"); > > > > + if (!node) > > > > + return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n"); > > > > + > > > > + dev->of_node = node; > > > > + dev->fwnode = &node->fwnode; > > > > > > Use device_set_of_node_from_dev(). > > > > That only works without child nodes in DT. Here I'm assigning the gpio > > child node, not the node of the parent device. > > > > > > + > > > > gc = &adp5585_gpio->gpio_chip; > > > > gc->parent = dev; > > > > gc->direction_input = adp5585_gpio_direction_input; > > > > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev) > > > > ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip, > > > > adp5585_gpio); > > > > if (ret) { > > > > + of_node_put(dev->of_node); > > > > + dev->of_node = NULL; > > > > + dev->fwnode = NULL; > > > > mutex_destroy(&adp5585_gpio->lock); > > > > return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n"); > > > > } > > > > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev) > > > > { > > > > struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev); > > > > > > > > + of_node_put(pdev->dev.of_node); > > > > + pdev->dev.of_node = NULL; > > > > + pdev->dev.fwnode = NULL; > > > > + > > > > mutex_destroy(&adp5585_gpio->lock); > > > > } > > > > > > > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > > > > index e39a6ea5f794..3b190567ea0b 100644 > > > > --- a/drivers/pwm/pwm-adp5585.c > > > > +++ b/drivers/pwm/pwm-adp5585.c > > > > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = { > > > > static int adp5585_pwm_probe(struct platform_device *pdev) > > > > { > > > > struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > > + struct device *dev = &pdev->dev; > > > > + struct device_node *node; > > > > struct pwm_chip *chip; > > > > int ret; > > > > > > > > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev) > > > > if (IS_ERR(chip)) > > > > return PTR_ERR(chip); > > > > > > > > + node = of_get_child_by_name(dev->parent->of_node, "pwm"); > > > > + if (!node) > > > > + return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n"); > > > > + > > > > + dev->of_node = node; > > > > + dev->fwnode = &node->fwnode; > > > > + > > > > pwmchip_set_drvdata(chip, adp5585->regmap); > > > > chip->ops = &adp5585_pwm_ops; > > > > > > > > ret = devm_pwmchip_add(&pdev->dev, chip); > > > > - if (ret) > > > > + if (ret) { > > > > + of_node_put(dev->of_node); > > > > + dev->of_node = NULL; > > > > + dev->fwnode = NULL; > > > > return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > > > > + } > > > > > > > > return 0; > > > > } > > > > > > > > +static void adp5585_pwm_remove(struct platform_device *pdev) > > > > +{ > > > > + of_node_put(pdev->dev.of_node); > > > > > > Wouldn't the driver core do this already? It's not going to know how or > > > when of_node was set, so should be doing a put regardless. > > > > It does, but only when the struct device is being destroyed. Unbinding > > and rebinding would leak a reference. Using > > device_set_of_node_from_dev() solves that problem, but doesn't work with > > child nodes. > > > > I'm going to send a v2 that squashes everything in a single DT node, > > which allows usage of device_set_of_node_from_dev(). Let's see if it > > will be more palatable. > > > > > > + pdev->dev.of_node = NULL; > > > > + pdev->dev.fwnode = NULL; > > > > +} > > > > + > > > > static struct platform_driver adp5585_pwm_driver = { > > > > .driver = { > > > > .name = "adp5585-pwm", > > > > > > > > Is this acceptable ? I'm a bit concerned about poking the internals of > > > > struct device directly from drivers. > > > > > > > > I have also refrained from setting fnode->dev to point back to the > > > > device as fone by cs42l43_pin_probe(), as a comment in struct > > > > fwnode_handle indicates that the dev field is for device links only and > > > > shouldn't be touched by anything else. I'm not sure if I should set it. > > > > > > I think no, but best for Saravana to comment. > > Don't set fwnode->dev. But I'd actually go even further and say don't > set dev->fwnode to NULL. If it ever needs to be set to NULL the driver > core will take care of it. And when it does that is when it'll set > fwnode->dev to NULL too. So, you setting dev->fwnode to NULL is > actually not good once you add the device. As far as I understand, the driver core will set dev->fwnode and dev->of_node to NULL when the device is destroyed. As dev->of_node is set in the probe function, an unbind-rebind cycle would result in a reference leak if I don't put the dev->of_node reference in the .remove() handler. And if I do so, I don't want to leave dev->of_node and dev->fwnode pointing to nodes whose reference has been released, as they could then possibly disappear (in theory at least). In any case, I've changed the DT bindings in v2, and the child devices now reuse the OF node of the parent MFD, instead of using subnodes. This allows using device_set_of_node_from_dev(), which seems to do fix this whole issue. > > > > > >>> routes, could you indicate what you have in mind, perhaps pointing to an > > > > > >>> existing driver as an example ? > > > > > >> > > > > > >> Most of them? OK, let's take the last added driver in MFD directory: > > > > > >> cirrus,cs42l43 > > > > > >> It has three children and only two nodes, because only these two devices > > > > > >> actually need/use/benefit the subnodes. > > > > > > > > > > > > Still trying to understand what bothers you here, is it the child nodes, > > > > > > or the fact that they have a compatible string and are documented in a > > > > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the > > > > > > > > > > What bothers me (and as expressed in many reviews by us) is representing > > > > > driver structure directly in DT. People model DT based how their Linux > > > > > drivers are represented. I don't care about driver stuff here, but DT/DTS. > > > > > > > > DT models the hardware as seen from a software point of view. > > > > > > True, but it's for all software's PoV, not some specific s/w. > > > > Agreed. > > > > > > It > > > > shouldn't reflect the structure of Linux drivers, but it has to be > > > > usable by drivers. > > > > > > Either way is usable.
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@ideasonboard.com> + +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 + +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@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: + $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 + 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>; + }; + + 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@ideasonboard.com> + +description: | + 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 + +required: + - compatible + - "#pwm-cells" + +additionalProperties: false + +... diff --git a/MAINTAINERS b/MAINTAINERS index 28e20975c26f..afecdb82e783 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -497,6 +497,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
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@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. --- .../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