[PATCH v2 0/8] Add MSI Wind U90/U100 support

[PATCH v2 0/8] 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 previous 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
	  previous series.

Tested on MSI Wind U100Plus laptop.

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

Maxim Mikityanskiy (7):
      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: 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                      | 259 +++++++++++++++++++++++++++++++++++++++------------------
 3 files changed, 546 insertions(+), 170 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-msi-laptop

[PATCH v2 1/8] 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 v2 2/8] 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 v2 3/8] 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>
---
 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

Re: [PATCH v2 3/8] msi-laptop: merge quirk tables to one

[joeyli]

於 五，2012-11-30 於 18:04 +0200，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>

Thanks for your patch
Joey Lee

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

[PATCH v2 4/8] 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>
---
Comparing to the previous series, I dropped 0xE4 and 0x64 scan codes
filtering. It was needed to filter out touchpad toggle key and send
only separate touchpad on/off keys. It was bad solution because 0xE4
and 0x64 are extended scan codes following after 0xE0. We can't filter
0xE0 when 0xE4/0x64 is arrived because we already passed 0xE0.
Filtering only 0xE4/0x64 resulted in having '0xE0, 0xE0' in stream and
it caused kernel warnings about unknown key.

The solution is to edit default udev keymap rules that remap 0xE4 scan
code to F21 key.

 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

Re: [PATCH v2 4/8] msi-laptop: Add MSI Wind U90/U100 support

[joeyli]

於 五，2012-11-30 於 18:04 +0200，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>

Thanks for your patch!
Joey Lee

> ---
> Comparing to the previous series, I dropped 0xE4 and 0x64 scan codes
> filtering. It was needed to filter out touchpad toggle key and send
> only separate touchpad on/off keys. It was bad solution because 0xE4
> and 0x64 are extended scan codes following after 0xE0. We can't filter
> 0xE0 when 0xE4/0x64 is arrived because we already passed 0xE0.
> Filtering only 0xE4/0x64 resulted in having '0xE0, 0xE0' in stream and
> it caused kernel warnings about unknown key.
> 
> The solution is to edit default udev keymap rules that remap 0xE4 scan
> code to F21 key.
> 
>  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:*");

[PATCH v2 5/8] 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>
---
Changed KernelVersion from 3.7 to 3.8 - I think this series will not be
merged to 3.7.
 .../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..f621027
--- /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 v2 6/8] 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>
---
 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

Re: [PATCH v2 6/8] msi-laptop: Disable brightness control for new EC

[joeyli]

於 五，2012-11-30 於 18:04 +0200，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>

Thanks for your patch!
Joey Lee

> ---
>  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");
>  }

[PATCH v2 7/8] 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>
---
 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

Re: [PATCH v2 7/8] msi-wmi: Fix memory leak

[Anisse Astier]

On Fri, 30 Nov 2012 18:04:36 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> 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>
> ---
>  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);
>  }
>  

For this patch:

Acked-by: Anisse Astier <anisse@astier.eu>

Re: [PATCH v2 7/8] msi-wmi: Fix memory leak

[joeyli]

於 五，2012-11-30 於 18:04 +0200，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>

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

Thanks a lot!
Joey Lee

> ---
>  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);
>  }
>  

[PATCH v2 8/8] 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
consequent 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 | 255 ++++++++++++++++++++++++++++-------------
 1 file changed, 177 insertions(+), 78 deletions(-)

diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index b96766b..31a7221 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -34,29 +34,63 @@ 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"
-
-#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)
+#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_MSI_EVENT_GUID);
+MODULE_ALIAS("wmi:" MSIWMI_WIND_EVENT_GUID);
+
+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;
+
+enum wind_scancodes {
+	/* Fn+F3 touchpad toggle */
+	WIND_KEY_TOUCHPAD	= 0x08,
+	/* Fn+F11 Bluetooth toggle */
+	WIND_KEY_BLUETOOTH	= 0x56,
+	/* Fn+F6 webcam toggle */
+	WIND_KEY_CAMERA		= 0x57,
+	/* Fn+F11 Wi-Fi toggle */
+	WIND_KEY_WLAN		= 0x5f,
+	/* Fn+F10 turbo mode toggle */
+	WIND_KEY_TURBO		= 0x60,
+	/* Fn+F10 ECO mode toggle */
+	WIND_KEY_ECO		= 0x69,
+};
+static struct key_entry wind_wmi_keymap[] = {
+	/* 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 */
+	{ KE_IGNORE, 0x00 },
+	{ KE_IGNORE, 0x62 },
+	{ KE_IGNORE, 0x63 },
+	/* These are 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 struct backlight_device *backlight;
 
@@ -64,6 +98,38 @@ static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF };
 
 static struct input_dev *msi_wmi_input_dev;
 
+struct msi_wmi_table_entry {
+	const char *guid;
+	struct key_entry *keymap;
+
+	/* Some MSI laptops send lots of identical events for one key press
+	 * within short period of time. Set quirk_last_pressed to true if such
+	 * repeated events need to be filtered. All scan codes must be
+	 * sequential numbers. Set scancode_base to first of them and
+	 * scancode_count to their count
+	 */
+	bool quirk_last_pressed;
+	u32 scancode_base;
+	size_t scancode_count;
+};
+
+static struct msi_wmi_table_entry msi_wmi_table[] = {
+	{
+		.guid			= MSIWMI_MSI_EVENT_GUID,
+		.keymap			= msi_wmi_keymap,
+		.quirk_last_pressed	= true,
+		.scancode_base		= MSI_SCANCODE_BASE,
+		.scancode_count		= ARRAY_SIZE(msi_wmi_keymap)-1,
+	},
+	{
+		.guid			= MSIWMI_WIND_EVENT_GUID,
+		.keymap			= wind_wmi_keymap,
+	},
+	{}
+};
+
+static struct msi_wmi_table_entry *wmi;
+
 static int msi_wmi_query_block(int instance, int *ret)
 {
 	acpi_status status;
@@ -165,11 +231,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 (wmi->quirk_last_pressed) {
 			ktime_t diff;
 			cur = ktime_get_real();
 			diff = ktime_sub(cur, last_pressed[key->code -
-					SCANCODE_BASE]);
+					wmi->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,21 +248,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 - 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))) {
-				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->code - wmi->scancode_base] = cur;
+		}
+
+		if (key->type == KE_KEY &&
+		/* Brightness is served via acpi video driver */
+		(!backlight ||
+		(key->keycode != KEY_BRIGHTNESSUP &&
+		key->keycode != 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");
 
@@ -200,10 +268,45 @@ msi_wmi_notify_exit:
 	kfree(response.pointer);
 }
 
+static int __init msi_wmi_backlight_setup(void)
+{
+	int err;
+	struct backlight_properties props;
+
+	if (!wmi_has_guid(MSIWMI_BIOS_GUID) || acpi_video_backlight_support())
+		return -ENODEV;
+
+	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;
 
+	if (wmi->quirk_last_pressed) {
+		last_pressed = kcalloc(wmi->scancode_count,
+			sizeof(last_pressed[0]), GFP_KERNEL);
+		if (!last_pressed)
+			return -ENOMEM;
+	}
+
 	msi_wmi_input_dev = input_allocate_device();
 	if (!msi_wmi_input_dev)
 		return -ENOMEM;
@@ -212,7 +315,7 @@ static int __init msi_wmi_input_setup(void)
 	msi_wmi_input_dev->phys = "wmi/input0";
 	msi_wmi_input_dev->id.bustype = BUS_HOST;
 
-	err = sparse_keymap_setup(msi_wmi_input_dev, msi_wmi_keymap, NULL);
+	err = sparse_keymap_setup(msi_wmi_input_dev, wmi->keymap, NULL);
 	if (err)
 		goto err_free_dev;
 
@@ -221,8 +324,6 @@ static int __init msi_wmi_input_setup(void)
 	if (err)
 		goto err_free_keymap;
 
-	memset(last_pressed, 0, sizeof(last_pressed));
-
 	return 0;
 
 err_free_keymap:
@@ -235,61 +336,59 @@ err_free_dev:
 static int __init msi_wmi_init(void)
 {
 	int err;
+	bool found = false;
+	size_t i;
+
+	for (i = 0; msi_wmi_table[i].guid; i++) {
+		if (wmi_has_guid(msi_wmi_table[i].guid)) {
+			found = true;
+			wmi = &msi_wmi_table[i];
+
+			err = msi_wmi_input_setup();
+			if (err) {
+				pr_err("Unable to setup input device\n");
+				wmi = NULL;
+				return 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;
+			err = wmi_install_notify_handler(wmi->guid,
+				msi_wmi_notify, NULL);
+			if (ACPI_FAILURE(err)) {
+				pr_err("Unable to setup WMI notify handler\n");
+				sparse_keymap_free(msi_wmi_input_dev);
+				input_unregister_device(msi_wmi_input_dev);
+				wmi = NULL;
+				return -EINVAL;
+			}
 
-	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);
-			goto err_free_input;
+			pr_debug("Event handler installed\n");
+			break;
 		}
+	}
 
-		err = bl_get(NULL);
-		if (err < 0)
-			goto err_free_backlight;
+	if (!msi_wmi_backlight_setup()) {
+		found = true;
+		pr_debug("Backlight device created\n");
+	}
 
-		backlight->props.brightness = err;
+	if (!found) {
+		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_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);
-	return err;
 }
 
 static void __exit msi_wmi_exit(void)
 {
-	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
-		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
+	kfree(last_pressed);
+	if (wmi) {
+		wmi_remove_notify_handler(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

Re: [PATCH v2 8/8] msi-wmi: Add MSI Wind support

[Anisse Astier]

On Fri, 30 Nov 2012 18:04:37 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> 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
> consequent 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 | 255 ++++++++++++++++++++++++++++-------------
>  1 file changed, 177 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
> index b96766b..31a7221 100644
> --- a/drivers/platform/x86/msi-wmi.c
> +++ b/drivers/platform/x86/msi-wmi.c
> @@ -34,29 +34,63 @@ 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"
> -
> -#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)
> +#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_MSI_EVENT_GUID);
> +MODULE_ALIAS("wmi:" MSIWMI_WIND_EVENT_GUID);
> +
> +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;
> +
> +enum wind_scancodes {
> +	/* Fn+F3 touchpad toggle */
> +	WIND_KEY_TOUCHPAD	= 0x08,
> +	/* Fn+F11 Bluetooth toggle */
> +	WIND_KEY_BLUETOOTH	= 0x56,
> +	/* Fn+F6 webcam toggle */
> +	WIND_KEY_CAMERA		= 0x57,
> +	/* Fn+F11 Wi-Fi toggle */
> +	WIND_KEY_WLAN		= 0x5f,
> +	/* Fn+F10 turbo mode toggle */
> +	WIND_KEY_TURBO		= 0x60,
> +	/* Fn+F10 ECO mode toggle */
> +	WIND_KEY_ECO		= 0x69,
> +};

Looking at the scancode numbers and the key function, I see no collision
that necessitate using a separate sparse keymap, or a separate enum.
Merging them would simplify a lot of your patch.


> +static struct key_entry wind_wmi_keymap[] = {
> +	/* 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 */
> +	{ KE_IGNORE, 0x00 },
> +	{ KE_IGNORE, 0x62 },
> +	{ KE_IGNORE, 0x63 },
> +	/* These are 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 struct backlight_device *backlight;
>  
> @@ -64,6 +98,38 @@ static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF };
>  
>  static struct input_dev *msi_wmi_input_dev;
>  
> +struct msi_wmi_table_entry {
> +	const char *guid;
> +	struct key_entry *keymap;
> +
> +	/* Some MSI laptops send lots of identical events for one key press
> +	 * within short period of time. Set quirk_last_pressed to true if such
> +	 * repeated events need to be filtered. All scan codes must be
> +	 * sequential numbers. Set scancode_base to first of them and
> +	 * scancode_count to their count
> +	 */
> +	bool quirk_last_pressed;
> +	u32 scancode_base;
> +	size_t scancode_count;
> +};
> +
> +static struct msi_wmi_table_entry msi_wmi_table[] = {
> +	{
> +		.guid			= MSIWMI_MSI_EVENT_GUID,
> +		.keymap			= msi_wmi_keymap,
> +		.quirk_last_pressed	= true,
> +		.scancode_base		= MSI_SCANCODE_BASE,
> +		.scancode_count		= ARRAY_SIZE(msi_wmi_keymap)-1,
> +	},
> +	{
> +		.guid			= MSIWMI_WIND_EVENT_GUID,
> +		.keymap			= wind_wmi_keymap,
> +	},
> +	{}
> +};
If you merge them you would only need the quirk_last_pressed variable,
and could do away with this struct.


> +
> +static struct msi_wmi_table_entry *wmi;
> +
>  static int msi_wmi_query_block(int instance, int *ret)
>  {
>  	acpi_status status;
> @@ -165,11 +231,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 (wmi->quirk_last_pressed) {
>  			ktime_t diff;
>  			cur = ktime_get_real();
>  			diff = ktime_sub(cur, last_pressed[key->code -
> -					SCANCODE_BASE]);
> +					wmi->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,21 +248,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 - SCANCODE_BASE] = cur;
> -
> -			if (key->type == KE_KEY &&
> -			/* Brightness is served via acpi video driver */
> -			(!acpi_video_backlight_support() ||

You modify behaviour by removing this test. Backlight might be
registered, but we may not want to modify it directly from here because
the acpi video driver does that for us.


> -			(key->code != MSI_WMI_BRIGHTNESSUP &&
> -			key->code != MSI_WMI_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);

Why did you remove this message ? It's useful because it helps users
report new keys for new hardware.


> +			last_pressed[key->code - wmi->scancode_base] = cur;
> +		}
> +
> +		if (key->type == KE_KEY &&
> +		/* Brightness is served via acpi video driver */
> +		(!backlight ||
> +		(key->keycode != KEY_BRIGHTNESSUP &&
> +		key->keycode != 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");
>  
> @@ -200,10 +268,45 @@ msi_wmi_notify_exit:
>  	kfree(response.pointer);
>  }
>  
> +static int __init msi_wmi_backlight_setup(void)
> +{
> +	int err;
> +	struct backlight_properties props;
> +
> +	if (!wmi_has_guid(MSIWMI_BIOS_GUID) || acpi_video_backlight_support())
> +		return -ENODEV;
> +
> +	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;
>  
> +	if (wmi->quirk_last_pressed) {
> +		last_pressed = kcalloc(wmi->scancode_count,
> +			sizeof(last_pressed[0]), GFP_KERNEL);

I see why you separated the keymaps. I'm pretty sure we can have an
unified keymap while keeping the last_pressed array working.

> +		if (!last_pressed)
> +			return -ENOMEM;
> +	}
> +
>  	msi_wmi_input_dev = input_allocate_device();
>  	if (!msi_wmi_input_dev)
>  		return -ENOMEM;
> @@ -212,7 +315,7 @@ static int __init msi_wmi_input_setup(void)
>  	msi_wmi_input_dev->phys = "wmi/input0";
>  	msi_wmi_input_dev->id.bustype = BUS_HOST;
>  
> -	err = sparse_keymap_setup(msi_wmi_input_dev, msi_wmi_keymap, NULL);
> +	err = sparse_keymap_setup(msi_wmi_input_dev, wmi->keymap, NULL);
>  	if (err)
>  		goto err_free_dev;
>  
> @@ -221,8 +324,6 @@ static int __init msi_wmi_input_setup(void)
>  	if (err)
>  		goto err_free_keymap;
>  
> -	memset(last_pressed, 0, sizeof(last_pressed));
> -
>  	return 0;
>  
>  err_free_keymap:
> @@ -235,61 +336,59 @@ err_free_dev:
>  static int __init msi_wmi_init(void)
>  {
>  	int err;
> +	bool found = false;
> +	size_t i;
> +
> +	for (i = 0; msi_wmi_table[i].guid; i++) {
> +		if (wmi_has_guid(msi_wmi_table[i].guid)) {
> +			found = true;
> +			wmi = &msi_wmi_table[i];
> +
> +			err = msi_wmi_input_setup();
> +			if (err) {
> +				pr_err("Unable to setup input device\n");
> +				wmi = NULL;
> +				return 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;
> +			err = wmi_install_notify_handler(wmi->guid,
> +				msi_wmi_notify, NULL);
> +			if (ACPI_FAILURE(err)) {
> +				pr_err("Unable to setup WMI notify handler\n");
> +				sparse_keymap_free(msi_wmi_input_dev);
> +				input_unregister_device(msi_wmi_input_dev);
> +				wmi = NULL;
> +				return -EINVAL;
> +			}
>  
> -	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);
> -			goto err_free_input;
> +			pr_debug("Event handler installed\n");
> +			break;
>  		}
> +	}
>  
> -		err = bl_get(NULL);
> -		if (err < 0)
> -			goto err_free_backlight;
> +	if (!msi_wmi_backlight_setup()) {
> +		found = true;
> +		pr_debug("Backlight device created\n");
> +	}
>  
> -		backlight->props.brightness = err;
> +	if (!found) {
> +		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_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);
> -	return err;
>  }
>  
>  static void __exit msi_wmi_exit(void)
>  {
> -	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
> -		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
> +	kfree(last_pressed);
> +	if (wmi) {
> +		wmi_remove_notify_handler(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);
>  }

There's a lot of restructuring in here that aren't directly related to
adding support for new hardware (enums, init path for backlight, quirk,
etc.), which make this patch hard to review. You should try to put these
in separate patches.

Regards,

Anisse

Re: [PATCH v2 8/8] msi-wmi: Add MSI Wind support

[Maxim Mikityanskiy]

2012/11/30 Anisse Astier <anisse@astier.eu>:
> On Fri, 30 Nov 2012 18:04:37 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :
>
>> 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
>> consequent 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 | 255 ++++++++++++++++++++++++++++-------------
>>  1 file changed, 177 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
>> index b96766b..31a7221 100644
>> --- a/drivers/platform/x86/msi-wmi.c
>> +++ b/drivers/platform/x86/msi-wmi.c
>> @@ -34,29 +34,63 @@ 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"
>> -
>> -#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)
>> +#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_MSI_EVENT_GUID);
>> +MODULE_ALIAS("wmi:" MSIWMI_WIND_EVENT_GUID);
>> +
>> +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;
>> +
>> +enum wind_scancodes {
>> +     /* Fn+F3 touchpad toggle */
>> +     WIND_KEY_TOUCHPAD       = 0x08,
>> +     /* Fn+F11 Bluetooth toggle */
>> +     WIND_KEY_BLUETOOTH      = 0x56,
>> +     /* Fn+F6 webcam toggle */
>> +     WIND_KEY_CAMERA         = 0x57,
>> +     /* Fn+F11 Wi-Fi toggle */
>> +     WIND_KEY_WLAN           = 0x5f,
>> +     /* Fn+F10 turbo mode toggle */
>> +     WIND_KEY_TURBO          = 0x60,
>> +     /* Fn+F10 ECO mode toggle */
>> +     WIND_KEY_ECO            = 0x69,
>> +};
>
> Looking at the scancode numbers and the key function, I see no collision
> that necessitate using a separate sparse keymap, or a separate enum.
> Merging them would simplify a lot of your patch.

By merging keymaps we will lose information about what laptop
corresponds the key to. Should I add this information in comments or
is it better to use some other way?

>
>> +static struct key_entry wind_wmi_keymap[] = {
>> +     /* 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 */
>> +     { KE_IGNORE, 0x00 },
>> +     { KE_IGNORE, 0x62 },
>> +     { KE_IGNORE, 0x63 },
>> +     /* These are 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 struct backlight_device *backlight;
>>
>> @@ -64,6 +98,38 @@ static int backlight_map[] = { 0x00, 0x33, 0x66, 0x99, 0xCC, 0xFF };
>>
>>  static struct input_dev *msi_wmi_input_dev;
>>
>> +struct msi_wmi_table_entry {
>> +     const char *guid;
>> +     struct key_entry *keymap;
>> +
>> +     /* Some MSI laptops send lots of identical events for one key press
>> +      * within short period of time. Set quirk_last_pressed to true if such
>> +      * repeated events need to be filtered. All scan codes must be
>> +      * sequential numbers. Set scancode_base to first of them and
>> +      * scancode_count to their count
>> +      */
>> +     bool quirk_last_pressed;
>> +     u32 scancode_base;
>> +     size_t scancode_count;
>> +};
>> +
>> +static struct msi_wmi_table_entry msi_wmi_table[] = {
>> +     {
>> +             .guid                   = MSIWMI_MSI_EVENT_GUID,
>> +             .keymap                 = msi_wmi_keymap,
>> +             .quirk_last_pressed     = true,
>> +             .scancode_base          = MSI_SCANCODE_BASE,
>> +             .scancode_count         = ARRAY_SIZE(msi_wmi_keymap)-1,
>> +     },
>> +     {
>> +             .guid                   = MSIWMI_WIND_EVENT_GUID,
>> +             .keymap                 = wind_wmi_keymap,
>> +     },
>> +     {}
>> +};
> If you merge them you would only need the quirk_last_pressed variable,
> and could do away with this struct.

Yes, you're right. But what if I would completely remove
quirk_last_pressed and would use last_pressed if event 0xD0 - 0xD4
happens, otherwise skip last_pressed check? Is it good way too?

>
>> +
>> +static struct msi_wmi_table_entry *wmi;
>> +
>>  static int msi_wmi_query_block(int instance, int *ret)
>>  {
>>       acpi_status status;
>> @@ -165,11 +231,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 (wmi->quirk_last_pressed) {
>>                       ktime_t diff;
>>                       cur = ktime_get_real();
>>                       diff = ktime_sub(cur, last_pressed[key->code -
>> -                                     SCANCODE_BASE]);
>> +                                     wmi->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,21 +248,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 - SCANCODE_BASE] = cur;
>> -
>> -                     if (key->type == KE_KEY &&
>> -                     /* Brightness is served via acpi video driver */
>> -                     (!acpi_video_backlight_support() ||
>
> You modify behaviour by removing this test. Backlight might be
> registered, but we may not want to modify it directly from here because
> the acpi video driver does that for us.

I don't remove the test. I only change !acpi_video_backlight_support()
condition by !backlight condition (actually, it's a typo, condition
have to look like 'backlight', not '!backlight', I'm sorry for this).
There is a check in msi_wmi_backlight_setup(). I call
acpi_video_backlight_support() from there and backlight will be NULL
if acpi video driver supports backlight. So checking backlight != NULL
is the same as checking acpi_video_backlight_support() plus fallback
if acpi video driver doesn't support backlight and WMI driver failed
to initialize backlight.

>
>> -                     (key->code != MSI_WMI_BRIGHTNESSUP &&
>> -                     key->code != MSI_WMI_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);
>
> Why did you remove this message ? It's useful because it helps users
> report new keys for new hardware.

No, I didn't remove the message, I only moved these lines above. I
understand importance of this message.

>
>> +                     last_pressed[key->code - wmi->scancode_base] = cur;
>> +             }
>> +
>> +             if (key->type == KE_KEY &&
>> +             /* Brightness is served via acpi video driver */
>> +             (!backlight ||
>> +             (key->keycode != KEY_BRIGHTNESSUP &&
>> +             key->keycode != 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");
>>
>> @@ -200,10 +268,45 @@ msi_wmi_notify_exit:
>>       kfree(response.pointer);
>>  }
>>
>> +static int __init msi_wmi_backlight_setup(void)
>> +{
>> +     int err;
>> +     struct backlight_properties props;
>> +
>> +     if (!wmi_has_guid(MSIWMI_BIOS_GUID) || acpi_video_backlight_support())
>> +             return -ENODEV;
>> +
>> +     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;
>>
>> +     if (wmi->quirk_last_pressed) {
>> +             last_pressed = kcalloc(wmi->scancode_count,
>> +                     sizeof(last_pressed[0]), GFP_KERNEL);
>
> I see why you separated the keymaps. I'm pretty sure we can have an
> unified keymap while keeping the last_pressed array working.

I think I could check in msi_wmi_notify if event is between 0xD0 and
0xD4 and if so, use last_pressed.

>> +             if (!last_pressed)
>> +                     return -ENOMEM;
>> +     }
>> +
>>       msi_wmi_input_dev = input_allocate_device();
>>       if (!msi_wmi_input_dev)
>>               return -ENOMEM;
>> @@ -212,7 +315,7 @@ static int __init msi_wmi_input_setup(void)
>>       msi_wmi_input_dev->phys = "wmi/input0";
>>       msi_wmi_input_dev->id.bustype = BUS_HOST;
>>
>> -     err = sparse_keymap_setup(msi_wmi_input_dev, msi_wmi_keymap, NULL);
>> +     err = sparse_keymap_setup(msi_wmi_input_dev, wmi->keymap, NULL);
>>       if (err)
>>               goto err_free_dev;
>>
>> @@ -221,8 +324,6 @@ static int __init msi_wmi_input_setup(void)
>>       if (err)
>>               goto err_free_keymap;
>>
>> -     memset(last_pressed, 0, sizeof(last_pressed));
>> -
>>       return 0;
>>
>>  err_free_keymap:
>> @@ -235,61 +336,59 @@ err_free_dev:
>>  static int __init msi_wmi_init(void)
>>  {
>>       int err;
>> +     bool found = false;
>> +     size_t i;
>> +
>> +     for (i = 0; msi_wmi_table[i].guid; i++) {
>> +             if (wmi_has_guid(msi_wmi_table[i].guid)) {
>> +                     found = true;
>> +                     wmi = &msi_wmi_table[i];
>> +
>> +                     err = msi_wmi_input_setup();
>> +                     if (err) {
>> +                             pr_err("Unable to setup input device\n");
>> +                             wmi = NULL;
>> +                             return 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;
>> +                     err = wmi_install_notify_handler(wmi->guid,
>> +                             msi_wmi_notify, NULL);
>> +                     if (ACPI_FAILURE(err)) {
>> +                             pr_err("Unable to setup WMI notify handler\n");
>> +                             sparse_keymap_free(msi_wmi_input_dev);
>> +                             input_unregister_device(msi_wmi_input_dev);
>> +                             wmi = NULL;
>> +                             return -EINVAL;
>> +                     }
>>
>> -     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);
>> -                     goto err_free_input;
>> +                     pr_debug("Event handler installed\n");
>> +                     break;
>>               }
>> +     }
>>
>> -             err = bl_get(NULL);
>> -             if (err < 0)
>> -                     goto err_free_backlight;
>> +     if (!msi_wmi_backlight_setup()) {
>> +             found = true;
>> +             pr_debug("Backlight device created\n");
>> +     }
>>
>> -             backlight->props.brightness = err;
>> +     if (!found) {
>> +             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_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);
>> -     return err;
>>  }
>>
>>  static void __exit msi_wmi_exit(void)
>>  {
>> -     if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
>> -             wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
>> +     kfree(last_pressed);
>> +     if (wmi) {
>> +             wmi_remove_notify_handler(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);
>>  }
>
> There's a lot of restructuring in here that aren't directly related to
> adding support for new hardware (enums, init path for backlight, quirk,
> etc.), which make this patch hard to review. You should try to put these
> in separate patches.

OK, I will do that and resend later, when I'll receive comments on
other patches and fix all needed things.

> Regards,
>
> Anisse

Re: [PATCH v2 8/8] msi-wmi: Add MSI Wind support

[Anisse Astier]

On Fri, 30 Nov 2012 20:36:48 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> 2012/11/30 Anisse Astier <anisse@astier.eu>:

[snip]
> >> +
> >> +enum wind_scancodes {
> >> +     /* Fn+F3 touchpad toggle */
> >> +     WIND_KEY_TOUCHPAD       = 0x08,
> >> +     /* Fn+F11 Bluetooth toggle */
> >> +     WIND_KEY_BLUETOOTH      = 0x56,
> >> +     /* Fn+F6 webcam toggle */
> >> +     WIND_KEY_CAMERA         = 0x57,
> >> +     /* Fn+F11 Wi-Fi toggle */
> >> +     WIND_KEY_WLAN           = 0x5f,
> >> +     /* Fn+F10 turbo mode toggle */
> >> +     WIND_KEY_TURBO          = 0x60,
> >> +     /* Fn+F10 ECO mode toggle */
> >> +     WIND_KEY_ECO            = 0x69,
> >> +};
> >
> > Looking at the scancode numbers and the key function, I see no collision
> > that necessitate using a separate sparse keymap, or a separate enum.
> > Merging them would simplify a lot of your patch.
> 
> By merging keymaps we will lose information about what laptop
> corresponds the key to. Should I add this information in comments or
> is it better to use some other way?

I don't think this information would be useful to the driver. Just add it
in a comment.



[snip]

> >> +
> >> +static struct msi_wmi_table_entry msi_wmi_table[] = {
> >> +     {
> >> +             .guid                   = MSIWMI_MSI_EVENT_GUID,
> >> +             .keymap                 = msi_wmi_keymap,
> >> +             .quirk_last_pressed     = true,
> >> +             .scancode_base          = MSI_SCANCODE_BASE,
> >> +             .scancode_count         = ARRAY_SIZE(msi_wmi_keymap)-1,
> >> +     },
> >> +     {
> >> +             .guid                   = MSIWMI_WIND_EVENT_GUID,
> >> +             .keymap                 = wind_wmi_keymap,
> >> +     },
> >> +     {}
> >> +};
> > If you merge them you would only need the quirk_last_pressed variable,
> > and could do away with this struct.
> 
> Yes, you're right. But what if I would completely remove
> quirk_last_pressed and would use last_pressed if event 0xD0 - 0xD4
> happens, otherwise skip last_pressed check? Is it good way too?

Hum, I don't know, this doesn't seem like a good design, hardcoding
values this way (even if the net-effect would be the same).


> 
> >
> >> +
> >> +static struct msi_wmi_table_entry *wmi;
> >> +
> >>  static int msi_wmi_query_block(int instance, int *ret)
> >>  {
> >>       acpi_status status;
> >> @@ -165,11 +231,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 (wmi->quirk_last_pressed) {
> >>                       ktime_t diff;
> >>                       cur = ktime_get_real();
> >>                       diff = ktime_sub(cur, last_pressed[key->code -
> >> -                                     SCANCODE_BASE]);
> >> +                                     wmi->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,21 +248,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 - SCANCODE_BASE] = cur;
> >> -
> >> -                     if (key->type == KE_KEY &&
> >> -                     /* Brightness is served via acpi video driver */
> >> -                     (!acpi_video_backlight_support() ||
> >
> > You modify behaviour by removing this test. Backlight might be
> > registered, but we may not want to modify it directly from here because
> > the acpi video driver does that for us.
> 
> I don't remove the test. I only change !acpi_video_backlight_support()
> condition by !backlight condition (actually, it's a typo, condition
> have to look like 'backlight', not '!backlight', I'm sorry for this).
> There is a check in msi_wmi_backlight_setup(). I call
> acpi_video_backlight_support() from there and backlight will be NULL
> if acpi video driver supports backlight. So checking backlight != NULL
> is the same as checking acpi_video_backlight_support() plus fallback
> if acpi video driver doesn't support backlight and WMI driver failed
> to initialize backlight.

You're right. This is true because acpi_video_backlight_support's result
doesn't change at runtime.

[snip]

> >>  static int __init msi_wmi_input_setup(void)
> >>  {
> >>       int err;
> >>
> >> +     if (wmi->quirk_last_pressed) {
> >> +             last_pressed = kcalloc(wmi->scancode_count,
> >> +                     sizeof(last_pressed[0]), GFP_KERNEL);
> >
> > I see why you separated the keymaps. I'm pretty sure we can have an
> > unified keymap while keeping the last_pressed array working.
> 
> I think I could check in msi_wmi_notify if event is between 0xD0 and
> 0xD4 and if so, use last_pressed.

As I said, not a good idea to hardcode the range.

[snip]

> >
> > There's a lot of restructuring in here that aren't directly related to
> > adding support for new hardware (enums, init path for backlight, quirk,
> > etc.), which make this patch hard to review. You should try to put these
> > in separate patches.
> 
> OK, I will do that and resend later, when I'll receive comments on
> other patches and fix all needed things.
> 

Thanks,

Anisse

Re: [PATCH v2 8/8] msi-wmi: Add MSI Wind support

[joeyli]

Hi Maxim, 

First, thanks for your effort for transform your patch to msi-wmi.

於 五，2012-11-30 於 18:22 +0100，Anisse Astier 提到：
> 
> There's a lot of restructuring in here that aren't directly related to
> adding support for new hardware (enums, init path for backlight,
> quirk,
> etc.), which make this patch hard to review. You should try to put
> these
> in separate patches.
> 
> Regards,
> 
> Anisse 

I also reviewed this patch and I agree Anisse's suggestion. This patch
is big, it not just add the Wind series support and also do some source
code clear up and refactory. 

Please try to separate them.


Thanks a lot!
Joey Lee