Re: [PATCH v3 02/12] dt-bindings: pinctrl: Add RaspberryPi RP1 gpio/pinctrl/pinmux bindings

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 31/10/2024 15:07, Andrea della Porta wrote:
> Hi Krzysztof,
> 
> On 08:26 Tue 29 Oct     , Krzysztof Kozlowski wrote:
>> On Mon, Oct 28, 2024 at 03:07:19PM +0100, Andrea della Porta wrote:
>>> Add device tree bindings for the gpio/pin/mux controller that is part of
>>> the RP1 multi function device, and relative entries in MAINTAINERS file.
>>>
>>> Signed-off-by: Andrea della Porta <andrea.porta@xxxxxxxx>
>>> ---
>>>  .../pinctrl/raspberrypi,rp1-gpio.yaml         | 163 ++++++++++++++++++
>>>  MAINTAINERS                                   |   2 +
>>>  2 files changed, 165 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..465a53a6d84f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
>>> @@ -0,0 +1,163 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pinctrl/raspberrypi,rp1-gpio.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: RaspberryPi RP1 GPIO/Pinconf/Pinmux Controller submodule
>>> +
>>> +maintainers:
>>> +  - Andrea della Porta <andrea.porta@xxxxxxxx>
>>> +
>>> +description:
>>> +  The RP1 chipset is a Multi Function Device containing, among other sub-peripherals,
>>> +  a gpio/pinconf/mux controller whose 54 pins are grouped into 3 banks. It works also
>>
>> Please wrap code according to coding style (checkpatch is not a coding
>> style description but only a tool).
> 
> Ack.
> 
>>
>>> +  as an interrupt controller for those gpios.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: raspberrypi,rp1-gpio
>>> +
>>> +  reg:
>>> +    maxItems: 3
>>> +    description: One reg specifier for each one of the 3 pin banks.
>>> +
>>> +  '#gpio-cells':
>>> +    description: The first cell is the pin number and the second cell is used
>>> +      to specify the flags (see include/dt-bindings/gpio/gpio.h).
>>> +    const: 2
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  gpio-ranges:
>>> +    maxItems: 1
>>> +
>>> +  gpio-line-names:
>>> +    maxItems: 54
>>> +
>>> +  interrupts:
>>> +    maxItems: 3
>>> +    description: One interrupt specifier for each one of the 3 pin banks.
>>> +
>>> +  '#interrupt-cells':
>>> +    description:
>>> +      Specifies the Bank number [0, 1, 2] and Flags as defined in
>>> +      include/dt-bindings/interrupt-controller/irq.h.
>>> +    const: 2
>>> +
>>> +  interrupt-controller: true
>>> +
>>> +additionalProperties:
>>
>> Not much improved. You are supposed to have here pattern, just like
>> other bindings. I asked for this last time.
>>
>> And there are examples using it - almost all or most of pinctrl
>> bindings, including bindings having subnodes (but you do not use such
>> case here).
> 
> This is the same approach used in [1], which seems quite recent. I did't

2021, so not that recent, but you are right that it's not the example I
would recommend. See rather:
git grep pins -- Documentation/devicetree/bindings/pinctrl/ | grep '\$'


pins, groups, states, etc.

> use pattern because I wouldn't really want to enforce a particular naming
> scheme. Subnodes are used, please see below. Since pinctrl.yaml explicitly

But we want to enforce, because it brings uniformity and matches
partially generic naming patterns.

> says that there is no common binding but each device has its own, I
> thought that was reasonable choice. Should I enforce some common pattern,
> then?

Yes, you should. Again, look at other bindings, e.g. qcom tlmm or lpass lpi.

> 
>>
>>> +  anyOf:
>>> +    - type: object
>>> +      additionalProperties: false
>>> +      allOf:
>>> +        - $ref: pincfg-node.yaml#
>>> +        - $ref: pinmux-node.yaml#
>>> +
>>> +      description:
>>> +        Pin controller client devices use pin configuration subnodes (children
>>> +        and grandchildren) for desired pin configuration.
>>> +        Client device subnodes use below standard properties.
>>> +
>>> +      properties:
>>> +        pins:
>>> +          description:
>>> +            A string (or list of strings) adhering to the pattern 'gpio[0-5][0-9]'
>>> +        function: true
>>> +        bias-disable: true
>>> +        bias-pull-down: true
>>> +        bias-pull-up: true
>>> +        slew-rate:
>>> +          description: 0 is slow slew rate, 1 is fast slew rate
>>> +          enum: [ 0, 1 ]
>>> +        drive-strength:
>>> +          enum: [ 2, 4, 8, 12 ]
>>> +
>>> +    - type: object
>>> +      additionalProperties:
>>> +        $ref: "#/additionalProperties/anyOf/0"
>>
>> Your example does not use any subnodes, so this looks not needed.
> 
> The example has subnodes, as in the following excerpt from the example:

I meant, you do not need properties in subnodes (1st level). You only
want properties in subnodes of subnodes, so 2nd level. What is the point
of allowing them in 1st level?



Best regards,
Krzysztof