diff mbox series

[2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

Message ID 20240520195942.11582-3-laurent.pinchart@ideasonboard.com
State New
Headers show
Series ADP5585 GPIO expander, PWM and keypad controller support | expand

Commit Message

Laurent Pinchart May 20, 2024, 7:59 p.m. UTC
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

Comments

Krzysztof Kozlowski May 21, 2024, 7:05 p.m. UTC | #1
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 = <&reg_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
Krzysztof Kozlowski May 22, 2024, 6:57 a.m. UTC | #2
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
Krzysztof Kozlowski May 22, 2024, 7:20 a.m. UTC | #3
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
Laurent Pinchart May 22, 2024, 7:22 a.m. UTC | #4
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.
Krzysztof Kozlowski May 22, 2024, 7:40 a.m. UTC | #5
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
Laurent Pinchart May 23, 2024, 11:16 p.m. UTC | #6
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?
Rob Herring (Arm) May 28, 2024, 3:12 p.m. UTC | #7
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
Laurent Pinchart May 28, 2024, 6:08 p.m. UTC | #8
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.
Saravana Kannan May 28, 2024, 10:02 p.m. UTC | #9
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
Laurent Pinchart May 28, 2024, 10:21 p.m. UTC | #10
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 mbox series

Patch

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 = <&reg_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