On Friday 04 December 2009 11:15:06 Anisse Astier wrote: > 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? I wrote it from scratch. I started by taking over from hp-wmi, but later, beside of the key array setup, most things changed... > > 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? I got that from MSI people (one of the rare infos I got and that the irq storm is a "HW" bug/feature also showing up on Windows), they told me that's the values that should be used (and yes, probably it's the way Windows works then as well). > > +#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? Eh, DYNAMIC_PRINTK? grep DYNAMIC_PRINTK include/linux/ -r is empty... > > + kfree(obj); > Good thing you're freeing the allocated acpi objects. :) Yep, I found that in hp-wmi when looking closer at it. > > + 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. Do you see a problem with above? You mean ktime_get_real() reads our the time via get getnstimeofday and accessing IO, while your get_jiffies_64() solution reads out a kernel variable incremented via timer irq which is a better solution? While gettimeofday should be a fast TSC read on modern HW, I still have to agree. Testing this I saw ~5-25 IRQs per key hit, showing up in about a ~30ms time frame, therefore I took 50ms ignoring the same IRQ and it worked out fine. Your last_time_pressed timeout of 10ms might be a bit too short... Beside that (backlight) switching takes some time, but this should not be due to above "debouncing". > > +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. That would be great! > I could also adapt it for sparse keymaps as I did with the other driver > (would reduce code size). Cool. Thanks for the review, Thomas -- 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
