On Thu, Dec 09, 2010 at 10:15:28PM +0100, Corentin Chary wrote: > Hi, > > On Thu, Dec 9, 2010 at 9:53 PM, Thadeu Lima de Souza Cascardo > <cascardo@xxxxxxxxxxxxxx> wrote: > > This driver supports some keys for Megaware notebook models. This particular > > driver was sponsored by Ulevel <http://ulevel.com>. > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx> > > --- > > > > Perhaps, some function names are a little odd or some of the functions > > should be open-coded instead. Comments, please? > > > > Note that it depends on both the last fix for WMI and the recent touchpad keys > > in input.h. > > > > Thanks, > > Cascardo. > > > > --- > > drivers/platform/x86/Kconfig | 8 ++ > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/megaware-wmi.c | 226 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 235 insertions(+), 0 deletions(-) > > create mode 100644 drivers/platform/x86/megaware-wmi.c > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index 4c7f8b9..9e20ee0 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -651,4 +651,12 @@ config XO1_RFKILL > > Support for enabling/disabling the WLAN interface on the OLPC XO-1 > > laptop. > > > > +config MEGAWARE_WMI > > + tristate "Megaware WMI input keys" > > + depends on ACPI_WMI > > + select INPUT > > depends on INPUT, only use select for small stuff. > > > + select INPUT_SPARSEKMAP > > + ---help--- > > + This adds support for the keys in some Megaware notebook models. > > + > > endif # X86_PLATFORM_DEVICES > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index 4ec4ff8..e7942f8 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -34,3 +34,4 @@ obj-$(CONFIG_INTEL_IPS) += intel_ips.o > > obj-$(CONFIG_GPIO_INTEL_PMIC) += intel_pmic_gpio.o > > obj-$(CONFIG_XO1_RFKILL) += xo1-rfkill.o > > obj-$(CONFIG_IBM_RTL) += ibm_rtl.o > > +obj-$(CONFIG_MEGAWARE_WMI) += megaware-wmi.o > > diff --git a/drivers/platform/x86/megaware-wmi.c b/drivers/platform/x86/megaware-wmi.c > > new file mode 100644 > > index 0000000..0ea0dac > > --- /dev/null > > +++ b/drivers/platform/x86/megaware-wmi.c > > @@ -0,0 +1,226 @@ > > +/* > > + * Copyright (C) 2010 Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License along > > + * with this program; if not, write to the Free Software Foundation, Inc., > > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/slab.h> > > +#include <linux/input.h> > > +#include <linux/input/sparse-keymap.h> > > +#include <linux/platform_device.h> > > +#include <linux/acpi.h> > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx>"); > > + > > +#define MEGAWARE_DRIVER_NAME "megaware" > > + > > +#define MEGAWARE_GUID_CODE "39142400-C6A3-40FA-BADB-8A2652834100" > > +#define MEGAWARE_GUID_EVENT "59142400-C6A3-40FA-BADB-8A2652834100" > > +#define MEGAWARE_GUID_INIT "79142400-C6A3-40FA-BADB-8A2652834100" > > + > > +MODULE_ALIAS("wmi:"MEGAWARE_GUID_INIT); > > + > > +struct megaware_device { > > + struct input_dev *idev; > > +}; > > + > > +static acpi_status megaware_call_wq00(acpi_handle handle, > > + unsigned long long *ret) > > +{ > > + struct acpi_buffer output; > > + union acpi_object *obj; > > + acpi_status status; > > add an empty line between variable declaration and code ? > I don't know if it's in the coding style, but I find it more readable. > Apply this comment to all functions. Yes, separating declarations and code is preferred style. In general, judicious use of empty lines to group related statements is very welcome. > > > + output.length = ACPI_ALLOCATE_BUFFER; > > + output.pointer = NULL; > > + status = wmi_query_block(MEGAWARE_GUID_CODE, 1, &output); > > + if (status) > > + return status; > > + obj = output.pointer; > > + if (obj && obj->type == ACPI_TYPE_INTEGER) > > + *ret = obj->integer.value; > > + else > > + status = AE_TYPE; > > + kfree(obj); > > + return status; > > +} > > + > > +static acpi_status megaware_call_init(struct megaware_device *megaware) > > +{ > > + struct acpi_buffer input; > > + unsigned long long value = 0; > > + acpi_status status; > > + input.length = sizeof(value); > > + input.pointer = &value; > > + status = wmi_evaluate_method(MEGAWARE_GUID_INIT, 1, 0, &input, NULL); > > + return status; Could probably be written as: unsigned long long value = 0; struct acpi_buffer input = { .length = sizeof(value), .pointer = &value, }; return wmi_evaluate_method(MEGAWARE_GUID_INIT, 1, 0, &input, NULL); > > +} > > + > > +static const struct key_entry megaware_keys[] = { > > + { KE_KEY, 0x100, { KEY_WLAN } }, > > + { KE_KEY, 0x101, { KEY_BLUETOOTH } }, > > + { KE_KEY, 0x102, { KEY_PROG1 } }, > > Maybe you could add a comment to describe the PROG1 key ? > > > + { KE_KEY, 0x107, { KEY_SWITCHVIDEOMODE } }, > > + { KE_KEY, 0x108, { KEY_TOUCHPAD_TOGGLE } }, > > + { KE_KEY, 0x109, { KEY_MUTE } }, > > + { KE_KEY, 0x10A, { KEY_VOLUMEUP } }, > > + { KE_KEY, 0x10B, { KEY_VOLUMEDOWN } }, > > + { KE_END, } > > +}; > > + > > +static int megaware_add(struct platform_device *dev) Can also be __devinit. And why don't you use struct megaware_device * as the argument instead of platform device? And call it megaware_input_init()? > > +{ > > + struct megaware_device *megaware = dev_get_drvdata(&dev->dev); > > + struct input_dev *inputdev; > > + int error; > > + acpi_status status; > > + > > + status = megaware_call_init(megaware); > > + if (!ACPI_SUCCESS(status)) > > + return -EIO; > > + > > + inputdev = input_allocate_device(); > > + if (!inputdev) > > + return -ENOMEM; > > + inputdev->name = MEGAWARE_DRIVER_NAME "_keys"; > > + inputdev->dev.parent = &dev->dev; > > + > > + error = sparse_keymap_setup(inputdev, megaware_keys, NULL); > > + if (error) > > + goto err_alloc; > > + > > + error = input_register_device(inputdev); > > + if (error) > > + goto err_keymap; > > + megaware->idev = inputdev; > > + return 0; > > + > > +err_keymap: > > + sparse_keymap_free(inputdev); > > +err_alloc: > > + input_free_device(inputdev); > > + return error; > > +} > > + > > +static int megaware_del(struct platform_device *dev) > > +{ Same here, struct megaware_device * as an argument would make more sense. > > + struct megaware_device *megaware = dev_get_drvdata(&dev->dev); > > + struct input_dev *inputdev = megaware->idev; > > + sparse_keymap_free(inputdev); > > + input_unregister_device(inputdev); > > + return 0; > > +} > > + > > +static void megaware_notify(u32 event, void *data) > > +{ > > + struct megaware_device *megaware = data; > > + struct input_dev *inputdev = megaware->idev; > > + acpi_status status; > > + unsigned long long value = 0; > > + if (event != 0x80) > > + return; > > + status = megaware_call_wq00(megaware, &value); > > + if (!ACPI_SUCCESS(status)) > > + return; > > + sparse_keymap_report_event(inputdev, value, 1, true); > > Add a trace for non-mapped keys ? Will help to support new models. > > > +} > > + > > +static __devinit int megaware_probe(struct platform_device *dev) > > +{ > > + struct megaware_device *megaware = dev_get_drvdata(&dev->dev); > > + int ret; > > + acpi_status status; > > + ret = megaware_add(dev); > > + if (ret) > > + return ret; > > + status = wmi_install_notify_handler(MEGAWARE_GUID_EVENT, > > + megaware_notify, megaware); > > + if (ACPI_FAILURE(status)) { > > + megaware_del(dev); > > + ret = -EIO; > > + } > > + return ret; > > +} > > + > > +static __devexit int megaware_remove(struct platform_device *dev) > > +{ > > + int ret; > > + wmi_remove_notify_handler(MEGAWARE_GUID_EVENT); > > + ret = megaware_del(dev); > > + return ret; > > +} > > + > > +static struct platform_driver megaware_driver = { > > + .driver = { > > + .owner = THIS_MODULE, > > + .name = MEGAWARE_DRIVER_NAME, > > + }, > > I'd suggest that you don't use .probe / .remove, because if probe fail you > won't know about it and the module will still be loaded. .remove is still needed but I guess Corentin refers to using platform_driver_probe() instead of platform_driver_register() so the megaware_probe can be marked __init and discarded after driver initialization has been completed. > > I recently sent a patch for eeepc-wmi (it's in linux-next) to fix the > same issue, calling > the megaware_add like function directly in the module init function. > > Another way to go is to use platform_driver_probe/platform_create_bundle. > > > + .probe = megaware_probe, > > + .remove = __devexit_p(megaware_remove), > > +}; > > + > > +static struct platform_device *megaware_device; > > + > > +static __init int megaware_init(void) > > +{ > > + struct megaware_device *megaware; > > + int ret; > > + if (!wmi_has_guid(MEGAWARE_GUID_INIT)) { > > Maybe you could also check MEGAWARE_GUID_CODE here ? > Or even the three GUID ? (but the event guid should already be checked > when installing the notify handler). > > > + printk(KERN_WARNING "Could not find megaware WMI device.\n"); > > Use pr_* functions instead of printk ? This way it will add a prefix > automatically. > > > + ret = -ENODEV; > > + goto out_guid; > > + } > > + megaware = kzalloc(sizeof(*megaware), GFP_KERNEL); > > + if (!megaware) { > > + ret = -ENOMEM; > > + goto out_alloc; > > + } I do not really see the point in dynamically allocating megaware structure since there can only be one instance of it. > > + megaware_device = platform_device_alloc(MEGAWARE_DRIVER_NAME, 0); > > + if (!megaware_device) { > > + ret = -ENOMEM; > > + goto out_devalloc; > > + } > > + ret = platform_device_add(megaware_device); > > + if (ret) > > + goto out_devadd; > > + dev_set_drvdata(&megaware_device->dev, megaware); > > + ret = platform_driver_register(&megaware_driver); > > + if (ret) > > + goto out_driver; > > + return 0; > > +out_driver: > > + platform_device_del(megaware_device); > > +out_devadd: > > + platform_device_put(megaware_device); > > +out_devalloc: > > + kfree(megaware); > > +out_alloc: > > +out_guid: > > + return ret; > > +} > > + > > +static __exit void megaware_exit(void) > > +{ > > + struct megaware_device *megaware; > > + platform_driver_unregister(&megaware_driver); > > + megaware = dev_get_drvdata(&megaware_device->dev); > > + platform_device_unregister(megaware_device); > > + kfree(megaware); > > +} > > + > > +module_init(megaware_init); > > +module_exit(megaware_exit); > > -- > > 1.7.2.3 > > Otherwise, this looks like a clean wmi driver :) > > Thanks. > > > -- > Corentin Chary > http://xf.iksaif.net > -- > To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
