On Wed, 2 Dec 2009 19:11:28 -0800, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote :
> Hi Anisse,
>
> On Wed, Dec 02, 2009 at 07:26:03PM +0100, Anisse Astier wrote:
> > +
> > + if (jiffies_to_msecs(get_jiffies_64() -
> > msi_wmi_time_last_press)
> > + > pression_timeout) {
>
> Why don't you use time_after() instead of manual computation?
>
> Also, what is the point of this? If you are trying to debounce the
> buttons this will not quite work. To do debouncing properly you need
> to store the value you just read and fire up a timer. When timer
> fires - that's the stable value.
Indeed, the point is to debounce the keys. I guess I’ll just use the
debounce mecanism in use in the gpio_keys driver.
But why use a timer instead of a delayed workqueue? Do we need the precison
of a timer for a simple debounce?
> > + printk(KERN_DEBUG
> > + "MSI WMI: event correctly
> > received: %llu\n",
> > + obj->integer.value);
>
> This is way too noisy for the mainline kernel, pr_debug() perhaps?
Sure.
> > + msi_wmi_input_setup();
>
>
> You need to handle errors returned by msi_wmi_input_setup() as well.
Yes, I reworked the init in the second patch, I'll put the clean init in
the first one for v2.
> > +MODULE_PARM_DESC(pression_timeout,
> > + "How much time interrupts are ignored between
> > each pression");
>
> This is not the best option name:
>
> MODULE_PARM_DESC(debounce_interval,
> "Controls how long driver will wait for button to
> debounce");
>
Thanks a lot for your comments.
Regards,
Anisse
--
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
