Re: [PATCH 3/6] msi-laptop: Code cleanups

[joeyli <jlee@suse.com>]

於 三，2012-11-28 於 19:32 +0200，Maxim Mikityanskiy 提到：
> 2012/11/28 joeyli <jlee@suse.com>:
> > 於 日，2012-11-25 於 00:28 +0200，Maxim Mikityanskiy 提到：
> >> Replaced msi_init_rfkill() by msi_update_rfkill(), removed delays by
> >> replacing delayed works by just works and made msi_laptop_i8042_filter()
> >> filter out KEY_TOUCHPAD_TOGGLE when sending KEY_TOUCHPAD_ON and
> >> KEY_TOUCHPAD_OFF.
> >
> > Please separate to 2 patches, one for touchpad key and other one for
> > rfkill.
> >
> > And,
> > I am not agree for the clean up to msi_init_rfkill unless it causes
> > problem on U90/U100. Did you find any issue on U90/U100?
> 
> In original code, we call get_wireless_state_ec_standard() in
> rfkill_init() in order to get initial rfkill state from hardware and
> after a second we call msi_init_rfkill() that sets rfkill state and
> writes it to hardware. msi_update_rfkill() reads rfkill state from
> hardware and sets it immediately. On U90/U100 writing to EC confuses
> it. In patch 4 I add a quirk in set_device_state() that blocks writing
> to EC so msi_init_rfkill() should not cause any issue, but we would
> have unneeded 1 second delay and function calls.
> 
> > I delay 1 magic second for msi_rfkill_init because there have other MSI
> > model need a delay time after we set SCM mode through write EC
> > register.
> > So, please don't change the code for msi_init_rfkill delay work, just
> > keep it works with other MSI shipping machines.
> 
> What do you think if I remove delay only for U90/U100 and keep it for
> other models?

Please consider to include this patch[1] to your msi-laptop patch set.
This patch introduced quirk_entry and merged quirk tables to
msi_dmi_table.

Currently I don't have msi machine on my hand, so I didn't test this
patch. Just feel free to modify or even separate it for put to your
patches.

And, I put a quirk that name is ec_delay, please consider to use this
quirk in your patch for add the 1 second (and 0.5 second) for other SCM
machines.

> 
> >>
> >> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> >> ---
> >>  drivers/platform/x86/msi-laptop.c | 67 ++++++++++++++-------------------------
> >>  1 file changed, 24 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
> >> index 7ba107a..3b6f494 100644
> >> --- a/drivers/platform/x86/msi-laptop.c
> >> +++ b/drivers/platform/x86/msi-laptop.c
> >> @@ -600,8 +600,23 @@ static const struct rfkill_ops rfkill_threeg_ops = {
> >>       .set_block = rfkill_threeg_set
> >>  };
...
> >>  static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
> >>                               struct serio *port)
> >> @@ -662,14 +664,16 @@ 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));
> >> -                     break;
> >> +                     schedule_work(&msi_touchpad_work);
> >
> > Don't remove the 0.5 magic delay, please! It will cause other shipped
> > MSI machines got problem could not capture the real change of EC
> > register.
> 
> The same question here.
> 

Here the same, please consider to use this quirk in your patch for add
the 0.5 second for other SCM machines.

> >> +             case 0x64:
> >> +                     /* Filter out KEY_TOUCHPAD_TOGGLE and send only
> >> +                      * KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF
> >> +                      */
> >> +                     return true;
> >
> > Why this patch filter out 0x64 key? What's the scan code emitted when
> > press the touchpad on/off Fn key on U100? Does it emit
> > KEY_TOUCHPAD_TOGGLE and KEY_TOUCHPAD_ON/OFF at the same time?
> 
> 0xE4 stands for Fn+F3 release and 0x64 stands for Fn+F3 press. So, if
> we do not use msi-laptop driver, we will get KEY_TOUCHPAD_TOGGLE
> (maybe from i8042 driver). If we load msi-laptop driver, it hooks
> Fn+F3 release and sends KEY_TOUCHPAD_ON/OFF, so userspace will receive
> KEY_TOUCHPAD_TOGGLE before KEY_TOUCHPAD_ON/OFF. So we need to filter
> out both Fn+F3 press and release, so that KEY_TOUCHPAD_TOGGLE will not
> be sent.

OK, understood!

> >>               case 0x54:
> >>               case 0x62:
> >>               case 0x76:
> >> -                     schedule_delayed_work(&msi_rfkill_work,
> >> -                             round_jiffies_relative(0.5 * HZ));
> >> +                     schedule_work(&msi_rfkill_work);
> >>                       break;
> >>               }
...

> >>       platform_device_del(msipf_device);
> >> @@ -976,7 +958,6 @@ static void __exit msi_cleanup(void)
> >>       if (load_scm_model) {
> >>               i8042_remove_filter(msi_laptop_i8042_filter);
> >>               msi_laptop_input_destroy();
> >> -             cancel_delayed_work_sync(&msi_rfkill_work);
> >>               rfkill_cleanup();
> >>       }


Thanks a lot!
Joey Lee

[1]

From 79db5e187ac10aea5f142ce0a352dcd17442c190 Mon Sep 17 00:00:00 2001
From: "Lee, Chun-Yi" <jlee@suse.com>
Date: Fri, 30 Nov 2012 01:22:21 +0800
Subject: [PATCH] msi-laptop: merge quirk tables to one

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>
---
 drivers/platform/x86/msi-laptop.c |  125 +++++++++++++++++++++----------------
 1 file changed, 72 insertions(+), 53 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 16e9863..2447128 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -119,29 +119,35 @@ 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;
+static struct rfkill *rfk_wlan, *rfk_bluetooth, *rfk_threeg;
 
-/* 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;
+/* MSI laptop quirks */
+struct quirk_entry {
+	u8 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
+	 */
+	u8 load_scm_model;
+	u8 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.
- */
-static bool ec_read_only;
+	/* 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.
+	 */
+	u8 ec_read_only;
+};
 
-static struct rfkill *rfk_wlan, *rfk_bluetooth, *rfk_threeg;
+static struct quirk_entry *quirks;
 
 /* Hardware access */
 
@@ -213,7 +219,7 @@ 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 (ec_read_only)
+	if (quirks->ec_read_only)
 		return -EOPNOTSUPP;
 
 	/* read current device state */
@@ -314,7 +320,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();
@@ -338,7 +344,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();
@@ -363,7 +369,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();
@@ -566,9 +572,29 @@ 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 = 1,
+};
+
+static struct quirk_entry quirk_load_scm_model = {
+	.load_scm_model = 1,
+	.ec_delay = 1,
+};
+
+static struct quirk_entry quirk_load_scm_ro_model = {
+	.load_scm_model = 1,
+	.ec_read_only = 1,
+};
+
+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);
+
+	if (dmi->driver_data)
+		quirks = dmi->driver_data;
+	else
+		quirks = &quirk_load_scm_model;
+
 	return 1;
 }
 
@@ -582,6 +608,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
 	},
 	{
@@ -592,6 +619,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
 	},
 	{
@@ -602,6 +630,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
 	},
 	{
@@ -613,12 +642,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 = {
@@ -628,6 +654,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
 	},
 	{
@@ -639,6 +666,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
 	},
 	{
@@ -648,6 +676,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
 	},
 	{
@@ -657,6 +686,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
 	},
 	{
@@ -666,12 +696,9 @@ 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
 	},
-	{ }
-};
-
-static struct dmi_system_id __initdata msi_load_scm_ro_models_dmi_table[] = {
 	{
 		.ident = "MSI U90/U100",
 		.matches = {
@@ -679,6 +706,7 @@ static struct dmi_system_id __initdata msi_load_scm_ro_models_dmi_table[] = {
 				"MICRO-STAR INTERNATIONAL CO., LTD"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "U90/U100"),
 		},
+		.driver_data = &quirk_load_scm_ro_model,
 		.callback = dmi_check_cb
 	},
 	{ }
@@ -727,7 +755,7 @@ static const struct rfkill_ops rfkill_threeg_ops = {
 
 static bool msi_rfkill_set_state(struct rfkill *rfkill, bool blocked)
 {
-	if (ec_read_only)
+	if (quirks->ec_read_only)
 		return rfkill_set_hw_state(rfkill, blocked);
 	else
 		return rfkill_set_sw_state(rfkill, blocked);
@@ -877,7 +905,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 */
@@ -935,7 +963,7 @@ static int __init load_scm_model_init(struct platform_device *sdev)
 	u8 data;
 	int result;
 
-	if (!ec_read_only) {
+	if (!quirks->ec_read_only) {
 		/* allow userland write sysfs file  */
 		dev_attr_bluetooth.store = store_bluetooth;
 		dev_attr_wlan.store = store_wlan;
@@ -992,21 +1020,12 @@ 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 (force)
+		quirks = &quirk_old_ec_model;
 
-	if (!old_ec_model) {
+	if (!quirks->old_ec_model)
 		get_threeg_exists();
-		if (dmi_check_system(msi_load_scm_models_dmi_table))
-			load_scm_model = 1;
-		else if (dmi_check_system(msi_load_scm_ro_models_dmi_table)) {
-			load_scm_model = 1;
-			ec_read_only = 1;
-		}
-	}
-
-	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;
@@ -1043,7 +1062,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;
 	}
@@ -1053,7 +1072,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);
@@ -1074,7 +1093,7 @@ 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);
 		rfkill_cleanup();
 	}
@@ -1097,14 +1116,14 @@ 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();
 		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.7.10.4