* [PATCH] Add WMI driver for some Megaware notebook models.
@ 2010-12-09 20:53 Thadeu Lima de Souza Cascardo
2010-12-09 21:15 ` Corentin Chary
0 siblings, 1 reply; 7+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2010-12-09 20:53 UTC (permalink / raw)
To: platform-driver-x86; +Cc: ceolin, Thadeu Lima de Souza Cascardo
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@holoscopio.com>
---
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
+ 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@holoscopio.com>
+ *
+ * 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@holoscopio.com>");
+
+#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;
+ 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;
+}
+
+static const struct key_entry megaware_keys[] = {
+ { KE_KEY, 0x100, { KEY_WLAN } },
+ { KE_KEY, 0x101, { KEY_BLUETOOTH } },
+ { KE_KEY, 0x102, { KEY_PROG1 } },
+ { 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)
+{
+ 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)
+{
+ 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);
+}
+
+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,
+ },
+ .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)) {
+ printk(KERN_WARNING "Could not find megaware WMI device.\n");
+ ret = -ENODEV;
+ goto out_guid;
+ }
+ megaware = kzalloc(sizeof(*megaware), GFP_KERNEL);
+ if (!megaware) {
+ ret = -ENOMEM;
+ goto out_alloc;
+ }
+ 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Add WMI driver for some Megaware notebook models.
2010-12-09 20:53 [PATCH] Add WMI driver for some Megaware notebook models Thadeu Lima de Souza Cascardo
@ 2010-12-09 21:15 ` Corentin Chary
2010-12-10 0:08 ` Dmitry Torokhov
0 siblings, 1 reply; 7+ messages in thread
From: Corentin Chary @ 2010-12-09 21:15 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: platform-driver-x86, ceolin
Hi,
On Thu, Dec 9, 2010 at 9:53 PM, Thadeu Lima de Souza Cascardo
<cascardo@holoscopio.com> 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@holoscopio.com>
> ---
>
> 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@holoscopio.com>
> + *
> + * 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@holoscopio.com>");
> +
> +#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.
> + 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;
> +}
> +
> +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)
> +{
> + 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)
> +{
> + 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.
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;
> + }
> + 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add WMI driver for some Megaware notebook models.
2010-12-09 21:15 ` Corentin Chary
@ 2010-12-10 0:08 ` Dmitry Torokhov
2010-12-10 7:12 ` Corentin Chary
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2010-12-10 0:08 UTC (permalink / raw)
To: Corentin Chary; +Cc: Thadeu Lima de Souza Cascardo, platform-driver-x86, ceolin
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@holoscopio.com> 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@holoscopio.com>
> > ---
> >
> > 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@holoscopio.com>
> > + *
> > + * 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@holoscopio.com>");
> > +
> > +#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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add WMI driver for some Megaware notebook models.
2010-12-10 0:08 ` Dmitry Torokhov
@ 2010-12-10 7:12 ` Corentin Chary
2010-12-10 7:31 ` Dmitry Torokhov
0 siblings, 1 reply; 7+ messages in thread
From: Corentin Chary @ 2010-12-10 7:12 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Thadeu Lima de Souza Cascardo, platform-driver-x86, ceolin
On Fri, Dec 10, 2010 at 1:08 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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@holoscopio.com> 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@holoscopio.com>
>> > ---
>> >
>> > 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@holoscopio.com>
>> > + *
>> > + * 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@holoscopio.com>");
>> > +
>> > +#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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Dmitry
>
--
Corentin Chary
http://xf.iksaif.net
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add WMI driver for some Megaware notebook models.
2010-12-10 7:12 ` Corentin Chary
@ 2010-12-10 7:31 ` Dmitry Torokhov
2010-12-10 7:53 ` Corentin Chary
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2010-12-10 7:31 UTC (permalink / raw)
To: Corentin Chary; +Cc: Thadeu Lima de Souza Cascardo, platform-driver-x86, ceolin
On Fri, Dec 10, 2010 at 08:12:53AM +0100, Corentin Chary wrote:
> On Fri, Dec 10, 2010 at 1:08 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > .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.
>
What's the other option (provided that we want to keep platform device)?
Manual binding device and driver? I think platform_driver_probe() is
better tested and is safer, emits all necessary uevents. etc, etc.
I'd suggest platform_create_bundle() except I am not quite happy with
the API at the moment (needs to also accept drvdata I think).
--
Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add WMI driver for some Megaware notebook models.
2010-12-10 7:31 ` Dmitry Torokhov
@ 2010-12-10 7:53 ` Corentin Chary
2010-12-10 8:09 ` Dmitry Torokhov
0 siblings, 1 reply; 7+ messages in thread
From: Corentin Chary @ 2010-12-10 7:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Thadeu Lima de Souza Cascardo, platform-driver-x86, ceolin
On Fri, Dec 10, 2010 at 8:31 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Dec 10, 2010 at 08:12:53AM +0100, Corentin Chary wrote:
>> On Fri, Dec 10, 2010 at 1:08 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> >
>> > .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.
>>
>
> What's the other option (provided that we want to keep platform device)?
> Manual binding device and driver? I think platform_driver_probe() is
> better tested and is safer, emits all necessary uevents. etc, etc.
>
> I'd suggest platform_create_bundle() except I am not quite happy with
> the API at the moment (needs to also accept drvdata I think).
>
Well, I'd suggest something like
http://git.kernel.org/?p=linux/kernel/git/mjg59/platform-drivers-x86.git;a=blob;f=drivers/platform/x86/eeepc-wmi.c;hb=linux-next#l887
It's like platform_create_bundle, but by hand, because I didn't like
the API too.
Please note that I'd gladly accept any comment/patch on eeepc-wmi if you
think it's not ok :).
Maybe the wmi_has_guid() calls should be in the probe callback,
and we should all call platform_driver_probe() ?
--
Corentin Chary
http://xf.iksaif.net
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add WMI driver for some Megaware notebook models.
2010-12-10 7:53 ` Corentin Chary
@ 2010-12-10 8:09 ` Dmitry Torokhov
0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2010-12-10 8:09 UTC (permalink / raw)
To: Corentin Chary; +Cc: Thadeu Lima de Souza Cascardo, platform-driver-x86, ceolin
On Fri, Dec 10, 2010 at 08:53:31AM +0100, Corentin Chary wrote:
> On Fri, Dec 10, 2010 at 8:31 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Dec 10, 2010 at 08:12:53AM +0100, Corentin Chary wrote:
> >> On Fri, Dec 10, 2010 at 1:08 AM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> >
> >> > .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.
> >>
> >
> > What's the other option (provided that we want to keep platform device)?
> > Manual binding device and driver? I think platform_driver_probe() is
> > better tested and is safer, emits all necessary uevents. etc, etc.
> >
> > I'd suggest platform_create_bundle() except I am not quite happy with
> > the API at the moment (needs to also accept drvdata I think).
> >
>
> Well, I'd suggest something like
> http://git.kernel.org/?p=linux/kernel/git/mjg59/platform-drivers-x86.git;a=blob;f=drivers/platform/x86/eeepc-wmi.c;hb=linux-next#l887
>
> It's like platform_create_bundle, but by hand, because I didn't like
> the API too.
>
> Please note that I'd gladly accept any comment/patch on eeepc-wmi if you
> think it's not ok :).
Well, I see one, more theoretical than practical issue here. Your
platform driver does not suppress bind/unbind via sysfs and when I
unbind a driver from a device I'd really expect children disappear. But
your implementation of eeepc-wmi it does not work this way.
>
> Maybe the wmi_has_guid() calls should be in the probe callback,
> and we should all call platform_driver_probe() ?
>
Yes, I think this would work.
--
Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-12-10 8:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-09 20:53 [PATCH] Add WMI driver for some Megaware notebook models Thadeu Lima de Souza Cascardo
2010-12-09 21:15 ` Corentin Chary
2010-12-10 0:08 ` Dmitry Torokhov
2010-12-10 7:12 ` Corentin Chary
2010-12-10 7:31 ` Dmitry Torokhov
2010-12-10 7:53 ` Corentin Chary
2010-12-10 8:09 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.