[PATCH 0/4] dell-wmi: Changes in WMI event code handling

[PATCH 0/4] dell-wmi: Changes in WMI event code handling

[Pali Rohár]

First patch describe problem about 0xe045 code. Second and third are just
cosmetic and last rework code which processing WMI events. It should be
properly tested on more Dell machines, to check that everything is still
working correctly.

Pali Rohár (4):
  dell-wmi: Ignore WMI event code 0xe045
  dell-wmi: Sort WMI event codes and update comments
  dell-wmi: Add information about other WMI event codes
  dell-wmi: Rework code for generating sparse keymap and processing WMI
    events

 drivers/platform/x86/dell-wmi.c |  285 +++++++++++++++++++++++----------------
 1 file changed, 166 insertions(+), 119 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] dell-wmi: Ignore WMI event code 0xe045

[Pali Rohár]

>From Dell we know that WMI event code 0xe045 is for Num Lock key, but it is
unclear due to message in commit 0b3f6109f0c9 ("dell-wmi: new driver for
hotkey control").

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Link: https://lkml.org/lkml/2015/7/7/830
---
 drivers/platform/x86/dell-wmi.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 15c6f11..4d23c91 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -88,7 +88,6 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
-	{ KE_KEY, 0xe045, { KEY_PROG1 } },
 	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
 
 	/* These also contain the brightness level at offset 6 */
@@ -130,7 +129,19 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
 	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
 	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
+
+	/*
+	 * This entry is very suspicious!
+	 * Originally Matthew Garrett created this dell-wmi driver specially for
+	 * "button with a picture of a battery" which has event code 0xe045.
+	 * Later Mario Limonciello from Dell told us that event code 0xe045 is
+	 * reported by Num Lock and should be ignored because key is send also
+	 * by keyboard controller.
+	 * So for now we will ignore this event to prevent potential double
+	 * Num Lock key press.
+	 */
 	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
+
 	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
-- 
1.7.9.5

[PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Pali Rohár]

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 4d23c91..363d927 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -86,31 +86,32 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
  */
 
 static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
-	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
-	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
+	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
-	/* These also contain the brightness level at offset 6 */
-	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
-	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
+	/* These also contain the brightness level after key code */
+	{ KE_KEY,    0xe006, { KEY_BRIGHTNESSUP } },
+	{ KE_KEY,    0xe005, { KEY_BRIGHTNESSDOWN } },
 
 	/* Battery health status button */
-	{ KE_KEY, 0xe007, { KEY_BATTERY } },
+	{ KE_KEY,    0xe007, { KEY_BATTERY } },
 
-	/* Radio devices state change */
+	/* Radio devices state change, also contains additional information */
 	{ KE_IGNORE, 0xe008, { KEY_RFKILL } },
 
-	/* The next device is at offset 6, the active devices are at
-	   offset 8 and the attached devices at offset 10 */
-	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
+	{ KE_KEY,    0xe009, { KEY_EJECTCD } },
 
+	/* After key code is: next device, active devices, attached devices */
+	{ KE_KEY,    0xe00b, { KEY_SWITCHVIDEOMODE } },
+
+	/* Also contains keyboard illumination level after key code */
 	{ KE_IGNORE, 0xe00c, { KEY_KBDILLUMTOGGLE } },
 
 	/* BIOS error detected */
 	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
 
 	/* Wifi Catcher */
-	{ KE_KEY, 0xe011, {KEY_PROG2 } },
+	{ KE_KEY,    0xe011, { KEY_PROG2 } },
 
 	/* Ambient light sensor toggle */
 	{ KE_IGNORE, 0xe013, { KEY_RESERVED } },
@@ -118,12 +119,14 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
 
 	/* Dell Instant Launch key */
-	{ KE_KEY, 0xe025, { KEY_PROG4 } },
-	{ KE_KEY, 0xe029, { KEY_PROG4 } },
+	{ KE_KEY,    0xe025, { KEY_PROG4 } },
 
 	/* Audio panel key */
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
 
+	/* Dell Instant Launch key */
+	{ KE_KEY,    0xe029, { KEY_PROG4 } },
+
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
@@ -146,7 +149,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
+
 	{ KE_END, 0 }
+
 };
 
 static bool dell_new_hk_type;
-- 
1.7.9.5

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Michał Kępień]

> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

My guess is that Darren won't let you off without at least a short
commit message.

> ---
>  drivers/platform/x86/dell-wmi.c |   31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 4d23c91..363d927 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -86,31 +86,32 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
>   */
>  
>  static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> -	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
>  
> -	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
> +	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
>  
> -	/* These also contain the brightness level at offset 6 */
> -	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
> -	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
> +	/* These also contain the brightness level after key code */
> +	{ KE_KEY,    0xe006, { KEY_BRIGHTNESSUP } },
> +	{ KE_KEY,    0xe005, { KEY_BRIGHTNESSDOWN } },

These two entries were left unsorted.

>  
>  	/* Battery health status button */
> -	{ KE_KEY, 0xe007, { KEY_BATTERY } },
> +	{ KE_KEY,    0xe007, { KEY_BATTERY } },
>  
> -	/* Radio devices state change */
> +	/* Radio devices state change, also contains additional information */
>  	{ KE_IGNORE, 0xe008, { KEY_RFKILL } },
>  
> -	/* The next device is at offset 6, the active devices are at
> -	   offset 8 and the attached devices at offset 10 */
> -	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
> +	{ KE_KEY,    0xe009, { KEY_EJECTCD } },
>  
> +	/* After key code is: next device, active devices, attached devices */
> +	{ KE_KEY,    0xe00b, { KEY_SWITCHVIDEOMODE } },
> +
> +	/* Also contains keyboard illumination level after key code */

I know I'm being really pedantic here, but as this is a cleanup patch,
maybe it makes sense to unify the comments a bit?  After this patch is
applied, the comments are:

    /* These also contain the brightness level after key code */
    /* Radio devices state change, also contains additional information */
    /* After key code is: next device, active devices, attached devices */
    /* Also contains keyboard illumination level after key code */

How about changing them to something like:

    /* Key code is followed by brightness level */
    /* Radio devices state change, key code is followed by additional information */
    /* Key code is followed by: next device, active devices, attached devices */
    /* Key code is followed by keyboard illumination level */

And looking at the bigger picture, do you think these comments
(especially the generic one: "also contains additional information") are
actually needed?  Anything that follows the key code is ignored by
kernel code anyway.

-- 
Best regards,
Michał Kępień

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Pali Rohár]

On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> 
> My guess is that Darren won't let you off without at least a short
> commit message.

I have no idea what else to write. I think that description is enough.

> > ---
> >  drivers/platform/x86/dell-wmi.c |   31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 4d23c91..363d927 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -86,31 +86,32 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
> >   */
> >  
> >  static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> > -	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
> >  
> > -	{ KE_KEY, 0xe009, { KEY_EJECTCD } },
> > +	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
> >  
> > -	/* These also contain the brightness level at offset 6 */
> > -	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
> > -	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
> > +	/* These also contain the brightness level after key code */
> > +	{ KE_KEY,    0xe006, { KEY_BRIGHTNESSUP } },
> > +	{ KE_KEY,    0xe005, { KEY_BRIGHTNESSDOWN } },
> 
> These two entries were left unsorted.

Thanks, I will fix it.

> >  
> >  	/* Battery health status button */
> > -	{ KE_KEY, 0xe007, { KEY_BATTERY } },
> > +	{ KE_KEY,    0xe007, { KEY_BATTERY } },
> >  
> > -	/* Radio devices state change */
> > +	/* Radio devices state change, also contains additional information */
> >  	{ KE_IGNORE, 0xe008, { KEY_RFKILL } },
> >  
> > -	/* The next device is at offset 6, the active devices are at
> > -	   offset 8 and the attached devices at offset 10 */
> > -	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
> > +	{ KE_KEY,    0xe009, { KEY_EJECTCD } },
> >  
> > +	/* After key code is: next device, active devices, attached devices */
> > +	{ KE_KEY,    0xe00b, { KEY_SWITCHVIDEOMODE } },
> > +
> > +	/* Also contains keyboard illumination level after key code */
> 
> I know I'm being really pedantic here, but as this is a cleanup patch,
> maybe it makes sense to unify the comments a bit?  After this patch is
> applied, the comments are:
> 
>     /* These also contain the brightness level after key code */
>     /* Radio devices state change, also contains additional information */
>     /* After key code is: next device, active devices, attached devices */
>     /* Also contains keyboard illumination level after key code */
> 
> How about changing them to something like:
> 
>     /* Key code is followed by brightness level */
>     /* Radio devices state change, key code is followed by additional information */
>     /* Key code is followed by: next device, active devices, attached devices */
>     /* Key code is followed by keyboard illumination level */

No problem, I will change it.

> And looking at the bigger picture, do you think these comments
> (especially the generic one: "also contains additional information") are
> actually needed?  Anything that follows the key code is ignored by
> kernel code anyway.

Currently it is ignored, but it is the only documentation which we have
for these WMI events. Somebody in future can use these documentation
information and extend code to process also these information...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Darren Hart]

On Wed, Jun 08, 2016 at 12:03:24AM +0200, Pali Rohár wrote:
> On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > My guess is that Darren won't let you off without at least a short
> > commit message.
> 
> I have no idea what else to write. I think that description is enough.

There is always something. For example, why? See Documentation/SubmittingPatches
section "14) The canonical patch format" for an explanation.

"Traceability" of changes is important. If it's worth preparing the patch, it's
worth documenting why.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1131 bytes --]

On Wednesday 08 June 2016 21:48:24 Darren Hart wrote:
> On Wed, Jun 08, 2016 at 12:03:24AM +0200, Pali Rohár wrote:
> > On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > 
> > > My guess is that Darren won't let you off without at least a
> > > short commit message.
> > 
> > I have no idea what else to write. I think that description is
> > enough.
> 
> There is always something. For example, why? See
> Documentation/SubmittingPatches section "14) The canonical patch
> format" for an explanation.
> 
> "Traceability" of changes is important. If it's worth preparing the
> patch, it's worth documenting why.

In my opinion current description is enough and cover everything what 
this patch is doing. I think it is clear from my description what this 
patch is doing and so it is documented.

But if it is not clear and something is missing, let me know or show 
what is wrong and how you change it... It is just my assumption that 
"Sort WMI event codes and update comments" is clear...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Darren Hart]

On Wed, Jun 08, 2016 at 09:57:26PM +0200, Pali Rohár wrote:
> On Wednesday 08 June 2016 21:48:24 Darren Hart wrote:
> > On Wed, Jun 08, 2016 at 12:03:24AM +0200, Pali Rohár wrote:
> > > On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > 
> > > > My guess is that Darren won't let you off without at least a
> > > > short commit message.
> > > 
> > > I have no idea what else to write. I think that description is
> > > enough.
> > 
> > There is always something. For example, why? See
> > Documentation/SubmittingPatches section "14) The canonical patch
> > format" for an explanation.
> > 
> > "Traceability" of changes is important. If it's worth preparing the
> > patch, it's worth documenting why.
> 
> In my opinion current description is enough and cover everything what 
> this patch is doing. I think it is clear from my description what this 
> patch is doing and so it is documented.
> 
> But if it is not clear and something is missing, let me know or show 
> what is wrong and how you change it... It is just my assumption that 
> "Sort WMI event codes and update comments" is clear...

The patch summary accurately states what it does. It does not explain why it
does it, or why it is necessary. I understand this is a trivial change, but also
understand that both maintainers and people doing maintenance and regression
analysis will appreciate understanding the motivation and intent of the patch,
in addition to the content of the patch.

>From the maintainer perspective, whether we have 20 or 200 patches to review, we
will naturally merge the ones that require the least effort first. This
maximizes our efficiency and benefits the most people with what time we have
available. For many of us, this is our nights and weekends (guessing that's the
case for you as well). It is in the submitter's best interest to take the time
document the why, what, and how of each patch in a way that minimizes the effort
on the part of the maintainer to decide if the patch should be merged. It is
also a matter of scale, if every patch conforms to these guidelines, the
workflow is much more efficient.

In this case, I don't know why you decided to sort the event codes or update the
comments. I assume the comments were wrong before, but maybe something changed.
Do you care about alphabetically order or optimizing the switch statements? Is
it purely for legibility? Etc.

If that isn't sufficient, then just do it out of self-interest, because I will
not send patches to Linus that do not provide both a summary and a description
in the commit which meet the guidelines of section 14 referenced above.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3327 bytes --]

On Wednesday 08 June 2016 22:15:18 Darren Hart wrote:
> On Wed, Jun 08, 2016 at 09:57:26PM +0200, Pali Rohár wrote:
> > On Wednesday 08 June 2016 21:48:24 Darren Hart wrote:
> > > On Wed, Jun 08, 2016 at 12:03:24AM +0200, Pali Rohár wrote:
> > > > On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > 
> > > > > My guess is that Darren won't let you off without at least a
> > > > > short commit message.
> > > > 
> > > > I have no idea what else to write. I think that description is
> > > > enough.
> > > 
> > > There is always something. For example, why? See
> > > Documentation/SubmittingPatches section "14) The canonical patch
> > > format" for an explanation.
> > > 
> > > "Traceability" of changes is important. If it's worth preparing
> > > the patch, it's worth documenting why.
> > 
> > In my opinion current description is enough and cover everything
> > what this patch is doing. I think it is clear from my description
> > what this patch is doing and so it is documented.
> > 
> > But if it is not clear and something is missing, let me know or
> > show what is wrong and how you change it... It is just my
> > assumption that "Sort WMI event codes and update comments" is
> > clear...
> 
> The patch summary accurately states what it does. It does not explain
> why it does it, or why it is necessary. I understand this is a
> trivial change, but also understand that both maintainers and people
> doing maintenance and regression analysis will appreciate
> understanding the motivation and intent of the patch, in addition to
> the content of the patch.
> 
> From the maintainer perspective, whether we have 20 or 200 patches to
> review, we will naturally merge the ones that require the least
> effort first. This maximizes our efficiency and benefits the most
> people with what time we have available. For many of us, this is our
> nights and weekends (guessing that's the case for you as well). It
> is in the submitter's best interest to take the time document the
> why, what, and how of each patch in a way that minimizes the effort
> on the part of the maintainer to decide if the patch should be
> merged. It is also a matter of scale, if every patch conforms to
> these guidelines, the workflow is much more efficient.
> 
> In this case, I don't know why you decided to sort the event codes or
> update the comments. I assume the comments were wrong before, but
> maybe something changed. Do you care about alphabetically order or
> optimizing the switch statements? Is it purely for legibility? Etc.
> 
> If that isn't sufficient, then just do it out of self-interest,
> because I will not send patches to Linus that do not provide both a
> summary and a description in the commit which meet the guidelines of
> section 14 referenced above.
> 
> Thanks,

I fully understand your maintainer perspective. I just though that my 
one line explain everything what is needed about my patch...

So do you want to include reason for my patch? What about this 
additional description?

===
For better readability of keymap table, sort events by codes and also 
update comments for events to be more informative.
===

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Darren Hart]

On Wed, Jun 08, 2016 at 10:27:20PM +0200, Pali Rohár wrote:
> On Wednesday 08 June 2016 22:15:18 Darren Hart wrote:
> > On Wed, Jun 08, 2016 at 09:57:26PM +0200, Pali Rohár wrote:
> > > On Wednesday 08 June 2016 21:48:24 Darren Hart wrote:
> > > > On Wed, Jun 08, 2016 at 12:03:24AM +0200, Pali Rohár wrote:
> > > > > On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > 
> > > > > > My guess is that Darren won't let you off without at least a
> > > > > > short commit message.
> > > > > 
> > > > > I have no idea what else to write. I think that description is
> > > > > enough.
> > > > 
> > > > There is always something. For example, why? See
> > > > Documentation/SubmittingPatches section "14) The canonical patch
> > > > format" for an explanation.
> > > > 
> > > > "Traceability" of changes is important. If it's worth preparing
> > > > the patch, it's worth documenting why.
> > > 
> > > In my opinion current description is enough and cover everything
> > > what this patch is doing. I think it is clear from my description
> > > what this patch is doing and so it is documented.
> > > 
> > > But if it is not clear and something is missing, let me know or
> > > show what is wrong and how you change it... It is just my
> > > assumption that "Sort WMI event codes and update comments" is
> > > clear...
> > 
> > The patch summary accurately states what it does. It does not explain
> > why it does it, or why it is necessary. I understand this is a
> > trivial change, but also understand that both maintainers and people
> > doing maintenance and regression analysis will appreciate
> > understanding the motivation and intent of the patch, in addition to
> > the content of the patch.
> > 
> > From the maintainer perspective, whether we have 20 or 200 patches to
> > review, we will naturally merge the ones that require the least
> > effort first. This maximizes our efficiency and benefits the most
> > people with what time we have available. For many of us, this is our
> > nights and weekends (guessing that's the case for you as well). It
> > is in the submitter's best interest to take the time document the
> > why, what, and how of each patch in a way that minimizes the effort
> > on the part of the maintainer to decide if the patch should be
> > merged. It is also a matter of scale, if every patch conforms to
> > these guidelines, the workflow is much more efficient.
> > 
> > In this case, I don't know why you decided to sort the event codes or
> > update the comments. I assume the comments were wrong before, but
> > maybe something changed. Do you care about alphabetically order or
> > optimizing the switch statements? Is it purely for legibility? Etc.
> > 
> > If that isn't sufficient, then just do it out of self-interest,
> > because I will not send patches to Linus that do not provide both a
> > summary and a description in the commit which meet the guidelines of
> > section 14 referenced above.
> > 
> > Thanks,
> 
> I fully understand your maintainer perspective. I just though that my 
> one line explain everything what is needed about my patch...
> 
> So do you want to include reason for my patch? What about this 
> additional description?
> 
> ===
> For better readability of keymap table, sort events by codes and also 
> update comments for events to be more informative.

Great, that works for me. Reason was readability and providing context.

I'm just getting to the series now, if it's otherwise ready, I'll include this
myself. If changes are required, I'll leave it to you. Thanks Pali.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 362 bytes --]

On Wednesday 08 June 2016 22:43:28 Darren Hart wrote:
> I'm just getting to the series now, if it's otherwise ready, I'll
> include this myself. If changes are required, I'll leave it to you.
> Thanks Pali.

There is already V2 of this series with fixed problems reported by 
Michał. So Michał should ack it...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

ACPI DSDT tables have defined other WMI codes, but does not contain any
description when those codes are emitted. Some other codes can be found in
logs on internet. In this patch are all which I saw, but lot of them are
not tested properly (e.g. for duplicate events with AT keyboard). Now we
have all WMI event codes at one place and in future after proper testing
those codes can be correctly enabled or disabled...

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 363d927..7aac1dc 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -110,6 +110,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	/* BIOS error detected */
 	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
 
+	/* Unknown, defined in ACPI DSDT */
+	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
+
 	/* Wifi Catcher */
 	{ KE_KEY,    0xe011, { KEY_PROG2 } },
 
@@ -118,21 +121,45 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 
 	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
 
+	/* Unknown, defined in ACPI DSDT */
+	/* { KE_IGNORE, 0xe023, { KEY_RESERVED } }, */
+
+	/* Untested, Dell Instant Launch key on Inspiron 7520 */
+	/* { KE_IGNORE, 0xe024, { KEY_RESERVED } }, */
+
 	/* Dell Instant Launch key */
 	{ KE_KEY,    0xe025, { KEY_PROG4 } },
 
 	/* Audio panel key */
 	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
 
+	/* Untested, Multimedia key on Dell Vostro 3560 */
+	/* { KE_IGNORE, 0xe028, { KEY_RESERVED } }, */
+
 	/* Dell Instant Launch key */
 	{ KE_KEY,    0xe029, { KEY_PROG4 } },
 
+	/* Untested, Windows Mobility Center button on Inspiron 7520 */
+	/* { KE_IGNORE, 0xe02a, { KEY_RESERVED } }, */
+
+	/* Unknown, defined in ACPI DSDT */
+	/* { KE_IGNORE, 0xe02b, { KEY_RESERVED } }, */
+
+	/* Untested, Dell Audio With Preset Switch button on Inspiron 7520 */
+	/* { KE_IGNORE, 0xe02c, { KEY_RESERVED } }, */
+
 	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
 	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
 	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
 	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
 
+	/* NIC Link is Up */
+	{ KE_IGNORE, 0xe043, { KEY_RESERVED } },
+
+	/* NIC Link is Down */
+	{ KE_IGNORE, 0xe044, { KEY_RESERVED } },
+
 	/*
 	 * This entry is very suspicious!
 	 * Originally Matthew Garrett created this dell-wmi driver specially for
@@ -145,7 +172,12 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	 */
 	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
 
+	/* Scroll lock and also going to tablet mode on portable devices */
 	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
+
+	/* Untested, going from tablet mode on portable devices */
+	/* { KE_IGNORE, 0xe047, { KEY_RESERVED } }, */
+
 	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
-- 
1.7.9.5

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Gabriele Mazzotta]

On 22/05/2016 13:36, Pali Rohár wrote:
> ACPI DSDT tables have defined other WMI codes, but does not contain any
> description when those codes are emitted. Some other codes can be found in
> logs on internet. In this patch are all which I saw, but lot of them are
> not tested properly (e.g. for duplicate events with AT keyboard). Now we
> have all WMI event codes at one place and in future after proper testing
> those codes can be correctly enabled or disabled...
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/platform/x86/dell-wmi.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 363d927..7aac1dc 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -110,6 +110,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  	/* BIOS error detected */
>  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
>  
> +	/* Unknown, defined in ACPI DSDT */
> +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> +

I'm interested in knowing what's the meaning of this 0xe00e. This
event is sent multiple times when I suspend/resume my laptop and
it's definitely not a keypress.

Anyway, I've been using this patch set and didn't notice any issue, so

Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>

>  	/* Wifi Catcher */
>  	{ KE_KEY,    0xe011, { KEY_PROG2 } },
>  
> @@ -118,21 +121,45 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  
>  	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
>  
> +	/* Unknown, defined in ACPI DSDT */
> +	/* { KE_IGNORE, 0xe023, { KEY_RESERVED } }, */
> +
> +	/* Untested, Dell Instant Launch key on Inspiron 7520 */
> +	/* { KE_IGNORE, 0xe024, { KEY_RESERVED } }, */
> +
>  	/* Dell Instant Launch key */
>  	{ KE_KEY,    0xe025, { KEY_PROG4 } },
>  
>  	/* Audio panel key */
>  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
>  
> +	/* Untested, Multimedia key on Dell Vostro 3560 */
> +	/* { KE_IGNORE, 0xe028, { KEY_RESERVED } }, */
> +
>  	/* Dell Instant Launch key */
>  	{ KE_KEY,    0xe029, { KEY_PROG4 } },
>  
> +	/* Untested, Windows Mobility Center button on Inspiron 7520 */
> +	/* { KE_IGNORE, 0xe02a, { KEY_RESERVED } }, */
> +
> +	/* Unknown, defined in ACPI DSDT */
> +	/* { KE_IGNORE, 0xe02b, { KEY_RESERVED } }, */
> +
> +	/* Untested, Dell Audio With Preset Switch button on Inspiron 7520 */
> +	/* { KE_IGNORE, 0xe02c, { KEY_RESERVED } }, */
> +
>  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
>  	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
>  	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
>  	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
>  	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
>  
> +	/* NIC Link is Up */
> +	{ KE_IGNORE, 0xe043, { KEY_RESERVED } },
> +
> +	/* NIC Link is Down */
> +	{ KE_IGNORE, 0xe044, { KEY_RESERVED } },
> +
>  	/*
>  	 * This entry is very suspicious!
>  	 * Originally Matthew Garrett created this dell-wmi driver specially for
> @@ -145,7 +172,12 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  	 */
>  	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
>  
> +	/* Scroll lock and also going to tablet mode on portable devices */
>  	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
> +
> +	/* Untested, going from tablet mode on portable devices */
> +	/* { KE_IGNORE, 0xe047, { KEY_RESERVED } }, */
> +
>  	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
>  	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
>  	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
> 

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> On 22/05/2016 13:36, Pali Rohár wrote:
> > ACPI DSDT tables have defined other WMI codes, but does not contain any
> > description when those codes are emitted. Some other codes can be found in
> > logs on internet. In this patch are all which I saw, but lot of them are
> > not tested properly (e.g. for duplicate events with AT keyboard). Now we
> > have all WMI event codes at one place and in future after proper testing
> > those codes can be correctly enabled or disabled...
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  drivers/platform/x86/dell-wmi.c |   32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 363d927..7aac1dc 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -110,6 +110,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> >  	/* BIOS error detected */
> >  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> >  
> > +	/* Unknown, defined in ACPI DSDT */
> > +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> > +
> 
> I'm interested in knowing what's the meaning of this 0xe00e. This
> event is sent multiple times when I suspend/resume my laptop and
> it's definitely not a keypress.

>From DSDT dumps which I have seen, I guess it could be something with
battery charging... but that is only my guess.

Mario, do you have any idea, what these unknown events are?

> Anyway, I've been using this patch set and didn't notice any issue, so
> 
> Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> 
> >  	/* Wifi Catcher */
> >  	{ KE_KEY,    0xe011, { KEY_PROG2 } },
> >  
> > @@ -118,21 +121,45 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> >  
> >  	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
> >  
> > +	/* Unknown, defined in ACPI DSDT */
> > +	/* { KE_IGNORE, 0xe023, { KEY_RESERVED } }, */
> > +
> > +	/* Untested, Dell Instant Launch key on Inspiron 7520 */
> > +	/* { KE_IGNORE, 0xe024, { KEY_RESERVED } }, */
> > +
> >  	/* Dell Instant Launch key */
> >  	{ KE_KEY,    0xe025, { KEY_PROG4 } },
> >  
> >  	/* Audio panel key */
> >  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
> >  
> > +	/* Untested, Multimedia key on Dell Vostro 3560 */
> > +	/* { KE_IGNORE, 0xe028, { KEY_RESERVED } }, */
> > +
> >  	/* Dell Instant Launch key */
> >  	{ KE_KEY,    0xe029, { KEY_PROG4 } },
> >  
> > +	/* Untested, Windows Mobility Center button on Inspiron 7520 */
> > +	/* { KE_IGNORE, 0xe02a, { KEY_RESERVED } }, */
> > +
> > +	/* Unknown, defined in ACPI DSDT */
> > +	/* { KE_IGNORE, 0xe02b, { KEY_RESERVED } }, */
> > +
> > +	/* Untested, Dell Audio With Preset Switch button on Inspiron 7520 */
> > +	/* { KE_IGNORE, 0xe02c, { KEY_RESERVED } }, */
> > +
> >  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> >  	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
> >  	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
> >  	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
> >  	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
> >  
> > +	/* NIC Link is Up */
> > +	{ KE_IGNORE, 0xe043, { KEY_RESERVED } },
> > +
> > +	/* NIC Link is Down */
> > +	{ KE_IGNORE, 0xe044, { KEY_RESERVED } },
> > +
> >  	/*
> >  	 * This entry is very suspicious!
> >  	 * Originally Matthew Garrett created this dell-wmi driver specially for
> > @@ -145,7 +172,12 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> >  	 */
> >  	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
> >  
> > +	/* Scroll lock and also going to tablet mode on portable devices */
> >  	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
> > +
> > +	/* Untested, going from tablet mode on portable devices */
> > +	/* { KE_IGNORE, 0xe047, { KEY_RESERVED } }, */
> > +
> >  	{ KE_IGNORE, 0xe0f7, { KEY_MUTE } },
> >  	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
> >  	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
> > 

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Tuesday, June 7, 2016 6:00 PM
> To: Gabriele Mazzotta <gabriele.mzt@gmail.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> <dvhart@infradead.org>; Michał Kępień <kernel@kempniu.pl>; Andy Lutomirski
> <luto@kernel.org>; Alex Hung <alex.hung@canonical.com>; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > On 22/05/2016 13:36, Pali Rohár wrote:
> > > ACPI DSDT tables have defined other WMI codes, but does not contain
> > > any description when those codes are emitted. Some other codes can
> > > be found in logs on internet. In this patch are all which I saw, but
> > > lot of them are not tested properly (e.g. for duplicate events with
> > > AT keyboard). Now we have all WMI event codes at one place and in
> > > future after proper testing those codes can be correctly enabled or
> disabled...
> > >
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > >  drivers/platform/x86/dell-wmi.c |   32
> ++++++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > > @@ -110,6 +110,9 @@ static const struct key_entry
> dell_wmi_legacy_keymap[] __initconst = {
> > >  	/* BIOS error detected */
> > >  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > >
> > > +	/* Unknown, defined in ACPI DSDT */
> > > +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> > > +
> >
> > I'm interested in knowing what's the meaning of this 0xe00e. This
> > event is sent multiple times when I suspend/resume my laptop and it's
> > definitely not a keypress.
> 
> From DSDT dumps which I have seen, I guess it could be something with battery
> charging... but that is only my guess.
> 
> Mario, do you have any idea, what these unknown events are?

Off-hand I'm not sure, it would require some more digging.

Can you please remind me what model numbers and BIOS combinations you have 
found e00e in DSDT and what context the events are actually happening?   
Anything released in the past two years?

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Gabriele Mazzotta]

On 08/06/2016 08:02, Mario_Limonciello@Dell.com wrote:
>> -----Original Message-----
>> From: Pali Rohár [mailto:pali.rohar@gmail.com]
>> Sent: Tuesday, June 7, 2016 6:00 PM
>> To: Gabriele Mazzotta <gabriele.mzt@gmail.com>; Limonciello, Mario
>> <Mario_Limonciello@Dell.com>
>> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
>> <dvhart@infradead.org>; Michał Kępień <kernel@kempniu.pl>; Andy Lutomirski
>> <luto@kernel.org>; Alex Hung <alex.hung@canonical.com>; platform-driver-
>> x86@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
>> codes
>>
>> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
>>> On 22/05/2016 13:36, Pali Rohár wrote:
>>>> ACPI DSDT tables have defined other WMI codes, but does not contain
>>>> any description when those codes are emitted. Some other codes can
>>>> be found in logs on internet. In this patch are all which I saw, but
>>>> lot of them are not tested properly (e.g. for duplicate events with
>>>> AT keyboard). Now we have all WMI event codes at one place and in
>>>> future after proper testing those codes can be correctly enabled or
>> disabled...
>>>>
>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>> ---
>>>>  drivers/platform/x86/dell-wmi.c |   32
>> ++++++++++++++++++++++++++++++++
>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/dell-wmi.c
>>>> b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
>>>> --- a/drivers/platform/x86/dell-wmi.c
>>>> +++ b/drivers/platform/x86/dell-wmi.c
>>>> @@ -110,6 +110,9 @@ static const struct key_entry
>> dell_wmi_legacy_keymap[] __initconst = {
>>>>  	/* BIOS error detected */
>>>>  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
>>>>
>>>> +	/* Unknown, defined in ACPI DSDT */
>>>> +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
>>>> +
>>>
>>> I'm interested in knowing what's the meaning of this 0xe00e. This
>>> event is sent multiple times when I suspend/resume my laptop and it's
>>> definitely not a keypress.
>>
>> From DSDT dumps which I have seen, I guess it could be something with battery
>> charging... but that is only my guess.
>>
>> Mario, do you have any idea, what these unknown events are?
> 
> Off-hand I'm not sure, it would require some more digging.
> 
> Can you please remind me what model numbers and BIOS combinations you have 
> found e00e in DSDT and what context the events are actually happening?   
> Anything released in the past two years?
> 

XPS13 9333, BIOS A07.

I think I saw the event only after resuming from suspend and
it's sent four times in a row.

As Pali says, it seems to be related to the battery. There are
three _Qxx ACPI methods in my DSDT sending this event: one stops
battery charging, one detaches the battery and the last one stores
a value on the GNVS.

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3250 bytes --]

On Wednesday 08 June 2016 12:44:44 Gabriele Mazzotta wrote:
> On 08/06/2016 08:02, Mario_Limonciello@Dell.com wrote:
> >> -----Original Message-----
> >> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> >> Sent: Tuesday, June 7, 2016 6:00 PM
> >> To: Gabriele Mazzotta <gabriele.mzt@gmail.com>; Limonciello, Mario
> >> <Mario_Limonciello@Dell.com>
> >> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> >> <dvhart@infradead.org>; Michał Kępień <kernel@kempniu.pl>; Andy
> >> Lutomirski <luto@kernel.org>; Alex Hung
> >> <alex.hung@canonical.com>; platform-driver- x86@vger.kernel.org;
> >> linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> >> event codes
> >> 
> >> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> >>> On 22/05/2016 13:36, Pali Rohár wrote:
> >>>> ACPI DSDT tables have defined other WMI codes, but does not
> >>>> contain any description when those codes are emitted. Some
> >>>> other codes can be found in logs on internet. In this patch are
> >>>> all which I saw, but lot of them are not tested properly (e.g.
> >>>> for duplicate events with AT keyboard). Now we have all WMI
> >>>> event codes at one place and in future after proper testing
> >>>> those codes can be correctly enabled or
> >> 
> >> disabled...
> >> 
> >>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >>>> ---
> >>>> 
> >>>>  drivers/platform/x86/dell-wmi.c |   32
> >> 
> >> ++++++++++++++++++++++++++++++++
> >> 
> >>>>  1 file changed, 32 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/platform/x86/dell-wmi.c
> >>>> b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> >>>> --- a/drivers/platform/x86/dell-wmi.c
> >>>> +++ b/drivers/platform/x86/dell-wmi.c
> >>>> @@ -110,6 +110,9 @@ static const struct key_entry
> >> 
> >> dell_wmi_legacy_keymap[] __initconst = {
> >> 
> >>>>  	/* BIOS error detected */
> >>>>  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> >>>> 
> >>>> +	/* Unknown, defined in ACPI DSDT */
> >>>> +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> >>>> +
> >>> 
> >>> I'm interested in knowing what's the meaning of this 0xe00e. This
> >>> event is sent multiple times when I suspend/resume my laptop and
> >>> it's definitely not a keypress.
> >> 
> >> From DSDT dumps which I have seen, I guess it could be something
> >> with battery charging... but that is only my guess.
> >> 
> >> Mario, do you have any idea, what these unknown events are?
> > 
> > Off-hand I'm not sure, it would require some more digging.
> > 
> > Can you please remind me what model numbers and BIOS combinations
> > you have found e00e in DSDT and what context the events are
> > actually happening? Anything released in the past two years?
> 
> XPS13 9333, BIOS A07.
> 
> I think I saw the event only after resuming from suspend and
> it's sent four times in a row.
> 
> As Pali says, it seems to be related to the battery. There are
> three _Qxx ACPI methods in my DSDT sending this event: one stops
> battery charging, one detaches the battery and the last one stores
> a value on the GNVS.

Mario, were you able to identify something?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, June 15, 2016 2:51 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Gabriele Mazzotta <gabriele.mzt@gmail.com>; mjg59@srcf.ucam.org;
> dvhart@infradead.org; kernel@kempniu.pl; luto@kernel.org;
> alex.hung@canonical.com; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 08 June 2016 12:44:44 Gabriele Mazzotta wrote:
> > On 08/06/2016 08:02, Mario_Limonciello@Dell.com wrote:
> > >> -----Original Message-----
> > >> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > >> Sent: Tuesday, June 7, 2016 6:00 PM
> > >> To: Gabriele Mazzotta <gabriele.mzt@gmail.com>; Limonciello, Mario
> > >> <Mario_Limonciello@Dell.com>
> > >> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> > >> <dvhart@infradead.org>; Michał Kępień <kernel@kempniu.pl>; Andy
> > >> Lutomirski <luto@kernel.org>; Alex Hung
> > >> <alex.hung@canonical.com>; platform-driver- x86@vger.kernel.org;
> > >> linux-kernel@vger.kernel.org
> > >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> > >> event codes
> > >>
> > >> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > >>> On 22/05/2016 13:36, Pali Rohár wrote:
> > >>>> ACPI DSDT tables have defined other WMI codes, but does not
> > >>>> contain any description when those codes are emitted. Some
> > >>>> other codes can be found in logs on internet. In this patch are
> > >>>> all which I saw, but lot of them are not tested properly (e.g.
> > >>>> for duplicate events with AT keyboard). Now we have all WMI
> > >>>> event codes at one place and in future after proper testing
> > >>>> those codes can be correctly enabled or
> > >>
> > >> disabled...
> > >>
> > >>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > >>>> ---
> > >>>>
> > >>>>  drivers/platform/x86/dell-wmi.c |   32
> > >>
> > >> ++++++++++++++++++++++++++++++++
> > >>
> > >>>>  1 file changed, 32 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/platform/x86/dell-wmi.c
> > >>>> b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> > >>>> --- a/drivers/platform/x86/dell-wmi.c
> > >>>> +++ b/drivers/platform/x86/dell-wmi.c
> > >>>> @@ -110,6 +110,9 @@ static const struct key_entry
> > >>
> > >> dell_wmi_legacy_keymap[] __initconst = {
> > >>
> > >>>>  	/* BIOS error detected */
> > >>>>  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > >>>>
> > >>>> +	/* Unknown, defined in ACPI DSDT */
> > >>>> +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> > >>>> +
> > >>>
> > >>> I'm interested in knowing what's the meaning of this 0xe00e. This
> > >>> event is sent multiple times when I suspend/resume my laptop and
> > >>> it's definitely not a keypress.
> > >>
> > >> From DSDT dumps which I have seen, I guess it could be something
> > >> with battery charging... but that is only my guess.
> > >>
> > >> Mario, do you have any idea, what these unknown events are?
> > >
> > > Off-hand I'm not sure, it would require some more digging.
> > >
> > > Can you please remind me what model numbers and BIOS combinations
> > > you have found e00e in DSDT and what context the events are
> > > actually happening? Anything released in the past two years?
> >
> > XPS13 9333, BIOS A07.
> >
> > I think I saw the event only after resuming from suspend and
> > it's sent four times in a row.
> >
> > As Pali says, it seems to be related to the battery. There are
> > three _Qxx ACPI methods in my DSDT sending this event: one stops
> > battery charging, one detaches the battery and the last one stores
> > a value on the GNVS.
> 
> Mario, were you able to identify something?
>
Pali,

You aren't seeing this on the DSDT of your Latitude right?

Gabriele,

Your machine is from the year before XPS switched over to running
the Dell business client (eg Latitude, Precision, Optiplex) BIOS.

The EC in that machine does have support for "Battery Health" via
that scancode.  On Windows it's used for relaying battery information
to an application called Quick Set.

Thanks,

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Tuesday 21 June 2016 19:51:06 Mario_Limonciello@Dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Wednesday, June 15, 2016 2:51 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: Gabriele Mazzotta <gabriele.mzt@gmail.com>; mjg59@srcf.ucam.org;
> > dvhart@infradead.org; kernel@kempniu.pl; luto@kernel.org;
> > alex.hung@canonical.com; platform-driver-x86@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> > codes
> > 
> > On Wednesday 08 June 2016 12:44:44 Gabriele Mazzotta wrote:
> > > On 08/06/2016 08:02, Mario_Limonciello@Dell.com wrote:
> > > >> -----Original Message-----
> > > >> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > >> Sent: Tuesday, June 7, 2016 6:00 PM
> > > >> To: Gabriele Mazzotta <gabriele.mzt@gmail.com>; Limonciello, Mario
> > > >> <Mario_Limonciello@Dell.com>
> > > >> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> > > >> <dvhart@infradead.org>; Michał Kępień <kernel@kempniu.pl>; Andy
> > > >> Lutomirski <luto@kernel.org>; Alex Hung
> > > >> <alex.hung@canonical.com>; platform-driver- x86@vger.kernel.org;
> > > >> linux-kernel@vger.kernel.org
> > > >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> > > >> event codes
> > > >>
> > > >> On Friday 27 May 2016 00:04:23 Gabriele Mazzotta wrote:
> > > >>> On 22/05/2016 13:36, Pali Rohár wrote:
> > > >>>> ACPI DSDT tables have defined other WMI codes, but does not
> > > >>>> contain any description when those codes are emitted. Some
> > > >>>> other codes can be found in logs on internet. In this patch are
> > > >>>> all which I saw, but lot of them are not tested properly (e.g.
> > > >>>> for duplicate events with AT keyboard). Now we have all WMI
> > > >>>> event codes at one place and in future after proper testing
> > > >>>> those codes can be correctly enabled or
> > > >>
> > > >> disabled...
> > > >>
> > > >>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > >>>> ---
> > > >>>>
> > > >>>>  drivers/platform/x86/dell-wmi.c |   32
> > > >>
> > > >> ++++++++++++++++++++++++++++++++
> > > >>
> > > >>>>  1 file changed, 32 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/platform/x86/dell-wmi.c
> > > >>>> b/drivers/platform/x86/dell-wmi.c index 363d927..7aac1dc 100644
> > > >>>> --- a/drivers/platform/x86/dell-wmi.c
> > > >>>> +++ b/drivers/platform/x86/dell-wmi.c
> > > >>>> @@ -110,6 +110,9 @@ static const struct key_entry
> > > >>
> > > >> dell_wmi_legacy_keymap[] __initconst = {
> > > >>
> > > >>>>  	/* BIOS error detected */
> > > >>>>  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
> > > >>>>
> > > >>>> +	/* Unknown, defined in ACPI DSDT */
> > > >>>> +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> > > >>>> +
> > > >>>
> > > >>> I'm interested in knowing what's the meaning of this 0xe00e. This
> > > >>> event is sent multiple times when I suspend/resume my laptop and
> > > >>> it's definitely not a keypress.
> > > >>
> > > >> From DSDT dumps which I have seen, I guess it could be something
> > > >> with battery charging... but that is only my guess.
> > > >>
> > > >> Mario, do you have any idea, what these unknown events are?
> > > >
> > > > Off-hand I'm not sure, it would require some more digging.
> > > >
> > > > Can you please remind me what model numbers and BIOS combinations
> > > > you have found e00e in DSDT and what context the events are
> > > > actually happening? Anything released in the past two years?
> > >
> > > XPS13 9333, BIOS A07.
> > >
> > > I think I saw the event only after resuming from suspend and
> > > it's sent four times in a row.
> > >
> > > As Pali says, it seems to be related to the battery. There are
> > > three _Qxx ACPI methods in my DSDT sending this event: one stops
> > > battery charging, one detaches the battery and the last one stores
> > > a value on the GNVS.
> > 
> > Mario, were you able to identify something?
> >
> Pali,
> 
> You aren't seeing this on the DSDT of your Latitude right?

Yes, I do not see it on Latitude.

> Gabriele,
> 
> Your machine is from the year before XPS switched over to running
> the Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> 
> The EC in that machine does have support for "Battery Health" via
> that scancode.  On Windows it's used for relaying battery information
> to an application called Quick Set.

Do you have some details when it is send to OS? And how to read that
that "battery health"?

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> > You aren't seeing this on the DSDT of your Latitude right?
> 
> Yes, I do not see it on Latitude.

Thanks, the usage of this scan code is specific to consumer BIOSes.

> 
> > Gabriele,
> >
> > Your machine is from the year before XPS switched over to running the
> > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> >
> > The EC in that machine does have support for "Battery Health" via that
> > scancode.  On Windows it's used for relaying battery information to an
> > application called Quick Set.
> 
> Do you have some details when it is send to OS? And how to read that that
> "battery health"?

When a battery is removed or inserted this event is supposed to be received
by quickset over WMI and then Quickset will re-read battery information.

For Linux I don’t think this is necessary and a NOOP is appropriate.

There is also a second place that some older laptops had a battery "hotkey"
that would also emit 0xE00E.  This was also picked up by quickset and would
show battery information.

This shouldn't be blocked by kernel, I'd expect if someone wants to bind this
to another application from userspace they should be able to.

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Wednesday 22 June 2016 13:40:57 Mario_Limonciello@Dell.com wrote:
> > > You aren't seeing this on the DSDT of your Latitude right?
> > 
> > Yes, I do not see it on Latitude.
> 
> Thanks, the usage of this scan code is specific to consumer BIOSes.
> 
> > 
> > > Gabriele,
> > >
> > > Your machine is from the year before XPS switched over to running the
> > > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> > >
> > > The EC in that machine does have support for "Battery Health" via that
> > > scancode.  On Windows it's used for relaying battery information to an
> > > application called Quick Set.
> > 
> > Do you have some details when it is send to OS? And how to read that that
> > "battery health"?
> 
> When a battery is removed or inserted this event is supposed to be received
> by quickset over WMI and then Quickset will re-read battery information.

So event is sent only if battery is removed or inserted?

> For Linux I don’t think this is necessary and a NOOP is appropriate.
> 
> There is also a second place that some older laptops had a battery "hotkey"
> that would also emit 0xE00E.  This was also picked up by quickset and would
> show battery information.
> 
> This shouldn't be blocked by kernel, I'd expect if someone wants to bind this
> to another application from userspace they should be able to.

Great! Can I send patch after which 0xe00e will be send to input layer as
event KEY_BATTERY?

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, June 22, 2016 9:13 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org; dvhart@infradead.org;
> kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com; platform-
> driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 13:40:57 Mario_Limonciello@Dell.com wrote:
> > > > You aren't seeing this on the DSDT of your Latitude right?
> > >
> > > Yes, I do not see it on Latitude.
> >
> > Thanks, the usage of this scan code is specific to consumer BIOSes.
> >
> > >
> > > > Gabriele,
> > > >
> > > > Your machine is from the year before XPS switched over to running the
> > > > Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> > > >
> > > > The EC in that machine does have support for "Battery Health" via that
> > > > scancode.  On Windows it's used for relaying battery information to an
> > > > application called Quick Set.
> > >
> > > Do you have some details when it is send to OS? And how to read that
> that
> > > "battery health"?
> >
> > When a battery is removed or inserted this event is supposed to be
> received
> > by quickset over WMI and then Quickset will re-read battery information.
> 
> So event is sent only if battery is removed or inserted?
> 

Yeah, that's what my spec says, I haven't tested this on actual system to see.

I'm guessing what's going on is that during suspend ACPI battery drops
off system and comes back up on resume.

Maybe Gabriele can comment if any other times were noticed, but in any
case I think it's appropriate for dell-wmi driver when receiving this on WMI
to not do anything.  Sending KEY_BATTERY would be wrong behavior.

> > For Linux I don’t think this is necessary and a NOOP is appropriate.
> >
> > There is also a second place that some older laptops had a battery "hotkey"
> > that would also emit 0xE00E.  This was also picked up by quickset and would
> > show battery information.
> >
> > This shouldn't be blocked by kernel, I'd expect if someone wants to bind
> this
> > to another application from userspace they should be able to.
> 
> Great! Can I send patch after which 0xe00e will be send to input layer as
> event KEY_BATTERY?
> 

Well that's why I was mentioning this in two places.  If it's received from keyboard
recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from WMI,
it really shouldn't be set as anything for Linux to do.

I don't think any apps will benefit from the notification to re-read battery
Information today.

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Wednesday 22 June 2016 14:21:25 Mario_Limonciello@Dell.com wrote:
> > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > >
> > > There is also a second place that some older laptops had a battery "hotkey"
> > > that would also emit 0xE00E.  This was also picked up by quickset and would
> > > show battery information.
> > >
> > > This shouldn't be blocked by kernel, I'd expect if someone wants to bind
> > this
> > > to another application from userspace they should be able to.
> > 
> > Great! Can I send patch after which 0xe00e will be send to input layer as
> > event KEY_BATTERY?
> > 
> 
> Well that's why I was mentioning this in two places.  If it's received from keyboard
> recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from WMI,
> it really shouldn't be set as anything for Linux to do.
> 
> I don't think any apps will benefit from the notification to re-read battery
> Information today.

I mean for dell-wmi.c code.
Why it should not be set to anything on Linux?

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, June 22, 2016 9:24 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org; dvhart@infradead.org;
> kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com; platform-
> driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 14:21:25 Mario_Limonciello@Dell.com wrote:
> > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > >
> > > > There is also a second place that some older laptops had a battery
> "hotkey"
> > > > that would also emit 0xE00E.  This was also picked up by quickset and
> would
> > > > show battery information.
> > > >
> > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> bind
> > > this
> > > > to another application from userspace they should be able to.
> > >
> > > Great! Can I send patch after which 0xe00e will be send to input layer as
> > > event KEY_BATTERY?
> > >
> >
> > Well that's why I was mentioning this in two places.  If it's received from
> keyboard
> > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from
> WMI,
> > it really shouldn't be set as anything for Linux to do.
> >
> > I don't think any apps will benefit from the notification to re-read battery
> > Information today.
> 
> I mean for dell-wmi.c code.
> Why it should not be set to anything on Linux?
> 

On Windows Quickset is a separate application from built-in Windows battery 
applet.  It shows similar types of information as gnome-power-manager does, 
but because it's not part of the OS, it doesn't get notifications like this so 
special BIOS hooks are in place for it to know when to re-read battery
information.

On Linux, there are already infrastructure for battery removal and adding 
notifications from ACPI.  Sending this up to user space won't be useful.

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Wednesday 22 June 2016 14:28:37 Mario_Limonciello@Dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Wednesday, June 22, 2016 9:24 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org; dvhart@infradead.org;
> > kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com; platform-
> > driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> > codes
> > 
> > On Wednesday 22 June 2016 14:21:25 Mario_Limonciello@Dell.com wrote:
> > > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > > >
> > > > > There is also a second place that some older laptops had a battery
> > "hotkey"
> > > > > that would also emit 0xE00E.  This was also picked up by quickset and
> > would
> > > > > show battery information.
> > > > >
> > > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> > bind
> > > > this
> > > > > to another application from userspace they should be able to.
> > > >
> > > > Great! Can I send patch after which 0xe00e will be send to input layer as
> > > > event KEY_BATTERY?
> > > >
> > >
> > > Well that's why I was mentioning this in two places.  If it's received from
> > keyboard
> > > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received from
> > WMI,
> > > it really shouldn't be set as anything for Linux to do.
> > >
> > > I don't think any apps will benefit from the notification to re-read battery
> > > Information today.
> > 
> > I mean for dell-wmi.c code.
> > Why it should not be set to anything on Linux?
> > 
> 
> On Windows Quickset is a separate application from built-in Windows battery 
> applet.  It shows similar types of information as gnome-power-manager does, 
> but because it's not part of the OS, it doesn't get notifications like this so 
> special BIOS hooks are in place for it to know when to re-read battery
> information.
> 
> On Linux, there are already infrastructure for battery removal and adding 
> notifications from ACPI.  Sending this up to user space won't be useful.

Errr. I mean WMI event 0xe00e which you wrote is emitted when user press
battery key. And I think this event should be sent by dell-wmi.c as
KEY_BATTERY.

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, June 22, 2016 9:31 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org; dvhart@infradead.org;
> kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com; platform-
> driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On Wednesday 22 June 2016 14:28:37 Mario_Limonciello@Dell.com wrote:
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Wednesday, June 22, 2016 9:24 AM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org;
> dvhart@infradead.org;
> > > kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com;
> platform-
> > > driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> event
> > > codes
> > >
> > > On Wednesday 22 June 2016 14:21:25 Mario_Limonciello@Dell.com
> wrote:
> > > > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > > > >
> > > > > > There is also a second place that some older laptops had a battery
> > > "hotkey"
> > > > > > that would also emit 0xE00E.  This was also picked up by quickset
> and
> > > would
> > > > > > show battery information.
> > > > > >
> > > > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> > > bind
> > > > > this
> > > > > > to another application from userspace they should be able to.
> > > > >
> > > > > Great! Can I send patch after which 0xe00e will be send to input layer
> as
> > > > > event KEY_BATTERY?
> > > > >
> > > >
> > > > Well that's why I was mentioning this in two places.  If it's received from
> > > keyboard
> > > > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received
> from
> > > WMI,
> > > > it really shouldn't be set as anything for Linux to do.
> > > >
> > > > I don't think any apps will benefit from the notification to re-read
> battery
> > > > Information today.
> > >
> > > I mean for dell-wmi.c code.
> > > Why it should not be set to anything on Linux?
> > >
> >
> > On Windows Quickset is a separate application from built-in Windows
> battery
> > applet.  It shows similar types of information as gnome-power-manager
> does,
> > but because it's not part of the OS, it doesn't get notifications like this so
> > special BIOS hooks are in place for it to know when to re-read battery
> > information.
> >
> > On Linux, there are already infrastructure for battery removal and adding
> > notifications from ACPI.  Sending this up to user space won't be useful.
> 
> Errr. I mean WMI event 0xe00e which you wrote is emitted when user press
> battery key. And I think this event should be sent by dell-wmi.c as
> KEY_BATTERY.
> 

No, I mean 0xe00e scancode is emitted when user presses the battery key on
older laptops.  I'm only mentioning it because it happens to have the same
scancode as is received for WMI for another purpose and I wanted to make sure
it was clear what both are for.  
This button press isn't emitted via WMI, it's over standard serio.

The event received over WMI for battery refresh that shares the same code shouldn't
be emitted as KEY_BATTERY to the OS is all I was trying to say.

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Wednesday 22 June 2016 14:34:01 Mario_Limonciello@Dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Wednesday, June 22, 2016 9:31 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org; dvhart@infradead.org;
> > kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com; platform-
> > driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> > codes
> > 
> > On Wednesday 22 June 2016 14:28:37 Mario_Limonciello@Dell.com wrote:
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > Sent: Wednesday, June 22, 2016 9:24 AM
> > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org;
> > dvhart@infradead.org;
> > > > kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com;
> > platform-
> > > > driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> > event
> > > > codes
> > > >
> > > > On Wednesday 22 June 2016 14:21:25 Mario_Limonciello@Dell.com
> > wrote:
> > > > > > > For Linux I don’t think this is necessary and a NOOP is appropriate.
> > > > > > >
> > > > > > > There is also a second place that some older laptops had a battery
> > > > "hotkey"
> > > > > > > that would also emit 0xE00E.  This was also picked up by quickset
> > and
> > > > would
> > > > > > > show battery information.
> > > > > > >
> > > > > > > This shouldn't be blocked by kernel, I'd expect if someone wants to
> > > > bind
> > > > > > this
> > > > > > > to another application from userspace they should be able to.
> > > > > >
> > > > > > Great! Can I send patch after which 0xe00e will be send to input layer
> > as
> > > > > > event KEY_BATTERY?
> > > > > >
> > > > >
> > > > > Well that's why I was mentioning this in two places.  If it's received from
> > > > keyboard
> > > > > recoding it as KEY_BATTERY sounds appropriate to me.  If it's received
> > from
> > > > WMI,
> > > > > it really shouldn't be set as anything for Linux to do.
> > > > >
> > > > > I don't think any apps will benefit from the notification to re-read
> > battery
> > > > > Information today.
> > > >
> > > > I mean for dell-wmi.c code.
> > > > Why it should not be set to anything on Linux?
> > > >
> > >
> > > On Windows Quickset is a separate application from built-in Windows
> > battery
> > > applet.  It shows similar types of information as gnome-power-manager
> > does,
> > > but because it's not part of the OS, it doesn't get notifications like this so
> > > special BIOS hooks are in place for it to know when to re-read battery
> > > information.
> > >
> > > On Linux, there are already infrastructure for battery removal and adding
> > > notifications from ACPI.  Sending this up to user space won't be useful.
> > 
> > Errr. I mean WMI event 0xe00e which you wrote is emitted when user press
> > battery key. And I think this event should be sent by dell-wmi.c as
> > KEY_BATTERY.
> > 
> 
> No, I mean 0xe00e scancode is emitted when user presses the battery key on
> older laptops.  I'm only mentioning it because it happens to have the same
> scancode as is received for WMI for another purpose and I wanted to make sure
> it was clear what both are for.  
> This button press isn't emitted via WMI, it's over standard serio.
> 
> The event received over WMI for battery refresh that shares the same code shouldn't
> be emitted as KEY_BATTERY to the OS is all I was trying to say.

Ah right, sorry I was confused. WMI e00e is that battery insert/delete
event which we should ignore in dell-wmi.

So I will prepare patch which disable e00e event in dell-wmi.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Gabriele Mazzotta]

On 22/06/2016 16:21, Mario_Limonciello@Dell.com wrote:
>> -----Original Message-----
>> From: Pali Rohár [mailto:pali.rohar@gmail.com]
>> Sent: Wednesday, June 22, 2016 9:13 AM
>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
>> Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org; dvhart@infradead.org;
>> kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com; platform-
>> driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
>> codes
>>
>> On Wednesday 22 June 2016 13:40:57 Mario_Limonciello@Dell.com wrote:
>>>>> You aren't seeing this on the DSDT of your Latitude right?
>>>>
>>>> Yes, I do not see it on Latitude.
>>>
>>> Thanks, the usage of this scan code is specific to consumer BIOSes.
>>>
>>>>
>>>>> Gabriele,
>>>>>
>>>>> Your machine is from the year before XPS switched over to running the
>>>>> Dell business client (eg Latitude, Precision, Optiplex) BIOS.
>>>>>
>>>>> The EC in that machine does have support for "Battery Health" via that
>>>>> scancode.  On Windows it's used for relaying battery information to an
>>>>> application called Quick Set.
>>>>
>>>> Do you have some details when it is send to OS? And how to read that
>> that
>>>> "battery health"?
>>>
>>> When a battery is removed or inserted this event is supposed to be
>> received
>>> by quickset over WMI and then Quickset will re-read battery information.
>>
>> So event is sent only if battery is removed or inserted?
>>
> 
> Yeah, that's what my spec says, I haven't tested this on actual system to see.
> 
> I'm guessing what's going on is that during suspend ACPI battery drops
> off system and comes back up on resume.
> 
> Maybe Gabriele can comment if any other times were noticed, but in any
> case I think it's appropriate for dell-wmi driver when receiving this on WMI
> to not do anything.  Sending KEY_BATTERY would be wrong behavior.

I think I saw the event only after resume, but I don't read my dmesg
that often to notice other special cases. Surely it's not related to
any hotkey nor actual battery removal.

FYI I have a "battery button" and the associated code is 0xe007. I
guess most of the laptop nowadays use that code for QuickSet, given
that the entry for it was added to dell-wmi.c back in 2009.

I would also like to remind that my laptop receives four WMI events
with code 0xe00e after resume. If we send an input event for each
WMI event with code 0xe00e, I'd get four bogus button keypresses.

RE: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Mario_Limonciello]

> -----Original Message-----
> From: Gabriele Mazzotta [mailto:gabriele.mzt@gmail.com]
> Sent: Wednesday, June 22, 2016 9:40 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; pali.rohar@gmail.com
> Cc: mjg59@srcf.ucam.org; dvhart@infradead.org; kernel@kempniu.pl;
> luto@kernel.org; alex.hung@canonical.com; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI event
> codes
> 
> On 22/06/2016 16:21, Mario_Limonciello@Dell.com wrote:
> >> -----Original Message-----
> >> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> >> Sent: Wednesday, June 22, 2016 9:13 AM
> >> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> >> Cc: gabriele.mzt@gmail.com; mjg59@srcf.ucam.org;
> dvhart@infradead.org;
> >> kernel@kempniu.pl; luto@kernel.org; alex.hung@canonical.com;
> platform-
> >> driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH 3/4] dell-wmi: Add information about other WMI
> event
> >> codes
> >>
> >> On Wednesday 22 June 2016 13:40:57 Mario_Limonciello@Dell.com
> wrote:
> >>>>> You aren't seeing this on the DSDT of your Latitude right?
> >>>>
> >>>> Yes, I do not see it on Latitude.
> >>>
> >>> Thanks, the usage of this scan code is specific to consumer BIOSes.
> >>>
> >>>>
> >>>>> Gabriele,
> >>>>>
> >>>>> Your machine is from the year before XPS switched over to running
> the
> >>>>> Dell business client (eg Latitude, Precision, Optiplex) BIOS.
> >>>>>
> >>>>> The EC in that machine does have support for "Battery Health" via that
> >>>>> scancode.  On Windows it's used for relaying battery information to an
> >>>>> application called Quick Set.
> >>>>
> >>>> Do you have some details when it is send to OS? And how to read that
> >> that
> >>>> "battery health"?
> >>>
> >>> When a battery is removed or inserted this event is supposed to be
> >> received
> >>> by quickset over WMI and then Quickset will re-read battery
> information.
> >>
> >> So event is sent only if battery is removed or inserted?
> >>
> >
> > Yeah, that's what my spec says, I haven't tested this on actual system to
> see.
> >
> > I'm guessing what's going on is that during suspend ACPI battery drops
> > off system and comes back up on resume.
> >
> > Maybe Gabriele can comment if any other times were noticed, but in any
> > case I think it's appropriate for dell-wmi driver when receiving this on WMI
> > to not do anything.  Sending KEY_BATTERY would be wrong behavior.
> 
> I think I saw the event only after resume, but I don't read my dmesg
> that often to notice other special cases. Surely it's not related to
> any hotkey nor actual battery removal.
> 
> FYI I have a "battery button" and the associated code is 0xe007. I
> guess most of the laptop nowadays use that code for QuickSet, given
> that the entry for it was added to dell-wmi.c back in 2009.

Ah yes to make sure that's clear - that battery button is 
"Battery Health Meter". For QuickSet that function was for just showing
a little popup with remaining battery life.  For Linux I do think KEY_BATTERY
is still appropriate there.

> 
> I would also like to remind that my laptop receives four WMI events
> with code 0xe00e after resume. If we send an input event for each
> WMI event with code 0xe00e, I'd get four bogus button keypresses.

Yep, exactly why dell-wmi should just ignore this code.

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Michał Kępień]

> ACPI DSDT tables have defined other WMI codes, but does not contain any
> description when those codes are emitted. Some other codes can be found in
> logs on internet. In this patch are all which I saw, but lot of them are
> not tested properly (e.g. for duplicate events with AT keyboard). Now we
> have all WMI event codes at one place and in future after proper testing
> those codes can be correctly enabled or disabled...
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/platform/x86/dell-wmi.c |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 363d927..7aac1dc 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -110,6 +110,9 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  	/* BIOS error detected */
>  	{ KE_IGNORE, 0xe00d, { KEY_RESERVED } },
>  
> +	/* Unknown, defined in ACPI DSDT */
> +	/* { KE_IGNORE, 0xe00e, { KEY_RESERVED } }, */
> +
>  	/* Wifi Catcher */
>  	{ KE_KEY,    0xe011, { KEY_PROG2 } },
>  
> @@ -118,21 +121,45 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  
>  	{ KE_IGNORE, 0xe020, { KEY_MUTE } },
>  
> +	/* Unknown, defined in ACPI DSDT */
> +	/* { KE_IGNORE, 0xe023, { KEY_RESERVED } }, */
> +
> +	/* Untested, Dell Instant Launch key on Inspiron 7520 */
> +	/* { KE_IGNORE, 0xe024, { KEY_RESERVED } }, */
> +
>  	/* Dell Instant Launch key */
>  	{ KE_KEY,    0xe025, { KEY_PROG4 } },
>  
>  	/* Audio panel key */
>  	{ KE_IGNORE, 0xe026, { KEY_RESERVED } },
>  
> +	/* Untested, Multimedia key on Dell Vostro 3560 */
> +	/* { KE_IGNORE, 0xe028, { KEY_RESERVED } }, */
> +
>  	/* Dell Instant Launch key */
>  	{ KE_KEY,    0xe029, { KEY_PROG4 } },
>  
> +	/* Untested, Windows Mobility Center button on Inspiron 7520 */
> +	/* { KE_IGNORE, 0xe02a, { KEY_RESERVED } }, */
> +
> +	/* Unknown, defined in ACPI DSDT */
> +	/* { KE_IGNORE, 0xe02b, { KEY_RESERVED } }, */
> +
> +	/* Untested, Dell Audio With Preset Switch button on Inspiron 7520 */
> +	/* { KE_IGNORE, 0xe02c, { KEY_RESERVED } }, */
> +
>  	{ KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
>  	{ KE_IGNORE, 0xe030, { KEY_VOLUMEUP } },
>  	{ KE_IGNORE, 0xe033, { KEY_KBDILLUMUP } },
>  	{ KE_IGNORE, 0xe034, { KEY_KBDILLUMDOWN } },
>  	{ KE_IGNORE, 0xe03a, { KEY_CAPSLOCK } },
>  
> +	/* NIC Link is Up */
> +	{ KE_IGNORE, 0xe043, { KEY_RESERVED } },
> +
> +	/* NIC Link is Down */
> +	{ KE_IGNORE, 0xe044, { KEY_RESERVED } },
> +
>  	/*
>  	 * This entry is very suspicious!
>  	 * Originally Matthew Garrett created this dell-wmi driver specially for
> @@ -145,7 +172,12 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  	 */
>  	{ KE_IGNORE, 0xe045, { KEY_NUMLOCK } },
>  
> +	/* Scroll lock and also going to tablet mode on portable devices */
>  	{ KE_IGNORE, 0xe046, { KEY_SCROLLLOCK } },
> +
> +	/* Untested, going from tablet mode on portable devices */
> +	/* { KE_IGNORE, 0xe047, { KEY_RESERVED } }, */
> +

I have one more entry to add here:

    { KE_IGNORE, 0xe06e, { KEY_RESERVED } },

WMI event 0xe06e is emitted on a Vostro V131 when the Dell Support
Center hotkey is pressed, along with an i8042 interrupt.

-- 
Best regards,
Michał Kępień

Re: [PATCH 3/4] dell-wmi: Add information about other WMI event codes

[Pali Rohár]

On Thursday 02 June 2016 12:41:53 Michał Kępień wrote:
> I have one more entry to add here:
> 
>     { KE_IGNORE, 0xe06e, { KEY_RESERVED } },
> 
> WMI event 0xe06e is emitted on a Vostro V131 when the Dell Support
> Center hotkey is pressed, along with an i8042 interrupt.

Ok, I will add it to this list.

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCH 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Pali Rohár]

This patch unify procedure for generating sparse keymap and unify also big
switch code for processing WMI events of different types. After this patch
dell-wmi driver does not differ between "old" and "new" hotkey type.

It construct sparse keymap table with all WMI codes. It is because on some
laptops (e.g. Dell Latitude E6440) ACPI/firmware send both event types (old
and new).

Each WMI code in sparse keymap table is prefixed by 16bit event type, so it
does not change functionality on laptops with "old" hotkey support (those
without scancodes in DMI).

This allow us to distinguish between same WMI codes with different types in
sparse keymap. Thanks to this WMI events of type 0x0011 were moved from big
switch into sparse keymap table too.

This patch also fixes possible bug in parsing WMI event buffer introduced
in commit 5ea2559726b7 ("dell-wmi: Add support for new Dell systems"). That
commit changed buffer type from int* to u16* without fixing code. More at:
http://lkml.iu.edu/hypermail/linux/kernel/1507.0/01950.html

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |  213 +++++++++++++++++++--------------------
 1 file changed, 106 insertions(+), 107 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 7aac1dc..24b8103 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -80,12 +80,13 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
 };
 
 /*
+ * Keymap for WMI events of type 0x0000
+ *
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
  * via the keyboard controller so should not be sent again.
  */
-
-static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0000[] __initconst = {
 
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
@@ -182,12 +183,8 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
 	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
 	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
 
-	{ KE_END, 0 }
-
 };
 
-static bool dell_new_hk_type;
-
 struct dell_bios_keymap_entry {
 	u16 scancode;
 	u16 keycode;
@@ -201,6 +198,7 @@ struct dell_bios_hotkey_table {
 
 struct dell_dmi_results {
 	int err;
+	int keymap_size;
 	struct key_entry *keymap;
 };
 
@@ -249,10 +247,12 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
 };
 
 /*
+ * Keymap for WMI events of type 0x0010
+ *
  * These are applied if the 0xB2 DMI hotkey table is present and doesn't
  * override them.
  */
-static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
 	/* Fn-lock */
 	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
 
@@ -272,21 +272,39 @@ static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
 	{ KE_IGNORE, 0x155, { KEY_RESERVED } },
 };
 
+/*
+ * Keymap for WMI events of type 0x0011
+ */
+static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
+	/* Battery unplugged */
+	{ KE_IGNORE, 0xfff0, { KEY_RESERVED } },
+
+	/* Battery inserted */
+	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
+
+	/* Keyboard backlight level changed */
+	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
+	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
+};
+
 static struct input_dev *dell_wmi_input_dev;
 
-static void dell_wmi_process_key(int reported_key)
+static void dell_wmi_process_key(int type, int code)
 {
 	const struct key_entry *key;
 
 	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
-						reported_key);
+						(type << 16) | code);
 	if (!key) {
-		pr_info("Unknown key with scancode 0x%x pressed\n",
-			reported_key);
+		pr_info("Unknown key with type 0x%x and code 0x%x pressed\n",
+			type, code);
 		return;
 	}
 
-	pr_debug("Key %x pressed\n", reported_key);
+	pr_debug("Key with type 0x%x and code 0x%x pressed\n", type, code);
 
 	/* Don't report brightness notifications that will also come via ACPI */
 	if ((key->keycode == KEY_BRIGHTNESSUP ||
@@ -294,7 +312,7 @@ static void dell_wmi_process_key(int reported_key)
 	    acpi_video_handles_brightness_key_presses())
 		return;
 
-	if (reported_key == 0xe025 && !wmi_requires_smbios_request)
+	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
 		return;
 
 	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
@@ -332,18 +350,6 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	buffer_entry = (u16 *)obj->buffer.pointer;
 	buffer_size = obj->buffer.length/2;
-
-	if (!dell_new_hk_type) {
-		if (buffer_size >= 3 && buffer_entry[1] == 0x0)
-			dell_wmi_process_key(buffer_entry[2]);
-		else if (buffer_size >= 2)
-			dell_wmi_process_key(buffer_entry[1]);
-		else
-			pr_info("Received unknown WMI event\n");
-		kfree(obj);
-		return;
-	}
-
 	buffer_end = buffer_entry + buffer_size;
 
 	/*
@@ -378,59 +384,21 @@ static void dell_wmi_notify(u32 value, void *context)
 		pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
 
 		switch (buffer_entry[1]) {
-		case 0x00:
-			for (i = 2; i < len; ++i) {
-				switch (buffer_entry[i]) {
-				case 0xe043:
-					/* NIC Link is Up */
-					pr_debug("NIC Link is Up\n");
-					break;
-				case 0xe044:
-					/* NIC Link is Down */
-					pr_debug("NIC Link is Down\n");
-					break;
-				case 0xe045:
-					/* Unknown event but defined in DSDT */
-				default:
-					/* Unknown event */
-					pr_info("Unknown WMI event type 0x00: "
-						"0x%x\n", (int)buffer_entry[i]);
-					break;
-				}
-			}
+		case 0x0000:
+			/* One key pressed or event occurred */
+			if (len > 2)
+				dell_wmi_process_key(0x0000, buffer_entry[2]);
+			/* Other entries in buffer could contain additional information */
 			break;
-		case 0x10:
-			/* Keys pressed */
+		case 0x0010:
+			/* Sequence of keys pressed */
 			for (i = 2; i < len; ++i)
-				dell_wmi_process_key(buffer_entry[i]);
+				dell_wmi_process_key(0x0010, buffer_entry[i]);
 			break;
-		case 0x11:
-			for (i = 2; i < len; ++i) {
-				switch (buffer_entry[i]) {
-				case 0xfff0:
-					/* Battery unplugged */
-					pr_debug("Battery unplugged\n");
-					break;
-				case 0xfff1:
-					/* Battery inserted */
-					pr_debug("Battery inserted\n");
-					break;
-				case 0x01e1:
-				case 0x02ea:
-				case 0x02eb:
-				case 0x02ec:
-				case 0x02f6:
-					/* Keyboard backlight level changed */
-					pr_debug("Keyboard backlight level "
-						 "changed\n");
-					break;
-				default:
-					/* Unknown event */
-					pr_info("Unknown WMI event type 0x11: "
-						"0x%x\n", (int)buffer_entry[i]);
-					break;
-				}
-			}
+		case 0x0011:
+			/* Sequence of events occurred */
+			for (i = 2; i < len; ++i)
+				dell_wmi_process_key(0x0011, buffer_entry[i]);
 			break;
 		default:
 			/* Unknown event */
@@ -458,7 +426,6 @@ static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
 }
 
 static void __init handle_dmi_entry(const struct dmi_header *dm,
-
 				    void *opaque)
 
 {
@@ -466,7 +433,6 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	struct dell_bios_hotkey_table *table;
 	int hotkey_num, i, pos = 0;
 	struct key_entry *keymap;
-	int num_bios_keys;
 
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
@@ -490,8 +456,7 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 		return;
 	}
 
-	keymap = kcalloc(hotkey_num + ARRAY_SIZE(dell_wmi_extra_keymap) + 1,
-			 sizeof(struct key_entry), GFP_KERNEL);
+	keymap = kcalloc(hotkey_num, sizeof(struct key_entry), GFP_KERNEL);
 	if (!keymap) {
 		results->err = -ENOMEM;
 		return;
@@ -528,31 +493,15 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 		pos++;
 	}
 
-	num_bios_keys = pos;
-
-	for (i = 0; i < ARRAY_SIZE(dell_wmi_extra_keymap); i++) {
-		const struct key_entry *entry = &dell_wmi_extra_keymap[i];
-
-		/*
-		 * Check if we've already found this scancode.  This takes
-		 * quadratic time, but it doesn't matter unless the list
-		 * of extra keys gets very long.
-		 */
-		if (!have_scancode(entry->code, keymap, num_bios_keys)) {
-			keymap[pos] = *entry;
-			pos++;
-		}
-	}
-
-	keymap[pos].type = KE_END;
-
 	results->keymap = keymap;
+	results->keymap_size = pos;
 }
 
 static int __init dell_wmi_input_setup(void)
 {
 	struct dell_dmi_results dmi_results = {};
-	int err;
+	struct key_entry *keymap;
+	int err, i, pos = 0;
 
 	dell_wmi_input_dev = input_allocate_device();
 	if (!dell_wmi_input_dev)
@@ -576,21 +525,71 @@ static int __init dell_wmi_input_setup(void)
 		goto err_free_dev;
 	}
 
+	keymap = kcalloc(dmi_results.keymap_size +
+			 ARRAY_SIZE(dell_wmi_keymap_type_0000) +
+			 ARRAY_SIZE(dell_wmi_keymap_type_0010) +
+			 ARRAY_SIZE(dell_wmi_keymap_type_0011) +
+			 1,
+			 sizeof(struct key_entry), GFP_KERNEL);
+	if (!keymap) {
+		err = -ENOMEM;
+		goto err_free_dev;
+	}
+
+	/* Append table with events of type 0x0010 which comes from DMI */
 	if (dmi_results.keymap) {
-		dell_new_hk_type = true;
+		for (i = 0; i < dmi_results.keymap_size; i++) {
+			keymap[pos] = dmi_results.keymap[i];
+			keymap[pos].code |= (0x0010 << 16);
+			pos++;
+		}
+		kfree(dmi_results.keymap);
+	}
 
-		err = sparse_keymap_setup(dell_wmi_input_dev,
-					  dmi_results.keymap, NULL);
+	/* Append table with extra events of type 0x0010 which are not in DMI */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0010); i++) {
+		const struct key_entry *entry = &dell_wmi_keymap_type_0010[i];
 
 		/*
-		 * Sparse keymap library makes a copy of keymap so we
-		 * don't need the original one that was allocated.
+		 * Check if we've already found this scancode.  This takes
+		 * quadratic time, but it doesn't matter unless the list
+		 * of extra keys gets very long.
 		 */
-		kfree(dmi_results.keymap);
-	} else {
-		err = sparse_keymap_setup(dell_wmi_input_dev,
-					  dell_wmi_legacy_keymap, NULL);
+		if (dmi_results.keymap_size &&
+		    have_scancode(entry->code | (0x0010 << 16),
+				  keymap, dmi_results.keymap_size)
+		   )
+			continue;
+
+		keymap[pos] = *entry;
+		keymap[pos].code |= (0x0010 << 16);
+		pos++;
+	}
+
+	/* Append table with events of type 0x0011 */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0011); i++) {
+		keymap[pos] = dell_wmi_keymap_type_0011[i];
+		keymap[pos].code |= (0x0011 << 16);
+		pos++;
 	}
+
+	/*
+	 * Now append also table with "legacy" events of type 0x0000. Some of
+	 * them are reported also on laptops which have scancodes in DMI.
+	 */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0000); i++) {
+		keymap[pos] = dell_wmi_keymap_type_0000[i];
+		pos++;
+	}
+
+	keymap[pos].type = KE_END;
+
+	err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL);
+	/*
+	 * Sparse keymap library makes a copy of keymap so we don't need the
+	 * original one that was allocated.
+	 */
+	kfree(keymap);
 	if (err)
 		goto err_free_dev;
 
-- 
1.7.9.5

Re: [PATCH 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Andy Lutomirski]

On Sun, May 22, 2016 at 4:36 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> This patch unify procedure for generating sparse keymap and unify also big
> switch code for processing WMI events of different types. After this patch
> dell-wmi driver does not differ between "old" and "new" hotkey type.
>
> It construct sparse keymap table with all WMI codes. It is because on some
> laptops (e.g. Dell Latitude E6440) ACPI/firmware send both event types (old
> and new).
>
> Each WMI code in sparse keymap table is prefixed by 16bit event type, so it
> does not change functionality on laptops with "old" hotkey support (those
> without scancodes in DMI).

This seems like a nice cleanup.  I'll try to test it soon.

--Andy

Re: [PATCH 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Michał Kępień]

> This patch unify procedure for generating sparse keymap and unify also big
> switch code for processing WMI events of different types. After this patch
> dell-wmi driver does not differ between "old" and "new" hotkey type.
> 
> It construct sparse keymap table with all WMI codes. It is because on some
> laptops (e.g. Dell Latitude E6440) ACPI/firmware send both event types (old
> and new).
> 
> Each WMI code in sparse keymap table is prefixed by 16bit event type, so it
> does not change functionality on laptops with "old" hotkey support (those
> without scancodes in DMI).
> 
> This allow us to distinguish between same WMI codes with different types in
> sparse keymap. Thanks to this WMI events of type 0x0011 were moved from big
> switch into sparse keymap table too.
> 
> This patch also fixes possible bug in parsing WMI event buffer introduced
> in commit 5ea2559726b7 ("dell-wmi: Add support for new Dell systems"). That
> commit changed buffer type from int* to u16* without fixing code. More at:
> http://lkml.iu.edu/hypermail/linux/kernel/1507.0/01950.html
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

First of all, thanks for your tireless efforts to get this code cleaned
up.  I like that there is now a common path for all models as this
means, among other things, that the "Process buffer" debug message will
be printed even for "old style" events, which makes it easier to see
which part of the WMI event buffer is actually processed.

> ---
>  drivers/platform/x86/dell-wmi.c |  213 +++++++++++++++++++--------------------
>  1 file changed, 106 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 7aac1dc..24b8103 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -80,12 +80,13 @@ static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
>  };
>  
>  /*
> + * Keymap for WMI events of type 0x0000
> + *
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
>   * via the keyboard controller so should not be sent again.
>   */
> -
> -static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
> +static const struct key_entry dell_wmi_keymap_type_0000[] __initconst = {
>  
>  	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
>  
> @@ -182,12 +183,8 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = {
>  	{ KE_IGNORE, 0xe0f8, { KEY_VOLUMEDOWN } },
>  	{ KE_IGNORE, 0xe0f9, { KEY_VOLUMEUP } },
>  
> -	{ KE_END, 0 }
> -
>  };
>  
> -static bool dell_new_hk_type;
> -
>  struct dell_bios_keymap_entry {
>  	u16 scancode;
>  	u16 keycode;
> @@ -201,6 +198,7 @@ struct dell_bios_hotkey_table {
>  
>  struct dell_dmi_results {
>  	int err;
> +	int keymap_size;
>  	struct key_entry *keymap;
>  };
>  
> @@ -249,10 +247,12 @@ static const u16 bios_to_linux_keycode[256] __initconst = {
>  };
>  
>  /*
> + * Keymap for WMI events of type 0x0010
> + *
>   * These are applied if the 0xB2 DMI hotkey table is present and doesn't
>   * override them.
>   */
> -static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
> +static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
>  	/* Fn-lock */
>  	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
>  
> @@ -272,21 +272,39 @@ static const struct key_entry dell_wmi_extra_keymap[] __initconst = {
>  	{ KE_IGNORE, 0x155, { KEY_RESERVED } },
>  };
>  
> +/*
> + * Keymap for WMI events of type 0x0011
> + */
> +static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
> +	/* Battery unplugged */
> +	{ KE_IGNORE, 0xfff0, { KEY_RESERVED } },
> +
> +	/* Battery inserted */
> +	{ KE_IGNORE, 0xfff1, { KEY_RESERVED } },
> +
> +	/* Keyboard backlight level changed */
> +	{ KE_IGNORE, 0x01e1, { KEY_RESERVED } },
> +	{ KE_IGNORE, 0x02ea, { KEY_RESERVED } },
> +	{ KE_IGNORE, 0x02eb, { KEY_RESERVED } },
> +	{ KE_IGNORE, 0x02ec, { KEY_RESERVED } },
> +	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
> +};
> +
>  static struct input_dev *dell_wmi_input_dev;
>  
> -static void dell_wmi_process_key(int reported_key)
> +static void dell_wmi_process_key(int type, int code)
>  {
>  	const struct key_entry *key;
>  
>  	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
> -						reported_key);
> +						(type << 16) | code);
>  	if (!key) {
> -		pr_info("Unknown key with scancode 0x%x pressed\n",
> -			reported_key);
> +		pr_info("Unknown key with type 0x%x and code 0x%x pressed\n",
> +			type, code);

Since you updated the switch cases below so that each of them now
consists of four digits, maybe it's a good idea to change the format
specifiers for type and code to %04x for coherency?

>  		return;
>  	}
>  
> -	pr_debug("Key %x pressed\n", reported_key);
> +	pr_debug("Key with type 0x%x and code 0x%x pressed\n", type, code);

The same applies here.

>  
>  	/* Don't report brightness notifications that will also come via ACPI */
>  	if ((key->keycode == KEY_BRIGHTNESSUP ||
> @@ -294,7 +312,7 @@ static void dell_wmi_process_key(int reported_key)
>  	    acpi_video_handles_brightness_key_presses())
>  		return;
>  
> -	if (reported_key == 0xe025 && !wmi_requires_smbios_request)
> +	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
>  		return;
>  
>  	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
> @@ -332,18 +350,6 @@ static void dell_wmi_notify(u32 value, void *context)
>  
>  	buffer_entry = (u16 *)obj->buffer.pointer;
>  	buffer_size = obj->buffer.length/2;
> -
> -	if (!dell_new_hk_type) {
> -		if (buffer_size >= 3 && buffer_entry[1] == 0x0)
> -			dell_wmi_process_key(buffer_entry[2]);
> -		else if (buffer_size >= 2)
> -			dell_wmi_process_key(buffer_entry[1]);
> -		else
> -			pr_info("Received unknown WMI event\n");
> -		kfree(obj);
> -		return;
> -	}
> -
>  	buffer_end = buffer_entry + buffer_size;
>  
>  	/*
> @@ -378,59 +384,21 @@ static void dell_wmi_notify(u32 value, void *context)
>  		pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
>  
>  		switch (buffer_entry[1]) {
> -		case 0x00:
> -			for (i = 2; i < len; ++i) {
> -				switch (buffer_entry[i]) {
> -				case 0xe043:
> -					/* NIC Link is Up */
> -					pr_debug("NIC Link is Up\n");
> -					break;
> -				case 0xe044:
> -					/* NIC Link is Down */
> -					pr_debug("NIC Link is Down\n");
> -					break;
> -				case 0xe045:
> -					/* Unknown event but defined in DSDT */
> -				default:
> -					/* Unknown event */
> -					pr_info("Unknown WMI event type 0x00: "
> -						"0x%x\n", (int)buffer_entry[i]);
> -					break;
> -				}
> -			}
> +		case 0x0000:
> +			/* One key pressed or event occurred */
> +			if (len > 2)
> +				dell_wmi_process_key(0x0000, buffer_entry[2]);

I am sure you spent way more time analysing this than me, but is this
documented anywhere?  Technically speaking, this is a behavioral change
which causes events to be lost on any Dell model which is capable of
stuffing more than one key code into a single type 0x0000 WMI event (not
that I know of any such model).

> +			/* Other entries in buffer could contain additional information */

This comment exceeds 80 characters, but do you think it is needed at
all?  What does the reader gain by reading it?  Any additional
information that follows the key code is ignored by kernel code anyway.

>  			break;
> -		case 0x10:
> -			/* Keys pressed */
> +		case 0x0010:
> +			/* Sequence of keys pressed */
>  			for (i = 2; i < len; ++i)
> -				dell_wmi_process_key(buffer_entry[i]);
> +				dell_wmi_process_key(0x0010, buffer_entry[i]);
>  			break;
> -		case 0x11:
> -			for (i = 2; i < len; ++i) {
> -				switch (buffer_entry[i]) {
> -				case 0xfff0:
> -					/* Battery unplugged */
> -					pr_debug("Battery unplugged\n");
> -					break;
> -				case 0xfff1:
> -					/* Battery inserted */
> -					pr_debug("Battery inserted\n");
> -					break;
> -				case 0x01e1:
> -				case 0x02ea:
> -				case 0x02eb:
> -				case 0x02ec:
> -				case 0x02f6:
> -					/* Keyboard backlight level changed */
> -					pr_debug("Keyboard backlight level "
> -						 "changed\n");
> -					break;
> -				default:
> -					/* Unknown event */
> -					pr_info("Unknown WMI event type 0x11: "
> -						"0x%x\n", (int)buffer_entry[i]);
> -					break;
> -				}
> -			}
> +		case 0x0011:
> +			/* Sequence of events occurred */
> +			for (i = 2; i < len; ++i)
> +				dell_wmi_process_key(0x0011, buffer_entry[i]);
>  			break;
>  		default:
>  			/* Unknown event */
> @@ -458,7 +426,6 @@ static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
>  }
>  
>  static void __init handle_dmi_entry(const struct dmi_header *dm,
> -
>  				    void *opaque)
>  
>  {
> @@ -466,7 +433,6 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>  	struct dell_bios_hotkey_table *table;
>  	int hotkey_num, i, pos = 0;
>  	struct key_entry *keymap;
> -	int num_bios_keys;
>  
>  	if (results->err || results->keymap)
>  		return;		/* We already found the hotkey table. */
> @@ -490,8 +456,7 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>  		return;
>  	}
>  
> -	keymap = kcalloc(hotkey_num + ARRAY_SIZE(dell_wmi_extra_keymap) + 1,
> -			 sizeof(struct key_entry), GFP_KERNEL);
> +	keymap = kcalloc(hotkey_num, sizeof(struct key_entry), GFP_KERNEL);
>  	if (!keymap) {
>  		results->err = -ENOMEM;
>  		return;
> @@ -528,31 +493,15 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>  		pos++;
>  	}
>  
> -	num_bios_keys = pos;
> -
> -	for (i = 0; i < ARRAY_SIZE(dell_wmi_extra_keymap); i++) {
> -		const struct key_entry *entry = &dell_wmi_extra_keymap[i];
> -
> -		/*
> -		 * Check if we've already found this scancode.  This takes
> -		 * quadratic time, but it doesn't matter unless the list
> -		 * of extra keys gets very long.
> -		 */
> -		if (!have_scancode(entry->code, keymap, num_bios_keys)) {
> -			keymap[pos] = *entry;
> -			pos++;
> -		}
> -	}
> -
> -	keymap[pos].type = KE_END;
> -
>  	results->keymap = keymap;
> +	results->keymap_size = pos;
>  }
>  
>  static int __init dell_wmi_input_setup(void)
>  {
>  	struct dell_dmi_results dmi_results = {};
> -	int err;
> +	struct key_entry *keymap;
> +	int err, i, pos = 0;
>  
>  	dell_wmi_input_dev = input_allocate_device();
>  	if (!dell_wmi_input_dev)
> @@ -576,21 +525,71 @@ static int __init dell_wmi_input_setup(void)
>  		goto err_free_dev;
>  	}
>  
> +	keymap = kcalloc(dmi_results.keymap_size +
> +			 ARRAY_SIZE(dell_wmi_keymap_type_0000) +
> +			 ARRAY_SIZE(dell_wmi_keymap_type_0010) +
> +			 ARRAY_SIZE(dell_wmi_keymap_type_0011) +
> +			 1,
> +			 sizeof(struct key_entry), GFP_KERNEL);
> +	if (!keymap) {
> +		err = -ENOMEM;
> +		goto err_free_dev;

You're potentially leaking dmi_results.keymap here.

> +	}
> +
> +	/* Append table with events of type 0x0010 which comes from DMI */
>  	if (dmi_results.keymap) {
> -		dell_new_hk_type = true;
> +		for (i = 0; i < dmi_results.keymap_size; i++) {
> +			keymap[pos] = dmi_results.keymap[i];
> +			keymap[pos].code |= (0x0010 << 16);
> +			pos++;
> +		}
> +		kfree(dmi_results.keymap);
> +	}

Nit, but is the enclosing conditional expression needed any more, now
that you got rid of dell_new_hk_type?  If the 0xB2 DMI table is not
found then dmi_results.keymap_size will be 0 and thus the for loop above
doesn't do anything and it is okay to pass a null pointer to kfree().

>  
> -		err = sparse_keymap_setup(dell_wmi_input_dev,
> -					  dmi_results.keymap, NULL);
> +	/* Append table with extra events of type 0x0010 which are not in DMI */
> +	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0010); i++) {
> +		const struct key_entry *entry = &dell_wmi_keymap_type_0010[i];
>  
>  		/*
> -		 * Sparse keymap library makes a copy of keymap so we
> -		 * don't need the original one that was allocated.
> +		 * Check if we've already found this scancode.  This takes
> +		 * quadratic time, but it doesn't matter unless the list
> +		 * of extra keys gets very long.
>  		 */
> -		kfree(dmi_results.keymap);
> -	} else {
> -		err = sparse_keymap_setup(dell_wmi_input_dev,
> -					  dell_wmi_legacy_keymap, NULL);
> +		if (dmi_results.keymap_size &&
> +		    have_scancode(entry->code | (0x0010 << 16),
> +				  keymap, dmi_results.keymap_size)
> +		   )
> +			continue;

Is the first part of this conditional expression really needed?  If
dmi_results.keymap_size is 0 then have_scancode() will simply return
false, so the only disadvantage of removing this check is the overhead
of calling have_scancode() for every iteration of the loop, but I
believe that overhead is negligible as this is not performance-critical
code.

-- 
Best regards,
Michał Kępień

Re: [PATCH 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

[Pali Rohár]

On Thursday 02 June 2016 12:42:11 Michał Kępień wrote:
> > -static void dell_wmi_process_key(int reported_key)
> > +static void dell_wmi_process_key(int type, int code)
> >  {
> >  	const struct key_entry *key;
> >  
> >  	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
> > -						reported_key);
> > +						(type << 16) | code);
> >  	if (!key) {
> > -		pr_info("Unknown key with scancode 0x%x pressed\n",
> > -			reported_key);
> > +		pr_info("Unknown key with type 0x%x and code 0x%x pressed\n",
> > +			type, code);
> 
> Since you updated the switch cases below so that each of them now
> consists of four digits, maybe it's a good idea to change the format
> specifiers for type and code to %04x for coherency?

Ok.

> >  		return;
> >  	}
> >  
> > -	pr_debug("Key %x pressed\n", reported_key);
> > +	pr_debug("Key with type 0x%x and code 0x%x pressed\n", type, code);
> 
> The same applies here.

Ok.

> > +		case 0x0000:
> > +			/* One key pressed or event occurred */
> > +			if (len > 2)
> > +				dell_wmi_process_key(0x0000, buffer_entry[2]);
> 
> I am sure you spent way more time analysing this than me, but is this
> documented anywhere?

Yes, this is the only documented part. Read my email linked to commit
message for more details.

> Technically speaking, this is a behavioral change
> which causes events to be lost on any Dell model which is capable of
> stuffing more than one key code into a single type 0x0000 WMI event (not
> that I know of any such model).

I do not think. Other values in that buffer are not scan codes of other
keys, but additional events. See that table with comments (those
comments which you suggested to change).

> > +			/* Other entries in buffer could contain additional information */
> 
> This comment exceeds 80 characters, but do you think it is needed at
> all?  What does the reader gain by reading it?  Any additional
> information that follows the key code is ignored by kernel code anyway.

It is just documentation purpose, to understand why we are processing
only buffer_entry[2] and why are ignoring anything else after it.

> >  			break;

> > @@ -576,21 +525,71 @@ static int __init dell_wmi_input_setup(void)
> >  		goto err_free_dev;
> >  	}
> >  
> > +	keymap = kcalloc(dmi_results.keymap_size +
> > +			 ARRAY_SIZE(dell_wmi_keymap_type_0000) +
> > +			 ARRAY_SIZE(dell_wmi_keymap_type_0010) +
> > +			 ARRAY_SIZE(dell_wmi_keymap_type_0011) +
> > +			 1,
> > +			 sizeof(struct key_entry), GFP_KERNEL);
> > +	if (!keymap) {
> > +		err = -ENOMEM;
> > +		goto err_free_dev;
> 
> You're potentially leaking dmi_results.keymap here.

You are right, I will fix it.

> > +	}
> > +
> > +	/* Append table with events of type 0x0010 which comes from DMI */
> >  	if (dmi_results.keymap) {
> > -		dell_new_hk_type = true;
> > +		for (i = 0; i < dmi_results.keymap_size; i++) {
> > +			keymap[pos] = dmi_results.keymap[i];
> > +			keymap[pos].code |= (0x0010 << 16);
> > +			pos++;
> > +		}
> > +		kfree(dmi_results.keymap);
> > +	}
> 
> Nit, but is the enclosing conditional expression needed any more, now
> that you got rid of dell_new_hk_type?  If the 0xB2 DMI table is not
> found then dmi_results.keymap_size will be 0 and thus the for loop above
> doesn't do anything and it is okay to pass a null pointer to kfree().

Yes, it is not needed anymore. I will delete it.

> >  
> > -		err = sparse_keymap_setup(dell_wmi_input_dev,
> > -					  dmi_results.keymap, NULL);
> > +	/* Append table with extra events of type 0x0010 which are not in DMI */
> > +	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0010); i++) {
> > +		const struct key_entry *entry = &dell_wmi_keymap_type_0010[i];
> >  
> >  		/*
> > -		 * Sparse keymap library makes a copy of keymap so we
> > -		 * don't need the original one that was allocated.
> > +		 * Check if we've already found this scancode.  This takes
> > +		 * quadratic time, but it doesn't matter unless the list
> > +		 * of extra keys gets very long.
> >  		 */
> > -		kfree(dmi_results.keymap);
> > -	} else {
> > -		err = sparse_keymap_setup(dell_wmi_input_dev,
> > -					  dell_wmi_legacy_keymap, NULL);
> > +		if (dmi_results.keymap_size &&
> > +		    have_scancode(entry->code | (0x0010 << 16),
> > +				  keymap, dmi_results.keymap_size)
> > +		   )
> > +			continue;
> 
> Is the first part of this conditional expression really needed?  If
> dmi_results.keymap_size is 0 then have_scancode() will simply return
> false, so the only disadvantage of removing this check is the overhead
> of calling have_scancode() for every iteration of the loop, but I
> believe that overhead is negligible as this is not performance-critical
> code.

It is linear scan of whole table and so above two loops has something
like quadratic time complexity. List dell_wmi_keymap_type_0010 is not
too long for now, so it is OK. But still it is better not to call
have_scancode() everytime if it is not needed. Once we have big list of
events we could switch code to use some hash tables...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/4] dell-wmi: Changes in WMI event code handling

[Michał Kępień]

> First patch describe problem about 0xe045 code. Second and third are just
> cosmetic and last rework code which processing WMI events. It should be
> properly tested on more Dell machines, to check that everything is still
> working correctly.

On a Vostro V131:

Tested-by: Michał Kępień <kernel@kempniu.pl>

-- 
Best regards,
Michał Kępień