On Fri, Dec 10, 2010 at 1:08 AM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > 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. Also, register won't return any error code if the probe function failed, and the driver will be left in a strange state. Calling directly platform_driver_probe() will return an error code. But since it's a singleton device, and since the real "probe" code is already in the module_init function (wmi_has_guid()) call, using the probe callback to initialize data structures doesn't seems to make sense. >> >> 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 > -- 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
