| Message ID | 200909071626.03334.herton@mandriva.com.br (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 9/7/09, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote: > Em Sáb 05 Set 2009, às 10:02:32, Alan Jenkins escreveu: >> On 9/5/09, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote: >> > This adds Topstar Laptop Extras ACPI driver. It enables hotkeys >> > functionality with Topstar N01 netbook. Besides hotkeys there are >> > other functions exposed by its ACPI firmware, but for now only >> > hotkeys reporting on Topstar N01 is supported. Topstar is a chinese >> > manufacturer, its website can be currently reached at >> > http://www.topstardigital.cn/ >> > >> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br> >> >> > +static void __exit acpi_topstar_exit(void) >> > +{ >> > + acpi_bus_unregister_driver(&acpi_topstar_driver); >> > + >> > + printk(KERN_INFO "Topstar Laptop ACPI extras driver unloaded\n"); >> >> Oh... and this type of message always strikes me as gratuitous. rmmod >> doesn't happen during normal operation. So if the user removed the >> module... they know they did it. They can check lsmod if they forget >> and are not sure. >> >> If (as a developer) you want to be sure the acpi driver was really >> unregistered, you can (and probably should) look in sysfs. > > Hi, thanks for review. I addressed all issues you pointed out, and renamed > the > driver to topstar-laptop as suggested by Corentin Chary. I'll attach also > the > diff on top of previous topstar_acpi.c as reference (before the rename and > without Kconfig/Makefile changes). I only compiled tested it, as because of > bank holidays here will only be able to retest wednesday this, but the > changes > are safe anyway. Here is new version of the patch (feel free to add your > Reviewed-by to it): > > From e4fb36e01af04248e1a4ddda0964187e4b7fdb4a Mon Sep 17 00:00:00 2001 > From: Herton Ronaldo Krzesinski <herton@mandriva.com.br> > Date: Mon, 7 Sep 2009 16:12:48 -0300 > Subject: [PATCH] topstar-laptop: add new driver for hotkeys support on > Topstar N01 > > This adds Topstar Laptop Extras ACPI driver. It enables hotkeys > functionality with Topstar N01 netbook. Besides hotkeys there are > other functions exposed by its ACPI firmware, but for now only > hotkeys reporting on Topstar N01 is supported. Topstar is a chinese > manufacturer, its website can be currently reached at > http://www.topstardigital.cn/ > > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br> Great! Reviewed-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk> I found two more style points to bug you with. > + > +#include <linux/module.h> The other drivers explicitly include <linux.init.h> for __init. The LDD3 book says to do so as well. It also seems conventional to include <linux/kernel.h> for printk. (LDD3 thinks module.h is enough, but I disagree since module.h doesn't directly include kernel.h). > +#include <acpi/acpi_bus.h> > +#include <acpi/acpi_drivers.h> [whereas the different prefix for the ac_types.h etc suggested that we should get away without them] > + > +static int is_tps_dev; > + bool? "is_tps_dev" seems confused. I think the phrase should be either "is a tps _laptop_" or "_has_ a tps device". Perhaps "found_tps_dev" would be even better. > []'s > Herton And the same to you :-). Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Ter 08 Set 2009, às 10:03:31, Alan Jenkins escreveu: > On 9/7/09, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote: > > Em Sáb 05 Set 2009, às 10:02:32, Alan Jenkins escreveu: > >> On 9/5/09, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote: > >> > This adds Topstar Laptop Extras ACPI driver. It enables hotkeys > >> > functionality with Topstar N01 netbook. Besides hotkeys there are > >> > other functions exposed by its ACPI firmware, but for now only > >> > hotkeys reporting on Topstar N01 is supported. Topstar is a chinese > >> > manufacturer, its website can be currently reached at > >> > http://www.topstardigital.cn/ > >> > > >> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br> > >> > > >> > +static void __exit acpi_topstar_exit(void) > >> > +{ > >> > + acpi_bus_unregister_driver(&acpi_topstar_driver); > >> > + > >> > + printk(KERN_INFO "Topstar Laptop ACPI extras driver unloaded\n"); > >> > >> Oh... and this type of message always strikes me as gratuitous. rmmod > >> doesn't happen during normal operation. So if the user removed the > >> module... they know they did it. They can check lsmod if they forget > >> and are not sure. > >> > >> If (as a developer) you want to be sure the acpi driver was really > >> unregistered, you can (and probably should) look in sysfs. > > > > Hi, thanks for review. I addressed all issues you pointed out, and > > renamed the > > driver to topstar-laptop as suggested by Corentin Chary. I'll attach also > > the > > diff on top of previous topstar_acpi.c as reference (before the rename > > and without Kconfig/Makefile changes). I only compiled tested it, as > > because of bank holidays here will only be able to retest wednesday this, > > but the changes > > are safe anyway. Here is new version of the patch (feel free to add your > > Reviewed-by to it): > > > > From e4fb36e01af04248e1a4ddda0964187e4b7fdb4a Mon Sep 17 00:00:00 2001 > > From: Herton Ronaldo Krzesinski <herton@mandriva.com.br> > > Date: Mon, 7 Sep 2009 16:12:48 -0300 > > Subject: [PATCH] topstar-laptop: add new driver for hotkeys support on > > Topstar N01 > > > > This adds Topstar Laptop Extras ACPI driver. It enables hotkeys > > functionality with Topstar N01 netbook. Besides hotkeys there are > > other functions exposed by its ACPI firmware, but for now only > > hotkeys reporting on Topstar N01 is supported. Topstar is a chinese > > manufacturer, its website can be currently reached at > > http://www.topstardigital.cn/ > > > > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br> > > Great! > > Reviewed-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk> > > I found two more style points to bug you with. > > > + > > +#include <linux/module.h> > > The other drivers explicitly include <linux.init.h> for __init. The > LDD3 book says to do so as well. > > It also seems conventional to include <linux/kernel.h> for printk. > (LDD3 thinks module.h is enough, but I disagree since module.h doesn't > directly include kernel.h). > > > +#include <acpi/acpi_bus.h> > > +#include <acpi/acpi_drivers.h> > > [whereas the different prefix for the ac_types.h etc suggested that we > should get away without them] I will replace them with <linux/acpi>, seems better. I'll fix the other two and repost the patch. > > > + > > +static int is_tps_dev; > > + > > bool? > > "is_tps_dev" seems confused. I think the phrase should be either "is > a tps _laptop_" or "_has_ a tps device". Perhaps "found_tps_dev" > would be even better. > > > []'s > > Herton > > And the same to you :-). > Alan >
diff --git a/drivers/platform/x86/topstar_acpi.c b/drivers/platform/x86/topstar_acpi.c index e852de2..7be5b45 100644 --- a/drivers/platform/x86/topstar_acpi.c +++ b/drivers/platform/x86/topstar_acpi.c @@ -11,11 +11,11 @@ * published by the Free Software Foundation. */ -#include <linux/mod_devicetable.h> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/module.h> #include <acpi/acpi_bus.h> #include <acpi/acpi_drivers.h> -#include <acpi/actypes.h> -#include <acpi/acpixf.h> #include <linux/input.h> #define ACPI_TOPSTAR_HID "TPSACPI01" @@ -23,10 +23,6 @@ #define ACPI_TOPSTAR_DRIVER_NAME "Topstar laptop ACPI driver" #define ACPI_TOPSTAR_CLASS "topstar" -#define TOPSTAR_ACPI_NAME "topstar_acpi" -#define TOPSTAR_ACPI_ERR KERN_ERR TOPSTAR_ACPI_NAME ": " -#define TOPSTAR_ACPI_INFO KERN_INFO TOPSTAR_ACPI_NAME ": " - static const struct acpi_device_id topstar_device_ids[] = { { ACPI_TOPSTAR_HID, 0 }, { "", 0 }, @@ -79,12 +75,12 @@ static struct tps_key_entry *tps_get_key_by_keycode(int code) return NULL; } -static void acpi_topstar_notify(acpi_handle handle, u32 event, void *data) +static void acpi_topstar_notify(struct acpi_device *device, u32 event) { struct tps_key_entry *key; static bool dup_evnt[2]; bool *dup; - struct topstar_hkey *hkey = data; + struct topstar_hkey *hkey = device->driver_data; /* 0x83 and 0x84 key events comes duplicated... */ if (event == 0x83 || event == 0x84) { @@ -116,7 +112,7 @@ static void acpi_topstar_notify(acpi_handle handle, u32 event, void *data) if (event == 0x8e || event == 0x8f || event == 0x90) return; - printk(TOPSTAR_ACPI_INFO "unknown event = 0x%02x\n", event); + pr_info("unknown event = 0x%02x\n", event); } static int acpi_topstar_fncx_switch(struct acpi_device *device, bool state) @@ -132,7 +128,7 @@ static int acpi_topstar_fncx_switch(struct acpi_device *device, bool state) status = acpi_get_handle(device->handle, "FNCX", &handle); if (ACPI_FAILURE(status)) { - printk(TOPSTAR_ACPI_ERR "FNCX method not found\n"); + pr_err("FNCX method not found\n"); return -ENODEV; } fncx_params[0].integer.value = state ? 0x86 : 0x87; @@ -140,8 +136,7 @@ static int acpi_topstar_fncx_switch(struct acpi_device *device, bool state) buf.pointer = &obj; status = acpi_evaluate_object(handle, NULL, &fncx_arg_list, &buf); if (ACPI_FAILURE(status)) { - printk(TOPSTAR_ACPI_ERR - "Unable to switch FNCX notifications\n"); + pr_err("Unable to switch FNCX notifications\n"); return -ENODEV; } @@ -187,7 +182,7 @@ static int acpi_topstar_init_hkey(struct topstar_hkey *hkey) hkey->inputdev = input_allocate_device(); if (!hkey->inputdev) { - printk(TOPSTAR_ACPI_ERR "Unable to allocate input device\n"); + pr_err("Unable to allocate input device\n"); return -ENODEV; } hkey->inputdev->name = "Topstar Laptop extra buttons"; @@ -200,7 +195,7 @@ static int acpi_topstar_init_hkey(struct topstar_hkey *hkey) set_bit(key->keycode, hkey->inputdev->keybit); } if (input_register_device(hkey->inputdev)) { - printk(TOPSTAR_ACPI_ERR "Unable to register input device\n"); + pr_err("Unable to register input device\n"); input_free_device(hkey->inputdev); return -ENODEV; } @@ -208,9 +203,10 @@ static int acpi_topstar_init_hkey(struct topstar_hkey *hkey) return 0; } +static int is_tps_dev; + static int acpi_topstar_add(struct acpi_device *device) { - acpi_status status; struct topstar_hkey *tps_hkey; if (!device) @@ -231,11 +227,7 @@ static int acpi_topstar_add(struct acpi_device *device) if (acpi_topstar_init_hkey(tps_hkey)) goto add_err; - status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, - acpi_topstar_notify, tps_hkey); - if (ACPI_FAILURE(status)) - goto add_err; - + is_tps_dev = 1; return 0; add_err: @@ -251,9 +243,6 @@ static int acpi_topstar_remove(struct acpi_device *device, int type) if (!device || !tps_hkey) return -EINVAL; - acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, - acpi_topstar_notify); - acpi_topstar_fncx_switch(device, false); input_unregister_device(tps_hkey->inputdev); @@ -270,31 +259,38 @@ static struct acpi_driver acpi_topstar_driver = { .ops = { .add = acpi_topstar_add, .remove = acpi_topstar_remove, + .notify = acpi_topstar_notify, }, }; -static int __init acpi_topstar_init(void) +static int __init topstar_laptop_init(void) { + int ret; + if (acpi_disabled) return -ENODEV; - if (acpi_bus_register_driver(&acpi_topstar_driver) < 0) + ret = acpi_bus_register_driver(&acpi_topstar_driver); + if (ret < 0) + return ret; + + if (!is_tps_dev) { + acpi_bus_unregister_driver(&acpi_topstar_driver); return -ENODEV; + } printk(KERN_INFO "Topstar Laptop ACPI extras driver loaded\n"); return 0; } -static void __exit acpi_topstar_exit(void) +static void __exit topstar_laptop_exit(void) { acpi_bus_unregister_driver(&acpi_topstar_driver); - - printk(KERN_INFO "Topstar Laptop ACPI extras driver unloaded\n"); } -module_init(acpi_topstar_init); -module_exit(acpi_topstar_exit); +module_init(topstar_laptop_init); +module_exit(topstar_laptop_exit); MODULE_AUTHOR("Herton Ronaldo Krzesinski"); MODULE_DESCRIPTION("Topstar Laptop ACPI Extras driver");