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

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> >>> implement and test the rest of the features the chip supports.
> >>> ---
> >>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> >>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> >>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> >>>  MAINTAINERS                                   |   7 ++
> >>>  4 files changed, 195 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>> new file mode 100644
> >>> index 000000000000..210e4d53e764
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>> @@ -0,0 +1,36 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Analog Devices ADP5585 GPIO Expander
> >>> +
> >>> +maintainers:
> >>> +  - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>> +
> >>> +description: |
> >>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> >>> +  node of the parent MFD device. See
> >>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> >>> +  well as an example.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: adi,adp5585-gpio
> >>> +
> >>> +  gpio-controller: true
> >>> +
> >>> +  '#gpio-cells':
> >>> +    const: 2
> >>> +
> >>> +  gpio-reserved-ranges: true
> >>
> >> There are no resources here, so new compatible is not really warranted.
> >> Squash the node into parent.
> > 
> > Child nodes seem (to me) to be the standard way to model functions in
> > MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> > function iterates over child nodes. I don't mind going a different
> 
> 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@xxxxxxxxxxxxxxxx>
> >>> +
> >>> +description: |
> >>
> >> Do not need '|' unless you need to preserve formatting.
> >>
> >>> +  The ADP5585 is a 10/11 input/output port expander with a built in keypad
> >>> +  matrix decoder, programmable logic, reset generator, and PWM generator.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - adi,adp5585-00  # Default
> >>> +          - adi,adp5585-01  # 11 GPIOs
> >>> +          - adi,adp5585-02  # No pull-up resistors by default on special pins
> >>> +          - adi,adp5585-03  # Alternate I2C address
> >>> +          - adi,adp5585-04  # Pull-down resistors on all pins by default
> >>> +      - const: adi,adp5585
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  vdd-supply: true
> >>> +
> >>> +  gpio:
> >>> +    $ref: /schemas/gpio/adi,adp5585-gpio.yaml
> >>> +
> >>> +  pwm:
> >>> +    $ref: /schemas/pwm/adi,adp5585-pwm.yaml
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - gpio
> >>> +  - pwm
> >>> +
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: adi,adp5585-01
> >>> +    then:
> >>> +      properties:
> >>> +        gpio:
> >>> +          properties:
> >>> +            gpio-reserved-ranges: false
> >>
> >> This also points to fact your child node is pointless. It does not stand
> >> on its own...
> > 
> > That doesn't make the child pointless just for that reason. There are
> > numerous examples of child nodes that don't stand on their own.
> 
> 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.

-- 
Regards,

Laurent Pinchart