diff mbox series

[RFC,v2,5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

Message ID 20231005025843.508689-6-takahiro.akashi@linaro.org
State New
Headers show
Series gpio: add pinctrl based generic gpio driver | expand

Commit Message

AKASHI Takahiro Oct. 5, 2023, 2:58 a.m. UTC
A dt binding for pin controller based generic gpio driver is defined in
this commit. One usable device is Arm's SCMI.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
RFC v2 (Oct 5, 2023)
* rename the binding to pin-control-gpio
* add the "description"
* remove nodename, hog properties, and a consumer example
RFC (Oct 2, 2023)
---
 .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml

Comments

Rob Herring (Arm) Oct. 6, 2023, 1:18 p.m. UTC | #1
On Thu, 05 Oct 2023 11:58:43 +0900, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)
> * rename the binding to pin-control-gpio
> * add the "description"
> * remove nodename, hog properties, and a consumer example
> RFC (Oct 2, 2023)
> ---
>  .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/gpio/pin-control-gpio.example.dts:18.23-26.11: Warning (unit_address_vs_reg): /example-0/gpio@0: node has a unit name, but no reg or ranges property

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231005025843.508689-6-takahiro.akashi@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring (Arm) Oct. 6, 2023, 1:23 p.m. UTC | #2
On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> A dt binding for pin controller based generic gpio driver is defined in
> this commit. One usable device is Arm's SCMI.

You don't need a "generic" binding to have a generic driver. Keep the 
binding specific and then decide in the OS to whether to use a generic 
or specific driver. That decision could change over time, but the 
binding can't. For example, see simple-panel.


> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> RFC v2 (Oct 5, 2023)
> * rename the binding to pin-control-gpio
> * add the "description"
> * remove nodename, hog properties, and a consumer example
> RFC (Oct 2, 2023)
> ---
>  .../bindings/gpio/pin-control-gpio.yaml       | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> new file mode 100644
> index 000000000000..bc935dbd7edb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/pin-control-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Pin control based generic GPIO controller
> +
> +description:
> +  The pin control-based GPIO will facilitate a pin controller's ability
> +  to drive electric lines high/low and other generic properties of a
> +  pin controller to perform general-purpose one-bit binary I/O.
> +
> +maintainers:
> +  - AKASHI Takahiro <akashi.takahiro@linaro.org>
> +
> +properties:
> +  compatible:
> +    const: pin-control-gpio
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-ranges: true
> +
> +  gpio-ranges-group-names: true
> +
> +patternProperties:
> +  "^.+-hog(-[0-9]+)?$":
> +    type: object
> +
> +    required:
> +      - gpio-hog
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio@0 {
> +        compatible = "pin-control-gpio";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> +                      <&scmi_pinctrl 5 0 0>;
> +        gpio-ranges-group-names = "",
> +                                  "pinmux_gpio";
> +    };
> -- 
> 2.34.1
>
Linus Walleij Oct. 9, 2023, 7:49 a.m. UTC | #3
On Fri, Oct 6, 2023 at 3:23 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:

> > A dt binding for pin controller based generic gpio driver is defined in
> > this commit. One usable device is Arm's SCMI.
>
> You don't need a "generic" binding to have a generic driver. Keep the
> binding specific and then decide in the OS to whether to use a generic
> or specific driver. That decision could change over time, but the
> binding can't. For example, see simple-panel.

What you say is true for simple-panel (a word like "simple" should
always cause red flags).

This case is more like mfd/syscon.yaml, where the singular
compatible = "syscon"; is in widespread use:

$ git grep 'compatible = \"syscon\";' |wc -l
50

I would accept adding a tuple compatible if you insist, so:

compatible = "foo-silicon", "pin-contro-gpio";

One case will be something like:

compatible = "optee-scmi-pin-control", "pin-control-gpio";

In this case I happen to know that we have the problem of
this being standardization work ahead of implementation on
actual hardware, and that is driven by the will known firmware
ambition to be completely abstract. It is supposed to sit on
top of pin control, or as part of pin control. Which leads me to
this thing (which I didn't think of before...)

> +    gpio0: gpio@0 {
> +        compatible = "pin-control-gpio";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> +                      <&scmi_pinctrl 5 0 0>;
> +        gpio-ranges-group-names = "",
> +                                  "pinmux_gpio";
> +    };

Maybe we should require that the pin-control-gpio node actually
be *inside* the pin control node, in this case whatever the label
&scmi_pinctrl is pointing to?

We can probably mandate that this has to be inside a pin controller
since it is a first.

Yours,
Linus Walleij
Cristian Marussi Oct. 9, 2023, 9:08 a.m. UTC | #4
On Mon, Oct 09, 2023 at 09:49:33AM +0200, Linus Walleij wrote:
> On Fri, Oct 6, 2023 at 3:23 PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> 

Hi Linus and all,

> > > A dt binding for pin controller based generic gpio driver is defined in
> > > this commit. One usable device is Arm's SCMI.
> >
> > You don't need a "generic" binding to have a generic driver. Keep the
> > binding specific and then decide in the OS to whether to use a generic
> > or specific driver. That decision could change over time, but the
> > binding can't. For example, see simple-panel.
> 
> What you say is true for simple-panel (a word like "simple" should
> always cause red flags).
> 
> This case is more like mfd/syscon.yaml, where the singular
> compatible = "syscon"; is in widespread use:
> 
> $ git grep 'compatible = \"syscon\";' |wc -l
> 50
> 
> I would accept adding a tuple compatible if you insist, so:
> 
> compatible = "foo-silicon", "pin-contro-gpio";
> 
> One case will be something like:
> 
> compatible = "optee-scmi-pin-control", "pin-control-gpio";
> 
> In this case I happen to know that we have the problem of
> this being standardization work ahead of implementation on
> actual hardware, and that is driven by the will known firmware
> ambition to be completely abstract. It is supposed to sit on
> top of pin control, or as part of pin control. Which leads me to
> this thing (which I didn't think of before...)
> 
> > +    gpio0: gpio@0 {
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > +                      <&scmi_pinctrl 5 0 0>;
> > +        gpio-ranges-group-names = "",
> > +                                  "pinmux_gpio";
> > +    };
> 

Assuming the above &scmi_pinctrl refers to the protocol node as we
usually do,  I am a bit puzzled by this example in this RFC series, because
usually in SCMI we DO refer to some resources using the phandle and the
domain IDs as in:

	scmi_sensor: protocol@15 {
		reg = <15>;
		#thermal-sensors-cells = <1>;
	};

	...

	thermal_zones {
		pmic {
			thermal-sensor = <&scmi_sensor 0>;
		};
	};
 
BUT in the SCMI Pinctrl case the current v4 Oleksii series takes advantage
of the existing Pinctrl bindings and naming to describe and refer to
pin/groups/functions, indeed #pinctrl-cells is defined as '0' in the
upcoming SCMI DT protocol node @19 in Oleksii v4, since indeed all the
parsing/matching is done by resource-names following the Picntrl
framework conventions. (AFAIU)

Moreover, due to how the SCMI Pinctrl protocol defines and describes the
pins/groups/functions using a tuple like (<TYPE>, <ID>) , with TYPE
being pin/group/function, a generic binding like the above would have to
be defined by at least 2 cells to be able to identify an SCMI PinCtrl
resource by index. (if that is the aim here...)

Am I right to think that such a generic SCMI PinCtrl binding is still to
be defined somewhere on the SCMI side, if needed as such by this GPIO driver ?

... or I am missing something ?

Thanks,
Cristian
Linus Walleij Oct. 9, 2023, 1:13 p.m. UTC | #5
On Mon, Oct 9, 2023 at 11:08 AM Cristian Marussi
<cristian.marussi@arm.com> wrote:

> > > +    gpio0: gpio@0 {
> > > +        compatible = "pin-control-gpio";
> > > +        gpio-controller;
> > > +        #gpio-cells = <2>;
> > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > +                      <&scmi_pinctrl 5 0 0>;
> > > +        gpio-ranges-group-names = "",
> > > +                                  "pinmux_gpio";
> > > +    };
> >
>
> Assuming the above &scmi_pinctrl refers to the protocol node as we
> usually do,

No it does not, it is a three-layer cake.

scmi <-> scmi_pinctrl <-> scmi_gpio

it refers to the scmi_pinctrl node.

There is no SCMI GPIO protocol, instead SCMI is using the
operations already available in the pin controller to exercise
GPIO. Generic pin control has operations to drive lines for
example, and Takahiro is adding the ability for a generic pin
controller to also read a line.

Yours,
Linus Walleij
Cristian Marussi Oct. 9, 2023, 3:08 p.m. UTC | #6
On Mon, Oct 09, 2023 at 03:13:24PM +0200, Linus Walleij wrote:
> On Mon, Oct 9, 2023 at 11:08 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> 
> > > > +    gpio0: gpio@0 {
> > > > +        compatible = "pin-control-gpio";
> > > > +        gpio-controller;
> > > > +        #gpio-cells = <2>;
> > > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > > +                      <&scmi_pinctrl 5 0 0>;
> > > > +        gpio-ranges-group-names = "",
> > > > +                                  "pinmux_gpio";
> > > > +    };
> > >
> >
> > Assuming the above &scmi_pinctrl refers to the protocol node as we
> > usually do,
> 
> No it does not, it is a three-layer cake.
> 
> scmi <-> scmi_pinctrl <-> scmi_gpio
> 
> it refers to the scmi_pinctrl node.
> 

Thanks, this explains a lot.
Cristian

> There is no SCMI GPIO protocol, instead SCMI is using the
> operations already available in the pin controller to exercise
> GPIO. Generic pin control has operations to drive lines for
> example, and Takahiro is adding the ability for a generic pin
> controller to also read a line.


> 
> Yours,
> Linus Walleij
AKASHI Takahiro Oct. 10, 2023, 5:14 a.m. UTC | #7
On Mon, Oct 09, 2023 at 04:08:13PM +0100, Cristian Marussi wrote:
> On Mon, Oct 09, 2023 at 03:13:24PM +0200, Linus Walleij wrote:
> > On Mon, Oct 9, 2023 at 11:08???AM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> > 
> > > > > +    gpio0: gpio@0 {
> > > > > +        compatible = "pin-control-gpio";
> > > > > +        gpio-controller;
> > > > > +        #gpio-cells = <2>;
> > > > > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > > > > +                      <&scmi_pinctrl 5 0 0>;
> > > > > +        gpio-ranges-group-names = "",
> > > > > +                                  "pinmux_gpio";
> > > > > +    };
> > > >
> > >
> > > Assuming the above &scmi_pinctrl refers to the protocol node as we
> > > usually do,
> > 
> > No it does not, it is a three-layer cake.
> > 
> > scmi <-> scmi_pinctrl <-> scmi_gpio
> > 
> > it refers to the scmi_pinctrl node.
> > 
> 
> Thanks, this explains a lot.
> Cristian

Just in case, 

    gpio-ranges = <&scmi_pinctrl 0 10 5>;

means that SCMI *pin* range [10..(10+5-1)] are mapped to this driver's
gpio range [0..(5-1)]. So any consumer driver can access a gpio pin
as:
    foo-gpios = <&gpio0 3>;

will refer to gpio pin#3 that is actually SCMI's 13.

    gpio-ranges = <&scmi_pinctrl 5 0 0>;
    gpio-ranges-group-names = "pinmux_gpio";

means that SCMI *group*, "pinmux_gpio", are mapped to this driver's
gpio range which starts with 5. If "pinmux_gpio" indicates SCMI *pin*
range [20..24],

    baa-gpios = <&gpio0 7>;
will refer to gpio pin#7 that is actually SCMI's 22 (=20 + (7-5)).

This way, we (consumer drivers) don't care what is the underlying pin
controller.

-Takahiro Akashi

> 
> > There is no SCMI GPIO protocol, instead SCMI is using the
> > operations already available in the pin controller to exercise
> > GPIO. Generic pin control has operations to drive lines for
> > example, and Takahiro is adding the ability for a generic pin
> > controller to also read a line.
> 
> 
> > 
> > Yours,
> > Linus Walleij
AKASHI Takahiro Oct. 10, 2023, 5:25 a.m. UTC | #8
On Mon, Oct 09, 2023 at 09:49:33AM +0200, Linus Walleij wrote:
> On Fri, Oct 6, 2023 at 3:23???PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Oct 05, 2023 at 11:58:43AM +0900, AKASHI Takahiro wrote:
> 
> > > A dt binding for pin controller based generic gpio driver is defined in
> > > this commit. One usable device is Arm's SCMI.
> >
> > You don't need a "generic" binding to have a generic driver. Keep the
> > binding specific and then decide in the OS to whether to use a generic
> > or specific driver. That decision could change over time, but the
> > binding can't. For example, see simple-panel.
> 
> What you say is true for simple-panel (a word like "simple" should
> always cause red flags).
> 
> This case is more like mfd/syscon.yaml, where the singular
> compatible = "syscon"; is in widespread use:
> 
> $ git grep 'compatible = \"syscon\";' |wc -l
> 50
> 
> I would accept adding a tuple compatible if you insist, so:
> 
> compatible = "foo-silicon", "pin-contro-gpio";
> 
> One case will be something like:
> 
> compatible = "optee-scmi-pin-control", "pin-control-gpio";
> 
> In this case I happen to know that we have the problem of
> this being standardization work ahead of implementation on
> actual hardware, and that is driven by the will known firmware
> ambition to be completely abstract. It is supposed to sit on
> top of pin control, or as part of pin control. Which leads me to
> this thing (which I didn't think of before...)
> 
> > +    gpio0: gpio@0 {
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio-ranges = <&scmi_pinctrl 0 10 5>,
> > +                      <&scmi_pinctrl 5 0 0>;
> > +        gpio-ranges-group-names = "",
> > +                                  "pinmux_gpio";
> > +    };
> 
> Maybe we should require that the pin-control-gpio node actually
> be *inside* the pin control node, in this case whatever the label
> &scmi_pinctrl is pointing to?

null (or '_' as dummy) if the dt schema allows such a value as
a trivial case?

> We can probably mandate that this has to be inside a pin controller
> since it is a first.

Yeah, my U-Boot implementation tentatively supports both (inside and
outside pin controller). But it is not a user's choice, but we should
decide which way to go.

Thanks,
-Takahiro Akashi

> Yours,
> Linus Walleij
AKASHI Takahiro Oct. 12, 2023, 1:15 a.m. UTC | #9
On Thu, Oct 05, 2023 at 09:48:09PM +0200, Krzysztof Kozlowski wrote:
> On 05/10/2023 04:58, AKASHI Takahiro wrote:
> > A dt binding for pin controller based generic gpio driver is defined in
> > this commit. One usable device is Arm's SCMI.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> > +
> > +required:
> > +  - compatible
> > +  - gpio-controller
> > +  - "#gpio-cells"
> > +  - gpio-ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gpio0: gpio@0 {
> 
> No reg, so no unit address.

My intention was to allow for multiple nodes (instances) of
pinctrl based gpio devices. But I don't care the naming.

> Drop also unused label.

Okay, I already dropped an example consumer device and have no need
for the label any longer.

-Takahiro Akashi

> 
> > +        compatible = "pin-control-gpio";
> > +        gpio-controller;
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Oct. 12, 2023, 7:27 a.m. UTC | #10
On 12/10/2023 03:15, AKASHI Takahiro wrote:

>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    gpio0: gpio@0 {
>>
>> No reg, so no unit address.
> 
> My intention was to allow for multiple nodes (instances) of
> pinctrl based gpio devices. But I don't care the naming.

How can you have unit address without reg? This causes warnings.

Best regards,
Krzysztof
AKASHI Takahiro Oct. 17, 2023, 2:32 a.m. UTC | #11
Hi Linus,

On Thu, Oct 12, 2023 at 09:25:20AM +0200, Linus Walleij wrote:
> On Tue, Oct 10, 2023 at 7:25???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > > We can probably mandate that this has to be inside a pin controller
> > > since it is a first.
> >
> > Yeah, my U-Boot implementation tentatively supports both (inside and
> > outside pin controller). But it is not a user's choice, but we should
> > decide which way to go.
> 
> OK I have decided we are going to put it inside the pin control node,
> as a subnode. (I don't expect anyone to object.)

While I'm still thinking of how I can modify my current implementation
to fit into 'inside' syntax, there are a couple of concerns:

1) invoke gpiochip_add_data() at probe function
Probably we no longer need "compatible" property, but instead we need to
call gpiochip_add_data() explicitly in SCMI pin controller's probe
as follows:

scmi_pinctrl_probe()
    ...
    devm_pinctrl_register_and_init(dev, ..., pctrldev);
    pinctrl_enable(pctrldev);

    device_for_each_child_node(dev, fwnode)
        if (fwnode contains "gpio-controller") {
            /* what pin_control_gpio_probe() does */
            gc->get_direction = ...;
            ...
            devm_gpiochip_data_add(dev, gc, ...);
        }

2) gpio-by-pinctrl.c
While this file is SCMI-independent now, due to a change at (1),
it would be better to move the whole content inside SCMI pin controller
driver (because there is no other user for now).

3) Then, pin-control-gpio.yaml may also be put into SCMI binding
(i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?

4) phandle in "gpio-ranges" property
(As you mentioned)
The first element in a tuple of "gpio-ranges" is a phandle to a pin
controller node. Now that the gpio node is a sub node of pin controller,
the phandle is trivial. But there is no easier way to represent it
than using an explicit label:
(My U-Boot implementation does this.)

scmi {
    ...
    scmi_pinctrl: protocol@19 {
        ...
        gpio {
            gpio-controller;
            ...
            gpio-ranges = <&scmi_pinctrl ... >;
        }
    }
}

I tried:
    gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
    gpio-ranges = <(-1) ...>; // dtc passed, but ...
    gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.

Do you have any other idea? Otherwise, I will modify my RFC
with the changes above.

-Takahiro Akashi


> It makes everything easier and clearer for users I think.
> 
> Yours,
> Linus Walleij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
new file mode 100644
index 000000000000..bc935dbd7edb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
@@ -0,0 +1,55 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/pin-control-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Pin control based generic GPIO controller
+
+description:
+  The pin control-based GPIO will facilitate a pin controller's ability
+  to drive electric lines high/low and other generic properties of a
+  pin controller to perform general-purpose one-bit binary I/O.
+
+maintainers:
+  - AKASHI Takahiro <akashi.takahiro@linaro.org>
+
+properties:
+  compatible:
+    const: pin-control-gpio
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-ranges-group-names: true
+
+patternProperties:
+  "^.+-hog(-[0-9]+)?$":
+    type: object
+
+    required:
+      - gpio-hog
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: gpio@0 {
+        compatible = "pin-control-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&scmi_pinctrl 0 10 5>,
+                      <&scmi_pinctrl 5 0 0>;
+        gpio-ranges-group-names = "",
+                                  "pinmux_gpio";
+    };