* [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
@ 2017-09-17 22:57 Jérôme de Bretagne
2017-09-18 21:29 ` Mario.Limonciello
2017-09-22 23:45 ` Darren Hart
0 siblings, 2 replies; 10+ messages in thread
From: Jérôme de Bretagne @ 2017-09-17 22:57 UTC (permalink / raw)
To: platform-driver-x86
Cc: Darren Hart, LKML, Linux ACPI, Rafael J. Wysocki,
Andy Shevchenko, Mario Limonciello, Alex Hung
From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
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.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
---
drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index a782c78e7c63..b19f8dcf9d8c 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -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 ||
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-17 22:57 [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275 Jérôme de Bretagne
@ 2017-09-18 21:29 ` Mario.Limonciello
2017-09-22 23:45 ` Darren Hart
1 sibling, 0 replies; 10+ messages in thread
From: Mario.Limonciello @ 2017-09-18 21:29 UTC (permalink / raw)
To: jerome.debretagne, platform-driver-x86
Cc: dvhart, linux-kernel, linux-acpi, rjw, andy.shevchenko, alex.hung
> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Sunday, September 17, 2017 5:57 PM
> To: platform-driver-x86@vger.kernel.org
> Cc: Darren Hart <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>;
> Linux ACPI <linux-acpi@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>;
> Andy Shevchenko <andy.shevchenko@gmail.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
> Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude
> 7275
>
> From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>
> 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.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> ---
> drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index a782c78e7c63..b19f8dcf9d8c 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -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 ||
LGTM, it's improved from what you posted to that bug already.
Acked-By: Mario Limonciello <mario.limonciello@dell.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
@ 2017-09-18 21:29 ` Mario.Limonciello
0 siblings, 0 replies; 10+ messages in thread
From: Mario.Limonciello @ 2017-09-18 21:29 UTC (permalink / raw)
To: jerome.debretagne, platform-driver-x86
Cc: dvhart, linux-kernel, linux-acpi, rjw, andy.shevchenko, alex.hung
> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Sunday, September 17, 2017 5:57 PM
> To: platform-driver-x86@vger.kernel.org
> Cc: Darren Hart <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>;
> Linux ACPI <linux-acpi@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>;
> Andy Shevchenko <andy.shevchenko@gmail.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
> Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude
> 7275
>
> From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>
> 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.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> ---
> drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index a782c78e7c63..b19f8dcf9d8c 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -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 ||
LGTM, it's improved from what you posted to that bug already.
Acked-By: Mario Limonciello <mario.limonciello@dell.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-18 21:29 ` Mario.Limonciello
(?)
@ 2017-09-18 22:40 ` Jérôme de Bretagne
2017-09-20 20:07 ` Mario.Limonciello
-1 siblings, 1 reply; 10+ messages in thread
From: Jérôme de Bretagne @ 2017-09-18 22:40 UTC (permalink / raw)
To: Mario Limonciello
Cc: platform-driver-x86, Darren Hart, LKML, ACPI Devel Maling List,
Rafael J. Wysocki, Andy Shevchenko, Alex Hung
2017-09-18 23:29 GMT+02:00 <Mario.Limonciello@dell.com>:
>> -----Original Message-----
>> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
>> Sent: Sunday, September 17, 2017 5:57 PM
>> To: platform-driver-x86@vger.kernel.org
>> Cc: Darren Hart <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>;
>> Linux ACPI <linux-acpi@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>;
>> Andy Shevchenko <andy.shevchenko@gmail.com>; Limonciello, Mario
>> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
>> Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude
>> 7275
>>
>> From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>>
>> 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.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
>> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>> ---
>> drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
>> index a782c78e7c63..b19f8dcf9d8c 100644
>> --- a/drivers/platform/x86/intel-hid.c
>> +++ b/drivers/platform/x86/intel-hid.c
>> @@ -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 ||
>
> LGTM, it's improved from what you posted to that bug already.
>
> Acked-By: Mario Limonciello <mario.limonciello@dell.com>
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?
[1] https://bugzilla.kernel.org/show_bug.cgi?id=196115#c12
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-18 22:40 ` Jérôme de Bretagne
@ 2017-09-20 20:07 ` Mario.Limonciello
0 siblings, 0 replies; 10+ messages in thread
From: Mario.Limonciello @ 2017-09-20 20:07 UTC (permalink / raw)
To: jerome.debretagne
Cc: platform-driver-x86, dvhart, linux-kernel, linux-acpi, rjw,
andy.shevchenko, alex.hung
> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Monday, September 18, 2017 5:41 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: platform-driver-x86@vger.kernel.org; Darren Hart <dvhart@infradead.org>;
> LKML <linux-kernel@vger.kernel.org>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Alex Hung <alex.hung@canonical.com>
> Subject: Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell
> Latitude 7275
>
> 2017-09-18 23:29 GMT+02:00 <Mario.Limonciello@dell.com>:
> >> -----Original Message-----
> >> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> >> Sent: Sunday, September 17, 2017 5:57 PM
> >> To: platform-driver-x86@vger.kernel.org
> >> Cc: Darren Hart <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>;
> >> Linux ACPI <linux-acpi@vger.kernel.org>; Rafael J. Wysocki
> <rjw@rjwysocki.net>;
> >> Andy Shevchenko <andy.shevchenko@gmail.com>; Limonciello, Mario
> >> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
> >> Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude
> >> 7275
> >>
> >> From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> >>
> >> 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.
> >>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
> >> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> >> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> >> ---
> >> drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
> >> 1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> >> index a782c78e7c63..b19f8dcf9d8c 100644
> >> --- a/drivers/platform/x86/intel-hid.c
> >> +++ b/drivers/platform/x86/intel-hid.c
> >> @@ -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 ||
> >
> > LGTM, it's improved from what you posted to that bug already.
> >
> > Acked-By: Mario Limonciello <mario.limonciello@dell.com>
>
> 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?
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=196115#c12
No I didn't get one (or some enterprise spam filter helpfully avoided
letting it into my inbox).
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
@ 2017-09-20 20:07 ` Mario.Limonciello
0 siblings, 0 replies; 10+ messages in thread
From: Mario.Limonciello @ 2017-09-20 20:07 UTC (permalink / raw)
To: jerome.debretagne
Cc: platform-driver-x86, dvhart, linux-kernel, linux-acpi, rjw,
andy.shevchenko, alex.hung
> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Monday, September 18, 2017 5:41 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: platform-driver-x86@vger.kernel.org; Darren Hart <dvhart@infradead.org>;
> LKML <linux-kernel@vger.kernel.org>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Alex Hung <alex.hung@canonical.com>
> Subject: Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell
> Latitude 7275
>
> 2017-09-18 23:29 GMT+02:00 <Mario.Limonciello@dell.com>:
> >> -----Original Message-----
> >> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> >> Sent: Sunday, September 17, 2017 5:57 PM
> >> To: platform-driver-x86@vger.kernel.org
> >> Cc: Darren Hart <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>;
> >> Linux ACPI <linux-acpi@vger.kernel.org>; Rafael J. Wysocki
> <rjw@rjwysocki.net>;
> >> Andy Shevchenko <andy.shevchenko@gmail.com>; Limonciello, Mario
> >> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
> >> Subject: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude
> >> 7275
> >>
> >> From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> >>
> >> 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.
> >>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
> >> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> >> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> >> ---
> >> drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
> >> 1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> >> index a782c78e7c63..b19f8dcf9d8c 100644
> >> --- a/drivers/platform/x86/intel-hid.c
> >> +++ b/drivers/platform/x86/intel-hid.c
> >> @@ -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 ||
> >
> > LGTM, it's improved from what you posted to that bug already.
> >
> > Acked-By: Mario Limonciello <mario.limonciello@dell.com>
>
> 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?
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=196115#c12
No I didn't get one (or some enterprise spam filter helpfully avoided
letting it into my inbox).
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-17 22:57 [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275 Jérôme de Bretagne
2017-09-18 21:29 ` Mario.Limonciello
@ 2017-09-22 23:45 ` Darren Hart
2017-09-25 10:57 ` Andy Shevchenko
1 sibling, 1 reply; 10+ messages in thread
From: Darren Hart @ 2017-09-22 23:45 UTC (permalink / raw)
To: Jérôme de Bretagne
Cc: platform-driver-x86, LKML, Linux ACPI, Rafael J. Wysocki,
Andy Shevchenko, Mario Limonciello, Alex Hung
On Mon, Sep 18, 2017 at 12:57:12AM +0200, Jérôme de Bretagne wrote:
> From: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>
> 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.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> ---
> drivers/platform/x86/intel-hid.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index a782c78e7c63..b19f8dcf9d8c 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -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;
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.
> + }
> +
> /* 0xC0 is for HID events, other values are for 5 button array */
> if (event != 0xc0) {
> if (!priv->array ||
>
--
Darren Hart
VMware Open Source Technology Center
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-22 23:45 ` Darren Hart
@ 2017-09-25 10:57 ` Andy Shevchenko
2017-09-27 7:31 ` Darren Hart
0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2017-09-25 10:57 UTC (permalink / raw)
To: Darren Hart
Cc: Jérôme de Bretagne, Platform Driver, LKML, Linux ACPI,
Rafael J. Wysocki, Mario Limonciello, Alex Hung
On Sat, Sep 23, 2017 at 2:45 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Sep 18, 2017 at 12:57:12AM +0200, Jérôme de Bretagne wrote:
>> + 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.
>
>> + }
Actually in this case even 'else' is redundant.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-25 10:57 ` Andy Shevchenko
@ 2017-09-27 7:31 ` Darren Hart
2017-09-27 8:58 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Darren Hart @ 2017-09-27 7:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jérôme de Bretagne, Platform Driver, LKML, Linux ACPI,
Rafael J. Wysocki, Mario Limonciello, Alex Hung
On Mon, Sep 25, 2017 at 01:57:48PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 23, 2017 at 2:45 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Mon, Sep 18, 2017 at 12:57:12AM +0200, Jérôme de Bretagne wrote:
>
> >> + 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.
> >
> >> + }
>
> 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 :-)
--
Darren Hart
VMware Open Source Technology Center
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275
2017-09-27 7:31 ` Darren Hart
@ 2017-09-27 8:58 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2017-09-27 8:58 UTC (permalink / raw)
To: Darren Hart
Cc: Jérôme de Bretagne, Platform Driver, LKML, Linux ACPI,
Rafael J. Wysocki, Mario Limonciello, Alex Hung
On Wed, Sep 27, 2017 at 10:31 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Sep 25, 2017 at 01:57:48PM +0300, Andy Shevchenko wrote:
>> On Sat, Sep 23, 2017 at 2:45 AM, Darren Hart <dvhart@infradead.org> wrote:
>> > On Mon, Sep 18, 2017 at 12:57:12AM +0200, Jérôme de Bretagne wrote:
>>
>> >> + 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.
>> >
>> >> + }
>>
>> 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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-27 8:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-17 22:57 [PATCH] platform/x86: intel-hid: Power button suspend on Dell Latitude 7275 Jérôme de Bretagne
2017-09-18 21:29 ` Mario.Limonciello
2017-09-18 21:29 ` Mario.Limonciello
2017-09-18 22:40 ` Jérôme de Bretagne
2017-09-20 20:07 ` Mario.Limonciello
2017-09-20 20:07 ` Mario.Limonciello
2017-09-22 23:45 ` Darren Hart
2017-09-25 10:57 ` Andy Shevchenko
2017-09-27 7:31 ` Darren Hart
2017-09-27 8:58 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.