Hi Thomas, On Thu, 3 Dec 2009 17:49:07 +0100, Thomas Renninger <trenn@xxxxxxx> wrote : > On Wednesday 02 December 2009 19:26:03 Anisse Astier wrote: > > > > Signed-off-by: Anisse Astier <anisse@xxxxxxxxx> > > --- > > Hi, > > > > This driver (intiated by > > http://sysmic.org/dotclear2/index.php?post/2009/06/02/83-msi-wmi-support) is > > aimed at supporting WMI based hotkeys on MSI Windtop all-in-one AE1900-WT. > I've already submitted a driver together with some other fixes a month ago. > Indeed, I am sorry I didn't see it earlier. Did you write it from scratch or base it on Jerome's patch? > My driver not only registers an input device for backlight/volume up, down, > but also registers a backlight driver for software based backlight switching. Indeed, and that's very nice! > > Not sure whether Len already submitted it to he's test branch, I hope so: > http://patchwork.kernel.org/patch/55944/ > > Would be great if you can give it a try. I gave it a try, it works great. Buttons works as expected, and the backlight interface works as well. Now I have a few questions/comments: > +static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF }; Why just 6 levels for backlight? Wasn't it possible to map the whole range? Does it work the same way in windows? > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/types.h> > +#include <linux/input.h> > +#include <acpi/acpi_drivers.h> > +#include <linux/acpi.h> > +#include <linux/string.h> > +#include <linux/hrtimer.h> > +#include <linux/backlight.h> Do you need all these #includes? (e.g string.h) > +#define dprintk(msg...) do { \ Isn't reimplementing some kind of dprintk a little outdated with DYNAMIC_PRINTK now in kernel? > + kfree(obj); Good thing you're freeing the allocated acpi objects. > + cur = ktime_get_real(); > + /* Ignore event if the same event happened in a 50 ms > + timeframe -> Key press may result in 10-20 GPEs */ > + if (ktime_to_us(ktime_sub(cur, key->last_pressed)) > + < 1000 * 50) { You're debouncing by computing the time difference instead of using a kernel timer. > +static int __init msi_wmi_init(void) You're init method could use some gotos to remove code duplication. (and merge some init with input_setup.) I would be happy to transform these comments into patches once I have the time, and if you don't mind. I could also adapt it for sparse keymaps as I did with the other driver (would reduce code size). Best Regards, Anisse -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
