| Message ID | 20231005025843.508689-6-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: add pinctrl based generic gpio driver | expand |
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.
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 >
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
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
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
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
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
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
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 >
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
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 --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"; + };
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