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

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

[Tony Lindgren]

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
 
       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";
+            wakeup-source;
         };
 
         key-down {
-- 
2.41.0

[PATCH 2/2] Input: gpio-keys - Add system suspend support for dedicated wakeirqs

[Tony Lindgren]

Some SoCs have a separate dedicated wake-up interrupt controller that can
be used to wake up the system from deeper idle states. We already support
configuring a separate interrupt for a gpio-keys button to be used with a
gpio line. However, we are lacking support system suspend for cases where
a separate interrupt needs to be used in deeper sleep modes.

Because of it's nature, gpio-keys does not know about the runtime PM state
of the button gpios, and may have several gpio buttons configured for each
gpio-keys device instance. Implementing runtime PM support for gpio-keys
does not help, and we cannot use drivers/base/power/wakeirq.c support. We
need to implement custom wakeirq support for gpio-keys.

For handling a dedicated wakeirq for system suspend, we enable and disable
it with gpio_keys_enable_wakeup() and gpio_keys_disable_wakeup() that we
already use based on device_may_wakeup().

Some systems may have a dedicated wakeirq that can also be used as the
main interrupt, this is already working for gpio-keys. Let's add some
wakeirq related comments while at it as the usage with a gpio line and
separate interrupt line may not be obvious.

Tested-by: Dhruva Gole <d-gole@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/gpio_keys.c | 69 ++++++++++++++++++++++++++++--
 include/linux/gpio_keys.h          |  2 +
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -45,7 +45,9 @@ struct gpio_button_data {
 	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
 
 	unsigned int irq;
+	unsigned int wakeirq;
 	unsigned int wakeup_trigger_type;
+
 	spinlock_t lock;
 	bool disabled;
 	bool key_pressed;
@@ -511,6 +513,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	struct gpio_button_data *bdata = &ddata->data[idx];
 	irq_handler_t isr;
 	unsigned long irqflags;
+	const char *wakedesc;
 	int irq;
 	int error;
 
@@ -575,6 +578,14 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 					!gpiod_cansleep(bdata->gpiod);
 		}
 
+		/*
+		 * If an interrupt was specified, use it instead of the gpio
+		 * interrupt and use the gpio for reading the state. A separate
+		 * interrupt may be used as the main button interrupt for
+		 * runtime PM to detect events also in deeper idle states. If a
+		 * dedicated wakeirq is used for system suspend only, see below
+		 * for bdata->wakeirq setup.
+		 */
 		if (button->irq) {
 			bdata->irq = button->irq;
 		} else {
@@ -672,6 +683,36 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		return error;
 	}
 
+	if (!button->wakeirq)
+		return 0;
+
+	/* Use :wakeup suffix like drivers/base/power/wakeirq.c does */
+	wakedesc = devm_kasprintf(dev, GFP_KERNEL, "%s:wakeup", desc);
+	if (!wakedesc)
+		return -ENOMEM;
+
+	bdata->wakeirq = button->wakeirq;
+	irqflags |= IRQF_NO_SUSPEND;
+
+	/*
+	 * Wakeirq shares the handler with the main interrupt, it's only
+	 * active during system suspend. See gpio_keys_button_enable_wakeup()
+	 * and gpio_keys_button_disable_wakeup().
+	 */
+	error = devm_request_any_context_irq(dev, bdata->wakeirq, isr,
+					     irqflags, wakedesc, bdata);
+	if (error < 0) {
+		dev_err(dev, "Unable to claim wakeirq %d; error %d\n",
+			bdata->irq, error);
+		return error;
+	}
+
+	/*
+	 * Disable wakeirq until suspend. IRQF_NO_AUTOEN won't work if
+	 * IRQF_SHARED was set based on !button->can_disable.
+	 */
+	disable_irq_nosync(bdata->wakeirq);
+
 	return 0;
 }
 
@@ -728,7 +769,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_button *button;
 	struct fwnode_handle *child;
-	int nbuttons;
+	int nbuttons, irq;
 
 	nbuttons = device_get_child_node_count(dev);
 	if (nbuttons == 0)
@@ -750,9 +791,19 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 	device_property_read_string(dev, "label", &pdata->name);
 
 	device_for_each_child_node(dev, child) {
-		if (is_of_node(child))
-			button->irq =
-				irq_of_parse_and_map(to_of_node(child), 0);
+		if (is_of_node(child)) {
+			irq = of_irq_get_byname(to_of_node(child), "irq");
+			if (irq > 0)
+				button->irq = irq;
+
+			irq = of_irq_get_byname(to_of_node(child), "wakeup");
+			if (irq > 0)
+				button->wakeirq = irq;
+
+			if (!button->irq && !button->wakeirq)
+				button->irq =
+					irq_of_parse_and_map(to_of_node(child), 0);
+		}
 
 		if (fwnode_property_read_u32(child, "linux,code",
 					     &button->code)) {
@@ -921,6 +972,11 @@ gpio_keys_button_enable_wakeup(struct gpio_button_data *bdata)
 		}
 	}
 
+	if (bdata->wakeirq) {
+		enable_irq(bdata->wakeirq);
+		disable_irq_nosync(bdata->irq);
+	}
+
 	return 0;
 }
 
@@ -929,6 +985,11 @@ gpio_keys_button_disable_wakeup(struct gpio_button_data *bdata)
 {
 	int error;
 
+	if (bdata->wakeirq) {
+		enable_irq(bdata->irq);
+		disable_irq_nosync(bdata->wakeirq);
+	}
+
 	/*
 	 * The trigger type is always both edges for gpio-based keys and we do
 	 * not support changing wakeup trigger for interrupt-based keys.
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -21,6 +21,7 @@ struct device;
  *			disable button via sysfs
  * @value:		axis value for %EV_ABS
  * @irq:		Irq number in case of interrupt keys
+ * @wakeirq:		Optional dedicated wake-up interrupt
  */
 struct gpio_keys_button {
 	unsigned int code;
@@ -34,6 +35,7 @@ struct gpio_keys_button {
 	bool can_disable;
 	int value;
 	unsigned int irq;
+	unsigned int wakeirq;
 };
 
 /**
-- 
2.41.0

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

[Rob Herring]

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.

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

[Rob Herring]

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

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

[Tony Lindgren]

* 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