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

  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
  

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