Re: [PATCH] topstar_acpi: add new driver for hotkeys support on Topstar N01

[Herton Ronaldo Krzesinski <herton@mandriva.com.br>]

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
> 

-- 
[]'s
Herton
--
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