Em Ter 08 Set 2009, às 10:03:31, Alan Jenkins escreveu: > 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] 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 > -- []'s Herton -- 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
