On 9/7/09, Herton Ronaldo Krzesinski <herton@xxxxxxxxxxxxxxx> wrote: > Em Sáb 05 Set 2009, às 10:02:32, Alan Jenkins escreveu: >> On 9/5/09, Herton Ronaldo Krzesinski <herton@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> >> >> > +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@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx> Great! Reviewed-by: Alan Jenkins <alan-jenkins@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
