| Message ID | 20231002021602.260100-5-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: add SCMI pinctrl based driver | expand |
On Mon, 02 Oct 2023 11:16:02 +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. > > 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 > 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/arm,scmi-gpio.example.dts:20.34-28.11: Warning (unit_address_vs_reg): /example-0/scmi_gpio@0: node has a unit name, but no reg or ranges property Documentation/devicetree/bindings/gpio/arm,scmi-gpio.example.dtb: /example-0/scmi_gpio@0: failed to match any schema with compatible: ['arm,scmi-gpio'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231002021602.260100-5-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 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. > > 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. > + > + compatible: > + const: arm,scmi-gpio-generic What makes it generic? No such thing. Just drop '-generic'. > + > + 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. > + > + 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. > + sdhci0_pwrseq { > + compatible = "mmc-pwrseq-emmc"; > + reset-gpios = <&scmi_gpio_0 0 GPIO_ACTIVE_LOW>; > + }; > -- > 2.34.1 >
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. [ snip] > > + 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? > My understanding is that this RFC GPIO driver from Akashi is built completely on Pinctrl facilities (as he says in the cover), it is not indeed a typical pure SCMI driver, it just happen to trigger the use of SCMI if the underlying backend pinctrl driver is pinctrl-scmi; but this driver does not really call directly into any SCMI API by itself, i.e. it does not get and call any SCMI protocol ops. (but it could indeed trigger the backend Pinctrl SCMI driver to issue such call on its behalf AFAIU...) I wonder why it has even a dependency on PINCTRL_SCMI at this point; is not that it could work (generically) even if the backend Pinctrl driver is NOT SCMI ? What makes it usable only against an SCMI Pinctrl backend ? Cannot be a generic GPIO driver based on top of Pinctrl, no matter which Pinctrl backend driver has been configured ? ...I maybe missing something here about Pinctrl AND GPIO frameworks :P Thanks, Cristian
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. > > > > 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? > > + > > + 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. -Takahiro Akashi > > + sdhci0_pwrseq { > > + compatible = "mmc-pwrseq-emmc"; > > + reset-gpios = <&scmi_gpio_0 0 GPIO_ACTIVE_LOW>; > > + }; > > -- > > 2.34.1 > >
Hi Rob, Cristian, On Mon, Oct 02, 2023 at 03:58:27PM +0100, Cristian Marussi wrote: > 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. > > [ snip] > > > > + 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? > > > > My understanding is that this RFC GPIO driver from Akashi is built > completely on Pinctrl facilities (as he says in the cover), it is not > indeed a typical pure SCMI driver, it just happen to trigger the use > of SCMI if the underlying backend pinctrl driver is pinctrl-scmi; > but this driver does not really call directly into any SCMI API by > itself, i.e. it does not get and call any SCMI protocol ops. > (but it could indeed trigger the backend Pinctrl SCMI driver to issue > such call on its behalf AFAIU...) It would be possible to implement this driver by directly using SCMI pinctrl interfaces (I mean drivers/firmware/arm,scmi/pinctrl.c) if the system wants to utilize SCMI solely for GPIO accesses and doesn't need pinctrl support. (Even so, "protocol@19" will be required due to the current SCMI binding.) But I didn't take this approach because the kernel's pinctrl framework (and many existing pinctrl drivers) instead adopts standard pinctrl- gpio mapping (I mean gpiolib(-of).c) and it just seems to work well. > I wonder why it has even a dependency on PINCTRL_SCMI at this point; > is not that it could work (generically) even if the backend Pinctrl > driver is NOT SCMI ? > What makes it usable only against an SCMI Pinctrl backend ? > Cannot be a generic GPIO driver based on top of Pinctrl, no matter which > Pinctrl backend driver has been configured ? That is one of my questions (See the issue (3) in my cover letter.) Why doesn't there exist a generic GPIO driver of this kind (based on gpiolib framework) even though it could apparently be possible? I guess that there a couple of reasons: 1) As I mentioned in the issue (1) in my cover letter, the current framework doesn't present an interface, especially for obtaining a value on a gpio input pin. Then it enforces each pinctrl-based gpio driver needs to have its own driver. 2) Furthermore, there may be driver-specific semantics required, say, for pinconf-related configurations? (I don't come up with any example, though) If my driver is good enough for applying to other gpio controllers as well, I would not hesitate to name it a genuine generic driver whether the backend may be SCMI or not. -> Linus, comment here please. Due to possible cases of (2), I still added "-generic" postfix to the "compatibles" property so that other variant drivers may be tagged as "arm,scmi-gpio-some-system" or "some-vendor,scmi-gpio". Thanks, -Takahiro Akashi > > ...I maybe missing something here about Pinctrl AND GPIO frameworks :P > > Thanks, > Cristian
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
Hi Takahiro, first, thanks for working on this important and crucial driver! I'll try to clarify and also explain something of what the others are saying (unless I misunderstand them...) On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > A dt binding for SCMI pinctrl based gpio driver is defined in this > commit. It basically conforms to generic pinctrl-gpio mapping framework. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> I think like Christian says that SCMI maybe has nothing to do with this binding? It is just one possible use case (though we don't know of any others.) The resource it is using is generic functionality that exist in any pin controller that provides ways to drive lines high and low etc. Would it be named a generic pin control-based GPIO? (...) > +++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml (...) > +$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml# So no ARM, no scmi, just pin-control-gpio.yaml, be bold! (I like this long unabbreviated name) > +title: SCMI pinctrl based generic GPIO controller Pin control-based generic GPIO controller Add description: The pin control-based GPIO will facilitate a pin controllers ability to drive electric lines high/low and other generic properties of a pin controller to perform general-purpose one-bit binary I/O. (At least I think this is the idea, I hope I understand correctly.) > +properties: > + $nodename: > + pattern: "^scmi_gpio(@[0-9a-f]+)$" These nodes are always just named gpio@... the resource marker is "this is a GPIO" that's all it means. > + compatible: > + const: arm,scmi-gpio-generic const: pin-control-gpio Other than that I am aboard with the solution! Yours, Linus Walleij
On Tue, Oct 03, 2023 at 03:16:49PM +0200, Linus Walleij wrote: > Hi Takahiro, > > first, thanks for working on this important and crucial driver! > > I'll try to clarify and also explain something of what the others > are saying (unless I misunderstand them...) Ah, thank you. > On Mon, Oct 2, 2023 at 4:17???AM AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > A dt binding for SCMI pinctrl based gpio driver is defined in this > > commit. It basically conforms to generic pinctrl-gpio mapping framework. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > I think like Christian says that SCMI maybe has nothing to do > with this binding? It is just one possible use case (though we don't know > of any others.) The resource it is using is generic functionality that exist > in any pin controller that provides ways to drive lines high and low > etc. > > Would it be named a generic pin control-based GPIO? If you like :) As I said, I was not confident that the driver be applicable to other pinctrl-gpio cases. > (...) > > +++ b/Documentation/devicetree/bindings/gpio/arm,scmi-gpio.yaml > (...) > > +$id: http://devicetree.org/schemas/gpio/arm,scmi-gpio.yaml# > > So no ARM, no scmi, just pin-control-gpio.yaml, be bold! I'm not so ambitious. > (I like this long unabbreviated name) > > > +title: SCMI pinctrl based generic GPIO controller > > Pin control-based generic GPIO controller > > Add > > description: > The pin control-based GPIO will facilitate a pin controllers ability > to drive electric lines high/low and other generic properties of a > pin controller to perform general-purpose one-bit binary I/O. > > (At least I think this is the idea, I hope I understand correctly.) Okay. > > +properties: > > + $nodename: > > + pattern: "^scmi_gpio(@[0-9a-f]+)$" > > These nodes are always just named gpio@... > the resource marker is "this is a GPIO" that's all it means. By following other gpio drivers' bindings, I will drop this rule. > > + compatible: > > + const: arm,scmi-gpio-generic > > const: pin-control-gpio > > Other than that I am aboard with the solution! Hope that the driver works on real hardware :) -Takahiro Akashi > Yours, > Linus Walleij
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]+)$" + + compatible: + const: arm,scmi-gpio-generic + + 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 + + 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 { + 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: + sdhci0_pwrseq { + compatible = "mmc-pwrseq-emmc"; + reset-gpios = <&scmi_gpio_0 0 GPIO_ACTIVE_LOW>; + };
A dt binding for SCMI pinctrl based gpio driver is defined in this commit. It basically conforms to generic pinctrl-gpio mapping framework. 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