linux-kernel - Re: [RFC 4/4] dt-bindings: gpio: Add bindings for SCMI pinctrl based gpio

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]

Message-ID: <ca6f8f30-c305-42bf-a812-b43561c4da2b@linaro.org>
Date:   Tue, 3 Oct 2023 10:43:31 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     AKASHI Takahiro <takahiro.akashi@...aro.org>,
        Rob Herring <robh@...nel.org>, sudeep.holla@....com,
        cristian.marussi@....com, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, linus.walleij@...aro.org,
        Oleksii_Moisieiev@...m.com, linux-arm-kernel@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org
Subject: Re: [RFC 4/4] dt-bindings: gpio: Add bindings for SCMI pinctrl based
 gpio

On 03/10/2023 02:41, AKASHI Takahiro wrote:
> Hi Rob,
> 
> On Mon, Oct 02, 2023 at 09:41:55AM -0500, Rob Herring wrote:
>> On Mon, Oct 02, 2023 at 11:16:02AM +0900, AKASHI Takahiro wrote:
>>> A dt binding for SCMI pinctrl based gpio driver is defined in this
>>> commit. It basically conforms to generic pinctrl-gpio mapping framework.
>>
>> What is "generic pinctrl-gpio mapping framework"? DT doesn't have 
>> frameworks.
> 
> I meant to refer to section 2.1-2.3 in "Documentation/devicetree/bindings/gpio/gpio.txt". The semantics is implemented in drivers/gpio/gpiolib(-of).c.

Linux specific GPIO library is as well outside of DT scope. Please focus
here on hardware, not Linux specifics.

> 
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@...aro.org>
>>> ---
>>>  .../bindings/gpio/arm,scmi-gpio.yaml          | 71 +++++++++++++++++++
>>>  1 file changed, 71 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..2601c5594567
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml
>>> @@ -0,0 +1,71 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: SCMI pinctrl based generic GPIO controller
>>> +
>>> +maintainers:
>>> +  - AKASHI Takahiro <akashi.takahiro@...aro.org>
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^scmi_gpio(@[0-9a-f]+)$"
>>
>> Not the correct name.
> 
> How not?

Underscores are no allowed and are pointed by dtc (W=2). scmi is
redundant here, because names should be generic. Anyway, we do not add
node name requirements to device schema.

> 
>>> +
>>> +  compatible:
>>> +    const: arm,scmi-gpio-generic
>>
>> What makes it generic? No such thing. Just drop '-generic'.
> 
> I will discuss this issue in following Cristian's comment.
> 
>>
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  "#gpio-cells":
>>> +    const: 2
>>> +
>>> +  gpio-ranges: true
>>> +
>>> +  gpio-ranges-group-names: true
>>> +
>>> +patternProperties:
>>> +  "^.+-hog(-[0-9]+)?$":
>>> +    type: object
>>> +    properties:
>>> +      gpio-hog: true
>>> +      gpios: true
>>> +      input: true
>>> +      output-high: true
>>> +      output-low: true
>>> +      line-name: true
>>> +
>>> +    required:
>>> +      - gpio-hog
>>> +      - gpios
>>
>> You don't need all this just 'required: [ gpio-hog ]'. Then the hog 
>> schema will check the rest.
> 
> Okay.
> 
>>> +
>>> +    additionalProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - gpio-controller
>>> +  - "#gpio-cells"
>>> +  - gpio-ranges
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +
>>> +    scmi_gpio_0: scmi_gpio@0 {
>>
>> gpio {
>>
>> But doesn't SCMI have protocol numbers?
>>
>>> +        compatible = "arm,scmi-gpio";
>>> +        gpio-controller;
>>> +        #gpio-cells = <2>;
>>> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
>>> +                      <&scmi_pinctrl 5 0 0>;
>>> +        gpio-ranges-group-names = "",
>>> +                                  "pinmux_gpio";
>>> +    };
>>> +
>>> +    // Consumer:
>>
>> Outside the scope of this binding. Drop this node.
> 
> Even though it's in an example?
> "#gpio-cells" has a meaning in consumer side.

Just look at any other bindings.

Best regards,
Krzysztof