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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 22 May 2024 08:57:56 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-pwm@...r.kernel.org,
 Alexandru Ardelean <alexandru.ardelean@...log.com>,
 Bartosz Golaszewski <brgl@...ev.pl>, Conor Dooley <conor+dt@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Lee Jones <lee@...nel.org>,
 Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
 Uwe Kleine-König <ukleinek@...nel.org>
Subject: Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices
 ADP5585

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@...asonboard.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@...asonboard.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@...asonboard.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