platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
Subsystems (2)
PATCH 1/1 platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
From: Jérôme de Bretagne <[email protected]>
On Dell Latitude 7275 the 5-button array is not exposed in the ACPI tables, but still notifies are sent to the Intel HID device object (device ID INT33D5) in response to power button actions. They were ignored as the intel-hid driver was not prepared to take care of them until recently.
Power button wakeup from suspend-to-idle was added in:635173a17b03 ("intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle"). However power button suspend doesn't work yet on this platform so it would be good to add it also.
On the affected platform (for which priv->array is NULL), add a new upfront check against the power button press notification (0xCE) to notify_handler(), outside the wakeup mode this time, which allows to report the power button press event and trigger the suspend. Also catch and ignore the corresponding power button release notification (0xCF) to stop it from being reported as an "unknown event" in the logs.
--- drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/platform/x86/intel-hid.c +16 -0
@@ -226,6 +226,22 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)return ; } +/* +* Needed for suspend to work on some platforms that don't expose +* the 5-button array, but still send notifies with power button +* event code to this device object on power button actions. +* +* Report the power button press; catch and ignore the button release. +*/ +if (!priv->array) { +if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ; + } +/* 0xC0 is for HID events, other values are for 5 button array */ if (event !=0xc0 ) { if (!priv->array ||
Quoted 66 lines
-----Original Message-----
From: Jérôme de Bretagne [mailto:[email protected]]
Sent: Sunday, September 17, 2017 5:57 PM
To: [email protected]
Cc: Darren Hart <[email protected]>; LKML <[email protected]>; Linux ACPI <[email protected]>; Rafael J. Wysocki <[email protected]>; Andy Shevchenko <[email protected]>; Limonciello, Mario <[email protected]>; Alex Hung <[email protected]>
Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275From: Jérôme de Bretagne <[email protected]>
On Dell Latitude 7275 the 5-button array is not exposed in the ACPI tables, but still notifies are sent to the Intel HID device object (device ID INT33D5) in response to power button actions. They were ignored as the intel-hid driver was not prepared to take care of them until recently.
Power button wakeup from suspend-to-idle was added in:
635173a17b03("intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle"). However power button suspend doesn't work yet on this platform so it would be good to add it also.On the affected platform (for which
priv->arrayis NULL), add a new upfront check against the power button press notification (0xCE) tonotify_handler(), outside the wakeup mode this time, which allows to report the power button press event and trigger the suspend. Also catch and ignore the corresponding power button release notification (0xCF) to stop it from being reported as an "unknown event" in the logs.Tested-by Jérôme de Bretagne <[email protected]>Signed-off-by Jérôme de Bretagne <[email protected]>--- drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/platform/x86/intel-hid.c +16
-0@@ -226,6 +226,22 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) return; } +/* +* Needed for suspend to work on some platforms that don't expose +* the 5-button array, but still send notifies with power button +* event code to this device object on power button actions. +* +* Report the power button press; catch and ignore the button release. +*/ +if (!priv->array) { +if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ; + } +/* 0xC0 is for HID events, other values are for 5 button array */ if (event !=0xc0 ) {
Quoted 1 line
if (!priv->array ||
LGTM, it's improved from what you posted to that bug already.
2017-09-18 23:29 GMT+02:00 <[email protected]>:
Quoted 70 lines
Quoted 66 lines
-----Original Message-----
From: Jérôme de Bretagne [mailto:[email protected]]
Sent: Sunday, September 17, 2017 5:57 PM
To: [email protected]
Cc: Darren Hart <[email protected]>; LKML <[email protected]>; Linux ACPI <[email protected]>; Rafael J. Wysocki <[email protected]>; Andy Shevchenko <[email protected]>; Limonciello, Mario <[email protected]>; Alex Hung <[email protected]>
Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275From: Jérôme de Bretagne <[email protected]>
On Dell Latitude 7275 the 5-button array is not exposed in the ACPI tables, but still notifies are sent to the Intel HID device object (device ID INT33D5) in response to power button actions. They were ignored as the intel-hid driver was not prepared to take care of them until recently.
Power button wakeup from suspend-to-idle was added in:
635173a17b03("intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle"). However power button suspend doesn't work yet on this platform so it would be good to add it also.On the affected platform (for which
priv->arrayis NULL), add a new upfront check against the power button press notification (0xCE) tonotify_handler(), outside the wakeup mode this time, which allows to report the power button press event and trigger the suspend. Also catch and ignore the corresponding power button release notification (0xCF) to stop it from being reported as an "unknown event" in the logs.Tested-by Jérôme de Bretagne <[email protected]>Signed-off-by Jérôme de Bretagne <[email protected]>--- drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/platform/x86/intel-hid.c +16
-0@@ -226,6 +226,22 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) return; } +/* +* Needed for suspend to work on some platforms that don't expose +* the 5-button array, but still send notifies with power button +* event code to this device object on power button actions. +* +* Report the power button press; catch and ignore the button release. +*/ +if (!priv->array) { +if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ; + } +/* 0xC0 is for HID events, other values are for 5 button array */ if (event !=0xc0 ) {Quoted 1 line
if (!priv->array ||LGTM, it's improved from what you posted to that bug already.
Quoted 1 line
Thanks Mario.
In fact, I updated my initial fix proposal on the 12th in [1] and this patch matches exactly the updated version. You didn't receive the update notification from Bugzilla?
Quoted 86 lines
-----Original Message-----
From: Jérôme de Bretagne [mailto:[email protected]]
Sent: Monday, September 18, 2017 5:41 PM
To: Limonciello, Mario <[email protected]>
Cc: [email protected]; Darren Hart <[email protected]>; LKML <[email protected]>; ACPI Devel Maling List <linux- [email protected]>; Rafael J. Wysocki <[email protected]>; Andy Shevchenko <[email protected]>; Alex Hung <[email protected]>
Subject: Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 72752017-09-18 23:29 GMT+02:00 <[email protected]>:
Quoted 6 lines
Quoted 6 lines
-----Original Message-----
From: Jérôme de Bretagne [mailto:[email protected]]
Sent: Sunday, September 17, 2017 5:57 PM
To: [email protected]
Cc: Darren Hart <[email protected]>; LKML <[email protected]>; Linux ACPI <[email protected]>; Rafael J. WysockiQuoted 33 lines
Quoted 33 lines
Andy Shevchenko <[email protected]>; Limonciello, Mario <[email protected]>; Alex Hung <[email protected]>
Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275From: Jérôme de Bretagne <[email protected]>
On Dell Latitude 7275 the 5-button array is not exposed in the ACPI tables, but still notifies are sent to the Intel HID device object (device ID INT33D5) in response to power button actions. They were ignored as the intel-hid driver was not prepared to take care of them until recently.
Power button wakeup from suspend-to-idle was added in:
635173a17b03("intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle"). However power button suspend doesn't work yet on this platform so it would be good to add it also.On the affected platform (for which
priv->arrayis NULL), add a new upfront check against the power button press notification (0xCE) tonotify_handler(), outside the wakeup mode this time, which allows to report the power button press event and trigger the suspend. Also catch and ignore the corresponding power button release notification (0xCF) to stop it from being reported as an "unknown event" in the logs.Tested-by Jérôme de Bretagne <[email protected]>Signed-off-by Jérôme de Bretagne <[email protected]>--- drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)Quoted 5 lines
drivers/platform/x86/intel-hid.c static void notify_handler(acpi_handle handle, u32
Quoted 5 lines
drivers/platform/x86/intel-hid.c +0
-0@@ -226,6 +226,22 @@ static void notify_handler(acpi_handle handle, u32event,
Quoted 24 lines
Quoted 22 lines
drivers/platform/x86/intel-hid.c +16
-0void *context) return; } +/* +* Needed for suspend to work on some platforms that don't expose +* the 5-button array, but still send notifies with power button +* event code to this device object on power button actions. +* +* Report the power button press; catch and ignore the button release. +*/ +if (!priv->array) { +if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ; + } +/* 0xC0 is for HID events, other values are for 5 button array */ if (event !=0xc0 ) {Quoted 1 line
if (!priv->array ||Quoted 3 lines
LGTM, it's improved from what you posted to that bug already.
Quoted 7 lines
Thanks Mario.
In fact, I updated my initial fix proposal on the 12th in [1] and this patch matches exactly the updated version. You didn't receive the update notification from Bugzilla?
No I didn't get one (or some enterprise spam filter helpfully avoided letting it into my inbox).
Quoted 40 lines · Jérôme de Bretagne
From: Jérôme de Bretagne <[email protected]>
On Dell Latitude 7275 the 5-button array is not exposed in the ACPI tables, but still notifies are sent to the Intel HID device object (device ID INT33D5) in response to power button actions. They were ignored as the intel-hid driver was not prepared to take care of them until recently.
Power button wakeup from suspend-to-idle was added in:
635173a17b03("intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle"). However power button suspend doesn't work yet on this platform so it would be good to add it also.On the affected platform (for which
priv->arrayis NULL), add a new upfront check against the power button press notification (0xCE) tonotify_handler(), outside the wakeup mode this time, which allows to report the power button press event and trigger the suspend. Also catch and ignore the corresponding power button release notification (0xCF) to stop it from being reported as an "unknown event" in the logs.Tested-by Jérôme de Bretagne <[email protected]>Signed-off-by Jérôme de Bretagne <[email protected]>--- drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/platform/x86/intel-hid.c +4
-0@@ -226,6 +226,22 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)return ; } +/* +* Needed for suspend to work on some platforms that don't expose +* the 5-button array, but still send notifies with power button +* event code to this device object on power button actions.
drivers/platform/x86/intel-hid.c static void notify_handler(acpi_handle handle, u32 event, void *context) · line 242
Quoted 10 lines
drivers/platform/x86/intel-hid.c +10
-0+* +* Report the power button press; catch and ignore the button release. +*/ +if (!priv->array) { +if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ;
Minor CodingStyle nit. If the if block uses parens, so does the else block. In this case, since the if returns, just skip the else entirely.
See Documentation/process/coding-style.rst
The example immediatley *before* 3.1) Spaces.
I've made this change and queued for testing.
Quoted 6 lines
drivers/platform/x86/intel-hid.c +2
-0+ } +/* 0xC0 is for HID events, other values are for 5 button array */ if (event !=0xc0 ) { if (!priv->array ||
Signature · 2 lines
Darren Hart VMware Open Source Technology Center
Quoted 9 lines · Darren Hart
drivers/platform/x86/intel-hid.c static void notify_handler(acpi_handle handle, u32 event, void *context) · line 242
Quoted 6 lines · Jérôme de Bretagne
drivers/platform/x86/intel-hid.c +6
-0+if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ;
Quoted 9 lines
Minor CodingStyle nit. If the if block uses parens, so does the else block. In this case, since the if returns, just skip the else entirely.
See
Documentation/process/coding-style.rst
The example immediatley *before* 3.1) Spaces.I've made this change and queued for testing.
Quoted 1 line
+ }
Actually in this case even 'else' is redundant.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Darren Hart
drivers/platform/x86/intel-hid.c static void notify_handler(acpi_handle handle, u32 event, void *context) · line 242
Quoted 6 lines · Jérôme de Bretagne
drivers/platform/x86/intel-hid.c +6
-0+if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ;Minor CodingStyle nit. If the if block uses parens, so does the else block. In this case, since the if returns, just skip the else entirely.
Quoted 8 lines
Quoted 6 lines
See
Documentation/process/coding-style.rst
The example immediatley *before* 3.1) Spaces.I've made this change and queued for testing.
Quoted 1 line
+ }
Actually in this case even 'else' is redundant.
Yes, this is what I meant above by "skip the else entirely", and is the change I made prior to committing. I was just pointing out the coding style at the same time :-)
Signature · 2 lines
Darren Hart VMware Open Source Technology Center
Quoted 19 lines · Darren Hart
Quoted 18 lines · Andy Shevchenko
Quoted 17 lines · Darren Hart
drivers/platform/x86/intel-hid.c static void notify_handler(acpi_handle handle, u32 event, void *context) · line 242
Quoted 6 lines · Jérôme de Bretagne
drivers/platform/x86/intel-hid.c +6
-0+if (event ==0xce ) { +input_report_key (priv->input_dev,KEY_POWER ,1 ); +input_sync (priv->input_dev); +return ; + }else if (event ==0xcf ) +return ;Minor CodingStyle nit. If the if block uses parens, so does the else block. In this case, since the if returns, just skip the else entirely.
See
Documentation/process/coding-style.rst
The example immediatley *before* 3.1) Spaces.I've made this change and queued for testing.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
+ }
Actually in this case even 'else' is redundant.
Yes, this is what I meant above by "skip the else entirely", and is the change I made prior to committing. I was just pointing out the coding style at the same time :-)
Good we are on the same page WRT such pattern.
Signature · 2 lines
With Best Regards, Andy Shevchenko