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

["Pali Rohár" <pali.rohar@gmail.com>]

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