public inbox for linux-acpi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] acpi: trigger wakeup key event from power button
@ 2023-09-07  7:43 Ken Xue
  2023-09-07 18:51 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ken Xue @ 2023-09-07  7:43 UTC (permalink / raw)
  To: linux-acpi, rafael, andriy.shevchenko; +Cc: Ken.Xue

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>
---
 drivers/acpi/button.c | 18 ++++++++++++++++++
 drivers/acpi/sleep.c  |  2 ++
 include/acpi/button.h |  5 +++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 1e76a64cce0a..22f41526731e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -363,6 +363,23 @@ static int acpi_button_remove_fs(struct acpi_device *device)
 	return 0;
 }
 
+void acpi_power_button_wakeup(struct acpi_device *device)
+{
+	struct acpi_button *button = acpi_driver_data(device);
+	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);
+	}
+
+	return;
+}
+EXPORT_SYMBOL(acpi_power_button_wakeup);
+
 /* Driver Interface */
 int acpi_lid_open(void)
 {
@@ -579,6 +596,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..67e2dab27401 100644
--- a/include/acpi/button.h
+++ b/include/acpi/button.h
@@ -8,11 +8,16 @@
 
 #if IS_ENABLED(CONFIG_ACPI_BUTTON)
 extern int acpi_lid_open(void);
+extern void acpi_power_button_wakeup(struct acpi_device *device);
 #else
 static inline int acpi_lid_open(void)
 {
 	return 1;
 }
+static inline void acpi_power_button_wakeup(struct acpi_device *device)
+{
+	return;
+}
 #endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */
 
 #endif /* ACPI_BUTTON_H */
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-07  7:43 Ken Xue
@ 2023-09-07 18:51 ` kernel test robot
  2023-09-07 19:34 ` kernel test robot
  2023-09-07 20:06 ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-09-07 18:51 UTC (permalink / raw)
  To: Ken Xue, linux-acpi, rafael, andriy.shevchenko; +Cc: oe-kbuild-all, Ken.Xue

Hi Ken,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master v6.5 next-20230907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ken-Xue/acpi-trigger-wakeup-key-event-from-power-button/20230907-232828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20230907074342.7722-1-Ken.Xue%40amd.com
patch subject: [PATCH] acpi: trigger wakeup key event from power button
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20230908/202309080239.IiC7uLpW-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230908/202309080239.IiC7uLpW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/nouveau/nouveau_connector.c:27:
>> include/acpi/button.h:11:45: warning: 'struct acpi_device' declared inside parameter list will not be visible outside of this definition or declaration
      11 | extern void acpi_power_button_wakeup(struct acpi_device *device);
         |                                             ^~~~~~~~~~~


vim +11 include/acpi/button.h

     8	
     9	#if IS_ENABLED(CONFIG_ACPI_BUTTON)
    10	extern int acpi_lid_open(void);
  > 11	extern void acpi_power_button_wakeup(struct acpi_device *device);
    12	#else
    13	static inline int acpi_lid_open(void)
    14	{
    15		return 1;
    16	}
    17	static inline void acpi_power_button_wakeup(struct acpi_device *device)
    18	{
    19		return;
    20	}
    21	#endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */
    22	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-07  7:43 Ken Xue
  2023-09-07 18:51 ` kernel test robot
@ 2023-09-07 19:34 ` kernel test robot
  2023-09-07 20:06 ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-09-07 19:34 UTC (permalink / raw)
  To: Ken Xue, linux-acpi, rafael, andriy.shevchenko; +Cc: oe-kbuild-all, Ken.Xue

Hi Ken,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master v6.5 next-20230907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ken-Xue/acpi-trigger-wakeup-key-event-from-power-button/20230907-232828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20230907074342.7722-1-Ken.Xue%40amd.com
patch subject: [PATCH] acpi: trigger wakeup key event from power button
config: i386-randconfig-r032-20230908 (https://download.01.org/0day-ci/archive/20230908/202309080315.txQUEyHQ-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230908/202309080315.txQUEyHQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/acpi/sleep.o: in function `acpi_pm_finish':
>> drivers/acpi/sleep.c:511: undefined reference to `acpi_power_button_wakeup'


vim +511 drivers/acpi/sleep.c

   469	
   470	/**
   471	 *	acpi_pm_finish - Instruct the platform to leave a sleep state.
   472	 *
   473	 *	This is called after we wake back up (or if entering the sleep state
   474	 *	failed).
   475	 */
   476	static void acpi_pm_finish(void)
   477	{
   478		struct acpi_device *pwr_btn_adev;
   479		u32 acpi_state = acpi_target_sleep_state;
   480	
   481		acpi_ec_unblock_transactions();
   482		suspend_nvs_free();
   483	
   484		if (acpi_state == ACPI_STATE_S0)
   485			return;
   486	
   487		pr_info("Waking up from system sleep state S%d\n", acpi_state);
   488		acpi_disable_wakeup_devices(acpi_state);
   489		acpi_leave_sleep_state(acpi_state);
   490	
   491		/* reset firmware waking vector */
   492		acpi_set_waking_vector(0);
   493	
   494		acpi_target_sleep_state = ACPI_STATE_S0;
   495	
   496		acpi_resume_power_resources();
   497	
   498		/* If we were woken with the fixed power button, provide a small
   499		 * hint to userspace in the form of a wakeup event on the fixed power
   500		 * button device (if it can be found).
   501		 *
   502		 * We delay the event generation til now, as the PM layer requires
   503		 * timekeeping to be running before we generate events. */
   504		if (!pwr_btn_event_pending)
   505			return;
   506	
   507		pwr_btn_event_pending = false;
   508		pwr_btn_adev = acpi_dev_get_first_match_dev(ACPI_BUTTON_HID_POWERF,
   509							    NULL, -1);
   510		if (pwr_btn_adev) {
 > 511			acpi_power_button_wakeup(pwr_btn_adev);
   512			pm_wakeup_event(&pwr_btn_adev->dev, 0);
   513			acpi_dev_put(pwr_btn_adev);
   514		}
   515	}
   516	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-07  7:43 Ken Xue
  2023-09-07 18:51 ` kernel test robot
  2023-09-07 19:34 ` kernel test robot
@ 2023-09-07 20:06 ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-09-07 20:06 UTC (permalink / raw)
  To: Ken Xue, linux-acpi, rafael, andriy.shevchenko
  Cc: llvm, oe-kbuild-all, Ken.Xue

Hi Ken,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on linus/master next-20230907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ken-Xue/acpi-trigger-wakeup-key-event-from-power-button/20230907-232828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20230907074342.7722-1-Ken.Xue%40amd.com
patch subject: [PATCH] acpi: trigger wakeup key event from power button
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20230908/202309080351.xHt2qhP2-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230908/202309080351.xHt2qhP2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/display/intel_lvds.c:30:
>> include/acpi/button.h:11:45: warning: declaration of 'struct acpi_device' will not be visible outside of this function [-Wvisibility]
   extern void acpi_power_button_wakeup(struct acpi_device *device);
                                               ^
   1 warning generated.


vim +11 include/acpi/button.h

     8	
     9	#if IS_ENABLED(CONFIG_ACPI_BUTTON)
    10	extern int acpi_lid_open(void);
  > 11	extern void acpi_power_button_wakeup(struct acpi_device *device);
    12	#else
    13	static inline int acpi_lid_open(void)
    14	{
    15		return 1;
    16	}
    17	static inline void acpi_power_button_wakeup(struct acpi_device *device)
    18	{
    19		return;
    20	}
    21	#endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */
    22	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] acpi: trigger wakeup key event from power button
@ 2023-09-08  9:57 Ken Xue
  2023-09-11  9:42 ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Ken Xue @ 2023-09-08  9:57 UTC (permalink / raw)
  To: linux-acpi
  Cc: Ken.Xue, andriy.shevchenko, oe-kbuild-all, rafael,
	kernel test robot

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.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/


Signed-off-by: Ken Xue <Ken.Xue@amd.com>
---
 drivers/acpi/button.c | 16 ++++++++++++++++
 drivers/acpi/sleep.c  |  2 ++
 include/acpi/button.h |  6 ++++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 1e76a64cce0a..0e0f30286c22 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 *device)
+{
+	struct acpi_button *button = acpi_driver_data(device);
+	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..fa0fb170cfb5 100644
--- a/include/acpi/button.h
+++ b/include/acpi/button.h
@@ -2,17 +2,23 @@
 #ifndef ACPI_BUTTON_H
 #define ACPI_BUTTON_H
 
+#include <linux/acpi.h>
+
 #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 *device);
 #else
 static inline int acpi_lid_open(void)
 {
 	return 1;
 }
+static inline void acpi_power_button_wakeup(struct acpi_device *device)
+{
+}
 #endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */
 
 #endif /* ACPI_BUTTON_H */

base-commit: b483d3b8a54a544ab8854ca6dbb8d99c423b3ba4
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-08  9:57 [PATCH] acpi: trigger wakeup key event from power button Ken Xue
@ 2023-09-11  9:42 ` Andy Shevchenko
  2023-09-12  5:32   ` Ken Xue
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2023-09-11  9:42 UTC (permalink / raw)
  To: Ken Xue; +Cc: linux-acpi, oe-kbuild-all, rafael, kernel test robot

On Fri, Sep 08, 2023 at 05:57:49PM +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.
> 
> 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.

> Reported-by: kernel test robot <lkp@intel.com>

Are you sure?

> Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/
> Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/

Are you sure?

> 
> 

Blank lines are not allowed in the tag block.

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

How this change is different to the previous patch you sent?
Do you forgot versioning?
Do you forgot changelog?

Please, read Submitting Patches documentation before trying again.
It will help to make your contribution nice and understandable.

...

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

	if (... != )
		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/acpi.h>

There is no users of this header.

Check how forward declaration can be used (as it's done in many other headers).


> +extern void acpi_power_button_wakeup(struct acpi_device *device);

...

> +static inline void acpi_power_button_wakeup(struct acpi_device *device)
> +{
> +}

This can be done on a single line.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-11  9:42 ` Andy Shevchenko
@ 2023-09-12  5:32   ` Ken Xue
  2023-09-12  9:30     ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Ken Xue @ 2023-09-12  5:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-acpi, oe-kbuild-all, rafael, kernel test robot, cwhuang


On 2023/9/11 17:42, Andy Shevchenko wrote:
> On Fri, Sep 08, 2023 at 05:57:49PM +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.
>>
>> 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.
>> Reported-by: kernel test robot <lkp@intel.com>
> Are you sure?

Thanks for review. Sorry for confusion.

1)The patch is trying to fix the android wakeup from power button issue

I can see Android can resume from S3 and wake up screen from USB keyboard.

but Android only can resume from S3 and fail to wake up screen from 
power button.


opensource android-x86 tried to fix this issue with a patch

(https://git.osdn.net/view?p=android-x86/system-hardware-interfaces.git;a=commit;h=78688b149314ec16cb2d90507a5908e5f2ba0fda) 


in upper layer system_suspend service as a WA.  It  emulates wakeup key 
during resume in a user space thread. And it is still not part of 
upstream Android.


I believe the root cause is no wakeup key event being reported in kernel 
during the resume from acpi power button.

And I have verified this patch on linux also, it seems no side effect 
for Linux resume with this patch.


2) test robot reported some compile warnings and errors detected by test 
robot which is fixed in V2.


>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/
>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/
>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/
> Are you sure?

Just some errors/warnings from the v1 patch.


>
>>
> Blank lines are not allowed in the tag block.
>
>> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
>> ---
> How this change is different to the previous patch you sent?
> Do you forgot versioning?
> Do you forgot changelog?
>
> Please, read Submitting Patches documentation before trying again.
> It will help to make your contribution nice and understandable.
>
> ...

sorry.

I will add V3 and change log later.


>
>> +	if (button->type == ACPI_BUTTON_TYPE_POWER) {
> 	if (... != )
> 		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/acpi.h>
> There is no users of this header.
>
> Check how forward declaration can be used (as it's done in many other headers).
>
Yes, "struct acpi_device" is defined in "include/acpi/acpi_bus.h", but include acpi_bus.h alone

will lead to more compile issues.

Regarding "forward declaration", how about

typedef struct acpi_device *acpi_device;

>> +extern void acpi_power_button_wakeup(struct acpi_device *device);
> ...
>
>> +static inline void acpi_power_button_wakeup(struct acpi_device *device)
>> +{
>> +}
> This can be done on a single line.
>
Ack.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-12  5:32   ` Ken Xue
@ 2023-09-12  9:30     ` Andy Shevchenko
  2023-09-12 12:53       ` Ken Xue
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2023-09-12  9:30 UTC (permalink / raw)
  To: Ken Xue; +Cc: linux-acpi, oe-kbuild-all, rafael, kernel test robot, cwhuang

On Tue, Sep 12, 2023 at 01:32:02PM +0800, Ken Xue wrote:
> On 2023/9/11 17:42, Andy Shevchenko wrote:
> > On Fri, Sep 08, 2023 at 05:57:49PM +0800, Ken Xue wrote:

...

> > > Reported-by: kernel test robot <lkp@intel.com>
> > Are you sure?
> 
> Thanks for review. Sorry for confusion.

> 2) test robot reported some compile warnings and errors detected by test
> robot which is fixed in V2.

Yes and that's what I'm asking about. You are not supposed to add it as the
initial problem, the patch is trying to solve, has _not_ been reported by LKP,
hasn't it?

...

> > > Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/
> > Are you sure?
> 
> Just some errors/warnings from the v1 patch.

Same as above.

...

> > > +#include <linux/acpi.h>
> > There are no users of this header.
> > 
> > Check how forward declaration can be used (as it's done in many other headers).
> > 
> Yes, "struct acpi_device" is defined in "include/acpi/acpi_bus.h", but
> include acpi_bus.h alone will lead to more compile issues.
> 
> Regarding "forward declaration", how about
> 
> typedef struct acpi_device *acpi_device;

Is it a forward declaration?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] acpi: trigger wakeup key event from power button
  2023-09-12  9:30     ` Andy Shevchenko
@ 2023-09-12 12:53       ` Ken Xue
  0 siblings, 0 replies; 9+ messages in thread
From: Ken Xue @ 2023-09-12 12:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-acpi, oe-kbuild-all, rafael, kernel test robot, cwhuang


On 2023/9/12 17:30, Andy Shevchenko wrote:
> On Tue, Sep 12, 2023 at 01:32:02PM +0800, Ken Xue wrote:
>> On 2023/9/11 17:42, Andy Shevchenko wrote:
>>> On Fri, Sep 08, 2023 at 05:57:49PM +0800, Ken Xue wrote:
...
>> 2) test robot reported some compile warnings and errors detected by test
>> robot which is fixed in V2.
> Yes and that's what I'm asking about. You are not supposed to add it as the
> initial problem, the patch is trying to solve, has _not_ been reported by LKP,
> hasn't it?
>
> ...
>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/
>>> Are you sure?
>> Just some errors/warnings from the v1 patch.
> Same as above.

Get it.  Those info will not be included in commit message.


>
> ...
>
>>>> +#include <linux/acpi.h>
>>> There are no users of this header.
>>>
>>> Check how forward declaration can be used (as it's done in many other headers).
>>>
>> Yes, "struct acpi_device" is defined in "include/acpi/acpi_bus.h", but
>> include acpi_bus.h alone will lead to more compile issues.
>>
>> Regarding "forward declaration", how about
>>
>> typedef struct acpi_device *acpi_device;
> Is it a forward declaration?
Ok, I will use "struct acpi_device;"

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-09-12 12:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-08  9:57 [PATCH] acpi: trigger wakeup key event from power button Ken Xue
2023-09-11  9:42 ` Andy Shevchenko
2023-09-12  5:32   ` Ken Xue
2023-09-12  9:30     ` Andy Shevchenko
2023-09-12 12:53       ` Ken Xue
  -- strict thread matches above, loose matches on Subject: below --
2023-09-07  7:43 Ken Xue
2023-09-07 18:51 ` kernel test robot
2023-09-07 19:34 ` kernel test robot
2023-09-07 20:06 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox