[PATCH 0/7] msi-wmi driver and cleanups

[PATCH 0/7] msi-wmi driver and cleanups

[Anisse Astier]

This driver adds support for WMI-based hotkeys and backlight for some MSI 
laptops and all-in-one computers.

This driver was sent in the end of october by Thomas Renninger
(cf http://patchwork.kernel.org/patch/55944/ ). I'm re-sending it as-is (I
just added my Reviewed-by:); it is the first patch of this serie because 
it doesn't look like it has been integrated yet in Len's ACPI tree.

Other patches are just a follow-up to my previous comments on this driver,
that I would like to see merged for 2.6.33.

Please note that the last patch of this serie requires Dmitry's input next
tree to have the sparse keymap library.

Regards,

Anisse


Anisse Astier (6):
  msi-wmi: remove useless includes
  msi-wmi: rework init
  msi-wmi: remove custom runtime debug implementation
  msi-wmi: remove unused field 'instance' in key_entry structure
  msi-wmi: replace one-condition switch-case with if statement
  msi-wmi: switch to using input sparse keymap library

Thomas Renninger (1):
  X86 drivers: Introduce msi-wmi driver

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

[PATCH 1/7] X86 drivers: Introduce msi-wmi driver

[Anisse Astier]

From: Thomas Renninger <trenn@suse.de>

This driver serves backlight (including switching) and volume up/down
keys for MSI machines providing a specific wmi interface:
551A1F84-FBDD-4125-91DB-3EA8F44F1D45
B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2

Signed-off-by: Thomas Renninger <trenn@suse.de>

CC: Carlos Corbacho <carlos@strangeworlds.co.uk>
CC: Matthew Garrett <mjg59@srcf.ucam.org>
CC: linux-acpi@vger.kernel.org
CC: Len Brown <lenb@kernel.org>
Tested-by: Matt Chen <machen@novell.com>
Reviewed-by: Anisse Astier <anisse@astier.eu>
---
 drivers/platform/x86/Kconfig   |   10 +
 drivers/platform/x86/Makefile  |    1 +
 drivers/platform/x86/msi-wmi.c |  369 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 380 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 55ca39d..98ec6bd 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -365,6 +365,16 @@ config ACPI_WMI
 	  It is safe to enable this driver even if your DSDT doesn't define
 	  any ACPI-WMI devices.
 
+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 laptops.
+
+	 To compile this driver as a module, choose M here: the module will
+	 be called msi-wmi.
+
 config ACPI_ASUS
 	tristate "ASUS/Medion Laptop Extras (DEPRECATED)"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index d1c1621..13aa373 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_FUJITSU_LAPTOP)	+= fujitsu-laptop.o
 obj-$(CONFIG_PANASONIC_LAPTOP)	+= panasonic-laptop.o
 obj-$(CONFIG_INTEL_MENLOW)	+= intel_menlow.o
 obj-$(CONFIG_ACPI_WMI)		+= wmi.o
+obj-$(CONFIG_MSI_WMI)		+= msi-wmi.o
 obj-$(CONFIG_ACPI_ASUS)		+= asus_acpi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
new file mode 100644
index 0000000..7e0dab6
--- /dev/null
+++ b/drivers/platform/x86/msi-wmi.c
@@ -0,0 +1,369 @@
+/*
+ * MSI WMI hotkeys
+ *
+ * Copyright (C) 2009 Novell <trenn@suse.de>
+ *
+ * Most stuff taken over from hp-wmi
+ *
+ *  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/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>
+
+MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
+MODULE_DESCRIPTION("MSI laptop WMI hotkeys driver");
+MODULE_LICENSE("GPL");
+
+static int debug;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Set this to 1 to let the driver be more verbose");
+
+MODULE_ALIAS("wmi:551A1F84-FBDD-4125-91DB-3EA8F44F1D45");
+MODULE_ALIAS("wmi:B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2");
+
+/* Temporary workaround until the WMI sysfs interface goes in
+		{ "svn", DMI_SYS_VENDOR },
+		{ "pn",  DMI_PRODUCT_NAME },
+		{ "pvr", DMI_PRODUCT_VERSION },
+		{ "rvn", DMI_BOARD_VENDOR },
+		{ "rn",  DMI_BOARD_NAME },
+*/
+
+MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-6638:*");
+
+#define DRV_NAME "msi-wmi"
+#define DRV_PFX DRV_NAME ": "
+
+#define MSIWMI_BIOS_GUID "551A1F84-FBDD-4125-91DB-3EA8F44F1D45"
+#define MSIWMI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
+
+#define dprintk(msg...)	do {			\
+	if (debug)				\
+		printk(KERN_INFO DRV_PFX  msg); \
+	} while (0)
+
+struct key_entry {
+	char type;		/* See KE_* below */
+	u16 code;
+	u16 keycode;
+	int instance;
+	ktime_t last_pressed;
+};
+
+/*
+ * KE_KEY the only used key type, but keep this, others might also
+ * show up in the future. Compare with hp-wmi.c
+ */
+enum { KE_KEY, KE_END };
+
+static struct key_entry msi_wmi_keymap[] = {
+	{ KE_KEY, 0xd0, KEY_BRIGHTNESSUP,   0, {0, } },
+	{ KE_KEY, 0xd1, KEY_BRIGHTNESSDOWN, 1, {0, } },
+	{ KE_KEY, 0xd2, KEY_VOLUMEUP,	2, {0, } },
+	{ KE_KEY, 0xd3, KEY_VOLUMEDOWN,	3, {0, } },
+	{ KE_END, 0}
+};
+
+struct backlight_device *backlight;
+
+static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF };
+
+static struct input_dev *msi_wmi_input_dev;
+
+static int msi_wmi_query_block(int instance, int *ret)
+{
+	acpi_status status;
+	union acpi_object *obj;
+
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	status = wmi_query_block(MSIWMI_BIOS_GUID, instance, &output);
+
+	obj = output.pointer;
+
+	if (!obj || obj->type != ACPI_TYPE_INTEGER) {
+		if (obj) {
+			printk(KERN_ERR DRV_PFX "query block returned object "
+			       "type: %d - buffer length:%d\n", obj->type,
+			       obj->type == ACPI_TYPE_BUFFER ?
+			       obj->buffer.length : 0);
+		}
+		kfree(obj);
+		return -EINVAL;
+	}
+	*ret = obj->integer.value;
+	kfree(obj);
+	return 0;
+}
+
+static int msi_wmi_set_block(int instance, int value)
+{
+	acpi_status status;
+
+	struct acpi_buffer input = { sizeof(int), &value };
+
+	dprintk("Going to set block of instance: %d - value: %d\n",
+		instance, value);
+
+	status = wmi_set_block(MSIWMI_BIOS_GUID, instance, &input);
+
+	return ACPI_SUCCESS(status) ? 0 : 1;
+}
+
+static int bl_get(struct backlight_device *bd)
+{
+	int level, err, ret = 0;
+
+	/* Instance 1 is "get backlight", cmp with DSDT */
+	err = msi_wmi_query_block(1, &ret);
+	if (err)
+		printk(KERN_ERR DRV_PFX "Could not query backlight: %d\n", err);
+	dprintk("Get: Query block returned: %d\n", ret);
+	for (level = 0; level < ARRAY_SIZE(backlight_map); level++) {
+		if (backlight_map[level] == ret) {
+			dprintk("Current backlight level: 0x%X - index: %d\n",
+				backlight_map[level], level);
+			break;
+		}
+	}
+	if (level == ARRAY_SIZE(backlight_map)) {
+		printk(KERN_ERR DRV_PFX "get: Invalid brightness value: 0x%X\n",
+		       ret);
+		return -EINVAL;
+	}
+	return level;
+}
+
+static int bl_set_status(struct backlight_device *bd)
+{
+	int bright = bd->props.brightness;
+	if (bright >= ARRAY_SIZE(backlight_map) || bright < 0)
+		return -EINVAL;
+
+	/* Instance 0 is "set backlight" */
+	return msi_wmi_set_block(0, backlight_map[bright]);
+}
+
+static struct backlight_ops msi_backlight_ops = {
+	.get_brightness	= bl_get,
+	.update_status	= bl_set_status,
+};
+
+static struct key_entry *msi_wmi_get_entry_by_scancode(int code)
+{
+	struct key_entry *key;
+
+	for (key = msi_wmi_keymap; key->type != KE_END; 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->type != KE_END; key++)
+		if (key->type == KE_KEY && keycode == key->keycode)
+			return key;
+
+	return NULL;
+}
+
+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->type == KE_KEY) {
+		*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->type == KE_KEY) {
+		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 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;
+	ktime_t cur;
+
+	wmi_get_event_data(value, &response);
+
+	obj = (union acpi_object *)response.pointer;
+
+	if (obj && obj->type == ACPI_TYPE_INTEGER) {
+		int eventcode = obj->integer.value;
+		dprintk("Eventcode: 0x%x\n", eventcode);
+		key = msi_wmi_get_entry_by_scancode(eventcode);
+		if (key) {
+			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) {
+				dprintk("Suppressed key event 0x%X - "
+					"Last press was %lld us ago\n",
+					 key->code,
+					 ktime_to_us(ktime_sub(cur,
+						       key->last_pressed)));
+				return;
+			}
+			key->last_pressed = cur;
+
+			switch (key->type) {
+			case KE_KEY:
+				/* Brightness is served via acpi video driver */
+				if (!backlight &&
+				    (key->keycode == KEY_BRIGHTNESSUP ||
+				     key->keycode == KEY_BRIGHTNESSDOWN))
+					break;
+
+				dprintk("Send key: 0x%X - "
+					"Input layer keycode: %d\n", key->code,
+					 key->keycode);
+				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);
+				break;
+			}
+		} else
+			printk(KERN_INFO "Unknown key pressed - %x\n",
+			       eventcode);
+	} else
+		printk(KERN_INFO DRV_PFX "Unknown event received\n");
+	kfree(response.pointer);
+}
+
+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->type != KE_END; key++) {
+		switch (key->type) {
+		case KE_KEY:
+			set_bit(EV_KEY, msi_wmi_input_dev->evbit);
+			set_bit(key->keycode, msi_wmi_input_dev->keybit);
+			break;
+		}
+	}
+
+	err = input_register_device(msi_wmi_input_dev);
+
+	if (err) {
+		input_free_device(msi_wmi_input_dev);
+		return err;
+	}
+
+	return 0;
+}
+
+static int __init msi_wmi_init(void)
+{
+	int err;
+
+	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
+		err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
+						 msi_wmi_notify, NULL);
+		if (err)
+			return -EINVAL;
+
+		err = msi_wmi_input_setup();
+		if (err) {
+			wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+			return -EINVAL;
+		}
+
+		if (!acpi_video_backlight_support()) {
+			backlight = backlight_device_register(DRV_NAME,
+					      NULL, NULL, &msi_backlight_ops);
+			if (IS_ERR(backlight)) {
+				wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+				input_unregister_device(msi_wmi_input_dev);
+				return -EINVAL;
+			}
+
+			backlight->props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
+			err = bl_get(NULL);
+			if (err < 0) {
+				wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+				input_unregister_device(msi_wmi_input_dev);
+				backlight_device_unregister(backlight);
+				return -EINVAL;
+			}
+			backlight->props.brightness = err;
+		}
+	}
+	printk(KERN_INFO DRV_PFX "Event handler installed\n");
+	return 0;
+}
+
+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);
+		backlight_device_unregister(backlight);
+	}
+}
+
+module_init(msi_wmi_init);
+module_exit(msi_wmi_exit);
-- 
1.6.5.3

Re: [PATCH 1/7] X86 drivers: Introduce msi-wmi driver

[Alan Jenkins]

Hi Anisse

I think there are a few theoretical issues remaining in this driver
(although I've not read your followup patches, I just checked the
subject lines).

On 12/10/09, Anisse Astier <anisse@astier.eu> wrote:
> +config MSI_WMI
> +	tristate "MSI WMI extras"
> +	depends on ACPI_WMI
> +	depends on INPUT

I think this driver depends on BACKLIGHT_CLASS_DEVICE as well.

> +	help
> +	 Say Y here if you want to support WMI-based hotkeys on MSI laptops.
> +
> +	 To compile this driver as a module, choose M here: the module will
> +	 be called msi-wmi.
> +


> +static int bl_get(struct backlight_device *bd)
> +{
> +	int level, err, ret = 0;
> +
> +	/* Instance 1 is "get backlight", cmp with DSDT */
> +	err = msi_wmi_query_block(1, &ret);
> +	if (err)
> +		printk(KERN_ERR DRV_PFX "Could not query backlight: %d\n", err);

It looks like we continue despite this error, and return 0?  I think
an error code would be more appropriate.  It would definitely be
better to use an explicit return statement here (and not set ret = 0
beforehand).

In reality the current backlight class doesn't support error codes...
but I figure it's better to give userspace an obviously wrong number,
rather than falsely claiming we know that the blacklight is set to 0.


> +	dprintk("Get: Query block returned: %d\n", ret);
> +	for (level = 0; level < ARRAY_SIZE(backlight_map); level++) {
> +		if (backlight_map[level] == ret) {
> +			dprintk("Current backlight level: 0x%X - index: %d\n",
> +				backlight_map[level], level);
> +			break;
> +		}
> +	}
> +	if (level == ARRAY_SIZE(backlight_map)) {
> +		printk(KERN_ERR DRV_PFX "get: Invalid brightness value: 0x%X\n",
> +		       ret);
> +		return -EINVAL;
> +	}
> +	return level;
> +}

> +static int bl_set_status(struct backlight_device *bd)
> +{
> +	int bright = bd->props.brightness;
> +	if (bright >= ARRAY_SIZE(backlight_map) || bright < 0)
> +		return -EINVAL;
> +
> +	/* Instance 0 is "set backlight" */
> +	return msi_wmi_set_block(0, backlight_map[bright]);
> +}
> +
> +static struct backlight_ops msi_backlight_ops = {
> +	.get_brightness	= bl_get,
> +	.update_status	= bl_set_status,
> +};

> +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;
> +	ktime_t cur;
> +
> +	wmi_get_event_data(value, &response);
> +
> +	obj = (union acpi_object *)response.pointer;
> +
> +	if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +		int eventcode = obj->integer.value;
> +		dprintk("Eventcode: 0x%x\n", eventcode);
> +		key = msi_wmi_get_entry_by_scancode(eventcode);
> +		if (key) {
> +			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) {
> +				dprintk("Suppressed key event 0x%X - "
> +					"Last press was %lld us ago\n",
> +					 key->code,
> +					 ktime_to_us(ktime_sub(cur,
> +						       key->last_pressed)));
> +				return;
> +			}
> +			key->last_pressed = cur;
> +
> +			switch (key->type) {
> +			case KE_KEY:
> +				/* Brightness is served via acpi video driver */
> +				if (!backlight &&
> +				    (key->keycode == KEY_BRIGHTNESSUP ||
> +				     key->keycode == KEY_BRIGHTNESSDOWN))
> +					break;

Given backlight == NULL, this will mysteriously prevent users from
remapping the volume keys to use as a brightness control.  I think it
would be better to check the original scancodes.

Also, can you please confirm that the ACPI BIOS does _not_ change the
brightness level in response to these keys?  If it does, the driver
should really use backlight_force_update() instead of generating
KEY_BRIGHTNESS* events.

> +				dprintk("Send key: 0x%X - "
> +					"Input layer keycode: %d\n", key->code,
> +					 key->keycode);
> +				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);
> +				break;
> +			}
> +		} else
> +			printk(KERN_INFO "Unknown key pressed - %x\n",
> +			       eventcode);
> +	} else
> +		printk(KERN_INFO DRV_PFX "Unknown event received\n");
> +	kfree(response.pointer);
> +}

Regards
Alan

Re: [PATCH 1/7] X86 drivers: Introduce msi-wmi driver

[Anisse Astier]

Hi Alan,

Thanks for taking the time to review it,

On Sat, Dec 12, 2009 at 18:37, Alan Jenkins
<sourcejedi.lkml@googlemail.com> wrote:
> Hi Anisse
>
> I think there are a few theoretical issues remaining in this driver
> (although I've not read your followup patches, I just checked the
> subject lines).
>
> On 12/10/09, Anisse Astier <anisse@astier.eu> wrote:
>> +config MSI_WMI
>> +     tristate "MSI WMI extras"
>> +     depends on ACPI_WMI
>> +     depends on INPUT
>
> I think this driver depends on BACKLIGHT_CLASS_DEVICE as well.

Indeed it depends on it..

>
>> +     help
>> +      Say Y here if you want to support WMI-based hotkeys on MSI laptops.
>> +
>> +      To compile this driver as a module, choose M here: the module will
>> +      be called msi-wmi.
>> +
>
>
>> +static int bl_get(struct backlight_device *bd)
>> +{
>> +     int level, err, ret = 0;
>> +
>> +     /* Instance 1 is "get backlight", cmp with DSDT */
>> +     err = msi_wmi_query_block(1, &ret);
>> +     if (err)
>> +             printk(KERN_ERR DRV_PFX "Could not query backlight: %d\n", err);
>
> It looks like we continue despite this error, and return 0?  I think
> an error code would be more appropriate.  It would definitely be
> better to use an explicit return statement here (and not set ret = 0
> beforehand).
>
> In reality the current backlight class doesn't support error codes...
> but I figure it's better to give userspace an obviously wrong number,
> rather than falsely claiming we know that the blacklight is set to 0.
>

Sure.

>
>
>> +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;
>> +     ktime_t cur;
>> +
>> +     wmi_get_event_data(value, &response);
>> +
>> +     obj = (union acpi_object *)response.pointer;
>> +
>> +     if (obj && obj->type == ACPI_TYPE_INTEGER) {
>> +             int eventcode = obj->integer.value;
>> +             dprintk("Eventcode: 0x%x\n", eventcode);
>> +             key = msi_wmi_get_entry_by_scancode(eventcode);
>> +             if (key) {
>> +                     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) {
>> +                             dprintk("Suppressed key event 0x%X - "
>> +                                     "Last press was %lld us ago\n",
>> +                                      key->code,
>> +                                      ktime_to_us(ktime_sub(cur,
>> +                                                    key->last_pressed)));
>> +                             return;
>> +                     }
>> +                     key->last_pressed = cur;
>> +
>> +                     switch (key->type) {
>> +                     case KE_KEY:
>> +                             /* Brightness is served via acpi video driver */
>> +                             if (!backlight &&
>> +                                 (key->keycode == KEY_BRIGHTNESSUP ||
>> +                                  key->keycode == KEY_BRIGHTNESSDOWN))
>> +                                     break;
>
> Given backlight == NULL, this will mysteriously prevent users from
> remapping the volume keys to use as a brightness control.  I think it
> would be better to check the original scancodes.

Agreed, this backlight == NULL should be replaced by the
acpi_video_backlight_support() test, which would be clearer, as it is
the only way that the driver could be fully initialized and backlight
still be NULL.

As for the remapping problem you are right, but please remember that
this code is theoretical and future-proofing, because on my
hardware(MSI Windtop AE1900-WT,  all-in-one Atom-based computer),
backlight is always controlled through WMI. Can you confirm that on
yours Thomas?


>
> Also, can you please confirm that the ACPI BIOS does _not_ change the
> brightness level in response to these keys?  If it does, the driver
> should really use backlight_force_update() instead of generating
> KEY_BRIGHTNESS* events.

I can confirm that the backlight is not controlled by the ACPI BIOS.
You must issue a WMI request to change it.


Regards,
Anisse
--
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: [PATCH 1/7] X86 drivers: Introduce msi-wmi driver

[Anisse Astier]

Hi,

This should fix the last issues. It applies at the end of the patch serie, (not
on top of the patch you reviewed), but it should be pretty similar.

Should I resend all the patches? Set up a git tree somewhere to pull from?

Regards.

From: Anisse Astier <anisse@astier.eu>
Date: Mon, 14 Dec 2009 10:21:39 +0100
Subject: [PATCH] msi-wmi: depend on backlight and fix corner-cases problems

Now depends on BACKLIGHT_CLASS_DEVICE.
Driver will return an error if it can't get actual backlight value
Fix remapping of brightness keys when backlight is not controlled by ACPI.

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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1f82d6d..a006dec 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -369,6 +369,7 @@ config MSI_WMI
 	tristate "MSI WMI extras"
 	depends on ACPI_WMI
 	depends on INPUT
+	depends on BACKLIGHT_CLASS_DEVICE
 	select INPUT_SPARSEKMAP
 	help
 	 Say Y here if you want to support WMI-based hotkeys on MSI laptops.
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index e25b80c..0c8fe14 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -53,11 +53,15 @@ MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-6638:*");
 #define dprintk(msg...) pr_debug(DRV_PFX msg)
 
 #define KEYCODE_BASE 0xD0
+#define MSI_WMI_BRIGHTNESSUP   KEYCODE_BASE
+#define MSI_WMI_BRIGHTNESSDOWN (KEYCODE_BASE + 1)
+#define MSI_WMI_VOLUMEUP       (KEYCODE_BASE + 2)
+#define MSI_WMI_VOLUMEDOWN     (KEYCODE_BASE + 3)
 static struct key_entry msi_wmi_keymap[] = {
-	{ KE_KEY, KEYCODE_BASE,     {KEY_BRIGHTNESSUP} },
-	{ KE_KEY, KEYCODE_BASE + 1, {KEY_BRIGHTNESSDOWN} },
-	{ KE_KEY, KEYCODE_BASE + 2, {KEY_VOLUMEUP} },
-	{ KE_KEY, KEYCODE_BASE + 3, {KEY_VOLUMEDOWN} },
+	{ KE_KEY, MSI_WMI_BRIGHTNESSUP,   {KEY_BRIGHTNESSUP} },
+	{ KE_KEY, MSI_WMI_BRIGHTNESSDOWN, {KEY_BRIGHTNESSDOWN} },
+	{ KE_KEY, MSI_WMI_VOLUMEUP,       {KEY_VOLUMEUP} },
+	{ KE_KEY, MSI_WMI_VOLUMEDOWN,     {KEY_VOLUMEDOWN} },
 	{ KE_END, 0}
 };
 static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
@@ -110,12 +114,14 @@ static int msi_wmi_set_block(int instance, int value)
 
 static int bl_get(struct backlight_device *bd)
 {
-	int level, err, ret = 0;
+	int level, err, ret;
 
 	/* Instance 1 is "get backlight", cmp with DSDT */
 	err = msi_wmi_query_block(1, &ret);
-	if (err)
+	if (err) {
 		printk(KERN_ERR DRV_PFX "Could not query backlight: %d\n", err);
+		return -EINVAL;
+	}
 	dprintk("Get: Query block returned: %d\n", ret);
 	for (level = 0; level < ARRAY_SIZE(backlight_map); level++) {
 		if (backlight_map[level] == ret) {
@@ -180,8 +186,9 @@ static void msi_wmi_notify(u32 value, void *context)
 
 			if (key->type == KE_KEY &&
 			/* Brightness is served via acpi video driver */
-			(backlight || (key->keycode != KEY_BRIGHTNESSUP &&
-			key->keycode != KEY_BRIGHTNESSDOWN))) {
+			(!acpi_video_backlight_support() ||
+			(key->code != MSI_WMI_BRIGHTNESSUP &&
+			key->code != MSI_WMI_BRIGHTNESSDOWN))) {
 				dprintk("Send key: 0x%X - "
 					"Input layer keycode: %d\n", key->code,
 					 key->keycode);
-- 
1.6.5.3

Re: [PATCH 1/7] X86 drivers: Introduce msi-wmi driver

[Alan Jenkins]

On 12/14/09, Anisse Astier <anisse@astier.eu> wrote:
> Hi,
>
> This should fix the last issues. It applies at the end of the patch serie,
> (not
> on top of the patch you reviewed), but it should be pretty similar.

Yes, that looks good.

> Should I resend all the patches? Set up a git tree somewhere to pull from?

Try just resending this last patch with a subject line "[PATCH 8/7]".

Thanks
Alan

Re: [PATCH 1/7] X86 drivers: Introduce msi-wmi driver

[Len Brown]

> [PATCH] msi-wmi: depend on backlight and fix corner-cases problems

8 patches applied to acpi-test

The driver needs a MAINTAINERS entry, Thomas, Anisse?

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH] MAINTAINERS: add maintainer for msi-wmi driver

[Anisse Astier]

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
Hi Len,

I'm willing to take maintainership for this driver, as long as I have access
to hardware (i.e my current job).

Regards,

Anisse

 MAINTAINERS |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a32c3e..b2d551c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3635,6 +3635,11 @@ W:	http://0pointer.de/lennart/tchibo.html
 S:	Maintained
 F:	drivers/platform/x86/msi-laptop.c
 
+MSI WMI SUPPORT
+M:	Anisse Astier <anisse@astier.eu>
+S:	Supported
+F:	drivers/platform/x86/msi-wmi.c
+
 MULTIFUNCTION DEVICES (MFD)
 M:	Samuel Ortiz <sameo@linux.intel.com>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git
-- 
1.6.5.3

Re: [PATCH] MAINTAINERS: add maintainer for msi-wmi driver

[Thomas Renninger]

On 12/17/2009 11:28 AM, Anisse Astier wrote:
> 
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> ---
> Hi Len,
> 
> I'm willing to take maintainership for this driver, as long as I have access
> to hardware (i.e my current job).

That's great.
I never had a machine (and probably never will) and had to implement
this remotely.

And I already posted the first patch without the maintainer added :)
But it's simple, I just removed the dmi alias, because of wmi
autoloading got added recently.

Thanks,

      Thomas

Re: [PATCH] MAINTAINERS: add maintainer for msi-wmi driver

[Anisse Astier]

On Fri, 18 Dec 2009 17:52:26 +0100, Thomas Renninger <trenn@suse.de> wrote :

> On 12/17/2009 11:28 AM, Anisse Astier wrote:
> > 
> > Signed-off-by: Anisse Astier <anisse@astier.eu>
> > ---
> > Hi Len,
> > 
> > I'm willing to take maintainership for this driver, as long as I have access
> > to hardware (i.e my current job).
> 
> That's great.
> I never had a machine (and probably never will) and had to implement
> this remotely.
Impressive :-)

> 
> And I already posted the first patch without the maintainer added :)
> But it's simple, I just removed the dmi alias, because of wmi
> autoloading got added recently.

Thanks !

Anisse

Re: [PATCH] MAINTAINERS: add maintainer for msi-wmi driver

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 2/7] msi-wmi: remove useless includes

[Anisse Astier]

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

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 7e0dab6..fb988d8 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -23,14 +23,8 @@
 
 
 #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>
 
 MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
-- 
1.6.5.3

[PATCH 3/7] msi-wmi: rework init

[Anisse Astier]

There should be less code duplication with usage of gotos
Driver won't load if there's no hardware to control
Safer error handling at input driver allocation

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

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index fb988d8..dcb048c 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -284,6 +284,8 @@ static int __init msi_wmi_input_setup(void)
 	int err;
 
 	msi_wmi_input_dev = input_allocate_device();
+	if (!msi_wmi_input_dev)
+		return -ENOMEM;
 
 	msi_wmi_input_dev->name = "MSI WMI hotkeys";
 	msi_wmi_input_dev->phys = "wmi/input0";
@@ -314,40 +316,44 @@ static int __init msi_wmi_init(void)
 {
 	int err;
 
-	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
-		err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
-						 msi_wmi_notify, NULL);
-		if (err)
-			return -EINVAL;
-
-		err = msi_wmi_input_setup();
-		if (err) {
-			wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
-			return -EINVAL;
-		}
+	if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
+		printk(KERN_ERR
+		       "This machine doesn't have MSI-hotkeys through WMI\n");
+		return -ENODEV;
+	}
+	err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
+			msi_wmi_notify, NULL);
+	if (err)
+		return -EINVAL;
 
-		if (!acpi_video_backlight_support()) {
-			backlight = backlight_device_register(DRV_NAME,
-					      NULL, NULL, &msi_backlight_ops);
-			if (IS_ERR(backlight)) {
-				wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
-				input_unregister_device(msi_wmi_input_dev);
-				return -EINVAL;
-			}
+	err = msi_wmi_input_setup();
+	if (err)
+		goto err_uninstall_notifier;
 
-			backlight->props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
-			err = bl_get(NULL);
-			if (err < 0) {
-				wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
-				input_unregister_device(msi_wmi_input_dev);
-				backlight_device_unregister(backlight);
-				return -EINVAL;
-			}
-			backlight->props.brightness = err;
-		}
+	if (!acpi_video_backlight_support()) {
+		backlight = backlight_device_register(DRV_NAME,
+				NULL, NULL, &msi_backlight_ops);
+		if (IS_ERR(backlight))
+			goto err_free_input;
+
+		backlight->props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
+		err = bl_get(NULL);
+		if (err < 0)
+			goto err_free_backlight;
+
+		backlight->props.brightness = err;
 	}
 	printk(KERN_INFO DRV_PFX "Event handler installed\n");
+
 	return 0;
+
+err_free_backlight:
+	backlight_device_unregister(backlight);
+err_free_input:
+	input_unregister_device(msi_wmi_input_dev);
+err_uninstall_notifier:
+	wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+	return err;
 }
 
 static void __exit msi_wmi_exit(void)
-- 
1.6.5.3

[PATCH 4/7] msi-wmi: remove custom runtime debug implementation

[Anisse Astier]

Rely on DYNAMIC_DEBUG instead if needed

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

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index dcb048c..75e422b 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -31,10 +31,6 @@ MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
 MODULE_DESCRIPTION("MSI laptop WMI hotkeys driver");
 MODULE_LICENSE("GPL");
 
-static int debug;
-module_param(debug, int, 0);
-MODULE_PARM_DESC(debug, "Set this to 1 to let the driver be more verbose");
-
 MODULE_ALIAS("wmi:551A1F84-FBDD-4125-91DB-3EA8F44F1D45");
 MODULE_ALIAS("wmi:B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2");
 
@@ -54,10 +50,7 @@ MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-6638:*");
 #define MSIWMI_BIOS_GUID "551A1F84-FBDD-4125-91DB-3EA8F44F1D45"
 #define MSIWMI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
 
-#define dprintk(msg...)	do {			\
-	if (debug)				\
-		printk(KERN_INFO DRV_PFX  msg); \
-	} while (0)
+#define dprintk(msg...) pr_debug(DRV_PFX msg)
 
 struct key_entry {
 	char type;		/* See KE_* below */
@@ -343,7 +336,7 @@ static int __init msi_wmi_init(void)
 
 		backlight->props.brightness = err;
 	}
-	printk(KERN_INFO DRV_PFX "Event handler installed\n");
+	dprintk("Event handler installed\n");
 
 	return 0;
 
-- 
1.6.5.3

[PATCH 5/7] msi-wmi: remove unused field 'instance' in key_entry structure

[Anisse Astier]

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
 drivers/platform/x86/msi-wmi.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 75e422b..d757935 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -56,7 +56,6 @@ struct key_entry {
 	char type;		/* See KE_* below */
 	u16 code;
 	u16 keycode;
-	int instance;
 	ktime_t last_pressed;
 };
 
@@ -67,10 +66,10 @@ struct key_entry {
 enum { KE_KEY, KE_END };
 
 static struct key_entry msi_wmi_keymap[] = {
-	{ KE_KEY, 0xd0, KEY_BRIGHTNESSUP,   0, {0, } },
-	{ KE_KEY, 0xd1, KEY_BRIGHTNESSDOWN, 1, {0, } },
-	{ KE_KEY, 0xd2, KEY_VOLUMEUP,	2, {0, } },
-	{ KE_KEY, 0xd3, KEY_VOLUMEDOWN,	3, {0, } },
+	{ KE_KEY, 0xd0, KEY_BRIGHTNESSUP,   {0, } },
+	{ KE_KEY, 0xd1, KEY_BRIGHTNESSDOWN, {0, } },
+	{ KE_KEY, 0xd2, KEY_VOLUMEUP,	{0, } },
+	{ KE_KEY, 0xd3, KEY_VOLUMEDOWN,	{0, } },
 	{ KE_END, 0}
 };
 
-- 
1.6.5.3

[PATCH 6/7] msi-wmi: replace one-condition switch-case with if statement

[Anisse Astier]

Signed-off-by: Anisse Astier <anisse@astier.eu>
---
 drivers/platform/x86/msi-wmi.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index d757935..2c2afc1 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -243,14 +243,10 @@ static void msi_wmi_notify(u32 value, void *context)
 			}
 			key->last_pressed = cur;
 
-			switch (key->type) {
-			case KE_KEY:
-				/* Brightness is served via acpi video driver */
-				if (!backlight &&
-				    (key->keycode == KEY_BRIGHTNESSUP ||
-				     key->keycode == KEY_BRIGHTNESSDOWN))
-					break;
-
+			if (key->type == KE_KEY &&
+			/* Brightness is served via acpi video driver */
+			(backlight || (key->keycode != KEY_BRIGHTNESSUP &&
+			key->keycode != KEY_BRIGHTNESSDOWN))) {
 				dprintk("Send key: 0x%X - "
 					"Input layer keycode: %d\n", key->code,
 					 key->keycode);
@@ -260,7 +256,6 @@ static void msi_wmi_notify(u32 value, void *context)
 				input_report_key(msi_wmi_input_dev,
 						 key->keycode, 0);
 				input_sync(msi_wmi_input_dev);
-				break;
 			}
 		} else
 			printk(KERN_INFO "Unknown key pressed - %x\n",
-- 
1.6.5.3

[PATCH 7/7] msi-wmi: switch to using input sparse keymap library

[Anisse Astier]

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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 98ec6bd..1f82d6d 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -369,6 +369,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 laptops.
 
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 2c2afc1..e25b80c 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -21,9 +21,9 @@
  */
 
 
-
 #include <linux/kernel.h>
 #include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 
@@ -52,26 +52,15 @@ MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-6638:*");
 
 #define dprintk(msg...) pr_debug(DRV_PFX msg)
 
-struct key_entry {
-	char type;		/* See KE_* below */
-	u16 code;
-	u16 keycode;
-	ktime_t last_pressed;
-};
-
-/*
- * KE_KEY the only used key type, but keep this, others might also
- * show up in the future. Compare with hp-wmi.c
- */
-enum { KE_KEY, KE_END };
-
+#define KEYCODE_BASE 0xD0
 static struct key_entry msi_wmi_keymap[] = {
-	{ KE_KEY, 0xd0, KEY_BRIGHTNESSUP,   {0, } },
-	{ KE_KEY, 0xd1, KEY_BRIGHTNESSDOWN, {0, } },
-	{ KE_KEY, 0xd2, KEY_VOLUMEUP,	{0, } },
-	{ KE_KEY, 0xd3, KEY_VOLUMEDOWN,	{0, } },
+	{ KE_KEY, KEYCODE_BASE,     {KEY_BRIGHTNESSUP} },
+	{ KE_KEY, KEYCODE_BASE + 1, {KEY_BRIGHTNESSDOWN} },
+	{ KE_KEY, KEYCODE_BASE + 2, {KEY_VOLUMEUP} },
+	{ KE_KEY, KEYCODE_BASE + 3, {KEY_VOLUMEDOWN} },
 	{ KE_END, 0}
 };
+static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
 
 struct backlight_device *backlight;
 
@@ -158,61 +147,6 @@ static struct backlight_ops msi_backlight_ops = {
 	.update_status	= bl_set_status,
 };
 
-static struct key_entry *msi_wmi_get_entry_by_scancode(int code)
-{
-	struct key_entry *key;
-
-	for (key = msi_wmi_keymap; key->type != KE_END; 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->type != KE_END; key++)
-		if (key->type == KE_KEY && keycode == key->keycode)
-			return key;
-
-	return NULL;
-}
-
-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->type == KE_KEY) {
-		*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->type == KE_KEY) {
-		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 void msi_wmi_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -227,21 +161,22 @@ static void msi_wmi_notify(u32 value, void *context)
 	if (obj && obj->type == ACPI_TYPE_INTEGER) {
 		int eventcode = obj->integer.value;
 		dprintk("Eventcode: 0x%x\n", eventcode);
-		key = msi_wmi_get_entry_by_scancode(eventcode);
+		key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
+				eventcode);
 		if (key) {
+			ktime_t diff;
 			cur = ktime_get_real();
+			diff = ktime_sub(cur, last_pressed[key->code -
+					KEYCODE_BASE]);
 			/* 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) {
+			if (ktime_to_us(diff) < 1000 * 50) {
 				dprintk("Suppressed key event 0x%X - "
 					"Last press was %lld us ago\n",
-					 key->code,
-					 ktime_to_us(ktime_sub(cur,
-						       key->last_pressed)));
+					 key->code, ktime_to_us(diff));
 				return;
 			}
-			key->last_pressed = cur;
+			last_pressed[key->code - KEYCODE_BASE] = cur;
 
 			if (key->type == KE_KEY &&
 			/* Brightness is served via acpi video driver */
@@ -250,12 +185,8 @@ static void msi_wmi_notify(u32 value, void *context)
 				dprintk("Send key: 0x%X - "
 					"Input layer keycode: %d\n", key->code,
 					 key->keycode);
-				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);
+				sparse_keymap_report_entry(msi_wmi_input_dev,
+						key, 1, true);
 			}
 		} else
 			printk(KERN_INFO "Unknown key pressed - %x\n",
@@ -267,7 +198,6 @@ static void msi_wmi_notify(u32 value, void *context)
 
 static int __init msi_wmi_input_setup(void)
 {
-	struct key_entry *key;
 	int err;
 
 	msi_wmi_input_dev = input_allocate_device();
@@ -277,26 +207,25 @@ 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->type != KE_END; key++) {
-		switch (key->type) {
-		case KE_KEY:
-			set_bit(EV_KEY, msi_wmi_input_dev->evbit);
-			set_bit(key->keycode, msi_wmi_input_dev->keybit);
-			break;
-		}
-	}
+
+	err = sparse_keymap_setup(msi_wmi_input_dev, msi_wmi_keymap, NULL);
+	if (err)
+		goto err_free_dev;
 
 	err = input_register_device(msi_wmi_input_dev);
 
-	if (err) {
-		input_free_device(msi_wmi_input_dev);
-		return err;
-	}
+	if (err)
+		goto err_free_keymap;
+
+	memset(last_pressed, 0, sizeof(last_pressed));
 
 	return 0;
+
+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)
@@ -347,6 +276,7 @@ static void __exit msi_wmi_exit(void)
 {
 	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
 		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+		sparse_keymap_free(msi_wmi_input_dev);
 		input_unregister_device(msi_wmi_input_dev);
 		backlight_device_unregister(backlight);
 	}
-- 
1.6.5.3

Re: [PATCH 0/7] msi-wmi driver and cleanups

[Thomas Renninger]

Hi Anisse,

thanks for these!
A quick comment on each:
 
> Anisse Astier (6):
>   msi-wmi: remove useless includes
    -> good catch, I probably C&P it from another acpi file.
>   msi-wmi: rework init
    -> might look a bit nicer now, thanks.
>   msi-wmi: remove custom runtime debug implementation
    -> fine.
>   msi-wmi: remove unused field 'instance' in key_entry structure
    -> nice cleanup
>   msi-wmi: replace one-condition switch-case with if statement
    -> might be nicer
>   msi-wmi: switch to using input sparse keymap library
    -> I didn't go into detail, but this looks like a very nice
    cleanup and code reduction.

Thanks,

   Thomas
> 
> Thomas Renninger (1):
>   X86 drivers: Introduce msi-wmi driver
> 
>  drivers/platform/x86/Kconfig   |   11 ++
>  drivers/platform/x86/Makefile  |    1 +
>  drivers/platform/x86/msi-wmi.c |  286 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 298 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/platform/x86/msi-wmi.c
> 
> 

Re: [PATCH 0/7] msi-wmi driver and cleanups

[Anisse Astier]

Hi Thomas,

On Thu, 10 Dec 2009 16:18:27 +0100, Thomas Renninger <trenn@suse.de> wrote :

> Hi Anisse,
> 
> thanks for these!
> A quick comment on each:
>  
> > Anisse Astier (6):
> >   msi-wmi: remove useless includes
>     -> good catch, I probably C&P it from another acpi file.
> >   msi-wmi: rework init
>     -> might look a bit nicer now, thanks.
> >   msi-wmi: remove custom runtime debug implementation
>     -> fine.
> >   msi-wmi: remove unused field 'instance' in key_entry structure
>     -> nice cleanup
> >   msi-wmi: replace one-condition switch-case with if statement
>     -> might be nicer
> >   msi-wmi: switch to using input sparse keymap library
>     -> I didn't go into detail, but this looks like a very nice
>     cleanup and code reduction.
> 

Thanks for taking the time to check this out.

Anisse

Re: [PATCH 0/7] msi-wmi driver and cleanups

[Dmitry Torokhov]

On Thu, Dec 10, 2009 at 02:18:12PM +0100, Anisse Astier wrote:
> This driver adds support for WMI-based hotkeys and backlight for some MSI 
> laptops and all-in-one computers.
> 
> This driver was sent in the end of october by Thomas Renninger
> (cf http://patchwork.kernel.org/patch/55944/ ). I'm re-sending it as-is (I
> just added my Reviewed-by:); it is the first patch of this serie because 
> it doesn't look like it has been integrated yet in Len's ACPI tree.
> 
> Other patches are just a follow-up to my previous comments on this driver,
> that I would like to see merged for 2.6.33.
> 
> Please note that the last patch of this serie requires Dmitry's input next
> tree to have the sparse keymap library.
> 

FWIW sparse keymap module is in mainline now. The last patch looks good
BTW.

-- 
Dmitry

Re: [PATCH 0/7] msi-wmi driver and cleanups

[Anisse Astier]

Hi Dmitry,

On Thu, 10 Dec 2009 09:47:50 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote :

> 
> FWIW sparse keymap module is in mainline now. The last patch looks good
> BTW.
> 

Thanks for pointing this out, I'm not following mainline closely.

Anisse

[PATCH 8/7] msi-wmi: depend on backlight and fix corner-cases problems

[Anisse Astier]

Now depends on BACKLIGHT_CLASS_DEVICE.
Driver will return an error if it can't get actual backlight value
Fix remapping of brightness keys when backlight is not controlled by ACPI.

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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1f82d6d..a006dec 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -369,6 +369,7 @@ config MSI_WMI
 	tristate "MSI WMI extras"
 	depends on ACPI_WMI
 	depends on INPUT
+	depends on BACKLIGHT_CLASS_DEVICE
 	select INPUT_SPARSEKMAP
 	help
 	 Say Y here if you want to support WMI-based hotkeys on MSI laptops.
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index e25b80c..0c8fe14 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -53,11 +53,15 @@ MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-6638:*");
 #define dprintk(msg...) pr_debug(DRV_PFX msg)
 
 #define KEYCODE_BASE 0xD0
+#define MSI_WMI_BRIGHTNESSUP   KEYCODE_BASE
+#define MSI_WMI_BRIGHTNESSDOWN (KEYCODE_BASE + 1)
+#define MSI_WMI_VOLUMEUP       (KEYCODE_BASE + 2)
+#define MSI_WMI_VOLUMEDOWN     (KEYCODE_BASE + 3)
 static struct key_entry msi_wmi_keymap[] = {
-	{ KE_KEY, KEYCODE_BASE,     {KEY_BRIGHTNESSUP} },
-	{ KE_KEY, KEYCODE_BASE + 1, {KEY_BRIGHTNESSDOWN} },
-	{ KE_KEY, KEYCODE_BASE + 2, {KEY_VOLUMEUP} },
-	{ KE_KEY, KEYCODE_BASE + 3, {KEY_VOLUMEDOWN} },
+	{ KE_KEY, MSI_WMI_BRIGHTNESSUP,   {KEY_BRIGHTNESSUP} },
+	{ KE_KEY, MSI_WMI_BRIGHTNESSDOWN, {KEY_BRIGHTNESSDOWN} },
+	{ KE_KEY, MSI_WMI_VOLUMEUP,       {KEY_VOLUMEUP} },
+	{ KE_KEY, MSI_WMI_VOLUMEDOWN,     {KEY_VOLUMEDOWN} },
 	{ KE_END, 0}
 };
 static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
@@ -110,12 +114,14 @@ static int msi_wmi_set_block(int instance, int value)
 
 static int bl_get(struct backlight_device *bd)
 {
-	int level, err, ret = 0;
+	int level, err, ret;
 
 	/* Instance 1 is "get backlight", cmp with DSDT */
 	err = msi_wmi_query_block(1, &ret);
-	if (err)
+	if (err) {
 		printk(KERN_ERR DRV_PFX "Could not query backlight: %d\n", err);
+		return -EINVAL;
+	}
 	dprintk("Get: Query block returned: %d\n", ret);
 	for (level = 0; level < ARRAY_SIZE(backlight_map); level++) {
 		if (backlight_map[level] == ret) {
@@ -180,8 +186,9 @@ static void msi_wmi_notify(u32 value, void *context)
 
 			if (key->type == KE_KEY &&
 			/* Brightness is served via acpi video driver */
-			(backlight || (key->keycode != KEY_BRIGHTNESSUP &&
-			key->keycode != KEY_BRIGHTNESSDOWN))) {
+			(!acpi_video_backlight_support() ||
+			(key->code != MSI_WMI_BRIGHTNESSUP &&
+			key->code != MSI_WMI_BRIGHTNESSDOWN))) {
 				dprintk("Send key: 0x%X - "
 					"Input layer keycode: %d\n", key->code,
 					 key->keycode);
-- 
1.6.5.3

Re: [PATCH 0/7] msi-wmi driver and cleanups

[Anisse Astier]

On Thu, 10 Dec 2009 14:18:12 +0100, Anisse Astier <anisse@astier.eu> wrote :

> This driver adds support for WMI-based hotkeys and backlight for some MSI 
> laptops and all-in-one computers.
> 
> This driver was sent in the end of october by Thomas Renninger
> (cf http://patchwork.kernel.org/patch/55944/ ). I'm re-sending it as-is (I
> just added my Reviewed-by:); it is the first patch of this serie because 
> it doesn't look like it has been integrated yet in Len's ACPI tree.
> 
> Other patches are just a follow-up to my previous comments on this driver,
> that I would like to see merged for 2.6.33.
> 
> Please note that the last patch of this serie requires Dmitry's input next
> tree to have the sparse keymap library.
> 
> Regards,
> 
> Anisse
> 
> 
> Anisse Astier (6):
 Anisse Astier (7):
>   msi-wmi: remove useless includes
>   msi-wmi: rework init
>   msi-wmi: remove custom runtime debug implementation
>   msi-wmi: remove unused field 'instance' in key_entry structure
>   msi-wmi: replace one-condition switch-case with if statement
>   msi-wmi: switch to using input sparse keymap library
   msi-wmi: depend on backlight and fix corner-cases problems
> 
> Thomas Renninger (1):
>   X86 drivers: Introduce msi-wmi driver
> 
>  drivers/platform/x86/Kconfig   |   11 ++
>  drivers/platform/x86/Makefile  |    1 +
>  drivers/platform/x86/msi-wmi.c |  286 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 298 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/platform/x86/msi-wmi.c
> 

So, if there is no other objection on this, what path can this (small) driver
take to go upstream?
Thomas' goal was to have it through Len's ACPI tree. Len, what's you opinion?

Regards,

Anisse