[1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq

  Allow configuring optional dedicated wakeirq that some SoCs have.
Let's use the interrupt naming "irq" and "wakeup" that we already have
in use for some drivers and subsystems like i2c.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../devicetree/bindings/input/gpio-keys.yaml      | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
  

On Fri, 11 Aug 2023 14:04:31 +0300, Tony Lindgren wrote:
> Allow configuring optional dedicated wakeirq that some SoCs have.
> Let's use the interrupt naming "irq" and "wakeup" that we already have
> in use for some drivers and subsystems like i2c.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../devicetree/bindings/input/gpio-keys.yaml      | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 

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:
./Documentation/devicetree/bindings/input/gpio-keys.yaml:40:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/input/gpio-keys.example.dts'
Documentation/devicetree/bindings/input/gpio-keys.yaml:40:1: found character '\t' that cannot start any token
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/input/gpio-keys.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/input/gpio-keys.yaml:40:1: found character '\t' that cannot start any token
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/gpio-keys.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230811110432.3968-1-tony@atomide.com

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 Fri, Aug 11, 2023 at 02:04:31PM +0300, Tony Lindgren wrote:
> Allow configuring optional dedicated wakeirq that some SoCs have.
> Let's use the interrupt naming "irq" and "wakeup" that we already have
> in use for some drivers and subsystems like i2c.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../devicetree/bindings/input/gpio-keys.yaml      | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.yaml b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> --- a/Documentation/devicetree/bindings/input/gpio-keys.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> @@ -31,7 +31,17 @@ patternProperties:
>          maxItems: 1
>  
>        interrupts:
> -        maxItems: 1
> +        description:
> +          Optional interrupts if different from the gpio interrupt
> +        maxItems: 2
> +
> +      interrupt-names:
> +        description:
> +	  Optional interrupt names, can be used to specify a separate
> +	  dedicated wake-up interrupt
> +        items:
> +          -const: irq
> +          -const: wakeup

Also need a space after '-'.

>  
>        label:
>          description: Descriptive name of the key.
> @@ -130,6 +140,9 @@ examples:
>              label = "GPIO Key UP";
>              linux,code = <103>;
>              gpios = <&gpio1 0 1>;
> +            interrupts-extended = <&intc_wakeup 0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "wakeup";

That's not what your schema allows. You need:

minItems: 1
items:
  - enum: [ irq, wakeup ]
  - const: wakeup

(repeating 'wakeup' is disallowed globally for ".*-names".)

> +            wakeup-source;

Of course with this, a single interrupt is the wake-up source and 
doesn't need a name. So you could define that 'interrupt-names' is only 
used when there are 2 interrupts. In that case, the schema is right and 
the example is wrong.

Rob
  

* Rob Herring <robh@kernel.org> [230811 15:10]:
> On Fri, Aug 11, 2023 at 02:04:31PM +0300, Tony Lindgren wrote:
> > Allow configuring optional dedicated wakeirq that some SoCs have.
> > Let's use the interrupt naming "irq" and "wakeup" that we already have
> > in use for some drivers and subsystems like i2c.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  .../devicetree/bindings/input/gpio-keys.yaml      | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/gpio-keys.yaml b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> > --- a/Documentation/devicetree/bindings/input/gpio-keys.yaml
> > +++ b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> > @@ -31,7 +31,17 @@ patternProperties:
> >          maxItems: 1
> >  
> >        interrupts:
> > -        maxItems: 1
> > +        description:
> > +          Optional interrupts if different from the gpio interrupt
> > +        maxItems: 2
> > +
> > +      interrupt-names:
> > +        description:
> > +	  Optional interrupt names, can be used to specify a separate
> > +	  dedicated wake-up interrupt
> > +        items:
> > +          -const: irq
> > +          -const: wakeup
> 
> Also need a space after '-'.

Oops sorry about that, obviously I did not run make dtbs_check on this
binding. I guess I just grepped so we don't have interrupt-names in use
right now.

> >  
> >        label:
> >          description: Descriptive name of the key.
> > @@ -130,6 +140,9 @@ examples:
> >              label = "GPIO Key UP";
> >              linux,code = <103>;
> >              gpios = <&gpio1 0 1>;
> > +            interrupts-extended = <&intc_wakeup 0 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "wakeup";
> 
> That's not what your schema allows. You need:
> 
> minItems: 1
> items:
>   - enum: [ irq, wakeup ]
>   - const: wakeup
> 
> (repeating 'wakeup' is disallowed globally for ".*-names".)

OK

> > +            wakeup-source;
> 
> Of course with this, a single interrupt is the wake-up source and 
> doesn't need a name. So you could define that 'interrupt-names' is only 
> used when there are 2 interrupts. In that case, the schema is right and 
> the example is wrong.

OK. So here's what gpio-keys currently allows:

1. gpios property with no interrupt in the dts, the driver tries to
   find the interrupt based on the gpio

2. gpios property with one interrupts property and no interrupt-names

And here's what we could allow in the binding with the wakeirq support
added:

1. gpios property with no interrupt in the dts, the driver tries to
   find the interrupt based on the gpio

2. gpios property with one interrupts property and no interrupt-names

3. gpios property with one interrupts property and interrupt-names = "irq"

4. gpios property with one wakeirq and interrupt-names = "wakeirq", the
   driver tries to find the io interrupt based on the gpio

5. gpios property with two interrupts and interrupt-names =
   "irq", "wakeirq"

So yeah we could only allow interrupt-names if there are two interrupts
like the attempted binding has. This would leave out #3 and #4 options
above. No need to limit these options from driver point of view though.
Any preferences on what the binding should have?

Regards,

Tony