[RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Anisse Astier]

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
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.

This patch contains both WMI and DMI auto loading. Once Mathew Garrett's
patch for wmi autoloading is upstream (now in acpi-test), DMI auto loading
could (should?) be removed.

Also, Dmitry is in the process of factoring code for sparse keymaps. The 
second patch adds support its sparse keymap lib, removing a lot of code.

These patches are based on linux-input tree, and have been tested on the
hardware with and without the sparse keymap patch.

 drivers/platform/x86/Kconfig   |   11 ++
 drivers/platform/x86/Makefile  |    1 +
 drivers/platform/x86/msi-wmi.c |  212 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+), 0 deletions(-)
 create mode 100644 drivers/platform/x86/msi-wmi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1b27e4d..fe142af 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -441,4 +441,15 @@ config ACPI_TOSHIBA
 
 	  If you have a legacy free Toshiba laptop (such as the Libretto L1
 	  series), say Y.
+
+config MSI_WMI
+	tristate "MSI WMI extras"
+	depends on ACPI_WMI
+	depends on INPUT
+	---help---
+	 Say Y here if you want to support WMI-based hotkeys on MSI all-in-one
+	 WindTop AE1900-WT.
+
+	 To compile this driver as a module, choose M here: the module will
+	 be called msi-wmi.
 endif # X86_PLATFORM_DEVICES
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index d1c1621..22a0c78 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_ACPI_WMI)		+= wmi.o
 obj-$(CONFIG_ACPI_ASUS)		+= asus_acpi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
+obj-$(CONFIG_MSI_WMI)		+= msi-wmi.o
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
new file mode 100644
index 0000000..fb6d5bd
--- /dev/null
+++ b/drivers/platform/x86/msi-wmi.c
@@ -0,0 +1,212 @@
+/*
+ * MSI WMI hotkeys
+ *
+ * Copyright (C) 2009 Jérôme Pouiller <jezz@sysmic.org>
+ *
+ * Portions based on hp-wmi.c:
+ * Copyright (C) 2008 Red Hat <mjg@redhat.com>
+ *
+ * Portions based on wistron_btns.c:
+ * Copyright (C) 2005 Miloslav Trmac <mitr@volny.cz>
+ * Copyright (C) 2005 Bernhard Rosenkraenzer <bero@arklinux.org>
+ * Copyright (C) 2005 Dmitry Torokhov <dtor@mail.ru>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/acpi.h>
+#include <linux/jiffies.h>
+
+#define MSIWMI_EVENT_GUID "b6f3eef2-3d2f-49dc-9de3-85bce18c62f2"
+
+struct key_entry {
+	u16 code;
+	u16 keycode;
+};
+
+static struct key_entry msi_wmi_keymap[] = {
+	/*Brightness keys should be used for a backlight driver*/
+	{208, KEY_BRIGHTNESSUP},
+	{209, KEY_BRIGHTNESSDOWN},
+	{210, KEY_VOLUMEUP},
+	{211, KEY_VOLUMEDOWN},
+	{0}
+};
+
+static struct input_dev *msi_wmi_input_dev;
+static unsigned long long msi_wmi_time_last_press;
+static unsigned pression_timeout = 10;
+
+static struct key_entry *msi_wmi_get_entry_by_scancode(int code)
+{
+	struct key_entry *key;
+
+	for (key = msi_wmi_keymap; key->code; key++)
+		if (code == key->code)
+			return key;
+
+	return NULL;
+}
+
+static struct key_entry *msi_wmi_get_entry_by_keycode(int keycode)
+{
+	struct key_entry *key;
+
+	for (key = msi_wmi_keymap; key->code; key++)
+		if (keycode == key->keycode)
+			return key;
+
+	return NULL;
+}
+
+static void msi_wmi_notify(u32 value, void *context)
+{
+	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
+	static struct key_entry *key;
+	union acpi_object *obj;
+
+	acpi_status ret;
+	ret = wmi_get_event_data(value, &response);
+	obj = (union acpi_object *)response.pointer;
+
+	if (!obj || obj->type != ACPI_TYPE_INTEGER) {
+		printk(KERN_INFO "MSI WMI: Unknown response received\n");
+		return;
+	}
+
+	if (jiffies_to_msecs(get_jiffies_64() - msi_wmi_time_last_press)
+			> pression_timeout) {
+		printk(KERN_DEBUG
+				"MSI WMI: event correctly received: %llu\n",
+				obj->integer.value);
+		msi_wmi_time_last_press = get_jiffies_64();
+		key = msi_wmi_get_entry_by_scancode(obj->integer.value);
+		input_report_key(msi_wmi_input_dev, key->keycode, 1);
+		input_sync(msi_wmi_input_dev);
+		input_report_key(msi_wmi_input_dev, key->keycode, 0);
+		input_sync(msi_wmi_input_dev);
+	}
+}
+
+static int msi_wmi_getkeycode(struct input_dev *dev, int scancode, int *keycode)
+{
+	struct key_entry *key = msi_wmi_get_entry_by_scancode(scancode);
+
+	if (key && key->code) {
+		*keycode = key->keycode;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int msi_wmi_setkeycode(struct input_dev *dev, int scancode, int keycode)
+{
+	struct key_entry *key;
+	int old_keycode;
+
+	if (keycode < 0 || keycode > KEY_MAX)
+		return -EINVAL;
+
+	key = msi_wmi_get_entry_by_scancode(scancode);
+	if (key && key->code) {
+		old_keycode = key->keycode;
+		key->keycode = keycode;
+		set_bit(keycode, dev->keybit);
+		if (!msi_wmi_get_entry_by_keycode(old_keycode))
+			clear_bit(old_keycode, dev->keybit);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int __init msi_wmi_input_setup(void)
+{
+	struct key_entry *key;
+	int err;
+
+	msi_wmi_input_dev = input_allocate_device();
+
+	msi_wmi_input_dev->name = "MSI WMI hotkeys";
+	msi_wmi_input_dev->phys = "wmi/input0";
+	msi_wmi_input_dev->id.bustype = BUS_HOST;
+	msi_wmi_input_dev->getkeycode = msi_wmi_getkeycode;
+	msi_wmi_input_dev->setkeycode = msi_wmi_setkeycode;
+
+	for (key = msi_wmi_keymap; key->code; key++) {
+		set_bit(EV_KEY, msi_wmi_input_dev->evbit);
+		set_bit(key->keycode, msi_wmi_input_dev->keybit);
+	}
+
+	err = input_register_device(msi_wmi_input_dev);
+
+	if (err)
+		input_free_device(msi_wmi_input_dev);
+
+	return err;
+}
+
+static int __init msi_wmi_init(void)
+{
+	int err;
+
+	msi_wmi_time_last_press = get_jiffies_64();
+	if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
+		printk(KERN_ERR
+		       "This machine doesn't have MSI-hotkeys through WMI\n");
+		goto load_error;
+	}
+	err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
+					 msi_wmi_notify, NULL);
+	if (err) {
+		printk(KERN_ERR
+		       "MSI WMI: Error while installing notify handler\n");
+		goto load_error;
+	}
+
+	msi_wmi_input_setup();
+
+	return 0;
+load_error:
+	return -ENODEV;
+}
+
+static void __exit msi_wmi_exit(void)
+{
+	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
+		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+		input_unregister_device(msi_wmi_input_dev);
+	}
+}
+
+module_init(msi_wmi_init);
+module_exit(msi_wmi_exit);
+module_param(pression_timeout, uint, 0);
+
+MODULE_AUTHOR("Jérôme Pouiller <jezz@sysmic.org>");
+MODULE_AUTHOR("Michael Bouchaud <michael@substantiel.fr");
+MODULE_AUTHOR("Anisse Astier <anisse@astier.eu");
+MODULE_DESCRIPTION("MSI all-in-one WMI hotkeys driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
+/*This one is useless if there is wmi-autoloading*/
+MODULE_ALIAS("dmi:bvnMICRO-STARINTERNATIONALCO.,LTD:*:svnMICRO-STARINTERNATIO"
+	     "NALCO.,LTD:pnMS-6638:*:rnMS-7438:*:cvnMICRO-STARINTERNATIONALCO."
+	     ",LTD:*");
+MODULE_PARM_DESC(pression_timeout,
+		 "How much time interrupts are ignored between each pression");
-- 
1.6.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 2/2] Input: msi-wmi - switch to using sparse keymap library

[Anisse Astier]

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
 drivers/platform/x86/Kconfig   |    1 +
 drivers/platform/x86/msi-wmi.c |  123 +++++++++++-----------------------------
 2 files changed, 35 insertions(+), 89 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index fe142af..d6925ad 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -446,6 +446,7 @@ config MSI_WMI
 	tristate "MSI WMI extras"
 	depends on ACPI_WMI
 	depends on INPUT
+	select INPUT_SPARSEKMAP
 	---help---
 	 Say Y here if you want to support WMI-based hotkeys on MSI all-in-one
 	 WindTop AE1900-WT.
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index fb6d5bd..5dcb6fb 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -1,6 +1,7 @@
 /*
  * MSI WMI hotkeys
  *
+ * Copyright (C) 2009 Anisse Astier <anisse@astier.eu>
  * Copyright (C) 2009 Jérôme Pouiller <jezz@sysmic.org>
  *
  * Portions based on hp-wmi.c:
@@ -28,51 +29,25 @@
 
 #include <linux/kernel.h>
 #include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
 #include <linux/jiffies.h>
 
 #define MSIWMI_EVENT_GUID "b6f3eef2-3d2f-49dc-9de3-85bce18c62f2"
 
-struct key_entry {
-	u16 code;
-	u16 keycode;
-};
-
-static struct key_entry msi_wmi_keymap[] = {
+static const struct key_entry msi_wmi_keymap[] = {
 	/*Brightness keys should be used for a backlight driver*/
-	{208, KEY_BRIGHTNESSUP},
-	{209, KEY_BRIGHTNESSDOWN},
-	{210, KEY_VOLUMEUP},
-	{211, KEY_VOLUMEDOWN},
-	{0}
+	{KE_KEY, 208, {KEY_BRIGHTNESSUP} },
+	{KE_KEY, 209, {KEY_BRIGHTNESSDOWN} },
+	{KE_KEY, 210, {KEY_VOLUMEUP} },
+	{KE_KEY, 211, {KEY_VOLUMEDOWN} },
+	{KE_END, 0}
 };
 
 static struct input_dev *msi_wmi_input_dev;
 static unsigned long long msi_wmi_time_last_press;
 static unsigned pression_timeout = 10;
 
-static struct key_entry *msi_wmi_get_entry_by_scancode(int code)
-{
-	struct key_entry *key;
-
-	for (key = msi_wmi_keymap; key->code; key++)
-		if (code == key->code)
-			return key;
-
-	return NULL;
-}
-
-static struct key_entry *msi_wmi_get_entry_by_keycode(int keycode)
-{
-	struct key_entry *key;
-
-	for (key = msi_wmi_keymap; key->code; key++)
-		if (keycode == key->keycode)
-			return key;
-
-	return NULL;
-}
-
 static void msi_wmi_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -94,7 +69,8 @@ static void msi_wmi_notify(u32 value, void *context)
 				"MSI WMI: event correctly received: %llu\n",
 				obj->integer.value);
 		msi_wmi_time_last_press = get_jiffies_64();
-		key = msi_wmi_get_entry_by_scancode(obj->integer.value);
+		key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
+				obj->integer.value);
 		input_report_key(msi_wmi_input_dev, key->keycode, 1);
 		input_sync(msi_wmi_input_dev);
 		input_report_key(msi_wmi_input_dev, key->keycode, 0);
@@ -102,42 +78,8 @@ static void msi_wmi_notify(u32 value, void *context)
 	}
 }
 
-static int msi_wmi_getkeycode(struct input_dev *dev, int scancode, int *keycode)
-{
-	struct key_entry *key = msi_wmi_get_entry_by_scancode(scancode);
-
-	if (key && key->code) {
-		*keycode = key->keycode;
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
-static int msi_wmi_setkeycode(struct input_dev *dev, int scancode, int keycode)
-{
-	struct key_entry *key;
-	int old_keycode;
-
-	if (keycode < 0 || keycode > KEY_MAX)
-		return -EINVAL;
-
-	key = msi_wmi_get_entry_by_scancode(scancode);
-	if (key && key->code) {
-		old_keycode = key->keycode;
-		key->keycode = keycode;
-		set_bit(keycode, dev->keybit);
-		if (!msi_wmi_get_entry_by_keycode(old_keycode))
-			clear_bit(old_keycode, dev->keybit);
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
 static int __init msi_wmi_input_setup(void)
 {
-	struct key_entry *key;
 	int err;
 
 	msi_wmi_input_dev = input_allocate_device();
@@ -145,45 +87,48 @@ static int __init msi_wmi_input_setup(void)
 	msi_wmi_input_dev->name = "MSI WMI hotkeys";
 	msi_wmi_input_dev->phys = "wmi/input0";
 	msi_wmi_input_dev->id.bustype = BUS_HOST;
-	msi_wmi_input_dev->getkeycode = msi_wmi_getkeycode;
-	msi_wmi_input_dev->setkeycode = msi_wmi_setkeycode;
 
-	for (key = msi_wmi_keymap; key->code; key++) {
-		set_bit(EV_KEY, msi_wmi_input_dev->evbit);
-		set_bit(key->keycode, msi_wmi_input_dev->keybit);
-	}
+	err = sparse_keymap_setup(msi_wmi_input_dev, msi_wmi_keymap, NULL);
+	if (err)
+		goto err_free_dev;
 
+	err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
+					 msi_wmi_notify, NULL);
+	if (err) {
+		printk(KERN_ERR
+		       "MSI WMI: Error while installing notify handler\n");
+		goto err_free_keymap;
+	}
 	err = input_register_device(msi_wmi_input_dev);
-
 	if (err)
-		input_free_device(msi_wmi_input_dev);
+		goto err_uninstall_notifier;
+
+	return 0;
+
+err_uninstall_notifier:
+	wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+err_free_keymap:
+	sparse_keymap_free(msi_wmi_input_dev);
+err_free_dev:
+	input_free_device(msi_wmi_input_dev);
 
 	return err;
 }
 
 static int __init msi_wmi_init(void)
 {
-	int err;
+	int err = 0;
 
 	msi_wmi_time_last_press = get_jiffies_64();
 	if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
 		printk(KERN_ERR
 		       "This machine doesn't have MSI-hotkeys through WMI\n");
-		goto load_error;
-	}
-	err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
-					 msi_wmi_notify, NULL);
-	if (err) {
-		printk(KERN_ERR
-		       "MSI WMI: Error while installing notify handler\n");
-		goto load_error;
+		return -ENODEV;
 	}
 
-	msi_wmi_input_setup();
+	err = msi_wmi_input_setup();
 
-	return 0;
-load_error:
-	return -ENODEV;
+	return err;
 }
 
 static void __exit msi_wmi_exit(void)
-- 
1.6.5.3

--
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

Re: [RFC PATCH 2/2] Input: msi-wmi - switch to using sparse keymap library

[Dmitry Torokhov]

On Wed, Dec 02, 2009 at 07:28:12PM +0100, Anisse Astier wrote:
>  
>  static void __exit msi_wmi_exit(void)

Anisse,

It does not look like you are freeing sparse keymap when you
unnregistering input device, otherwise looks good.

-- 
Dmitry

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Dmitry Torokhov]

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.

> +		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?

> +		msi_wmi_time_last_press = get_jiffies_64();
> +		key = msi_wmi_get_entry_by_scancode(obj->integer.value);
> +		input_report_key(msi_wmi_input_dev, key->keycode, 1);
> +		input_sync(msi_wmi_input_dev);
> +		input_report_key(msi_wmi_input_dev, key->keycode, 0);
> +		input_sync(msi_wmi_input_dev);
> +	}
> +}
> +
> +static int msi_wmi_getkeycode(struct input_dev *dev, int scancode, int *keycode)
> +{
> +	struct key_entry *key = msi_wmi_get_entry_by_scancode(scancode);
> +
> +	if (key && key->code) {
> +		*keycode = key->keycode;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int msi_wmi_setkeycode(struct input_dev *dev, int scancode, int keycode)
> +{
> +	struct key_entry *key;
> +	int old_keycode;
> +
> +	if (keycode < 0 || keycode > KEY_MAX)
> +		return -EINVAL;
> +
> +	key = msi_wmi_get_entry_by_scancode(scancode);
> +	if (key && key->code) {
> +		old_keycode = key->keycode;
> +		key->keycode = keycode;
> +		set_bit(keycode, dev->keybit);
> +		if (!msi_wmi_get_entry_by_keycode(old_keycode))
> +			clear_bit(old_keycode, dev->keybit);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int __init msi_wmi_input_setup(void)
> +{
> +	struct key_entry *key;
> +	int err;
> +
> +	msi_wmi_input_dev = input_allocate_device();
> +
> +	msi_wmi_input_dev->name = "MSI WMI hotkeys";
> +	msi_wmi_input_dev->phys = "wmi/input0";
> +	msi_wmi_input_dev->id.bustype = BUS_HOST;
> +	msi_wmi_input_dev->getkeycode = msi_wmi_getkeycode;
> +	msi_wmi_input_dev->setkeycode = msi_wmi_setkeycode;
> +
> +	for (key = msi_wmi_keymap; key->code; key++) {
> +		set_bit(EV_KEY, msi_wmi_input_dev->evbit);
> +		set_bit(key->keycode, msi_wmi_input_dev->keybit);
> +	}
> +
> +	err = input_register_device(msi_wmi_input_dev);
> +
> +	if (err)
> +		input_free_device(msi_wmi_input_dev);
> +
> +	return err;
> +}
> +
> +static int __init msi_wmi_init(void)
> +{
> +	int err;
> +
> +	msi_wmi_time_last_press = get_jiffies_64();
> +	if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
> +		printk(KERN_ERR
> +		       "This machine doesn't have MSI-hotkeys through WMI\n");
> +		goto load_error;
> +	}
> +	err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
> +					 msi_wmi_notify, NULL);
> +	if (err) {
> +		printk(KERN_ERR
> +		       "MSI WMI: Error while installing notify handler\n");
> +		goto load_error;
> +	}
> +
> +	msi_wmi_input_setup();


You need to handle errors returned by msi_wmi_input_setup() as well.

> +
> +	return 0;
> +load_error:
> +	return -ENODEV;
> +}
> +
> +static void __exit msi_wmi_exit(void)
> +{
> +	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
> +		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
> +		input_unregister_device(msi_wmi_input_dev);
> +	}
> +}
> +
> +module_init(msi_wmi_init);
> +module_exit(msi_wmi_exit);
> +module_param(pression_timeout, uint, 0);
> +
> +MODULE_AUTHOR("Jérôme Pouiller <jezz@sysmic.org>");
> +MODULE_AUTHOR("Michael Bouchaud <michael@substantiel.fr");
> +MODULE_AUTHOR("Anisse Astier <anisse@astier.eu");
> +MODULE_DESCRIPTION("MSI all-in-one WMI hotkeys driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
> +/*This one is useless if there is wmi-autoloading*/
> +MODULE_ALIAS("dmi:bvnMICRO-STARINTERNATIONALCO.,LTD:*:svnMICRO-STARINTERNATIO"
> +	     "NALCO.,LTD:pnMS-6638:*:rnMS-7438:*:cvnMICRO-STARINTERNATIONALCO."
> +	     ",LTD:*");
> +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");

-- 
Dmitry
--
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

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Anisse Astier]

On Wed, 2 Dec 2009 19:11:28 -0800, Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Dmitry Torokhov]

On Thu, Dec 03, 2009 at 10:08:12AM +0100, Anisse Astier wrote:
> On Wed, 2 Dec 2009 19:11:28 -0800, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> 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?

Unlike timer workqueue will not be rescheduled if it is already pending.
mod_timer() will chnage the firing time.

-- 
Dmitry
--
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

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Thomas Renninger]

On Wednesday 02 December 2009 19:26:03 Anisse Astier wrote:
> 
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> ---
> 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.

My driver not only registers an input device for backlight/volume up, down,
but also registers a backlight driver for software based backlight switching.

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.

Thanks,

    Thomas

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Anisse Astier]

Hi Thomas,

On Thu, 3 Dec 2009 17:49:07 +0100, Thomas Renninger <trenn@suse.de>
wrote :

> On Wednesday 02 December 2009 19:26:03 Anisse Astier wrote:
> > 
> > Signed-off-by: Anisse Astier <anisse@astier.eu>
> > ---
> > 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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Thomas Renninger]

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@suse.de>
> wrote :
> 
> > On Wednesday 02 December 2009 19:26:03 Anisse Astier wrote:
> > > 
> > > Signed-off-by: Anisse Astier <anisse@astier.eu>
> > > ---
> > > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Anisse Astier]

On Fri, 4 Dec 2009 11:55:53 +0100, Thomas Renninger <trenn@suse.de> wrote :

> > 
> > > +#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...

My mistake, the option name is actually DYNAMIC_DEBUG, and is named "Enable
dynamic printk() support" in Kernel Hacking config.
cf Documentation/dynamic-debug-howto.txt

> > > +			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...
I see no problem with the above. It just works fine, whether you use
get_jiffies or a ktime. 

The thing Dmitry was proposing to do in his review to my patch, is to use a 
kernel timer (struct timer_list, mod_timer, etc.), and change the algorithm.
Instead  of having as a reference the first interrupt of (or bounce), you use
the last one:
Every time an interrupt is fired, you call mod_timer() to the kick the timer
further in time(say 20ms). Once you didn't have any interrupt for this given
period of time, it means your button is stabilized, and the timer will fire 
your report_input function.

You can see how it's done in drivers/input/keybourd/gpio_keys.c

> 
> Beside that (backlight) switching takes some time, but this should not
> be due to above "debouncing".
Yep, I noticed that as well.
I verified, backlight behave the same in Windows. Slow switch, and just 6
levels.


Anisse

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Thomas Renninger]

On Friday 04 December 2009 14:51:02 Anisse Astier wrote:
> On Fri, 4 Dec 2009 11:55:53 +0100, Thomas Renninger <trenn@suse.de> wrote :
> 
> > > 
> > > > +#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...
> 
> My mistake, the option name is actually DYNAMIC_DEBUG, and is named "Enable
> dynamic printk() support" in Kernel Hacking config.
> cf Documentation/dynamic-debug-howto.txt
Good catch.

> > > > +			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) {
> > > 
...
> I see no problem with the above. It just works fine, whether you use
> get_jiffies or a ktime.
> 
> The thing Dmitry was proposing to do in his review to my patch, is to use a 
> kernel timer (struct timer_list, mod_timer, etc.), and change the algorithm.
> Instead  of having as a reference the first interrupt of (or bounce), you use
> the last one:
> Every time an interrupt is fired, you call mod_timer() to the kick the timer
> further in time(say 20ms). Once you didn't have any interrupt for this given
> period of time, it means your button is stabilized, and the timer will fire 
> your report_input function.

I see these disadvantages:
  - It would be more complicated code
  - You have one extra irq (the one fired by your aimed timer which finally
    does the work)
  - You react after the timeout (+50ms delay).
The latter two are not sever.

The advantage is:
  - If you get constant irqs over the 30ms/50ms timeout, you would still
    only switch once.

But in this HW case, it really should be save with a 50ms timeout and I'd vote
to take the ktime or get_jiffies approach.

> You can see how it's done in drivers/input/keybourd/gpio_keys.c
> 
> > 
> > Beside that (backlight) switching takes some time, but this should not
> > be due to above "debouncing".
> Yep, I noticed that as well.
> I verified, backlight behave the same in Windows. Slow switch, and just 6
> levels.
Thanks!

I'd prefer if Len could take on top patches, I don't want to touch this
again any time soon if not necessary.

    Thomas

Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

[Anisse Astier]

On Fri, 4 Dec 2009 16:20:47 +0100, Thomas Renninger <trenn@suse.de> wrote :

> 
> I'd prefer if Len could take on top patches, I don't want to touch this
> again any time soon if not necessary.
> 
>     Thomas

FWIW, I agree.

Anisse