Re: [PATCH] Enable Dell All-In-One volume up/down keys

[Colin Ian King]

 Thanks for your feedback Dmitry,

On Wed, 2011-02-16 at 23:52 -0800, Dmitry Torokhov wrote:
> Hi Colin,
> 
> On Mon, Feb 14, 2011 at 01:02:04PM +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> > 
> > Enable volume up and down hotkeys on WMI events
> > GUID 284A0E6B-380E-472A-921F-E52786257FB and
> > GUID 02314822-307C-4F66-bf0E-48AEAEB26CC8.
> > 
> > Also works around a firmware bug where the _WED method
> > should return an integer containing the key code and in fact
> > the method returns the key code in element zero of a buffer.
> > 
> > BugLink: http://bugs.launchpad.net/bugs/701530
> > BugLink: http://bugs.launchpad.net/bugs/676997
> 
> Looks generally good, one question though - should't it be part of
> dell-wmi driver?

Good question.  A couple of points:

1. The Dell All-In-One WMI hotkey mechanism is a little different from
the
normal Dell WMI hotkey mechanism, so I didn't really want to clutter up
the
Dell WMI driver with exceptions for these particular GUIDs and notifier
IDs.

2. These keys appear on a very limited subset of Dell machines, so
keeping
this code out of the Dell WMI driver for the majority of Dell machines
will
keep the original Dell WMI driver smaller. 

3. I expect further additions in functionality for these machines, which
makes sense to keep it specific to this limited All-In-One driver.

As it stands, this All-In-One interface has already been implemented by
two different BIOS vendors and already there are two different
implementations. I'd like to keep this all in one specific driver for
this class of machine if possible.

> > +config DELL_WMI_AIO
> > +       tristate "WMI Hotkeys for Dell All-In-One series"
> > +       depends on ACPI_WMI
> > +       depends on INPUT
> 
> select INPUT_SPARSEKMAP
> 
> > +
> > +#define AIO_PREFIX	"dell-wmi-aio: "
> 
> Just use
> 
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> and drop AIO_PREFIX from messages.
> 
> > +
> > +MODULE_DESCRIPTION("WMI hotkeys driver for Dell All-In-One series");
> > +MODULE_LICENSE("GPL");
> > +
> > +#define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
> > +#define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
> > +
> > +static char *dell_wmi_aio_guids[] = {
> > +	EVENT_GUID1,
> > +	EVENT_GUID2,
> > +	NULL
> > +};
> > +
> > +MODULE_ALIAS("wmi:"EVENT_GUID1);
> > +MODULE_ALIAS("wmi:"EVENT_GUID2);
> > +
> > +static struct key_entry dell_wmi_aio_keymap[] = {
> 
> const
> 
> > +
> > +static char *dell_wmi_aio_find(void)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; dell_wmi_aio_guids[i] != NULL; i++)
> > +		if (wmi_has_guid(dell_wmi_aio_guids[i]))
> > +			return dell_wmi_aio_guids[i];
> > +
> > +	return NULL;
> > +}
> > +
> > +static int __init dell_wmi_aio_init(void)
> > +{
> > +	int err;
> > +	char *guid;
> > +
> > +	guid = dell_wmi_aio_find();
> > +	if (guid) {
> > +		err = dell_wmi_aio_input_setup();
> > +
> > +		if (err)
> > +			return err;
> > +
> > +		err = wmi_install_notify_handler(guid,
> > +			dell_wmi_aio_notify, NULL);
> > +		if (err) {
> > +			input_unregister_device(dell_wmi_aio_input_dev);
> > +			pr_err(AIO_PREFIX "Unable to register"
> > +			       " notify handler - %d\n", err);
> 
> Do not break the strings - 80-columr rule does not apply to messages.
> 
> > +			return err;
> > +		}
> > +	} else
> > +		pr_warning(AIO_PREFIX "No known WMI GUID found\n");
> > +
> > +	return 0;
> 
> If you did not find known guid you shold return -ENXIO so that the
> driver aborts loading into memory (if it is compiled as a module -
> likely).
> 
> > +}
> > +
> > +static void __exit dell_wmi_aio_exit(void)
> > +{
> > +	char *guid;
> > +
> > +	guid = dell_wmi_aio_find();
> 
> And if you fail loading you do not need to check it here.
> 
> > +	if (guid) {
> > +		wmi_remove_notify_handler(guid);
> > +		input_unregister_device(dell_wmi_aio_input_dev);
> > +	}
> > +}
> > +
> > +module_init(dell_wmi_aio_init);
> > +module_exit(dell_wmi_aio_exit);
> 
> Thanks,
> 
I took your recommendations and attached is the updated patch.

Thanks,

Colin