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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 14 Aug 2023 08:25:58 +0300
From:   Tony Lindgren <tony@...mide.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] dt-bindings: input: gpio-keys: Allow optional
 dedicated wakeirq

* Rob Herring <robh@...nel.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@...mide.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