| Message ID | 1456298416-29683-5-git-send-email-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wednesday 24 February 2016 08:20:14 Micha? K?pie? wrote: > On models on which an SMBIOS request needs to be issued in order for WMI > events to be generated, pressing the Dell Instant Launch hotkey does not > raise an i8042 interrupt - only a WMI event is generated (0xe025 on Dell > Vostro V131). Thus, the 0xe025 event should only be ignored on machines > which do not require an SMBIOS request for enabling WMI. > > Signed-off-by: Micha? K?pie? <kernel@kempniu.pl> > --- > drivers/platform/x86/dell-wmi.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index 65edd93..ffc957b5 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -111,7 +111,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = { > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > /* Shortcut and audio panel keys */ > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key) > acpi_video_handles_brightness_key_presses()) > return; > > + if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request) > + return; > + Here I would rather test against reported_key, not keycode. If somebody in future adds KEY_PROG4 for something else we will have problem... > sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); > } >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > > index 65edd93..ffc957b5 100644 > > --- a/drivers/platform/x86/dell-wmi.c > > +++ b/drivers/platform/x86/dell-wmi.c > > @@ -111,7 +111,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = { > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > /* Shortcut and audio panel keys */ > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key) > > acpi_video_handles_brightness_key_presses()) > > return; > > > > + if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request) > > + return; > > + > > Here I would rather test against reported_key, not keycode. If somebody > in future adds KEY_PROG4 for something else we will have problem... As 0xe025 is currently the only event we know about that should be ignored on some machines and processed on others, this makes sense, at least for now. If I change the first condition to: reported_key == 0xe025 will you be okay with adding your Reviewed-by for this patch? Then, for Darren's convenience, I could post a v5 of the whole series with the above change and all your Acked-by and Reviewed-by tags added.
On Monday 29 February 2016 21:31:23 Micha? K?pie? wrote: > > > diff --git a/drivers/platform/x86/dell-wmi.c > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 100644 > > > --- a/drivers/platform/x86/dell-wmi.c > > > +++ b/drivers/platform/x86/dell-wmi.c > > > @@ -111,7 +111,7 @@ static const struct key_entry > > > dell_wmi_legacy_keymap[] __initconst = { > > > > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > /* Shortcut and audio panel keys */ > > > > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > reported_key) > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > return; > > > > > > + if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request) > > > + return; > > > + > > > > Here I would rather test against reported_key, not keycode. If > > somebody in future adds KEY_PROG4 for something else we will have > > problem... > > As 0xe025 is currently the only event we know about that should be > ignored on some machines and processed on others, this makes sense, > at least for now. If I change the first condition to: > > reported_key == 0xe025 There will be need also change for 5/5 patch... > will you be okay with adding your Reviewed-by for this patch? Then, > for Darren's convenience, I could post a v5 of the whole series with > the above change and all your Acked-by and Reviewed-by tags added. This is my suggestion as I'm thinking about future changes to this driver... But it is also on Darren as maintainer of platform x86 subsystem. Basically you do not want to check if pressed key is KEY_PROG4. But you want to check if dell wmi sent event 0xe025. This is what I say.
> On Monday 29 February 2016 21:31:23 Micha? K?pie? wrote: > > > > diff --git a/drivers/platform/x86/dell-wmi.c > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 100644 > > > > --- a/drivers/platform/x86/dell-wmi.c > > > > +++ b/drivers/platform/x86/dell-wmi.c > > > > @@ -111,7 +111,7 @@ static const struct key_entry > > > > dell_wmi_legacy_keymap[] __initconst = { > > > > > > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > > > /* Shortcut and audio panel keys */ > > > > > > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > reported_key) > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > return; > > > > > > > > + if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request) > > > > + return; > > > > + > > > > > > Here I would rather test against reported_key, not keycode. If > > > somebody in future adds KEY_PROG4 for something else we will have > > > problem... > > > > As 0xe025 is currently the only event we know about that should be > > ignored on some machines and processed on others, this makes sense, > > at least for now. If I change the first condition to: > > > > reported_key == 0xe025 > > There will be need also change for 5/5 patch... Why? Are you aware of any model which sends a 0xe029 WMI event _and_ generates an i8042 interrupt? If not, WMI event 0xe029 should always be turned into a key event, as per the keymap. > > will you be okay with adding your Reviewed-by for this patch? Then, > > for Darren's convenience, I could post a v5 of the whole series with > > the above change and all your Acked-by and Reviewed-by tags added. > > This is my suggestion as I'm thinking about future changes to this > driver... But it is also on Darren as maintainer of platform x86 > subsystem. > > Basically you do not want to check if pressed key is KEY_PROG4. But you > want to check if dell wmi sent event 0xe025. This is what I say. Well, FWIW, I agree with you and I'll be happy to make that change. It would make the code and the commit message more coherent.
On Monday 29 February 2016 21:49:27 you wrote: > > On Monday 29 February 2016 21:31:23 Micha? K?pie? wrote: > > > > > diff --git a/drivers/platform/x86/dell-wmi.c > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c > > > > > +++ b/drivers/platform/x86/dell-wmi.c > > > > > @@ -111,7 +111,7 @@ static const struct key_entry > > > > > dell_wmi_legacy_keymap[] __initconst = { > > > > > > > > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > > > > > /* Shortcut and audio panel keys */ > > > > > > > > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > > reported_key) > > > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > > > return; > > > > > > > > > > + if (key->keycode == KEY_PROG4 && > > > > > !wmi_requires_smbios_request) + return; > > > > > + > > > > > > > > Here I would rather test against reported_key, not keycode. If > > > > somebody in future adds KEY_PROG4 for something else we will > > > > have problem... > > > > > > As 0xe025 is currently the only event we know about that should > > > be ignored on some machines and processed on others, this makes > > > sense, > > > > > > at least for now. If I change the first condition to: > > > reported_key == 0xe025 > > > > There will be need also change for 5/5 patch... > > Why? Are you aware of any model which sends a 0xe029 WMI event _and_ > generates an i8042 interrupt? If not, WMI event 0xe029 should always > be turned into a key event, as per the keymap. No, but your current patch 4/5 and 5/5 do that (because it checks KEY_PROG4). But if it is not needed, I'm happy because of one hook less.
On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote: > On Monday 29 February 2016 21:49:27 you wrote: > > > On Monday 29 February 2016 21:31:23 Micha? K?pie? wrote: > > > > > > diff --git a/drivers/platform/x86/dell-wmi.c > > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 > > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c > > > > > > +++ b/drivers/platform/x86/dell-wmi.c > > > > > > @@ -111,7 +111,7 @@ static const struct key_entry > > > > > > dell_wmi_legacy_keymap[] __initconst = { > > > > > > > > > > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > > > > > > > /* Shortcut and audio panel keys */ > > > > > > > > > > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > > > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > > > reported_key) > > > > > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > > > > > return; > > > > > > > > > > > > + if (key->keycode == KEY_PROG4 && > > > > > > !wmi_requires_smbios_request) + return; > > > > > > + > > > > > > > > > > Here I would rather test against reported_key, not keycode. If > > > > > somebody in future adds KEY_PROG4 for something else we will > > > > > have problem... > > > > > > > > As 0xe025 is currently the only event we know about that should > > > > be ignored on some machines and processed on others, this makes > > > > sense, > > > > > > > > at least for now. If I change the first condition to: > > > > reported_key == 0xe025 > > > > > > There will be need also change for 5/5 patch... > > > > Why? Are you aware of any model which sends a 0xe029 WMI event _and_ > > generates an i8042 interrupt? If not, WMI event 0xe029 should always > > be turned into a key event, as per the keymap. > > No, but your current patch 4/5 and 5/5 do that (because it checks > KEY_PROG4). But if it is not needed, I'm happy because of one hook less. From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be ignored, so the test would need to include both if it isn't using the common keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not the correct reading of 5/5? > > -- > Pali Rohár > pali.rohar@gmail.com
On Mon, Feb 29, 2016 at 09:31:23PM +0100, Micha? K?pie? wrote: > > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > > > index 65edd93..ffc957b5 100644 > > > --- a/drivers/platform/x86/dell-wmi.c > > > +++ b/drivers/platform/x86/dell-wmi.c > > > @@ -111,7 +111,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = { > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > /* Shortcut and audio panel keys */ > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key) > > > acpi_video_handles_brightness_key_presses()) > > > return; > > > > > > + if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request) > > > + return; > > > + > > > > Here I would rather test against reported_key, not keycode. If somebody > > in future adds KEY_PROG4 for something else we will have problem... > > As 0xe025 is currently the only event we know about that should be > ignored on some machines and processed on others, this makes sense, at > least for now. If I change the first condition to: > > reported_key == 0xe025 > > will you be okay with adding your Reviewed-by for this patch? Then, for > Darren's convenience, I could post a v5 of the whole series with the > above change and all your Acked-by and Reviewed-by tags added. Yes, please do. That way I'm sure I have the right bits.
> On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote: > > On Monday 29 February 2016 21:49:27 you wrote: > > > > On Monday 29 February 2016 21:31:23 Micha? K?pie? wrote: > > > > > > > diff --git a/drivers/platform/x86/dell-wmi.c > > > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 > > > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c > > > > > > > +++ b/drivers/platform/x86/dell-wmi.c > > > > > > > @@ -111,7 +111,7 @@ static const struct key_entry > > > > > > > dell_wmi_legacy_keymap[] __initconst = { > > > > > > > > > > > > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > > > > > > > > > /* Shortcut and audio panel keys */ > > > > > > > > > > > > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > > > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > > > > > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > > > > reported_key) > > > > > > > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > > > > > > > return; > > > > > > > > > > > > > > + if (key->keycode == KEY_PROG4 && > > > > > > > !wmi_requires_smbios_request) + return; > > > > > > > + > > > > > > > > > > > > Here I would rather test against reported_key, not keycode. If > > > > > > somebody in future adds KEY_PROG4 for something else we will > > > > > > have problem... > > > > > > > > > > As 0xe025 is currently the only event we know about that should > > > > > be ignored on some machines and processed on others, this makes > > > > > sense, > > > > > > > > > > at least for now. If I change the first condition to: > > > > > reported_key == 0xe025 > > > > > > > > There will be need also change for 5/5 patch... > > > > > > Why? Are you aware of any model which sends a 0xe029 WMI event _and_ > > > generates an i8042 interrupt? If not, WMI event 0xe029 should always > > > be turned into a key event, as per the keymap. > > > > No, but your current patch 4/5 and 5/5 do that (because it checks > > KEY_PROG4). But if it is not needed, I'm happy because of one hook less. > > From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be > ignored, so the test would need to include both if it isn't using the common > keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not > the correct reading of 5/5? It's the other way round :) Perhaps explaining the issue once again will help. Until this patch series, dell-wmi was only "aware" of laptops which generate _both_ an i8042 interrupt and a WMI event when Dell Instant Launch is pressed. Thus, as the i8042 interrupt already caused a key event to be generated, there was no point in generating another one for the WMI event, hence the KE_IGNORE entry for 0xe025 in the keymap. Enter Vostro V131, which does _not_ generate an i8042 interrupt when Dell Instant Launch is pressed, yet still generates a WMI event. In other words, there was no way of generating a key event for Dell Instant Launch on that model without changing the relevant keymap entry into a KE_KEY one. However, I still needed to ensure that for most machines that WMI event would _not_ be turned into a key event. That's why patch 4/5 changes the keymap entry to a KE_KEY one, while also adding a conditional return to the key processing function, thus making sure that the previous behavior (ignoring WMI event 0xe025) is preserved on most machines. I used KEY_PROG4 in the conditional expression because a mapped keycode is also used in a comparison just above the code inserted by patch 4/5. For event 0xe029, however, the conditional expression added by patch 4/5 should always evaluate to false. Note that if we use KEY_PROG4 in the comparison, the second condition will be false; if we use 0xe025 in the comparison, the first condition will be false. The latter variant (suggested by Pali) will work fine until we hear of a model which generates _both_ WMI event 0xe029 and an i8042 interrupt upon pressing some hotkey. I was hoping the commit message for patch 4/5 would be clear enough to convey my intent, yet it seems I managed to confuse you. Maybe that's a sign that the commit message and/or code should be rephrased, but I can't be a judge on this one myself. Let me know what you think. Pali, if you think this is good enough as it is, please add your Reviewed-by. If you have doubts, I'm all ears.
On Wed, Mar 02, 2016 at 01:35:04PM +0100, Micha? K?pie? wrote: > > On Mon, Feb 29, 2016 at 09:56:03PM +0100, Pali Rohár wrote: > > > On Monday 29 February 2016 21:49:27 you wrote: > > > > > On Monday 29 February 2016 21:31:23 Micha? K?pie? wrote: I understand this better now, thank you for the additional explanation. > > > > > > > > diff --git a/drivers/platform/x86/dell-wmi.c > > > > > > > > b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 > > > > > > > > 100644 --- a/drivers/platform/x86/dell-wmi.c > > > > > > > > +++ b/drivers/platform/x86/dell-wmi.c > > > > > > > > @@ -111,7 +111,7 @@ static const struct key_entry > > > > > > > > dell_wmi_legacy_keymap[] __initconst = { > > > > > > > > > > > > > > > > { KE_IGNORE, 0xe020, { KEY_MUTE } }, > > > > > > > > > > > > > > > > /* Shortcut and audio panel keys */ > > > > > > > > > > > > > > > > - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, > > > > > > > > + { KE_KEY, 0xe025, { KEY_PROG4 } }, > > > > > > > > Your description below helped explain why the KE_KEY change was necessary, the commit message didn't do that for me. Just explicitly stating "when there is no i8042 interrupt, the WMI even must generate a valid KE_KEY" or something along those lines would help. > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > > > > > reported_key) > > > > > > > > > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > > > > > > > > > return; > > > > > > > > > > > > > > > > + if (key->keycode == KEY_PROG4 && > > > > > > > > !wmi_requires_smbios_request) + return; > > > > > > > > + > > > > > > > > > > > > > > Here I would rather test against reported_key, not keycode. If > > > > > > > somebody in future adds KEY_PROG4 for something else we will > > > > > > > have problem... And ultimately, that is under our control. So let's just not do that :-) A comment by the definition of KEY_PROG4 that notes it's meaning in this driver should prevent any future attempts at overloading it and breaking this. > > > > > > > > > > > > As 0xe025 is currently the only event we know about that should > > > > > > be ignored on some machines and processed on others, this makes > > > > > > sense, > > > > > > > > > > > > at least for now. If I change the first condition to: > > > > > > reported_key == 0xe025 > > > > > > > > > > There will be need also change for 5/5 patch... > > > > > > > > Why? Are you aware of any model which sends a 0xe029 WMI event _and_ > > > > generates an i8042 interrupt? If not, WMI event 0xe029 should always > > > > be turned into a key event, as per the keymap. > > > > > > No, but your current patch 4/5 and 5/5 do that (because it checks > > > KEY_PROG4). But if it is not needed, I'm happy because of one hook less. > > > > From my reading, patch 5/5 adds 0xe029 to the reported keys that need to be > > ignored, so the test would need to include both if it isn't using the common > > keycode KEY_PROG4. I believe that is what Pali is saying as well. Is this not > > the correct reading of 5/5? > > It's the other way round :) Perhaps explaining the issue once again > will help. Got it! Thanks! A couple of comments and I think this is a reasonable solution. > > Until this patch series, dell-wmi was only "aware" of laptops which > generate _both_ an i8042 interrupt and a WMI event when Dell Instant > Launch is pressed. Thus, as the i8042 interrupt already caused a key > event to be generated, there was no point in generating another one for > the WMI event, hence the KE_IGNORE entry for 0xe025 in the keymap. > > Enter Vostro V131, which does _not_ generate an i8042 interrupt when > Dell Instant Launch is pressed, yet still generates a WMI event. In > other words, there was no way of generating a key event for Dell Instant > Launch on that model without changing the relevant keymap entry into a > KE_KEY one. However, I still needed to ensure that for most machines > that WMI event would _not_ be turned into a key event. That's why patch > 4/5 changes the keymap entry to a KE_KEY one, while also adding a > conditional return to the key processing function, thus making sure that > the previous behavior (ignoring WMI event 0xe025) is preserved on most > machines. > > I used KEY_PROG4 in the conditional expression because a mapped keycode > is also used in a comparison just above the code inserted by patch 4/5. > For event 0xe029, however, the conditional expression added by patch 4/5 > should always evaluate to false. Note that if we use KEY_PROG4 in the > comparison, the second condition will be false; if we use 0xe025 in the > comparison, the first condition will be false. The latter variant > (suggested by Pali) will work fine until we hear of a model which > generates _both_ WMI event 0xe029 and an i8042 interrupt upon pressing > some hotkey. > > I was hoping the commit message for patch 4/5 would be clear enough to > convey my intent, yet it seems I managed to confuse you. Maybe that's a > sign that the commit message and/or code should be rephrased, but I > can't be a judge on this one myself. Let me know what you think. Pali, > if you think this is good enough as it is, please add your Reviewed-by. > If you have doubts, I'm all ears. > > -- > Best regards, > Micha? K?pie? >
> Your description below helped explain why the KE_KEY change was necessary, the > commit message didn't do that for me. Just explicitly stating "when there is no > i8042 interrupt, the WMI even must generate a valid KE_KEY" or something along > those lines would help. I will do that in v5, then. > > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > > > > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > > > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > > > > > > reported_key) > > > > > > > > > > > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > > > > > > > > > > > return; > > > > > > > > > > > > > > > > > > + if (key->keycode == KEY_PROG4 && > > > > > > > > > !wmi_requires_smbios_request) + return; > > > > > > > > > + > > > > > > > > > > > > > > > > Here I would rather test against reported_key, not keycode. If > > > > > > > > somebody in future adds KEY_PROG4 for something else we will > > > > > > > > have problem... > > And ultimately, that is under our control. So let's just not do that :-) > > A comment by the definition of KEY_PROG4 that notes it's meaning in this driver > should prevent any future attempts at overloading it and breaking this. As I'll be sending a v5 anyway, do you think Pali's idea is bad? Personally, I'm leaning towards it. IMHO comparing against reported_key would emphasize the fact that only event 0xe025 is "special" and chances are that there are no other WMI event codes which need to be handled this way.
On Thu, Mar 03, 2016 at 07:46:32PM +0100, Micha? K?pie? wrote: > > Your description below helped explain why the KE_KEY change was necessary, the > > commit message didn't do that for me. Just explicitly stating "when there is no > > i8042 interrupt, the WMI even must generate a valid KE_KEY" or something along > > those lines would help. > > I will do that in v5, then. > > > > > > > > > > > { KE_IGNORE, 0xe026, { KEY_RESERVED } }, > > > > > > > > > > > > > > > > > > > > { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, > > > > > > > > > > > > > > > > > > > > @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int > > > > > > > > > > reported_key) > > > > > > > > > > > > > > > > > > > > acpi_video_handles_brightness_key_presses()) > > > > > > > > > > > > > > > > > > > > return; > > > > > > > > > > > > > > > > > > > > + if (key->keycode == KEY_PROG4 && > > > > > > > > > > !wmi_requires_smbios_request) + return; > > > > > > > > > > + > > > > > > > > > > > > > > > > > > Here I would rather test against reported_key, not keycode. If > > > > > > > > > somebody in future adds KEY_PROG4 for something else we will > > > > > > > > > have problem... > > > > And ultimately, that is under our control. So let's just not do that :-) > > > > A comment by the definition of KEY_PROG4 that notes it's meaning in this driver > > should prevent any future attempts at overloading it and breaking this. > > As I'll be sending a v5 anyway, do you think Pali's idea is bad? > Personally, I'm leaning towards it. IMHO comparing against reported_key > would emphasize the fact that only event 0xe025 is "special" and chances > are that there are no other WMI event codes which need to be handled > this way. I took another look at the discussion on 5/5 regarding event 0xe029, and after that, no, no objection from me. Go ahead with Pali's recommendation to test against reported_key.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index 65edd93..ffc957b5 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -111,7 +111,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] __initconst = { { KE_IGNORE, 0xe020, { KEY_MUTE } }, /* Shortcut and audio panel keys */ - { KE_IGNORE, 0xe025, { KEY_RESERVED } }, + { KE_KEY, 0xe025, { KEY_PROG4 } }, { KE_IGNORE, 0xe026, { KEY_RESERVED } }, { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } }, @@ -235,6 +235,9 @@ static void dell_wmi_process_key(int reported_key) acpi_video_handles_brightness_key_presses()) return; + if (key->keycode == KEY_PROG4 && !wmi_requires_smbios_request) + return; + sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true); }
On models on which an SMBIOS request needs to be issued in order for WMI events to be generated, pressing the Dell Instant Launch hotkey does not raise an i8042 interrupt - only a WMI event is generated (0xe025 on Dell Vostro V131). Thus, the 0xe025 event should only be ignored on machines which do not require an SMBIOS request for enabling WMI. Signed-off-by: Micha? K?pie? <kernel@kempniu.pl> --- drivers/platform/x86/dell-wmi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)