[PATCH v3 00/12] Add MSI Wind U90/U100 support

[PATCH v3 00/12] Add MSI Wind U90/U100 support

[Maxim Mikityanskiy]

This series adds support for MSI Wind U90/U100 laptops to msi-laptop
platform driver and to msi-wmi WMI driver.

Changes comparing to the first series:
	* new patch for quirk tables in msi-laptop;
	* new patch to disable brightness control for machines with
	  newer EC;
	* new patch to fix memory leak in msi-wmi;
	* new patch to add MSI Wind support in msi-wmi;
	* fixed code that breaks compatibility with old models;
	* fixed bug with touchpad toggle key filtering appeared in
	  the first series;
	* separated and cleaned up msi-wmi patch.

Tested on MSI Wind U100Plus laptop.

----------------------------------------------------------------
Lee, Chun-Yi (1):
      msi-laptop: merge quirk tables to one

Maxim Mikityanskiy (11):
      msi-laptop: Use proper return codes instead of -1
      msi-laptop: Work around gcc warning
      msi-laptop: Add MSI Wind U90/U100 support
      msi-laptop: Add missing ABI documentation
      msi-laptop: Disable brightness control for new EC
      msi-wmi: Fix memory leak
      msi-wmi: Avoid repeating constants
      msi-wmi: Use enums for scancodes
      msi-wmi: Make keys and backlight independent
      msi-wmi: Introduced quirk_last_pressed
      msi-wmi: Add MSI Wind support

 Documentation/ABI/testing/sysfs-platform-msi-laptop |  83 +++++++++++++++++++
 drivers/platform/x86/msi-laptop.c                   | 374 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------
 drivers/platform/x86/msi-wmi.c                      | 213 +++++++++++++++++++++++++++++++----------------
 3 files changed, 507 insertions(+), 163 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-msi-laptop

[PATCH v3 01/12] msi-laptop: Use proper return codes instead of -1

[Maxim Mikityanskiy]

Use proper function return codes instead of -1

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/msi-laptop.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 2111dbb..063113c 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -198,7 +198,7 @@ static ssize_t set_device_state(const char *buf, size_t count, u8 mask)
 	/* read current device state */
 	result = ec_read(MSI_STANDARD_EC_COMMAND_ADDRESS, &rdata);
 	if (result < 0)
-		return -EINVAL;
+		return result;
 
 	if (!!(rdata & mask) != status) {
 		/* reverse device bit */
@@ -209,7 +209,7 @@ static ssize_t set_device_state(const char *buf, size_t count, u8 mask)
 
 		result = ec_write(MSI_STANDARD_EC_COMMAND_ADDRESS, wdata);
 		if (result < 0)
-			return -EINVAL;
+			return result;
 	}
 
 	return count;
@@ -222,7 +222,7 @@ static int get_wireless_state(int *wlan, int *bluetooth)
 
 	result = ec_transaction(MSI_EC_COMMAND_WIRELESS, &wdata, 1, &rdata, 1);
 	if (result < 0)
-		return -1;
+		return result;
 
 	if (wlan)
 		*wlan = !!(rdata & 8);
@@ -240,7 +240,7 @@ static int get_wireless_state_ec_standard(void)
 
 	result = ec_read(MSI_STANDARD_EC_COMMAND_ADDRESS, &rdata);
 	if (result < 0)
-		return -1;
+		return result;
 
 	wlan_s = !!(rdata & MSI_STANDARD_EC_WLAN_MASK);
 
@@ -258,7 +258,7 @@ static int get_threeg_exists(void)
 
 	result = ec_read(MSI_STANDARD_EC_DEVICES_EXISTS_ADDRESS, &rdata);
 	if (result < 0)
-		return -1;
+		return result;
 
 	threeg_exists = !!(rdata & MSI_STANDARD_EC_3G_MASK);
 
@@ -343,7 +343,7 @@ static ssize_t show_threeg(struct device *dev,
 
 	/* old msi ec not support 3G */
 	if (old_ec_model)
-		return -1;
+		return -ENODEV;
 
 	ret = get_wireless_state_ec_standard();
 	if (ret < 0)
-- 
1.8.0.1

[PATCH v3 02/12] msi-laptop: Work around gcc warning

[Maxim Mikityanskiy]

Assign initial value to variable in order to prevent gcc warning about
uninitialized variable.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/msi-laptop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 063113c..7ba107a 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -291,7 +291,7 @@ static ssize_t show_wlan(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 
-	int ret, enabled;
+	int ret, enabled = 0;
 
 	if (old_ec_model) {
 		ret = get_wireless_state(&enabled, NULL);
@@ -315,7 +315,7 @@ static ssize_t show_bluetooth(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 
-	int ret, enabled;
+	int ret, enabled = 0;
 
 	if (old_ec_model) {
 		ret = get_wireless_state(NULL, &enabled);
-- 
1.8.0.1

[PATCH v3 03/12] msi-laptop: merge quirk tables to one

[Maxim Mikityanskiy]

From: Lee, Chun-Yi <jlee@suse.com>

This patch introduced a quirk_entry struct, then we merged all quirk
tables to msi_dmi_table. Then we can more easily to set different quirk
attributes for different machine.

Signed-off-by: Lee, Chun-Yi <jlee@suse.com>

Changed this patch so that it could be applied before MSI Wind U100
support patch. Changed rfkill logic for ec_read_only quirk support.
Removed delays if ec_delay = false.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Acked-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/msi-laptop.c | 196 ++++++++++++++++++++++++--------------
 1 file changed, 127 insertions(+), 69 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 7ba107a..0bf94b5 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -108,23 +108,38 @@ static const struct key_entry msi_laptop_keymap[] = {
 
 static struct input_dev *msi_laptop_input_dev;
 
-static bool old_ec_model;
 static int wlan_s, bluetooth_s, threeg_s;
 static int threeg_exists;
-
-/* Some MSI 3G netbook only have one fn key to control Wlan/Bluetooth/3G,
- * those netbook will load the SCM (windows app) to disable the original
- * Wlan/Bluetooth control by BIOS when user press fn key, then control
- * Wlan/Bluetooth/3G by SCM (software control by OS). Without SCM, user
- * cann't on/off 3G module on those 3G netbook.
- * On Linux, msi-laptop driver will do the same thing to disable the
- * original BIOS control, then might need use HAL or other userland
- * application to do the software control that simulate with SCM.
- * e.g. MSI N034 netbook
- */
-static bool load_scm_model;
 static struct rfkill *rfk_wlan, *rfk_bluetooth, *rfk_threeg;
 
+/* MSI laptop quirks */
+struct quirk_entry {
+	bool old_ec_model;
+
+	/* Some MSI 3G netbook only have one fn key to control
+	 * Wlan/Bluetooth/3G, those netbook will load the SCM (windows app) to
+	 * disable the original Wlan/Bluetooth control by BIOS when user press
+	 * fn key, then control Wlan/Bluetooth/3G by SCM (software control by
+	 * OS). Without SCM, user cann't on/off 3G module on those 3G netbook.
+	 * On Linux, msi-laptop driver will do the same thing to disable the
+	 * original BIOS control, then might need use HAL or other userland
+	 * application to do the software control that simulate with SCM.
+	 * e.g. MSI N034 netbook
+	 */
+	bool load_scm_model;
+
+	/* Some MSI laptops need delay before reading from EC */
+	bool ec_delay;
+
+	/* Some MSI Wind netbooks (e.g. MSI Wind U100) need loading SCM to get
+	 * some features working (e.g. ECO mode), but we cannot change
+	 * Wlan/Bluetooth state in software and we can only read its state.
+	 */
+	bool ec_read_only;
+};
+
+static struct quirk_entry *quirks;
+
 /* Hardware access */
 
 static int set_lcd_level(int level)
@@ -195,6 +210,9 @@ static ssize_t set_device_state(const char *buf, size_t count, u8 mask)
 	if (sscanf(buf, "%i", &status) != 1 || (status < 0 || status > 1))
 		return -EINVAL;
 
+	if (quirks->ec_read_only)
+		return -EOPNOTSUPP;
+
 	/* read current device state */
 	result = ec_read(MSI_STANDARD_EC_COMMAND_ADDRESS, &rdata);
 	if (result < 0)
@@ -293,7 +311,7 @@ static ssize_t show_wlan(struct device *dev,
 
 	int ret, enabled = 0;
 
-	if (old_ec_model) {
+	if (quirks->old_ec_model) {
 		ret = get_wireless_state(&enabled, NULL);
 	} else {
 		ret = get_wireless_state_ec_standard();
@@ -317,7 +335,7 @@ static ssize_t show_bluetooth(struct device *dev,
 
 	int ret, enabled = 0;
 
-	if (old_ec_model) {
+	if (quirks->old_ec_model) {
 		ret = get_wireless_state(NULL, &enabled);
 	} else {
 		ret = get_wireless_state_ec_standard();
@@ -342,7 +360,7 @@ static ssize_t show_threeg(struct device *dev,
 	int ret;
 
 	/* old msi ec not support 3G */
-	if (old_ec_model)
+	if (quirks->old_ec_model)
 		return -ENODEV;
 
 	ret = get_wireless_state_ec_standard();
@@ -448,9 +466,26 @@ static struct platform_device *msipf_device;
 
 /* Initialization */
 
-static int dmi_check_cb(const struct dmi_system_id *id)
+static struct quirk_entry quirk_old_ec_model = {
+	.old_ec_model = true,
+};
+
+static struct quirk_entry quirk_load_scm_model = {
+	.load_scm_model = true,
+	.ec_delay = true,
+};
+
+static struct quirk_entry quirk_load_scm_ro_model = {
+	.load_scm_model = true,
+	.ec_read_only = true,
+};
+
+static int dmi_check_cb(const struct dmi_system_id *dmi)
 {
-	pr_info("Identified laptop model '%s'\n", id->ident);
+	pr_info("Identified laptop model '%s'\n", dmi->ident);
+
+	quirks = dmi->driver_data;
+
 	return 1;
 }
 
@@ -464,6 +499,7 @@ static struct dmi_system_id __initdata msi_dmi_table[] = {
 			DMI_MATCH(DMI_CHASSIS_VENDOR,
 				  "MICRO-STAR INT'L CO.,LTD")
 		},
+		.driver_data = &quirk_old_ec_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -474,6 +510,7 @@ static struct dmi_system_id __initdata msi_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_VERSION, "0581"),
 			DMI_MATCH(DMI_BOARD_NAME, "MS-1058")
 		},
+		.driver_data = &quirk_old_ec_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -484,6 +521,7 @@ static struct dmi_system_id __initdata msi_dmi_table[] = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "MSI"),
 			DMI_MATCH(DMI_BOARD_NAME, "MS-1412")
 		},
+		.driver_data = &quirk_old_ec_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -495,12 +533,9 @@ static struct dmi_system_id __initdata msi_dmi_table[] = {
 			DMI_MATCH(DMI_CHASSIS_VENDOR,
 				  "MICRO-STAR INT'L CO.,LTD")
 		},
+		.driver_data = &quirk_old_ec_model,
 		.callback = dmi_check_cb
 	},
-	{ }
-};
-
-static struct dmi_system_id __initdata msi_load_scm_models_dmi_table[] = {
 	{
 		.ident = "MSI N034",
 		.matches = {
@@ -510,6 +545,7 @@ static struct dmi_system_id __initdata msi_load_scm_models_dmi_table[] = {
 			DMI_MATCH(DMI_CHASSIS_VENDOR,
 			"MICRO-STAR INTERNATIONAL CO., LTD")
 		},
+		.driver_data = &quirk_load_scm_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -521,6 +557,7 @@ static struct dmi_system_id __initdata msi_load_scm_models_dmi_table[] = {
 			DMI_MATCH(DMI_CHASSIS_VENDOR,
 			"MICRO-STAR INTERNATIONAL CO., LTD")
 		},
+		.driver_data = &quirk_load_scm_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -530,6 +567,7 @@ static struct dmi_system_id __initdata msi_load_scm_models_dmi_table[] = {
 				"MICRO-STAR INTERNATIONAL CO., LTD"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "MS-N014"),
 		},
+		.driver_data = &quirk_load_scm_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -539,6 +577,7 @@ static struct dmi_system_id __initdata msi_load_scm_models_dmi_table[] = {
 				"Micro-Star International"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "CR620"),
 		},
+		.driver_data = &quirk_load_scm_model,
 		.callback = dmi_check_cb
 	},
 	{
@@ -548,6 +587,7 @@ static struct dmi_system_id __initdata msi_load_scm_models_dmi_table[] = {
 				"Micro-Star International Co., Ltd."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "U270 series"),
 		},
+		.driver_data = &quirk_load_scm_model,
 		.callback = dmi_check_cb
 	},
 	{ }
@@ -560,32 +600,26 @@ static int rfkill_bluetooth_set(void *data, bool blocked)
 	 * blocked == false is on
 	 * blocked == true is off
 	 */
-	if (blocked)
-		set_device_state("0", 0, MSI_STANDARD_EC_BLUETOOTH_MASK);
-	else
-		set_device_state("1", 0, MSI_STANDARD_EC_BLUETOOTH_MASK);
+	int result = set_device_state(blocked ? "0" : "1", 0,
+			MSI_STANDARD_EC_BLUETOOTH_MASK);
 
-	return 0;
+	return min(result, 0);
 }
 
 static int rfkill_wlan_set(void *data, bool blocked)
 {
-	if (blocked)
-		set_device_state("0", 0, MSI_STANDARD_EC_WLAN_MASK);
-	else
-		set_device_state("1", 0, MSI_STANDARD_EC_WLAN_MASK);
+	int result = set_device_state(blocked ? "0" : "1", 0,
+			MSI_STANDARD_EC_WLAN_MASK);
 
-	return 0;
+	return min(result, 0);
 }
 
 static int rfkill_threeg_set(void *data, bool blocked)
 {
-	if (blocked)
-		set_device_state("0", 0, MSI_STANDARD_EC_3G_MASK);
-	else
-		set_device_state("1", 0, MSI_STANDARD_EC_3G_MASK);
+	int result = set_device_state(blocked ? "0" : "1", 0,
+			MSI_STANDARD_EC_3G_MASK);
 
-	return 0;
+	return min(result, 0);
 }
 
 static const struct rfkill_ops rfkill_bluetooth_ops = {
@@ -618,18 +652,27 @@ static void rfkill_cleanup(void)
 	}
 }
 
+static bool msi_rfkill_set_state(struct rfkill *rfkill, bool blocked)
+{
+	if (quirks->ec_read_only)
+		return rfkill_set_hw_state(rfkill, blocked);
+	else
+		return rfkill_set_sw_state(rfkill, blocked);
+}
+
 static void msi_update_rfkill(struct work_struct *ignored)
 {
 	get_wireless_state_ec_standard();
 
 	if (rfk_wlan)
-		rfkill_set_sw_state(rfk_wlan, !wlan_s);
+		msi_rfkill_set_state(rfk_wlan, !wlan_s);
 	if (rfk_bluetooth)
-		rfkill_set_sw_state(rfk_bluetooth, !bluetooth_s);
+		msi_rfkill_set_state(rfk_bluetooth, !bluetooth_s);
 	if (rfk_threeg)
-		rfkill_set_sw_state(rfk_threeg, !threeg_s);
+		msi_rfkill_set_state(rfk_threeg, !threeg_s);
 }
-static DECLARE_DELAYED_WORK(msi_rfkill_work, msi_update_rfkill);
+static DECLARE_DELAYED_WORK(msi_rfkill_dwork, msi_update_rfkill);
+static DECLARE_WORK(msi_rfkill_work, msi_update_rfkill);
 
 static void msi_send_touchpad_key(struct work_struct *ignored)
 {
@@ -644,7 +687,8 @@ static void msi_send_touchpad_key(struct work_struct *ignored)
 		(rdata & MSI_STANDARD_EC_TOUCHPAD_MASK) ?
 		KEY_TOUCHPAD_ON : KEY_TOUCHPAD_OFF, 1, true);
 }
-static DECLARE_DELAYED_WORK(msi_touchpad_work, msi_send_touchpad_key);
+static DECLARE_DELAYED_WORK(msi_touchpad_dwork, msi_send_touchpad_key);
+static DECLARE_WORK(msi_touchpad_work, msi_send_touchpad_key);
 
 static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
 				struct serio *port)
@@ -662,14 +706,20 @@ static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
 		extended = false;
 		switch (data) {
 		case 0xE4:
-			schedule_delayed_work(&msi_touchpad_work,
-				round_jiffies_relative(0.5 * HZ));
+			if (quirks->ec_delay) {
+				schedule_delayed_work(&msi_touchpad_dwork,
+					round_jiffies_relative(0.5 * HZ));
+			} else
+				schedule_work(&msi_touchpad_work);
 			break;
 		case 0x54:
 		case 0x62:
 		case 0x76:
-			schedule_delayed_work(&msi_rfkill_work,
-				round_jiffies_relative(0.5 * HZ));
+			if (quirks->ec_delay) {
+				schedule_delayed_work(&msi_rfkill_dwork,
+					round_jiffies_relative(0.5 * HZ));
+			} else
+				schedule_work(&msi_rfkill_work);
 			break;
 		}
 	}
@@ -736,8 +786,11 @@ static int rfkill_init(struct platform_device *sdev)
 	}
 
 	/* schedule to run rfkill state initial */
-	schedule_delayed_work(&msi_rfkill_init,
-				round_jiffies_relative(1 * HZ));
+	if (quirks->ec_delay) {
+		schedule_delayed_work(&msi_rfkill_init,
+			round_jiffies_relative(1 * HZ));
+	} else
+		schedule_work(&msi_rfkill_work);
 
 	return 0;
 
@@ -761,7 +814,7 @@ static int msi_laptop_resume(struct device *device)
 	u8 data;
 	int result;
 
-	if (!load_scm_model)
+	if (!quirks->load_scm_model)
 		return 0;
 
 	/* set load SCM to disable hardware control by fn key */
@@ -819,13 +872,15 @@ static int __init load_scm_model_init(struct platform_device *sdev)
 	u8 data;
 	int result;
 
-	/* allow userland write sysfs file  */
-	dev_attr_bluetooth.store = store_bluetooth;
-	dev_attr_wlan.store = store_wlan;
-	dev_attr_threeg.store = store_threeg;
-	dev_attr_bluetooth.attr.mode |= S_IWUSR;
-	dev_attr_wlan.attr.mode |= S_IWUSR;
-	dev_attr_threeg.attr.mode |= S_IWUSR;
+	if (!quirks->ec_read_only) {
+		/* allow userland write sysfs file  */
+		dev_attr_bluetooth.store = store_bluetooth;
+		dev_attr_wlan.store = store_wlan;
+		dev_attr_threeg.store = store_threeg;
+		dev_attr_bluetooth.attr.mode |= S_IWUSR;
+		dev_attr_wlan.attr.mode |= S_IWUSR;
+		dev_attr_threeg.attr.mode |= S_IWUSR;
+	}
 
 	/* disable hardware control by fn key */
 	result = ec_read(MSI_STANDARD_EC_SCM_LOAD_ADDRESS, &data);
@@ -874,15 +929,16 @@ static int __init msi_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
-	if (force || dmi_check_system(msi_dmi_table))
-		old_ec_model = 1;
+	dmi_check_system(msi_dmi_table);
+	if (!quirks)
+		/* quirks may be NULL if no match in DMI table */
+		quirks = &quirk_load_scm_model;
+	if (force)
+		quirks = &quirk_old_ec_model;
 
-	if (!old_ec_model)
+	if (!quirks->old_ec_model)
 		get_threeg_exists();
 
-	if (!old_ec_model && dmi_check_system(msi_load_scm_models_dmi_table))
-		load_scm_model = 1;
-
 	if (auto_brightness < 0 || auto_brightness > 2)
 		return -EINVAL;
 
@@ -918,7 +974,7 @@ static int __init msi_init(void)
 	if (ret)
 		goto fail_platform_device1;
 
-	if (load_scm_model && (load_scm_model_init(msipf_device) < 0)) {
+	if (quirks->load_scm_model && (load_scm_model_init(msipf_device) < 0)) {
 		ret = -EINVAL;
 		goto fail_platform_device1;
 	}
@@ -928,7 +984,7 @@ static int __init msi_init(void)
 	if (ret)
 		goto fail_platform_device2;
 
-	if (!old_ec_model) {
+	if (!quirks->old_ec_model) {
 		if (threeg_exists)
 			ret = device_create_file(&msipf_device->dev,
 						&dev_attr_threeg);
@@ -949,9 +1005,10 @@ static int __init msi_init(void)
 
 fail_platform_device2:
 
-	if (load_scm_model) {
+	if (quirks->load_scm_model) {
 		i8042_remove_filter(msi_laptop_i8042_filter);
-		cancel_delayed_work_sync(&msi_rfkill_work);
+		cancel_delayed_work_sync(&msi_rfkill_dwork);
+		cancel_work_sync(&msi_rfkill_work);
 		rfkill_cleanup();
 	}
 	platform_device_del(msipf_device);
@@ -973,15 +1030,16 @@ fail_backlight:
 
 static void __exit msi_cleanup(void)
 {
-	if (load_scm_model) {
+	if (quirks->load_scm_model) {
 		i8042_remove_filter(msi_laptop_i8042_filter);
 		msi_laptop_input_destroy();
-		cancel_delayed_work_sync(&msi_rfkill_work);
+		cancel_delayed_work_sync(&msi_rfkill_dwork);
+		cancel_work_sync(&msi_rfkill_work);
 		rfkill_cleanup();
 	}
 
 	sysfs_remove_group(&msipf_device->dev.kobj, &msipf_attribute_group);
-	if (!old_ec_model && threeg_exists)
+	if (!quirks->old_ec_model && threeg_exists)
 		device_remove_file(&msipf_device->dev, &dev_attr_threeg);
 	platform_device_unregister(msipf_device);
 	platform_driver_unregister(&msipf_driver);
-- 
1.8.0.1

[PATCH v3 04/12] msi-laptop: Add MSI Wind U90/U100 support

[Maxim Mikityanskiy]

Add MSI Wind U90/U100 to DMI table and add some missing EC features
support such as basic fan control, turbo and ECO modes and touchpad
state. Tested on MSI Wind U100.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/msi-laptop.c | 123 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 121 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 0bf94b5..28bcbb2 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -82,8 +82,19 @@
 #define MSI_STANDARD_EC_SCM_LOAD_ADDRESS	0x2d
 #define MSI_STANDARD_EC_SCM_LOAD_MASK		(1 << 0)
 
-#define MSI_STANDARD_EC_TOUCHPAD_ADDRESS	0xe4
+#define MSI_STANDARD_EC_FUNCTIONS_ADDRESS	0xe4
+/* Power LED is orange - Turbo mode */
+#define MSI_STANDARD_EC_TURBO_MASK		(1 << 1)
+/* Power LED is green - ECO mode */
+#define MSI_STANDARD_EC_ECO_MASK		(1 << 3)
+/* Touchpad is turned on */
 #define MSI_STANDARD_EC_TOUCHPAD_MASK		(1 << 4)
+/* If this bit != bit 1, turbo mode can't be toggled */
+#define MSI_STANDARD_EC_TURBO_COOLDOWN_MASK	(1 << 7)
+
+#define MSI_STANDARD_EC_FAN_ADDRESS		0x33
+/* If zero, fan rotates at maximal speed */
+#define MSI_STANDARD_EC_AUTOFAN_MASK		(1 << 0)
 
 #ifdef CONFIG_PM_SLEEP
 static int msi_laptop_resume(struct device *device);
@@ -435,18 +446,115 @@ static ssize_t store_auto_brightness(struct device *dev,
 	return count;
 }
 
+static ssize_t show_touchpad(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+
+	u8 rdata;
+	int result;
+
+	result = ec_read(MSI_STANDARD_EC_FUNCTIONS_ADDRESS, &rdata);
+	if (result < 0)
+		return result;
+
+	return sprintf(buf, "%i\n", !!(rdata & MSI_STANDARD_EC_TOUCHPAD_MASK));
+}
+
+static ssize_t show_turbo(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+
+	u8 rdata;
+	int result;
+
+	result = ec_read(MSI_STANDARD_EC_FUNCTIONS_ADDRESS, &rdata);
+	if (result < 0)
+		return result;
+
+	return sprintf(buf, "%i\n", !!(rdata & MSI_STANDARD_EC_TURBO_MASK));
+}
+
+static ssize_t show_eco(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+
+	u8 rdata;
+	int result;
+
+	result = ec_read(MSI_STANDARD_EC_FUNCTIONS_ADDRESS, &rdata);
+	if (result < 0)
+		return result;
+
+	return sprintf(buf, "%i\n", !!(rdata & MSI_STANDARD_EC_ECO_MASK));
+}
+
+static ssize_t show_turbo_cooldown(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+
+	u8 rdata;
+	int result;
+
+	result = ec_read(MSI_STANDARD_EC_FUNCTIONS_ADDRESS, &rdata);
+	if (result < 0)
+		return result;
+
+	return sprintf(buf, "%i\n", (!!(rdata & MSI_STANDARD_EC_TURBO_MASK)) |
+		(!!(rdata & MSI_STANDARD_EC_TURBO_COOLDOWN_MASK) << 1));
+}
+
+static ssize_t show_auto_fan(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+
+	u8 rdata;
+	int result;
+
+	result = ec_read(MSI_STANDARD_EC_FAN_ADDRESS, &rdata);
+	if (result < 0)
+		return result;
+
+	return sprintf(buf, "%i\n", !!(rdata & MSI_STANDARD_EC_AUTOFAN_MASK));
+}
+
+static ssize_t store_auto_fan(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+
+	int enable, result;
+
+	if (sscanf(buf, "%i", &enable) != 1 || (enable != (enable & 1)))
+		return -EINVAL;
+
+	result = ec_write(MSI_STANDARD_EC_FAN_ADDRESS, enable);
+	if (result < 0)
+		return result;
+
+	return count;
+}
+
 static DEVICE_ATTR(lcd_level, 0644, show_lcd_level, store_lcd_level);
 static DEVICE_ATTR(auto_brightness, 0644, show_auto_brightness,
 		   store_auto_brightness);
 static DEVICE_ATTR(bluetooth, 0444, show_bluetooth, NULL);
 static DEVICE_ATTR(wlan, 0444, show_wlan, NULL);
 static DEVICE_ATTR(threeg, 0444, show_threeg, NULL);
+static DEVICE_ATTR(touchpad, 0444, show_touchpad, NULL);
+static DEVICE_ATTR(turbo_mode, 0444, show_turbo, NULL);
+static DEVICE_ATTR(eco_mode, 0444, show_eco, NULL);
+static DEVICE_ATTR(turbo_cooldown, 0444, show_turbo_cooldown, NULL);
+static DEVICE_ATTR(auto_fan, 0644, show_auto_fan, store_auto_fan);
 
 static struct attribute *msipf_attributes[] = {
 	&dev_attr_lcd_level.attr,
 	&dev_attr_auto_brightness.attr,
 	&dev_attr_bluetooth.attr,
 	&dev_attr_wlan.attr,
+	&dev_attr_touchpad.attr,
+	&dev_attr_turbo_mode.attr,
+	&dev_attr_eco_mode.attr,
+	&dev_attr_turbo_cooldown.attr,
+	&dev_attr_auto_fan.attr,
 	NULL
 };
 
@@ -590,6 +698,16 @@ static struct dmi_system_id __initdata msi_dmi_table[] = {
 		.driver_data = &quirk_load_scm_model,
 		.callback = dmi_check_cb
 	},
+	{
+		.ident = "MSI U90/U100",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR,
+				"MICRO-STAR INTERNATIONAL CO., LTD"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "U90/U100"),
+		},
+		.driver_data = &quirk_load_scm_ro_model,
+		.callback = dmi_check_cb
+	},
 	{ }
 };
 
@@ -679,7 +797,7 @@ static void msi_send_touchpad_key(struct work_struct *ignored)
 	u8 rdata;
 	int result;
 
-	result = ec_read(MSI_STANDARD_EC_TOUCHPAD_ADDRESS, &rdata);
+	result = ec_read(MSI_STANDARD_EC_FUNCTIONS_ADDRESS, &rdata);
 	if (result < 0)
 		return;
 
@@ -1069,3 +1187,4 @@ MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-N051:*");
 MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnMS-N014:*");
 MODULE_ALIAS("dmi:*:svnMicro-StarInternational*:pnCR620:*");
 MODULE_ALIAS("dmi:*:svnMicro-StarInternational*:pnU270series:*");
+MODULE_ALIAS("dmi:*:svnMICRO-STARINTERNATIONAL*:pnU90/U100:*");
-- 
1.8.0.1

[PATCH v3 05/12] msi-laptop: Add missing ABI documentation

[Maxim Mikityanskiy]

Add ABI documentation for all sysfs files exposed by msi-laptop driver.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 .../ABI/testing/sysfs-platform-msi-laptop          | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-msi-laptop

diff --git a/Documentation/ABI/testing/sysfs-platform-msi-laptop b/Documentation/ABI/testing/sysfs-platform-msi-laptop
new file mode 100644
index 0000000..307a247
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-msi-laptop
@@ -0,0 +1,83 @@
+What:		/sys/devices/platform/msi-laptop-pf/lcd_level
+Date:		Oct 2006
+KernelVersion:	2.6.19
+Contact:	"Lennart Poettering <mzxreary@0pointer.de>"
+Description:
+		Screen brightness: contains a single integer in the range 0..8.
+
+What:		/sys/devices/platform/msi-laptop-pf/auto_brightness
+Date:		Oct 2006
+KernelVersion:	2.6.19
+Contact:	"Lennart Poettering <mzxreary@0pointer.de>"
+Description:
+		Enable automatic brightness control: contains either 0 or 1. If
+		set to 1 the hardware adjusts the screen brightness
+		automatically when the power cord is plugged/unplugged.
+
+What:		/sys/devices/platform/msi-laptop-pf/wlan
+Date:		Oct 2006
+KernelVersion:	2.6.19
+Contact:	"Lennart Poettering <mzxreary@0pointer.de>"
+Description:
+		WLAN subsystem enabled: contains either 0 or 1.
+
+What:		/sys/devices/platform/msi-laptop-pf/bluetooth
+Date:		Oct 2006
+KernelVersion:	2.6.19
+Contact:	"Lennart Poettering <mzxreary@0pointer.de>"
+Description:
+		Bluetooth subsystem enabled: contains either 0 or 1. Please
+		note that this file is constantly 0 if no Bluetooth hardware is
+		available.
+
+What:		/sys/devices/platform/msi-laptop-pf/touchpad
+Date:		Nov 2012
+KernelVersion:	3.8
+Contact:	"Maxim Mikityanskiy <maxtram95@gmail.com>"
+Description:
+		Contains either 0 or 1 and indicates if touchpad is turned on.
+		Touchpad state can only be toggled by pressing Fn+F3.
+
+What:		/sys/devices/platform/msi-laptop-pf/turbo_mode
+Date:		Nov 2012
+KernelVersion:	3.8
+Contact:	"Maxim Mikityanskiy <maxtram95@gmail.com>"
+Description:
+		Contains either 0 or 1 and indicates if turbo mode is turned
+		on. In turbo mode power LED is orange and processor is
+		overclocked. Turbo mode is available only if charging. It is
+		only possible to toggle turbo mode state by pressing Fn+F10,
+		and there is a few seconds cooldown between subsequent toggles.
+		If user presses Fn+F10 too frequent, turbo mode state is not
+		changed.
+
+What:		/sys/devices/platform/msi-laptop-pf/eco_mode
+Date:		Nov 2012
+KernelVersion:	3.8
+Contact:	"Maxim Mikityanskiy <maxtram95@gmail.com>"
+Description:
+		Contains either 0 or 1 and indicates if ECO mode is turned on.
+		In ECO mode power LED is green and userspace should do some
+		powersaving actions. ECO mode is available only on battery
+		power. ECO mode can only be toggled by pressing Fn+F10.
+
+What:		/sys/devices/platform/msi-laptop-pf/turbo_cooldown
+Date:		Nov 2012
+KernelVersion:	3.8
+Contact:	"Maxim Mikityanskiy <maxtram95@gmail.com>"
+Description:
+		Contains value in range 0..3:
+			* 0 -> Turbo mode is off
+			* 1 -> Turbo mode is on, cannot be turned off yet
+			* 2 -> Turbo mode is off, cannot be turned on yet
+			* 3 -> Turbo mode is on
+
+What:		/sys/devices/platform/msi-laptop-pf/auto_fan
+Date:		Nov 2012
+KernelVersion:	3.8
+Contact:	"Maxim Mikityanskiy <maxtram95@gmail.com>"
+Description:
+		Contains either 0 or 1 and indicates if fan speed is controlled
+		automatically (1) or fan runs at maximal speed (0). Can be
+		toggled in software.
+
-- 
1.8.0.1

[PATCH v3 06/12] msi-laptop: Disable brightness control for new EC

[Maxim Mikityanskiy]

It seems that existing brightness control works only for old EC models.
On newer ones auto_brightness access always timeouts and lcd_level
always shows 0. So disable brightness control for new EC models. It
works fine with ACPI video driver anyway.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/msi-laptop.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 28bcbb2..6b22938 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -546,8 +546,6 @@ static DEVICE_ATTR(turbo_cooldown, 0444, show_turbo_cooldown, NULL);
 static DEVICE_ATTR(auto_fan, 0644, show_auto_fan, store_auto_fan);
 
 static struct attribute *msipf_attributes[] = {
-	&dev_attr_lcd_level.attr,
-	&dev_attr_auto_brightness.attr,
 	&dev_attr_bluetooth.attr,
 	&dev_attr_wlan.attr,
 	&dev_attr_touchpad.attr,
@@ -558,10 +556,20 @@ static struct attribute *msipf_attributes[] = {
 	NULL
 };
 
+static struct attribute *msipf_old_attributes[] = {
+	&dev_attr_lcd_level.attr,
+	&dev_attr_auto_brightness.attr,
+	NULL
+};
+
 static struct attribute_group msipf_attribute_group = {
 	.attrs = msipf_attributes
 };
 
+static struct attribute_group msipf_old_attribute_group = {
+	.attrs = msipf_old_attributes
+};
+
 static struct platform_driver msipf_driver = {
 	.driver = {
 		.name = "msi-laptop-pf",
@@ -1062,7 +1070,7 @@ static int __init msi_init(void)
 
 	/* Register backlight stuff */
 
-	if (acpi_video_backlight_support()) {
+	if (!quirks->old_ec_model || acpi_video_backlight_support()) {
 		pr_info("Brightness ignored, must be controlled by ACPI video driver\n");
 	} else {
 		struct backlight_properties props;
@@ -1108,14 +1116,19 @@ static int __init msi_init(void)
 						&dev_attr_threeg);
 		if (ret)
 			goto fail_platform_device2;
-	}
+	} else {
+		ret = sysfs_create_group(&msipf_device->dev.kobj,
+					 &msipf_old_attribute_group);
+		if (ret)
+			goto fail_platform_device2;
 
-	/* Disable automatic brightness control by default because
-	 * this module was probably loaded to do brightness control in
-	 * software. */
+		/* Disable automatic brightness control by default because
+		 * this module was probably loaded to do brightness control in
+		 * software. */
 
-	if (auto_brightness != 2)
-		set_auto_brightness(auto_brightness);
+		if (auto_brightness != 2)
+			set_auto_brightness(auto_brightness);
+	}
 
 	pr_info("driver " MSI_DRIVER_VERSION " successfully loaded\n");
 
@@ -1163,9 +1176,11 @@ static void __exit msi_cleanup(void)
 	platform_driver_unregister(&msipf_driver);
 	backlight_device_unregister(msibl_device);
 
-	/* Enable automatic brightness control again */
-	if (auto_brightness != 2)
-		set_auto_brightness(1);
+	if (quirks->old_ec_model) {
+		/* Enable automatic brightness control again */
+		if (auto_brightness != 2)
+			set_auto_brightness(1);
+	}
 
 	pr_info("driver unloaded\n");
 }
-- 
1.8.0.1

[PATCH v3 07/12] msi-wmi: Fix memory leak

[Maxim Mikityanskiy]

Fix memory leak - don't forget to kfree ACPI object when returning from
msi_wmi_notify() after suppressing key event.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Acked-by: Anisse Astier <anisse@astier.eu>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/msi-wmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 2264331..b96766b 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -176,7 +176,7 @@ static void msi_wmi_notify(u32 value, void *context)
 				pr_debug("Suppressed key event 0x%X - "
 					 "Last press was %lld us ago\n",
 					 key->code, ktime_to_us(diff));
-				return;
+				goto msi_wmi_notify_exit;
 			}
 			last_pressed[key->code - SCANCODE_BASE] = cur;
 
@@ -195,6 +195,8 @@ static void msi_wmi_notify(u32 value, void *context)
 			pr_info("Unknown key pressed - %x\n", eventcode);
 	} else
 		pr_info("Unknown event received\n");
+
+msi_wmi_notify_exit:
 	kfree(response.pointer);
 }
 
-- 
1.8.0.1

[PATCH v3 08/12] msi-wmi: Avoid repeating constants

[Maxim Mikityanskiy]

Use GUID defines in MODULE_ALIAS strings to avoid repeating strings.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/platform/x86/msi-wmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index b96766b..4db0c55 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -34,14 +34,14 @@ MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
 MODULE_DESCRIPTION("MSI laptop WMI hotkeys driver");
 MODULE_LICENSE("GPL");
 
-MODULE_ALIAS("wmi:551A1F84-FBDD-4125-91DB-3EA8F44F1D45");
-MODULE_ALIAS("wmi:B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2");
-
 #define DRV_NAME "msi-wmi"
 
 #define MSIWMI_BIOS_GUID "551A1F84-FBDD-4125-91DB-3EA8F44F1D45"
 #define MSIWMI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
 
+MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
+MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
+
 #define SCANCODE_BASE 0xD0
 #define MSI_WMI_BRIGHTNESSUP   SCANCODE_BASE
 #define MSI_WMI_BRIGHTNESSDOWN (SCANCODE_BASE + 1)
-- 
1.8.0.1

Re: [PATCH v3 08/12] msi-wmi: Avoid repeating constants

[joeyli]

於 五，2012-12-07 於 15:49 +0200，Maxim Mikityanskiy 提到：
> Use GUID defines in MODULE_ALIAS strings to avoid repeating strings.
> 
> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>

Acked-by: Lee, Chun-Yi <jlee@suse.com>

Thanks your patch
Joey Lee

> ---
>  drivers/platform/x86/msi-wmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
> index b96766b..4db0c55 100644
> --- a/drivers/platform/x86/msi-wmi.c
> +++ b/drivers/platform/x86/msi-wmi.c
> @@ -34,14 +34,14 @@ MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
>  MODULE_DESCRIPTION("MSI laptop WMI hotkeys driver");
>  MODULE_LICENSE("GPL");
>  
> -MODULE_ALIAS("wmi:551A1F84-FBDD-4125-91DB-3EA8F44F1D45");
> -MODULE_ALIAS("wmi:B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2");
> -
>  #define DRV_NAME "msi-wmi"
>  
>  #define MSIWMI_BIOS_GUID "551A1F84-FBDD-4125-91DB-3EA8F44F1D45"
>  #define MSIWMI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
>  
> +MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
> +MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
> +
>  #define SCANCODE_BASE 0xD0
>  #define MSI_WMI_BRIGHTNESSUP   SCANCODE_BASE
>  #define MSI_WMI_BRIGHTNESSDOWN (SCANCODE_BASE + 1)

[PATCH v3 09/12] msi-wmi: Use enums for scancodes

[Maxim Mikityanskiy]

Use enums for consecutive scancodes, rename key names from MSI_WMI_* to
MSI_KEY_* and use tabs for whitespace in msi_wmi_keymap.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/platform/x86/msi-wmi.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 4db0c55..112ec14 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -42,19 +42,21 @@ MODULE_LICENSE("GPL");
 MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
 MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
 
-#define SCANCODE_BASE 0xD0
-#define MSI_WMI_BRIGHTNESSUP   SCANCODE_BASE
-#define MSI_WMI_BRIGHTNESSDOWN (SCANCODE_BASE + 1)
-#define MSI_WMI_VOLUMEUP       (SCANCODE_BASE + 2)
-#define MSI_WMI_VOLUMEDOWN     (SCANCODE_BASE + 3)
-#define MSI_WMI_MUTE           (SCANCODE_BASE + 4)
+enum msi_scancodes {
+	MSI_SCANCODE_BASE	= 0xD0,
+	MSI_KEY_BRIGHTNESSUP	= MSI_SCANCODE_BASE,
+	MSI_KEY_BRIGHTNESSDOWN,
+	MSI_KEY_VOLUMEUP,
+	MSI_KEY_VOLUMEDOWN,
+	MSI_KEY_MUTE,
+};
 static struct key_entry msi_wmi_keymap[] = {
-	{ 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_KEY, MSI_WMI_MUTE,           {KEY_MUTE} },
-	{ KE_END, 0}
+	{ KE_KEY, MSI_KEY_BRIGHTNESSUP,		{KEY_BRIGHTNESSUP} },
+	{ KE_KEY, MSI_KEY_BRIGHTNESSDOWN,	{KEY_BRIGHTNESSDOWN} },
+	{ KE_KEY, MSI_KEY_VOLUMEUP,		{KEY_VOLUMEUP} },
+	{ KE_KEY, MSI_KEY_VOLUMEDOWN,		{KEY_VOLUMEDOWN} },
+	{ KE_KEY, MSI_KEY_MUTE,			{KEY_MUTE} },
+	{ KE_END, 0 }
 };
 static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
 
@@ -169,7 +171,7 @@ static void msi_wmi_notify(u32 value, void *context)
 			ktime_t diff;
 			cur = ktime_get_real();
 			diff = ktime_sub(cur, last_pressed[key->code -
-					SCANCODE_BASE]);
+					MSI_SCANCODE_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(diff) < 1000 * 50) {
@@ -178,13 +180,13 @@ static void msi_wmi_notify(u32 value, void *context)
 					 key->code, ktime_to_us(diff));
 				goto msi_wmi_notify_exit;
 			}
-			last_pressed[key->code - SCANCODE_BASE] = cur;
+			last_pressed[key->code - MSI_SCANCODE_BASE] = cur;
 
 			if (key->type == KE_KEY &&
 			/* Brightness is served via acpi video driver */
 			(!acpi_video_backlight_support() ||
-			(key->code != MSI_WMI_BRIGHTNESSUP &&
-			key->code != MSI_WMI_BRIGHTNESSDOWN))) {
+			(key->code != MSI_KEY_BRIGHTNESSUP &&
+			key->code != MSI_KEY_BRIGHTNESSDOWN))) {
 				pr_debug("Send key: 0x%X - "
 					 "Input layer keycode: %d\n",
 					 key->code, key->keycode);
-- 
1.8.0.1

Re: [PATCH v3 09/12] msi-wmi: Use enums for scancodes

[joeyli]

於 五，2012-12-07 於 15:49 +0200，Maxim Mikityanskiy 提到：
> Use enums for consecutive scancodes, rename key names from MSI_WMI_* to
> MSI_KEY_* and use tabs for whitespace in msi_wmi_keymap.
> 
> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>

Acked-by: Lee, Chun-Yi <jlee@suse.com>

Thanks
Joey Lee

> ---
>  drivers/platform/x86/msi-wmi.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
> index 4db0c55..112ec14 100644
> --- a/drivers/platform/x86/msi-wmi.c
> +++ b/drivers/platform/x86/msi-wmi.c
> @@ -42,19 +42,21 @@ MODULE_LICENSE("GPL");
>  MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
>  MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
>  
> -#define SCANCODE_BASE 0xD0
> -#define MSI_WMI_BRIGHTNESSUP   SCANCODE_BASE
> -#define MSI_WMI_BRIGHTNESSDOWN (SCANCODE_BASE + 1)
> -#define MSI_WMI_VOLUMEUP       (SCANCODE_BASE + 2)
> -#define MSI_WMI_VOLUMEDOWN     (SCANCODE_BASE + 3)
> -#define MSI_WMI_MUTE           (SCANCODE_BASE + 4)
> +enum msi_scancodes {
> +	MSI_SCANCODE_BASE	= 0xD0,
> +	MSI_KEY_BRIGHTNESSUP	= MSI_SCANCODE_BASE,
> +	MSI_KEY_BRIGHTNESSDOWN,
> +	MSI_KEY_VOLUMEUP,
> +	MSI_KEY_VOLUMEDOWN,
> +	MSI_KEY_MUTE,
> +};
>  static struct key_entry msi_wmi_keymap[] = {
> -	{ 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_KEY, MSI_WMI_MUTE,           {KEY_MUTE} },
> -	{ KE_END, 0}
> +	{ KE_KEY, MSI_KEY_BRIGHTNESSUP,		{KEY_BRIGHTNESSUP} },
> +	{ KE_KEY, MSI_KEY_BRIGHTNESSDOWN,	{KEY_BRIGHTNESSDOWN} },
> +	{ KE_KEY, MSI_KEY_VOLUMEUP,		{KEY_VOLUMEUP} },
> +	{ KE_KEY, MSI_KEY_VOLUMEDOWN,		{KEY_VOLUMEDOWN} },
> +	{ KE_KEY, MSI_KEY_MUTE,			{KEY_MUTE} },
> +	{ KE_END, 0 }
>  };
>  static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
>  
> @@ -169,7 +171,7 @@ static void msi_wmi_notify(u32 value, void *context)
>  			ktime_t diff;
>  			cur = ktime_get_real();
>  			diff = ktime_sub(cur, last_pressed[key->code -
> -					SCANCODE_BASE]);
> +					MSI_SCANCODE_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(diff) < 1000 * 50) {
> @@ -178,13 +180,13 @@ static void msi_wmi_notify(u32 value, void *context)
>  					 key->code, ktime_to_us(diff));
>  				goto msi_wmi_notify_exit;
>  			}
> -			last_pressed[key->code - SCANCODE_BASE] = cur;
> +			last_pressed[key->code - MSI_SCANCODE_BASE] = cur;
>  
>  			if (key->type == KE_KEY &&
>  			/* Brightness is served via acpi video driver */
>  			(!acpi_video_backlight_support() ||
> -			(key->code != MSI_WMI_BRIGHTNESSUP &&
> -			key->code != MSI_WMI_BRIGHTNESSDOWN))) {
> +			(key->code != MSI_KEY_BRIGHTNESSUP &&
> +			key->code != MSI_KEY_BRIGHTNESSDOWN))) {
>  				pr_debug("Send key: 0x%X - "
>  					 "Input layer keycode: %d\n",
>  					 key->code, key->keycode);

[PATCH v3 10/12] msi-wmi: Make keys and backlight independent

[Maxim Mikityanskiy]

Introduced function msi_wmi_backlight_setup() that initializes backlight
device. Made driver load and work if only one WMI (only for hotkeys or
only for backlight) is present.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/platform/x86/msi-wmi.c | 101 ++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 112ec14..3a60619 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -60,6 +60,8 @@ static struct key_entry msi_wmi_keymap[] = {
 };
 static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
 
+static const char *event_wmi_guid;
+
 static struct backlight_device *backlight;
 
 static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF };
@@ -184,7 +186,7 @@ static void msi_wmi_notify(u32 value, void *context)
 
 			if (key->type == KE_KEY &&
 			/* Brightness is served via acpi video driver */
-			(!acpi_video_backlight_support() ||
+			(backlight ||
 			(key->code != MSI_KEY_BRIGHTNESSUP &&
 			key->code != MSI_KEY_BRIGHTNESSDOWN))) {
 				pr_debug("Send key: 0x%X - "
@@ -202,6 +204,31 @@ msi_wmi_notify_exit:
 	kfree(response.pointer);
 }
 
+static int __init msi_wmi_backlight_setup(void)
+{
+	int err;
+	struct backlight_properties props;
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_PLATFORM;
+	props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
+	backlight = backlight_device_register(DRV_NAME, NULL, NULL,
+					      &msi_backlight_ops,
+					      &props);
+	if (IS_ERR(backlight))
+		return PTR_ERR(backlight);
+
+	err = bl_get(NULL);
+	if (err < 0) {
+		backlight_device_unregister(backlight);
+		return err;
+	}
+
+	backlight->props.brightness = err;
+
+	return 0;
+}
+
 static int __init msi_wmi_input_setup(void)
 {
 	int err;
@@ -238,60 +265,60 @@ static int __init msi_wmi_init(void)
 {
 	int err;
 
-	if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
-		pr_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 (ACPI_FAILURE(err))
-		return -EINVAL;
+	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
+		err = msi_wmi_input_setup();
+		if (err) {
+			pr_err("Unable to setup input device\n");
+			return err;
+		}
 
-	err = msi_wmi_input_setup();
-	if (err)
-		goto err_uninstall_notifier;
-
-	if (!acpi_video_backlight_support()) {
-		struct backlight_properties props;
-		memset(&props, 0, sizeof(struct backlight_properties));
-		props.type = BACKLIGHT_PLATFORM;
-		props.max_brightness = ARRAY_SIZE(backlight_map) - 1;
-		backlight = backlight_device_register(DRV_NAME, NULL, NULL,
-						      &msi_backlight_ops,
-						      &props);
-		if (IS_ERR(backlight)) {
-			err = PTR_ERR(backlight);
+		err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
+			msi_wmi_notify, NULL);
+		if (ACPI_FAILURE(err)) {
+			pr_err("Unable to setup WMI notify handler\n");
 			goto err_free_input;
 		}
 
-		err = bl_get(NULL);
-		if (err < 0)
-			goto err_free_backlight;
+		pr_debug("Event handler installed\n");
+		event_wmi_guid = MSIWMI_EVENT_GUID;
+	}
+
+	if (wmi_has_guid(MSIWMI_BIOS_GUID) && !acpi_video_backlight_support()) {
+		err = msi_wmi_backlight_setup();
+		if (err) {
+			pr_err("Unable to setup backlight device\n");
+			goto err_uninstall_handler;
+		}
+		pr_debug("Backlight device created\n");
+	}
 
-		backlight->props.brightness = err;
+	if (!event_wmi_guid && !backlight) {
+		pr_err("This machine doesn't have neither MSI-hotkeys nor backlight through WMI\n");
+		return -ENODEV;
 	}
-	pr_debug("Event handler installed\n");
 
 	return 0;
 
-err_free_backlight:
-	backlight_device_unregister(backlight);
+err_uninstall_handler:
+	if (event_wmi_guid)
+		wmi_remove_notify_handler(event_wmi_guid);
 err_free_input:
-	sparse_keymap_free(msi_wmi_input_dev);
-	input_unregister_device(msi_wmi_input_dev);
-err_uninstall_notifier:
-	wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+	if (event_wmi_guid) {
+		sparse_keymap_free(msi_wmi_input_dev);
+		input_unregister_device(msi_wmi_input_dev);
+	}
 	return err;
 }
 
 static void __exit msi_wmi_exit(void)
 {
-	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
-		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+	if (event_wmi_guid) {
+		wmi_remove_notify_handler(event_wmi_guid);
 		sparse_keymap_free(msi_wmi_input_dev);
 		input_unregister_device(msi_wmi_input_dev);
-		backlight_device_unregister(backlight);
 	}
+	if (backlight)
+		backlight_device_unregister(backlight);
 }
 
 module_init(msi_wmi_init);
-- 
1.8.0.1

[PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Maxim Mikityanskiy]

Introduced quirk_last_pressed variable that would indicate if
last_pressed is used or not. Made it work even if scancode sequence is
sparse.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/platform/x86/msi-wmi.c | 43 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 3a60619..273f647 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -43,8 +43,7 @@ MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
 MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
 
 enum msi_scancodes {
-	MSI_SCANCODE_BASE	= 0xD0,
-	MSI_KEY_BRIGHTNESSUP	= MSI_SCANCODE_BASE,
+	MSI_KEY_BRIGHTNESSUP	= 0xD0,
 	MSI_KEY_BRIGHTNESSDOWN,
 	MSI_KEY_VOLUMEUP,
 	MSI_KEY_VOLUMEDOWN,
@@ -59,6 +58,7 @@ static struct key_entry msi_wmi_keymap[] = {
 	{ KE_END, 0 }
 };
 static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
+static bool quirk_last_pressed;
 
 static const char *event_wmi_guid;
 
@@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
 		pr_debug("Eventcode: 0x%x\n", eventcode);
 		key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
 				eventcode);
-		if (key) {
+		if (!key) {
+			pr_info("Unknown key pressed - %x\n", eventcode);
+			goto msi_wmi_notify_exit;
+		}
+		if (quirk_last_pressed) {
+			size_t key_index = key - msi_wmi_keymap;
 			ktime_t diff;
 			cur = ktime_get_real();
-			diff = ktime_sub(cur, last_pressed[key->code -
-					MSI_SCANCODE_BASE]);
+			diff = ktime_sub(cur, last_pressed[key_index]);
 			/* Ignore event if the same event happened in a 50 ms
 			   timeframe -> Key press may result in 10-20 GPEs */
 			if (ktime_to_us(diff) < 1000 * 50) {
@@ -182,21 +186,19 @@ static void msi_wmi_notify(u32 value, void *context)
 					 key->code, ktime_to_us(diff));
 				goto msi_wmi_notify_exit;
 			}
-			last_pressed[key->code - MSI_SCANCODE_BASE] = cur;
-
-			if (key->type == KE_KEY &&
-			/* Brightness is served via acpi video driver */
-			(backlight ||
-			(key->code != MSI_KEY_BRIGHTNESSUP &&
-			key->code != MSI_KEY_BRIGHTNESSDOWN))) {
-				pr_debug("Send key: 0x%X - "
-					 "Input layer keycode: %d\n",
-					 key->code, key->keycode);
-				sparse_keymap_report_entry(msi_wmi_input_dev,
-						key, 1, true);
-			}
-		} else
-			pr_info("Unknown key pressed - %x\n", eventcode);
+			last_pressed[key_index] = cur;
+		}
+
+		if (key->type == KE_KEY &&
+		/* Brightness is served via acpi video driver */
+		(backlight ||
+		(key->code != MSI_KEY_BRIGHTNESSUP &&
+		key->code != MSI_KEY_BRIGHTNESSDOWN))) {
+			pr_debug("Send key: 0x%X - Input layer keycode: %d\n",
+				 key->code, key->keycode);
+			sparse_keymap_report_entry(msi_wmi_input_dev, key, 1,
+						   true);
+		}
 	} else
 		pr_info("Unknown event received\n");
 
@@ -281,6 +283,7 @@ static int __init msi_wmi_init(void)
 
 		pr_debug("Event handler installed\n");
 		event_wmi_guid = MSIWMI_EVENT_GUID;
+		quirk_last_pressed = true;
 	}
 
 	if (wmi_has_guid(MSIWMI_BIOS_GUID) && !acpi_video_backlight_support()) {
-- 
1.8.0.1

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Anisse Astier]

On Fri,  7 Dec 2012 15:49:21 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> Introduced quirk_last_pressed variable that would indicate if
> last_pressed is used or not. Made it work even if scancode sequence is
> sparse.
> 
> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> ---
>  drivers/platform/x86/msi-wmi.c | 43 ++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
> index 3a60619..273f647 100644
> --- a/drivers/platform/x86/msi-wmi.c
> +++ b/drivers/platform/x86/msi-wmi.c
> @@ -43,8 +43,7 @@ MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
>  MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
>  
>  enum msi_scancodes {
> -	MSI_SCANCODE_BASE	= 0xD0,
> -	MSI_KEY_BRIGHTNESSUP	= MSI_SCANCODE_BASE,
> +	MSI_KEY_BRIGHTNESSUP	= 0xD0,
>  	MSI_KEY_BRIGHTNESSDOWN,
>  	MSI_KEY_VOLUMEUP,
>  	MSI_KEY_VOLUMEDOWN,
> @@ -59,6 +58,7 @@ static struct key_entry msi_wmi_keymap[] = {
>  	{ KE_END, 0 }
>  };
>  static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
> +static bool quirk_last_pressed;
>  
>  static const char *event_wmi_guid;
>  
> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
>  		pr_debug("Eventcode: 0x%x\n", eventcode);
>  		key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
>  				eventcode);
> -		if (key) {
> +		if (!key) {
> +			pr_info("Unknown key pressed - %x\n", eventcode);
> +			goto msi_wmi_notify_exit;
> +		}
> +		if (quirk_last_pressed) {
> +			size_t key_index = key - msi_wmi_keymap;
Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
intent here otherwise.


>  			ktime_t diff;
>  			cur = ktime_get_real();
> -			diff = ktime_sub(cur, last_pressed[key->code -
> -					MSI_SCANCODE_BASE]);
> +			diff = ktime_sub(cur, last_pressed[key_index]);
>  			/* Ignore event if the same event happened in a 50 ms
>  			   timeframe -> Key press may result in 10-20 GPEs */
>  			if (ktime_to_us(diff) < 1000 * 50) {
> @@ -182,21 +186,19 @@ static void msi_wmi_notify(u32 value, void *context)
>  					 key->code, ktime_to_us(diff));
>  				goto msi_wmi_notify_exit;
>  			}
> -			last_pressed[key->code - MSI_SCANCODE_BASE] = cur;
> -
> -			if (key->type == KE_KEY &&
> -			/* Brightness is served via acpi video driver */
> -			(backlight ||
> -			(key->code != MSI_KEY_BRIGHTNESSUP &&
> -			key->code != MSI_KEY_BRIGHTNESSDOWN))) {
> -				pr_debug("Send key: 0x%X - "
> -					 "Input layer keycode: %d\n",
> -					 key->code, key->keycode);
> -				sparse_keymap_report_entry(msi_wmi_input_dev,
> -						key, 1, true);
> -			}
> -		} else
> -			pr_info("Unknown key pressed - %x\n", eventcode);
> +			last_pressed[key_index] = cur;
> +		}
> +
> +		if (key->type == KE_KEY &&
> +		/* Brightness is served via acpi video driver */
> +		(backlight ||
> +		(key->code != MSI_KEY_BRIGHTNESSUP &&
> +		key->code != MSI_KEY_BRIGHTNESSDOWN))) {
> +			pr_debug("Send key: 0x%X - Input layer keycode: %d\n",
> +				 key->code, key->keycode);
> +			sparse_keymap_report_entry(msi_wmi_input_dev, key, 1,
> +						   true);
> +		}
>  	} else
>  		pr_info("Unknown event received\n");
>  
> @@ -281,6 +283,7 @@ static int __init msi_wmi_init(void)
>  
>  		pr_debug("Event handler installed\n");
>  		event_wmi_guid = MSIWMI_EVENT_GUID;
> +		quirk_last_pressed = true;
>  	}
>  
>  	if (wmi_has_guid(MSIWMI_BIOS_GUID) && !acpi_video_backlight_support()) {

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Maxim Mikityanskiy]

2012/12/11 Anisse Astier <anisse@astier.eu>:
> On Fri,  7 Dec 2012 15:49:21 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>
>> Introduced quirk_last_pressed variable that would indicate if
>> last_pressed is used or not. Made it work even if scancode sequence is
>> sparse.
>>
>> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
>> ---
>>  drivers/platform/x86/msi-wmi.c | 43 ++++++++++++++++++++++--------------------
>>  1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
>> index 3a60619..273f647 100644
>> --- a/drivers/platform/x86/msi-wmi.c
>> +++ b/drivers/platform/x86/msi-wmi.c
>> @@ -43,8 +43,7 @@ MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
>>  MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
>>
>>  enum msi_scancodes {
>> -     MSI_SCANCODE_BASE       = 0xD0,
>> -     MSI_KEY_BRIGHTNESSUP    = MSI_SCANCODE_BASE,
>> +     MSI_KEY_BRIGHTNESSUP    = 0xD0,
>>       MSI_KEY_BRIGHTNESSDOWN,
>>       MSI_KEY_VOLUMEUP,
>>       MSI_KEY_VOLUMEDOWN,
>> @@ -59,6 +58,7 @@ static struct key_entry msi_wmi_keymap[] = {
>>       { KE_END, 0 }
>>  };
>>  static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
>> +static bool quirk_last_pressed;
>>
>>  static const char *event_wmi_guid;
>>
>> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
>>               pr_debug("Eventcode: 0x%x\n", eventcode);
>>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
>>                               eventcode);
>> -             if (key) {
>> +             if (!key) {
>> +                     pr_info("Unknown key pressed - %x\n", eventcode);
>> +                     goto msi_wmi_notify_exit;
>> +             }
>> +             if (quirk_last_pressed) {
>> +                     size_t key_index = key - msi_wmi_keymap;
> Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> intent here otherwise.

msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
first item. key is a pointer to some array's item. So 'key -
msi_wmi_keymap' is a difference between pointers, i.e. index of key in
msi_wmi_keymap.

I do pointer arithmetic here because in patch 12 I add some new
scancodes, and holes appear in scancode sequence, so we can't just use
'key->code - MSI_SCANCODE_BASE' to get item index in array.

>
>>                       ktime_t diff;
>>                       cur = ktime_get_real();
>> -                     diff = ktime_sub(cur, last_pressed[key->code -
>> -                                     MSI_SCANCODE_BASE]);
>> +                     diff = ktime_sub(cur, last_pressed[key_index]);
>>                       /* Ignore event if the same event happened in a 50 ms
>>                          timeframe -> Key press may result in 10-20 GPEs */
>>                       if (ktime_to_us(diff) < 1000 * 50) {
>> @@ -182,21 +186,19 @@ static void msi_wmi_notify(u32 value, void *context)
>>                                        key->code, ktime_to_us(diff));
>>                               goto msi_wmi_notify_exit;
>>                       }
>> -                     last_pressed[key->code - MSI_SCANCODE_BASE] = cur;
>> -
>> -                     if (key->type == KE_KEY &&
>> -                     /* Brightness is served via acpi video driver */
>> -                     (backlight ||
>> -                     (key->code != MSI_KEY_BRIGHTNESSUP &&
>> -                     key->code != MSI_KEY_BRIGHTNESSDOWN))) {
>> -                             pr_debug("Send key: 0x%X - "
>> -                                      "Input layer keycode: %d\n",
>> -                                      key->code, key->keycode);
>> -                             sparse_keymap_report_entry(msi_wmi_input_dev,
>> -                                             key, 1, true);
>> -                     }
>> -             } else
>> -                     pr_info("Unknown key pressed - %x\n", eventcode);
>> +                     last_pressed[key_index] = cur;
>> +             }
>> +
>> +             if (key->type == KE_KEY &&
>> +             /* Brightness is served via acpi video driver */
>> +             (backlight ||
>> +             (key->code != MSI_KEY_BRIGHTNESSUP &&
>> +             key->code != MSI_KEY_BRIGHTNESSDOWN))) {
>> +                     pr_debug("Send key: 0x%X - Input layer keycode: %d\n",
>> +                              key->code, key->keycode);
>> +                     sparse_keymap_report_entry(msi_wmi_input_dev, key, 1,
>> +                                                true);
>> +             }
>>       } else
>>               pr_info("Unknown event received\n");
>>
>> @@ -281,6 +283,7 @@ static int __init msi_wmi_init(void)
>>
>>               pr_debug("Event handler installed\n");
>>               event_wmi_guid = MSIWMI_EVENT_GUID;
>> +             quirk_last_pressed = true;
>>       }
>>
>>       if (wmi_has_guid(MSIWMI_BIOS_GUID) && !acpi_video_backlight_support()) {

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Anisse Astier]

On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> >>                               eventcode);
> >> -             if (key) {
> >> +             if (!key) {
> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> >> +                     goto msi_wmi_notify_exit;
> >> +             }
> >> +             if (quirk_last_pressed) {
> >> +                     size_t key_index = key - msi_wmi_keymap;  
> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> > intent here otherwise.  
> 
> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> first item. key is a pointer to some array's item. So 'key -
> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> msi_wmi_keymap.
> 
> I do pointer arithmetic here because in patch 12 I add some new
> scancodes, and holes appear in scancode sequence, so we can't just use
> 'key->code - MSI_SCANCODE_BASE' to get item index in array.

Oh, I see. This is very clever, but a bit too clever. You have no
guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
to *your* key_entry. In fact, it doesn't.

In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
want to use this method, you might need to re-compute the index by
iterating over the elements and comparing key->code for each.


Regards,

Anisse

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Maxim Mikityanskiy]

2012/12/11 Anisse Astier <anisse@astier.eu>:
> On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>
>> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
>> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
>> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
>> >>                               eventcode);
>> >> -             if (key) {
>> >> +             if (!key) {
>> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
>> >> +                     goto msi_wmi_notify_exit;
>> >> +             }
>> >> +             if (quirk_last_pressed) {
>> >> +                     size_t key_index = key - msi_wmi_keymap;
>> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
>> > intent here otherwise.
>>
>> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
>> first item. key is a pointer to some array's item. So 'key -
>> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
>> msi_wmi_keymap.
>>
>> I do pointer arithmetic here because in patch 12 I add some new
>> scancodes, and holes appear in scancode sequence, so we can't just use
>> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
>
> Oh, I see. This is very clever, but a bit too clever. You have no
> guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> to *your* key_entry. In fact, it doesn't.
>
> In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> want to use this method, you might need to re-compute the index by
> iterating over the elements and comparing key->code for each.

Oops, I'm sorry, I was looking only into
sparse_keymap_entry_from_scancode() and I didn't discover the fact
that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
avoid iterating through the array second time. Also, can I use
msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
input_dev::keycode field is documented at include/linux/input.h:62.

It's strange that I didn't get a crash or data corruption on my system
when I forced usage of last_pressed for my laptop, loaded this module
and tried to press keys.

It's very unconvenient that struct key_entry does not have some field
for driver-specific extra data. In such field we could store index in
last_pressed or event last press time. But we haven't such field.

>
> Regards,
>
> Anisse

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Anisse Astier]

On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> 2012/12/11 Anisse Astier <anisse@astier.eu>:
> > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> >
> >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> >> >>                               eventcode);
> >> >> -             if (key) {
> >> >> +             if (!key) {
> >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> >> >> +                     goto msi_wmi_notify_exit;
> >> >> +             }
> >> >> +             if (quirk_last_pressed) {
> >> >> +                     size_t key_index = key - msi_wmi_keymap;
> >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> >> > intent here otherwise.
> >>
> >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> >> first item. key is a pointer to some array's item. So 'key -
> >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> >> msi_wmi_keymap.
> >>
> >> I do pointer arithmetic here because in patch 12 I add some new
> >> scancodes, and holes appear in scancode sequence, so we can't just use
> >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
> >
> > Oh, I see. This is very clever, but a bit too clever. You have no
> > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> > to *your* key_entry. In fact, it doesn't.
> >
> > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> > want to use this method, you might need to re-compute the index by
> > iterating over the elements and comparing key->code for each.
> 
> Oops, I'm sorry, I was looking only into
> sparse_keymap_entry_from_scancode() and I didn't discover the fact
> that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
> avoid iterating through the array second time. Also, can I use
> msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
> input_dev::keycode field is documented at include/linux/input.h:62.

As I said, you could iterate over msi_wmi_keymap and compare key->code to
each keycode to compute key_index each time.

> 
> It's strange that I didn't get a crash or data corruption on my system
> when I forced usage of last_pressed for my laptop, loaded this module
> and tried to press keys.
> 
> It's very unconvenient that struct key_entry does not have some field
> for driver-specific extra data. In such field we could store index in
> last_pressed or event last press time. But we haven't such field.

If that's really needed, such field could be added. Cc-ing Dmitry
Torokhov to ask what he thinks about altering the sparse keymap API for
this use case, ie having per key_entry variables to store information,
here used for debouncing.
Previously the keymap was contiguous, but we're adding new keys, so it'll
really be sparse, so we can't use the same trick substraction trick.

Regards,

Anisse

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Dmitry Torokhov]

On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
> On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> 
> > 2012/12/11 Anisse Astier <anisse@astier.eu>:
> > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> > >
> > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> > >> >>                               eventcode);
> > >> >> -             if (key) {
> > >> >> +             if (!key) {
> > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> > >> >> +                     goto msi_wmi_notify_exit;
> > >> >> +             }
> > >> >> +             if (quirk_last_pressed) {
> > >> >> +                     size_t key_index = key - msi_wmi_keymap;
> > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> > >> > intent here otherwise.
> > >>
> > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> > >> first item. key is a pointer to some array's item. So 'key -
> > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> > >> msi_wmi_keymap.
> > >>
> > >> I do pointer arithmetic here because in patch 12 I add some new
> > >> scancodes, and holes appear in scancode sequence, so we can't just use
> > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
> > >
> > > Oh, I see. This is very clever, but a bit too clever. You have no
> > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> > > to *your* key_entry. In fact, it doesn't.
> > >
> > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> > > want to use this method, you might need to re-compute the index by
> > > iterating over the elements and comparing key->code for each.
> > 
> > Oops, I'm sorry, I was looking only into
> > sparse_keymap_entry_from_scancode() and I didn't discover the fact
> > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
> > avoid iterating through the array second time. Also, can I use
> > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
> > input_dev::keycode field is documented at include/linux/input.h:62.
> 
> As I said, you could iterate over msi_wmi_keymap and compare key->code to
> each keycode to compute key_index each time.
> 
> > 
> > It's strange that I didn't get a crash or data corruption on my system
> > when I forced usage of last_pressed for my laptop, loaded this module
> > and tried to press keys.
> > 
> > It's very unconvenient that struct key_entry does not have some field
> > for driver-specific extra data. In such field we could store index in
> > last_pressed or event last press time. But we haven't such field.
> 
> If that's really needed, such field could be added. Cc-ing Dmitry
> Torokhov to ask what he thinks about altering the sparse keymap API for
> this use case, ie having per key_entry variables to store information,
> here used for debouncing.
> Previously the keymap was contiguous, but we're adding new keys, so it'll
> really be sparse, so we can't use the same trick substraction trick.

A different question - do you really need to store times for all
possible keys or you actually need to know just the last key that was
pressed?

Thanks.

-- 
Dmitry

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Anisse Astier]

On Wed, 12 Dec 2012 10:58:56 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote :

> On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
> > On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> > 
> > > 2012/12/11 Anisse Astier <anisse@astier.eu>:
> > > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> > > >
> > > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> > > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> > > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> > > >> >>                               eventcode);
> > > >> >> -             if (key) {
> > > >> >> +             if (!key) {
> > > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> > > >> >> +                     goto msi_wmi_notify_exit;
> > > >> >> +             }
> > > >> >> +             if (quirk_last_pressed) {
> > > >> >> +                     size_t key_index = key - msi_wmi_keymap;
> > > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> > > >> > intent here otherwise.
> > > >>
> > > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> > > >> first item. key is a pointer to some array's item. So 'key -
> > > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> > > >> msi_wmi_keymap.
> > > >>
> > > >> I do pointer arithmetic here because in patch 12 I add some new
> > > >> scancodes, and holes appear in scancode sequence, so we can't just use
> > > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
> > > >
> > > > Oh, I see. This is very clever, but a bit too clever. You have no
> > > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> > > > to *your* key_entry. In fact, it doesn't.
> > > >
> > > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> > > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> > > > want to use this method, you might need to re-compute the index by
> > > > iterating over the elements and comparing key->code for each.
> > > 
> > > Oops, I'm sorry, I was looking only into
> > > sparse_keymap_entry_from_scancode() and I didn't discover the fact
> > > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
> > > avoid iterating through the array second time. Also, can I use
> > > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
> > > input_dev::keycode field is documented at include/linux/input.h:62.
> > 
> > As I said, you could iterate over msi_wmi_keymap and compare key->code to
> > each keycode to compute key_index each time.
> > 
> > > 
> > > It's strange that I didn't get a crash or data corruption on my system
> > > when I forced usage of last_pressed for my laptop, loaded this module
> > > and tried to press keys.
> > > 
> > > It's very unconvenient that struct key_entry does not have some field
> > > for driver-specific extra data. In such field we could store index in
> > > last_pressed or event last press time. But we haven't such field.
> > 
> > If that's really needed, such field could be added. Cc-ing Dmitry
> > Torokhov to ask what he thinks about altering the sparse keymap API for
> > this use case, ie having per key_entry variables to store information,
> > here used for debouncing.
> > Previously the keymap was contiguous, but we're adding new keys, so it'll
> > really be sparse, so we can't use the same trick substraction trick.
> 
> A different question - do you really need to store times for all
> possible keys or you actually need to know just the last key that was
> pressed?

Not at all. This isn't a gamepad, so I think this quirk could be
simplified by just storing the last trigger time overall, not per-key
trigger time. 1 key every 50ms means you can still input 20 keys per
second, which isn't bad.

Maxim, if you make this modification it would simplify your patch even
further.

Cc-ing original author Thomas Renninger for feedback.

Anisse

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Maxim Mikityanskiy]

2012/12/13 Anisse Astier <anisse@astier.eu>:
> On Wed, 12 Dec 2012 10:58:56 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote :
>
>> On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
>> > On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>> >
>> > > 2012/12/11 Anisse Astier <anisse@astier.eu>:
>> > > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>> > > >
>> > > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
>> > > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
>> > > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
>> > > >> >>                               eventcode);
>> > > >> >> -             if (key) {
>> > > >> >> +             if (!key) {
>> > > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
>> > > >> >> +                     goto msi_wmi_notify_exit;
>> > > >> >> +             }
>> > > >> >> +             if (quirk_last_pressed) {
>> > > >> >> +                     size_t key_index = key - msi_wmi_keymap;
>> > > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
>> > > >> > intent here otherwise.
>> > > >>
>> > > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
>> > > >> first item. key is a pointer to some array's item. So 'key -
>> > > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
>> > > >> msi_wmi_keymap.
>> > > >>
>> > > >> I do pointer arithmetic here because in patch 12 I add some new
>> > > >> scancodes, and holes appear in scancode sequence, so we can't just use
>> > > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
>> > > >
>> > > > Oh, I see. This is very clever, but a bit too clever. You have no
>> > > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
>> > > > to *your* key_entry. In fact, it doesn't.
>> > > >
>> > > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
>> > > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
>> > > > want to use this method, you might need to re-compute the index by
>> > > > iterating over the elements and comparing key->code for each.
>> > >
>> > > Oops, I'm sorry, I was looking only into
>> > > sparse_keymap_entry_from_scancode() and I didn't discover the fact
>> > > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
>> > > avoid iterating through the array second time. Also, can I use
>> > > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
>> > > input_dev::keycode field is documented at include/linux/input.h:62.
>> >
>> > As I said, you could iterate over msi_wmi_keymap and compare key->code to
>> > each keycode to compute key_index each time.
>> >
>> > >
>> > > It's strange that I didn't get a crash or data corruption on my system
>> > > when I forced usage of last_pressed for my laptop, loaded this module
>> > > and tried to press keys.
>> > >
>> > > It's very unconvenient that struct key_entry does not have some field
>> > > for driver-specific extra data. In such field we could store index in
>> > > last_pressed or event last press time. But we haven't such field.
>> >
>> > If that's really needed, such field could be added. Cc-ing Dmitry
>> > Torokhov to ask what he thinks about altering the sparse keymap API for
>> > this use case, ie having per key_entry variables to store information,
>> > here used for debouncing.
>> > Previously the keymap was contiguous, but we're adding new keys, so it'll
>> > really be sparse, so we can't use the same trick substraction trick.
>>
>> A different question - do you really need to store times for all
>> possible keys or you actually need to know just the last key that was
>> pressed?
>
> Not at all. This isn't a gamepad, so I think this quirk could be
> simplified by just storing the last trigger time overall, not per-key
> trigger time. 1 key every 50ms means you can still input 20 keys per
> second, which isn't bad.
>
> Maxim, if you make this modification it would simplify your patch even
> further.

I agree, it would simplify things a lot. But I think it could cause
problems when a user presses two different keys simultaneously. We
would receive lots of events from both keys in some undefined order. A
variable holding last key code would change its value several times so
we would send more key press events than user really made (more than
two). I know, this case is very unlikely to happen (nobody presses two
special keys at a time), maybe it's even impossible due to some
hardware restrictions (I don't know is it true).

We could ignore this rare case at all, or we could filter out all
events within 50ms, not only the same ones. What do you think on this?

> Cc-ing original author Thomas Renninger for feedback.
>
> Anisse

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Dmitry Torokhov]

On Thu, Dec 13, 2012 at 08:06:47PM +0200, Maxim Mikityanskiy wrote:
> 2012/12/13 Anisse Astier <anisse@astier.eu>:
> > On Wed, 12 Dec 2012 10:58:56 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote :
> >
> >> On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
> >> > On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> >> >
> >> > > 2012/12/11 Anisse Astier <anisse@astier.eu>:
> >> > > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> >> > > >
> >> > > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> >> > > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> >> > > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> >> > > >> >>                               eventcode);
> >> > > >> >> -             if (key) {
> >> > > >> >> +             if (!key) {
> >> > > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> >> > > >> >> +                     goto msi_wmi_notify_exit;
> >> > > >> >> +             }
> >> > > >> >> +             if (quirk_last_pressed) {
> >> > > >> >> +                     size_t key_index = key - msi_wmi_keymap;
> >> > > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> >> > > >> > intent here otherwise.
> >> > > >>
> >> > > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> >> > > >> first item. key is a pointer to some array's item. So 'key -
> >> > > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> >> > > >> msi_wmi_keymap.
> >> > > >>
> >> > > >> I do pointer arithmetic here because in patch 12 I add some new
> >> > > >> scancodes, and holes appear in scancode sequence, so we can't just use
> >> > > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
> >> > > >
> >> > > > Oh, I see. This is very clever, but a bit too clever. You have no
> >> > > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> >> > > > to *your* key_entry. In fact, it doesn't.
> >> > > >
> >> > > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> >> > > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> >> > > > want to use this method, you might need to re-compute the index by
> >> > > > iterating over the elements and comparing key->code for each.
> >> > >
> >> > > Oops, I'm sorry, I was looking only into
> >> > > sparse_keymap_entry_from_scancode() and I didn't discover the fact
> >> > > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
> >> > > avoid iterating through the array second time. Also, can I use
> >> > > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
> >> > > input_dev::keycode field is documented at include/linux/input.h:62.
> >> >
> >> > As I said, you could iterate over msi_wmi_keymap and compare key->code to
> >> > each keycode to compute key_index each time.
> >> >
> >> > >
> >> > > It's strange that I didn't get a crash or data corruption on my system
> >> > > when I forced usage of last_pressed for my laptop, loaded this module
> >> > > and tried to press keys.
> >> > >
> >> > > It's very unconvenient that struct key_entry does not have some field
> >> > > for driver-specific extra data. In such field we could store index in
> >> > > last_pressed or event last press time. But we haven't such field.
> >> >
> >> > If that's really needed, such field could be added. Cc-ing Dmitry
> >> > Torokhov to ask what he thinks about altering the sparse keymap API for
> >> > this use case, ie having per key_entry variables to store information,
> >> > here used for debouncing.
> >> > Previously the keymap was contiguous, but we're adding new keys, so it'll
> >> > really be sparse, so we can't use the same trick substraction trick.
> >>
> >> A different question - do you really need to store times for all
> >> possible keys or you actually need to know just the last key that was
> >> pressed?
> >
> > Not at all. This isn't a gamepad, so I think this quirk could be
> > simplified by just storing the last trigger time overall, not per-key
> > trigger time. 1 key every 50ms means you can still input 20 keys per
> > second, which isn't bad.
> >
> > Maxim, if you make this modification it would simplify your patch even
> > further.
> 
> I agree, it would simplify things a lot. But I think it could cause
> problems when a user presses two different keys simultaneously. We
> would receive lots of events from both keys in some undefined order. A
> variable holding last key code would change its value several times so
> we would send more key press events than user really made (more than
> two). I know, this case is very unlikely to happen (nobody presses two
> special keys at a time), maybe it's even impossible due to some
> hardware restrictions (I don't know is it true).
> 
> We could ignore this rare case at all, or we could filter out all
> events within 50ms, not only the same ones. What do you think on this?

I think for the kind of keys the device provides simply ignoring events
coming within 50 msecs would work well enough.

But you have the hardware so try experimenting and see what kind of
response you can get from it and what strategy is the best.

Thanks.

-- 
Dmitry

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Maxim Mikityanskiy]

2012/12/13 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Thu, Dec 13, 2012 at 08:06:47PM +0200, Maxim Mikityanskiy wrote:
>> 2012/12/13 Anisse Astier <anisse@astier.eu>:
>> > On Wed, 12 Dec 2012 10:58:56 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote :
>> >
>> >> On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
>> >> > On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>> >> >
>> >> > > 2012/12/11 Anisse Astier <anisse@astier.eu>:
>> >> > > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>> >> > > >
>> >> > > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
>> >> > > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
>> >> > > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
>> >> > > >> >>                               eventcode);
>> >> > > >> >> -             if (key) {
>> >> > > >> >> +             if (!key) {
>> >> > > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
>> >> > > >> >> +                     goto msi_wmi_notify_exit;
>> >> > > >> >> +             }
>> >> > > >> >> +             if (quirk_last_pressed) {
>> >> > > >> >> +                     size_t key_index = key - msi_wmi_keymap;
>> >> > > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
>> >> > > >> > intent here otherwise.
>> >> > > >>
>> >> > > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
>> >> > > >> first item. key is a pointer to some array's item. So 'key -
>> >> > > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
>> >> > > >> msi_wmi_keymap.
>> >> > > >>
>> >> > > >> I do pointer arithmetic here because in patch 12 I add some new
>> >> > > >> scancodes, and holes appear in scancode sequence, so we can't just use
>> >> > > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
>> >> > > >
>> >> > > > Oh, I see. This is very clever, but a bit too clever. You have no
>> >> > > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
>> >> > > > to *your* key_entry. In fact, it doesn't.
>> >> > > >
>> >> > > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
>> >> > > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
>> >> > > > want to use this method, you might need to re-compute the index by
>> >> > > > iterating over the elements and comparing key->code for each.
>> >> > >
>> >> > > Oops, I'm sorry, I was looking only into
>> >> > > sparse_keymap_entry_from_scancode() and I didn't discover the fact
>> >> > > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
>> >> > > avoid iterating through the array second time. Also, can I use
>> >> > > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
>> >> > > input_dev::keycode field is documented at include/linux/input.h:62.
>> >> >
>> >> > As I said, you could iterate over msi_wmi_keymap and compare key->code to
>> >> > each keycode to compute key_index each time.
>> >> >
>> >> > >
>> >> > > It's strange that I didn't get a crash or data corruption on my system
>> >> > > when I forced usage of last_pressed for my laptop, loaded this module
>> >> > > and tried to press keys.
>> >> > >
>> >> > > It's very unconvenient that struct key_entry does not have some field
>> >> > > for driver-specific extra data. In such field we could store index in
>> >> > > last_pressed or event last press time. But we haven't such field.
>> >> >
>> >> > If that's really needed, such field could be added. Cc-ing Dmitry
>> >> > Torokhov to ask what he thinks about altering the sparse keymap API for
>> >> > this use case, ie having per key_entry variables to store information,
>> >> > here used for debouncing.
>> >> > Previously the keymap was contiguous, but we're adding new keys, so it'll
>> >> > really be sparse, so we can't use the same trick substraction trick.
>> >>
>> >> A different question - do you really need to store times for all
>> >> possible keys or you actually need to know just the last key that was
>> >> pressed?
>> >
>> > Not at all. This isn't a gamepad, so I think this quirk could be
>> > simplified by just storing the last trigger time overall, not per-key
>> > trigger time. 1 key every 50ms means you can still input 20 keys per
>> > second, which isn't bad.
>> >
>> > Maxim, if you make this modification it would simplify your patch even
>> > further.
>>
>> I agree, it would simplify things a lot. But I think it could cause
>> problems when a user presses two different keys simultaneously. We
>> would receive lots of events from both keys in some undefined order. A
>> variable holding last key code would change its value several times so
>> we would send more key press events than user really made (more than
>> two). I know, this case is very unlikely to happen (nobody presses two
>> special keys at a time), maybe it's even impossible due to some
>> hardware restrictions (I don't know is it true).
>>
>> We could ignore this rare case at all, or we could filter out all
>> events within 50ms, not only the same ones. What do you think on this?
>
> I think for the kind of keys the device provides simply ignoring events
> coming within 50 msecs would work well enough.

OK, thanks.

> But you have the hardware so try experimenting and see what kind of
> response you can get from it and what strategy is the best.

Unfortunately, I can't do the experiment, because my laptop sends only
one event per key press and does not need this quirk at all. The quirk
is needed for older models. Maybe, Anisse has suitable hardware.

> Thanks.
>
> --
> Dmitry

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[joeyli]

於 四，2012-12-13 於 20:30 +0200，Maxim Mikityanskiy 提到：
> 2012/12/13 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > On Thu, Dec 13, 2012 at 08:06:47PM +0200, Maxim Mikityanskiy wrote:
> >> 2012/12/13 Anisse Astier <anisse@astier.eu>:
> >> > On Wed, 12 Dec 2012 10:58:56 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote :
> >> >
> >> >> On Wed, Dec 12, 2012 at 10:58:29AM +0100, Anisse Astier wrote:
> >> >> > On Tue, 11 Dec 2012 20:27:26 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> >> >> >
> >> >> > > 2012/12/11 Anisse Astier <anisse@astier.eu>:
> >> >> > > > On Tue, 11 Dec 2012 19:07:51 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
> >> >> > > >
> >> >> > > >> >> @@ -169,11 +169,15 @@ static void msi_wmi_notify(u32 value, void *context)
> >> >> > > >> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> >> >> > > >> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> >> >> > > >> >>                               eventcode);
> >> >> > > >> >> -             if (key) {
> >> >> > > >> >> +             if (!key) {
> >> >> > > >> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> >> >> > > >> >> +                     goto msi_wmi_notify_exit;
> >> >> > > >> >> +             }
> >> >> > > >> >> +             if (quirk_last_pressed) {
> >> >> > > >> >> +                     size_t key_index = key - msi_wmi_keymap;
> >> >> > > >> > Do you mean key->code - MSI_SCANCODE_BASE ? I'm not sure I understand the
> >> >> > > >> > intent here otherwise.
> >> >> > > >>
> >> >> > > >> msi_wmi_keymap is array of 'struct key_entry', i.e. pointer to array's
> >> >> > > >> first item. key is a pointer to some array's item. So 'key -
> >> >> > > >> msi_wmi_keymap' is a difference between pointers, i.e. index of key in
> >> >> > > >> msi_wmi_keymap.
> >> >> > > >>
> >> >> > > >> I do pointer arithmetic here because in patch 12 I add some new
> >> >> > > >> scancodes, and holes appear in scancode sequence, so we can't just use
> >> >> > > >> 'key->code - MSI_SCANCODE_BASE' to get item index in array.
> >> >> > > >
> >> >> > > > Oh, I see. This is very clever, but a bit too clever. You have no
> >> >> > > > guarantee, that sparse_keymap_entry_from_scancode will give you a pointer
> >> >> > > > to *your* key_entry. In fact, it doesn't.
> >> >> > > >
> >> >> > > > In sparse_keymap_setup (drivers/input/sparse-keymap.c), the keymap
> >> >> > > > array(msi_wmi_keymap) is mempcy-ed, at line 187 (kernel ~3.7). So if you
> >> >> > > > want to use this method, you might need to re-compute the index by
> >> >> > > > iterating over the elements and comparing key->code for each.
> >> >> > >
> >> >> > > Oops, I'm sorry, I was looking only into
> >> >> > > sparse_keymap_entry_from_scancode() and I didn't discover the fact
> >> >> > > that dev->keycode is not equal to msi_wmi_keymap. I just wanted to
> >> >> > > avoid iterating through the array second time. Also, can I use
> >> >> > > msi_wmi_input_dev->keycode instead of msi_wmi_keymap as array base?
> >> >> > > input_dev::keycode field is documented at include/linux/input.h:62.
> >> >> >
> >> >> > As I said, you could iterate over msi_wmi_keymap and compare key->code to
> >> >> > each keycode to compute key_index each time.
> >> >> >
> >> >> > >
> >> >> > > It's strange that I didn't get a crash or data corruption on my system
> >> >> > > when I forced usage of last_pressed for my laptop, loaded this module
> >> >> > > and tried to press keys.
> >> >> > >
> >> >> > > It's very unconvenient that struct key_entry does not have some field
> >> >> > > for driver-specific extra data. In such field we could store index in
> >> >> > > last_pressed or event last press time. But we haven't such field.
> >> >> >
> >> >> > If that's really needed, such field could be added. Cc-ing Dmitry
> >> >> > Torokhov to ask what he thinks about altering the sparse keymap API for
> >> >> > this use case, ie having per key_entry variables to store information,
> >> >> > here used for debouncing.
> >> >> > Previously the keymap was contiguous, but we're adding new keys, so it'll
> >> >> > really be sparse, so we can't use the same trick substraction trick.
> >> >>
> >> >> A different question - do you really need to store times for all
> >> >> possible keys or you actually need to know just the last key that was
> >> >> pressed?
> >> >
> >> > Not at all. This isn't a gamepad, so I think this quirk could be
> >> > simplified by just storing the last trigger time overall, not per-key
> >> > trigger time. 1 key every 50ms means you can still input 20 keys per
> >> > second, which isn't bad.
> >> >
> >> > Maxim, if you make this modification it would simplify your patch even
> >> > further.
> >>
> >> I agree, it would simplify things a lot. But I think it could cause
> >> problems when a user presses two different keys simultaneously. We
> >> would receive lots of events from both keys in some undefined order. A
> >> variable holding last key code would change its value several times so
> >> we would send more key press events than user really made (more than
> >> two). I know, this case is very unlikely to happen (nobody presses two
> >> special keys at a time), maybe it's even impossible due to some
> >> hardware restrictions (I don't know is it true).
> >>
> >> We could ignore this rare case at all, or we could filter out all
> >> events within 50ms, not only the same ones. What do you think on this?
> >
> > I think for the kind of keys the device provides simply ignoring events
> > coming within 50 msecs would work well enough.
> 
> OK, thanks.
> 
> > But you have the hardware so try experimenting and see what kind of
> > response you can get from it and what strategy is the best.
> 
> Unfortunately, I can't do the experiment, because my laptop sends only
> one event per key press and does not need this quirk at all. The quirk
> is needed for older models. Maybe, Anisse has suitable hardware.
> 
> > Thanks.
> >
> > --
> > Dmitry
> 

As I remember this quirk is for a old MSI AIO machine that implemented
side buttons. We have no machine for testing and confirm new change,
now.

Per my understand, we didn't test the behavior that was pressed 2 side
buttons at the same time. IMHO capture the latest pressed button within
50ms is make sense.

Hi Thomas, do you have good suggestion if we filter out all events
within 50ms?


Thanks a lot!
Joey Lee

Re: [PATCH v3 11/12] msi-wmi: Introduced quirk_last_pressed

[Thomas Renninger]

On Friday, December 14, 2012 11:37:00 AM joeyli wrote:
...
> Hi Thomas, do you have good suggestion if we filter out all events
> within 50ms?
Puhh, I do not know the details anymore.
I never had this machine, but worked on it remotely
while someone else hit the key.
Iirc this was a BIOS bug and instead of one event, several
showed up. I cannot recall the exact behaviour, I can hardly
remember that the number of events was random (1-3?) and they
came "immediatly".

I also do not know the input layer well, Dimitry should be
better in judging what the most elegant and safe way is
to handle this.


  Thomas

[PATCH v3 12/12] msi-wmi: Add MSI Wind support

[Maxim Mikityanskiy]

Add MSI Wind support to msi-wmi driver. MSI Wind has different GUID for
key events, different WMI key scan codes, it does not need filtering
consecutive identical events and it does not support backlight control
via MSIWMI_BIOS_GUID WMI. Tested on MSI Wind U100.

Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/platform/x86/msi-wmi.c | 65 ++++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 273f647..ddbb0ca 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -37,17 +37,27 @@ MODULE_LICENSE("GPL");
 #define DRV_NAME "msi-wmi"
 
 #define MSIWMI_BIOS_GUID "551A1F84-FBDD-4125-91DB-3EA8F44F1D45"
-#define MSIWMI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
+#define MSIWMI_MSI_EVENT_GUID "B6F3EEF2-3D2F-49DC-9DE3-85BCE18C62F2"
+#define MSIWMI_WIND_EVENT_GUID "5B3CC38A-40D9-7245-8AE6-1145B751BE3F"
 
 MODULE_ALIAS("wmi:" MSIWMI_BIOS_GUID);
-MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
+MODULE_ALIAS("wmi:" MSIWMI_MSI_EVENT_GUID);
+MODULE_ALIAS("wmi:" MSIWMI_WIND_EVENT_GUID);
 
 enum msi_scancodes {
+	/* Generic MSI keys (not present on MSI Wind) */
 	MSI_KEY_BRIGHTNESSUP	= 0xD0,
 	MSI_KEY_BRIGHTNESSDOWN,
 	MSI_KEY_VOLUMEUP,
 	MSI_KEY_VOLUMEDOWN,
 	MSI_KEY_MUTE,
+	/* MSI Wind keys */
+	WIND_KEY_TOUCHPAD	= 0x08,	/* Fn+F3 touchpad toggle */
+	WIND_KEY_BLUETOOTH	= 0x56,	/* Fn+F11 Bluetooth toggle */
+	WIND_KEY_CAMERA,		/* Fn+F6 webcam toggle */
+	WIND_KEY_WLAN		= 0x5f,	/* Fn+F11 Wi-Fi toggle */
+	WIND_KEY_TURBO,			/* Fn+F10 turbo mode toggle */
+	WIND_KEY_ECO		= 0x69,	/* Fn+F10 ECO mode toggle */
 };
 static struct key_entry msi_wmi_keymap[] = {
 	{ KE_KEY, MSI_KEY_BRIGHTNESSUP,		{KEY_BRIGHTNESSUP} },
@@ -55,12 +65,33 @@ static struct key_entry msi_wmi_keymap[] = {
 	{ KE_KEY, MSI_KEY_VOLUMEUP,		{KEY_VOLUMEUP} },
 	{ KE_KEY, MSI_KEY_VOLUMEDOWN,		{KEY_VOLUMEDOWN} },
 	{ KE_KEY, MSI_KEY_MUTE,			{KEY_MUTE} },
+
+	/* These keys work without WMI. Ignore them to avoid double keycodes */
+	{ KE_IGNORE, WIND_KEY_TOUCHPAD,		{KEY_TOUCHPAD_TOGGLE} },
+	{ KE_IGNORE, WIND_KEY_BLUETOOTH,	{KEY_BLUETOOTH} },
+	{ KE_IGNORE, WIND_KEY_CAMERA,		{KEY_CAMERA} },
+	{ KE_IGNORE, WIND_KEY_WLAN,		{KEY_WLAN} },
+
+	/* These are unknown WMI events found on MSI Wind */
+	{ KE_IGNORE, 0x00 },
+	{ KE_IGNORE, 0x62 },
+	{ KE_IGNORE, 0x63 },
+
+	/* These are MSI Wind keys that should be handled via WMI */
+	{ KE_KEY, WIND_KEY_TURBO,		{KEY_PROG1} },
+	{ KE_KEY, WIND_KEY_ECO,			{KEY_PROG2} },
+
 	{ KE_END, 0 }
 };
 static ktime_t last_pressed[ARRAY_SIZE(msi_wmi_keymap) - 1];
-static bool quirk_last_pressed;
 
-static const char *event_wmi_guid;
+static const struct {
+	const char *guid;
+	bool quirk_last_pressed;
+} *event_wmi, event_wmis[] = {
+	{ MSIWMI_MSI_EVENT_GUID, true },
+	{ MSIWMI_WIND_EVENT_GUID, false },
+};
 
 static struct backlight_device *backlight;
 
@@ -173,7 +204,7 @@ static void msi_wmi_notify(u32 value, void *context)
 			pr_info("Unknown key pressed - %x\n", eventcode);
 			goto msi_wmi_notify_exit;
 		}
-		if (quirk_last_pressed) {
+		if (event_wmi->quirk_last_pressed) {
 			size_t key_index = key - msi_wmi_keymap;
 			ktime_t diff;
 			cur = ktime_get_real();
@@ -266,15 +297,19 @@ err_free_dev:
 static int __init msi_wmi_init(void)
 {
 	int err;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(event_wmis); i++) {
+		if (!wmi_has_guid(event_wmis[i].guid))
+			continue;
 
-	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
 		err = msi_wmi_input_setup();
 		if (err) {
 			pr_err("Unable to setup input device\n");
 			return err;
 		}
 
-		err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
+		err = wmi_install_notify_handler(event_wmis[i].guid,
 			msi_wmi_notify, NULL);
 		if (ACPI_FAILURE(err)) {
 			pr_err("Unable to setup WMI notify handler\n");
@@ -282,8 +317,8 @@ static int __init msi_wmi_init(void)
 		}
 
 		pr_debug("Event handler installed\n");
-		event_wmi_guid = MSIWMI_EVENT_GUID;
-		quirk_last_pressed = true;
+		event_wmi = &event_wmis[i];
+		break;
 	}
 
 	if (wmi_has_guid(MSIWMI_BIOS_GUID) && !acpi_video_backlight_support()) {
@@ -295,7 +330,7 @@ static int __init msi_wmi_init(void)
 		pr_debug("Backlight device created\n");
 	}
 
-	if (!event_wmi_guid && !backlight) {
+	if (!event_wmi && !backlight) {
 		pr_err("This machine doesn't have neither MSI-hotkeys nor backlight through WMI\n");
 		return -ENODEV;
 	}
@@ -303,10 +338,10 @@ static int __init msi_wmi_init(void)
 	return 0;
 
 err_uninstall_handler:
-	if (event_wmi_guid)
-		wmi_remove_notify_handler(event_wmi_guid);
+	if (event_wmi)
+		wmi_remove_notify_handler(event_wmi->guid);
 err_free_input:
-	if (event_wmi_guid) {
+	if (event_wmi) {
 		sparse_keymap_free(msi_wmi_input_dev);
 		input_unregister_device(msi_wmi_input_dev);
 	}
@@ -315,8 +350,8 @@ err_free_input:
 
 static void __exit msi_wmi_exit(void)
 {
-	if (event_wmi_guid) {
-		wmi_remove_notify_handler(event_wmi_guid);
+	if (event_wmi) {
+		wmi_remove_notify_handler(event_wmi->guid);
 		sparse_keymap_free(msi_wmi_input_dev);
 		input_unregister_device(msi_wmi_input_dev);
 	}
-- 
1.8.0.1