diff mbox series

[V3] acpi: trigger wakeup key event from power button

Message ID 20230912131605.734829-1-Ken.Xue@amd.com (mailing list archive)
State Superseded, archived
Headers show
Series [V3] acpi: trigger wakeup key event from power button | expand

Commit Message

Ken Xue Sept. 12, 2023, 1:16 p.m. UTC 
Andorid can wakeup from various wakeup sources,
but only several wakeup sources can wake up screen
with right events(POWER, WAKEUP) from input device.

Regarding pressing acpi power button, it can resume system and
ACPI_BITMASK_WAKE_STATUS and ACPI_BITMASK_POWER_BUTTON_STATUS
are set in pm1a_sts, but kernel does not report any key
event to user space during resume by default.

So, trigger wakeup key event to user space during resume
from power button.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>

---
V1->V2: fix some compile warning/error caused by lack of
        "struct acpi_device" declaration by including acpi.h.
V2->V3: use "forward declaration" to fix compile warning/error.

 drivers/acpi/button.c | 16 ++++++++++++++++
 drivers/acpi/sleep.c  |  2 ++
 include/acpi/button.h |  4 ++++
 3 files changed, 22 insertions(+)


base-commit: b483d3b8a54a544ab8854ca6dbb8d99c423b3ba4

Comments

Andy Shevchenko Sept. 12, 2023, 3:22 p.m. UTC | #1 
On Tue, Sep 12, 2023 at 09:16:06PM +0800, Ken Xue wrote:
> Andorid can wakeup from various wakeup sources,
> but only several wakeup sources can wake up screen
> with right events(POWER, WAKEUP) from input device.


You still have a room for 10+ characters on each line in the above paragraph.
Why not use them?

> Regarding pressing acpi power button, it can resume system and
> ACPI_BITMASK_WAKE_STATUS and ACPI_BITMASK_POWER_BUTTON_STATUS
> are set in pm1a_sts, but kernel does not report any key
> event to user space during resume by default.
> 
> So, trigger wakeup key event to user space during resume
> from power button.
> 
> Signed-off-by: Ken Xue <Ken.Xue@amd.com>

...

> +void acpi_power_button_wakeup(struct acpi_device *dev)
> +{
> +	struct acpi_button *button = acpi_driver_data(dev);
> +	struct input_dev *input;

> +	if (button->type == ACPI_BUTTON_TYPE_POWER) {

It seems you missed the suggestion and I haven't heard an objection on written
the above as

	if (button->type != ACPI_BUTTON_TYPE_POWER)
		return;

> +		input = button->input;
> +		input_report_key(input, KEY_WAKEUP, 1);
> +		input_sync(input);
> +		input_report_key(input, KEY_WAKEUP, 0);
> +		input_sync(input);
> +	}
> +}

...

>  #include <linux/syscore_ops.h>
>  #include <asm/io.h>
>  #include <trace/events/power.h>
> +#include <acpi/button.h>

While at it, I would add a blank line here to distinguish groups of header
inclusions.

  #include <linux/syscore_ops.h>
  #include <asm/io.h>
  #include <trace/events/power.h>

  #include <acpi/button.h>

Or even

  #include <linux/syscore_ops.h>

  #include <asm/io.h>

  #include <trace/events/power.h>

  #include <acpi/button.h>
Ken Xue Sept. 13, 2023, 12:20 a.m. UTC | #2 
On 2023/9/12 23:22, Andy Shevchenko wrote:
> On Tue, Sep 12, 2023 at 09:16:06PM +0800, Ken Xue wrote:
>> Andorid can wakeup from various wakeup sources,
>> but only several wakeup sources can wake up screen
>> with right events(POWER, WAKEUP) from input device.
>
> You still have a room for 10+ characters on each line in the above paragraph.
> Why not use them?
>
ACK


>> Regarding pressing acpi power button, it can resume system and
>> ACPI_BITMASK_WAKE_STATUS and ACPI_BITMASK_POWER_BUTTON_STATUS
>> are set in pm1a_sts, but kernel does not report any key
>> event to user space during resume by default.
>>
>> So, trigger wakeup key event to user space during resume
>> from power button.
>>
>> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> ...
>
>> +void acpi_power_button_wakeup(struct acpi_device *dev)
>> +{
>> +	struct acpi_button *button = acpi_driver_data(dev);
>> +	struct input_dev *input;
>> +	if (button->type == ACPI_BUTTON_TYPE_POWER) {
> It seems you missed the suggestion and I haven't heard an objection on written
> the above as
>
> 	if (button->type != ACPI_BUTTON_TYPE_POWER)
> 		return;

ACK. sorry that I do missed the suggestion at first place.


>> +		input = button->input;
>> +		input_report_key(input, KEY_WAKEUP, 1);
>> +		input_sync(input);
>> +		input_report_key(input, KEY_WAKEUP, 0);
>> +		input_sync(input);
>> +	}
>> +}
> ...
>
>>   #include <linux/syscore_ops.h>
>>   #include <asm/io.h>
>>   #include <trace/events/power.h>
>> +#include <acpi/button.h>
> While at it, I would add a blank line here to distinguish groups of header
> inclusions.
>
>    #include <linux/syscore_ops.h>
>    #include <asm/io.h>
>    #include <trace/events/power.h>
>
>    #include <acpi/button.h>
>
> Or even
>
>    #include <linux/syscore_ops.h>
>
>    #include <asm/io.h>
>
>    #include <trace/events/power.h>
>
>    #include <acpi/button.h>
>
ACK
diff mbox series

Patch

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 1e76a64cce0a..3b8aa071732b 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -363,6 +363,21 @@  static int acpi_button_remove_fs(struct acpi_device *device)
 	return 0;
 }
 
+void acpi_power_button_wakeup(struct acpi_device *dev)
+{
+	struct acpi_button *button = acpi_driver_data(dev);
+	struct input_dev *input;
+
+	if (button->type == ACPI_BUTTON_TYPE_POWER) {
+		input = button->input;
+		input_report_key(input, KEY_WAKEUP, 1);
+		input_sync(input);
+		input_report_key(input, KEY_WAKEUP, 0);
+		input_sync(input);
+	}
+}
+EXPORT_SYMBOL(acpi_power_button_wakeup);
+
 /* Driver Interface */
 int acpi_lid_open(void)
 {
@@ -579,6 +594,7 @@  static int acpi_button_add(struct acpi_device *device)
 	switch (button->type) {
 	case ACPI_BUTTON_TYPE_POWER:
 		input_set_capability(input, EV_KEY, KEY_POWER);
+		input_set_capability(input, EV_KEY, KEY_WAKEUP);
 		break;
 
 	case ACPI_BUTTON_TYPE_SLEEP:
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 808484d11209..dcd5d0237eeb 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -22,6 +22,7 @@ 
 #include <linux/syscore_ops.h>
 #include <asm/io.h>
 #include <trace/events/power.h>
+#include <acpi/button.h>
 
 #include "internal.h"
 #include "sleep.h"
@@ -507,6 +508,7 @@  static void acpi_pm_finish(void)
 	pwr_btn_adev = acpi_dev_get_first_match_dev(ACPI_BUTTON_HID_POWERF,
 						    NULL, -1);
 	if (pwr_btn_adev) {
+		acpi_power_button_wakeup(pwr_btn_adev);
 		pm_wakeup_event(&pwr_btn_adev->dev, 0);
 		acpi_dev_put(pwr_btn_adev);
 	}
diff --git a/include/acpi/button.h b/include/acpi/button.h
index af2fce5d2ee3..6126d665aa42 100644
--- a/include/acpi/button.h
+++ b/include/acpi/button.h
@@ -2,17 +2,21 @@ 
 #ifndef ACPI_BUTTON_H
 #define ACPI_BUTTON_H
 
+struct acpi_device;
+
 #define ACPI_BUTTON_HID_POWER	"PNP0C0C"
 #define ACPI_BUTTON_HID_LID	"PNP0C0D"
 #define ACPI_BUTTON_HID_SLEEP	"PNP0C0E"
 
 #if IS_ENABLED(CONFIG_ACPI_BUTTON)
 extern int acpi_lid_open(void);
+extern void acpi_power_button_wakeup(struct acpi_device *dev);
 #else
 static inline int acpi_lid_open(void)
 {
 	return 1;
 }
+static inline void acpi_power_button_wakeup(struct acpi_device *dev) {}
 #endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */
 
 #endif /* ACPI_BUTTON_H */