Re: [PATCH v4 4/5] dell-wmi: properly process Dell Instant Launch hotkey

[Pali RohÃr]

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.

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.