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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>> ---
>>>  .../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@xxxxxxxxxx>
>>> +
>>> +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