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

Krzysztof Kozlowski
2023-10-03T08:43:42Z

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