[PATCH 0/4] Topstar ACPI LED Workaround

[PATCH 0/4] Topstar ACPI LED Workaround

[Guillaume Douézan-Grard]

Hi Darren,

On Topstar U931 Notebooks, an issue prevents the WLAN toggle key from
being properly managed by the Embedded Controller and successfully
disconnect the adapter. A specific ACPI method allows to toggle the WLAN
LED state regardless.

These are barebone laptops, sold under quite a lot of brands and
configurations, with different firmwares and so on. I can only say for sure
that this issue is present for all the models sold under a specific brand,
that's why I'm reluctant to enable this by default with a DMI check.

Thus, the new `led_workaround` option registers this LED with the
corresponding subsystem, making possible to use a software-based trigger
(rfkill for instance to synchronize the LED with the softkill state).

Thank you for your time,

--
Guillaume Douézan-Grard

Guillaume Douézan-Grard (4):
  platform/x86: topstar-laptop: non-functional changes
  platform/x86: topstar-laptop: change to generic module
  platform/x86: topstar-laptop: add platform device
  platform/x86: topstar-laptop: add optional WLAN LED workaround

 drivers/platform/x86/Kconfig          |   2 +
 drivers/platform/x86/topstar-laptop.c | 339 ++++++++++++++++++++++++++--------
 2 files changed, 267 insertions(+), 74 deletions(-)

-- 
2.14.1

[PATCH 1/4] platform/x86: topstar-laptop: non-functional changes

[Guillaume Douézan-Grard]

Minor consistency changes to prepare further addition of platform device
and LED.

More precisely:

* more consistent naming (module description, header text, devices names
and programming constructs),

* clear separation between systems (ACPI events and input handling),

* copyright and module authors update.

Signed-off-by: Guillaume Douézan-Grard <gdouezangrard@gmail.com>
---
 drivers/platform/x86/topstar-laptop.c | 185 ++++++++++++++++++++--------------
 1 file changed, 110 insertions(+), 75 deletions(-)

diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index 1032c00b907b..d3197302aa91 100644
--- a/drivers/platform/x86/topstar-laptop.c
+++ b/drivers/platform/x86/topstar-laptop.c
@@ -1,10 +1,11 @@
 /*
- * ACPI driver for Topstar notebooks (hotkeys support only)
+ * Topstar Laptop ACPI Extras
  *
  * Copyright (c) 2009 Herton Ronaldo Krzesinski <herton@mandriva.com.br>
+ * Copyright (c) 2017 Guillaume Douézan-Grard
  *
  * Implementation inspired by existing x86 platform drivers, in special
- * asus/eepc/fujitsu-laptop, thanks to their authors
+ * asus/eepc/fujitsu-laptop, thanks to their authors.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -21,12 +22,17 @@
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 
-#define ACPI_TOPSTAR_CLASS "topstar"
+#define TOPSTAR_LAPTOP_CLASS "topstar"
 
-struct topstar_hkey {
-	struct input_dev *inputdev;
+struct topstar_laptop {
+	struct acpi_device *device;
+	struct input_dev *input;
 };
 
+/*
+ * Input
+ */
+
 static const struct key_entry topstar_keymap[] = {
 	{ KE_KEY, 0x80, { KEY_BRIGHTNESSUP } },
 	{ KE_KEY, 0x81, { KEY_BRIGHTNESSDOWN } },
@@ -57,32 +63,60 @@ static const struct key_entry topstar_keymap[] = {
 	{ KE_END, 0 }
 };
 
-static void acpi_topstar_notify(struct acpi_device *device, u32 event)
+static void topstar_input_notify(struct topstar_laptop *topstar, int event)
 {
-	static bool dup_evnt[2];
-	bool *dup;
-	struct topstar_hkey *hkey = acpi_driver_data(device);
+	if (!sparse_keymap_report_event(topstar->input, event, 1, true))
+		pr_info("Unknown key %x pressed\n", event);
+}
 
-	/* 0x83 and 0x84 key events comes duplicated... */
-	if (event == 0x83 || event == 0x84) {
-		dup = &dup_evnt[event - 0x83];
-		if (*dup) {
-			*dup = false;
-			return;
-		}
-		*dup = true;
+static int topstar_input_init(struct topstar_laptop *topstar)
+{
+	struct input_dev *input;
+	int err;
+
+	input = input_allocate_device();
+	if (!input)
+		return -ENOMEM;
+
+	input->name = "Topstar Laptop Extra Buttons";
+	input->phys = TOPSTAR_LAPTOP_CLASS "/input0";
+	input->id.bustype = BUS_HOST;
+
+	err = sparse_keymap_setup(input, topstar_keymap, NULL);
+	if (err) {
+		pr_err("Unable to setup input device keymap\n");
+		goto err_input;
+	}
+
+	err = input_register_device(input);
+	if (err) {
+		pr_err("Unable to register input device\n");
+		goto err_input;
 	}
 
-	if (!sparse_keymap_report_event(hkey->inputdev, event, 1, true))
-		pr_info("unknown event = 0x%02x\n", event);
+	topstar->input = input;
+	return 0;
+
+err_input:
+	input_free_device(input);
+	return err;
+}
+
+static void topstar_input_exit(struct topstar_laptop *topstar)
+{
+	input_unregister_device(topstar->input);
 }
 
-static int acpi_topstar_fncx_switch(struct acpi_device *device, bool state)
+/*
+ * ACPI
+ */
+
+static int topstar_acpi_fncx_switch(struct acpi_device *device, bool state)
 {
 	acpi_status status;
 
 	status = acpi_execute_simple_method(device->handle, "FNCX",
-						state ? 0x86 : 0x87);
+			state ? 0x86 : 0x87);
 	if (ACPI_FAILURE(status)) {
 		pr_err("Unable to switch FNCX notifications\n");
 		return -ENODEV;
@@ -91,73 +125,74 @@ static int acpi_topstar_fncx_switch(struct acpi_device *device, bool state)
 	return 0;
 }
 
-static int acpi_topstar_init_hkey(struct topstar_hkey *hkey)
+static int topstar_acpi_init(struct topstar_laptop *topstar)
 {
-	struct input_dev *input;
-	int error;
-
-	input = input_allocate_device();
-	if (!input)
-		return -ENOMEM;
+	return topstar_acpi_fncx_switch(topstar->device, true);
+}
 
-	input->name = "Topstar Laptop extra buttons";
-	input->phys = "topstar/input0";
-	input->id.bustype = BUS_HOST;
+static void topstar_acpi_exit(struct topstar_laptop *topstar)
+{
+	topstar_acpi_fncx_switch(topstar->device, false);
+}
 
-	error = sparse_keymap_setup(input, topstar_keymap, NULL);
-	if (error) {
-		pr_err("Unable to setup input device keymap\n");
-		goto err_free_dev;
-	}
+static void topstar_acpi_notify(struct acpi_device *device, u32 event)
+{
+	struct topstar_laptop *topstar = acpi_driver_data(device);
+	static bool dup_evnt[2];
+	bool *dup;
 
-	error = input_register_device(input);
-	if (error) {
-		pr_err("Unable to register input device\n");
-		goto err_free_dev;
+	/* 0x83 and 0x84 key events comes duplicated... */
+	if (event == 0x83 || event == 0x84) {
+		dup = &dup_evnt[event - 0x83];
+		if (*dup) {
+			*dup = false;
+			return;
+		}
+		*dup = true;
 	}
 
-	hkey->inputdev = input;
-	return 0;
-
- err_free_dev:
-	input_free_device(input);
-	return error;
+	topstar_input_notify(topstar, event);
 }
 
-static int acpi_topstar_add(struct acpi_device *device)
+static int topstar_acpi_add(struct acpi_device *device)
 {
-	struct topstar_hkey *tps_hkey;
+	struct topstar_laptop *topstar;
+	int err;
 
-	tps_hkey = kzalloc(sizeof(struct topstar_hkey), GFP_KERNEL);
-	if (!tps_hkey)
+	topstar = kzalloc(sizeof(struct topstar_laptop), GFP_KERNEL);
+	if (!topstar)
 		return -ENOMEM;
 
-	strcpy(acpi_device_name(device), "Topstar TPSACPI");
-	strcpy(acpi_device_class(device), ACPI_TOPSTAR_CLASS);
+	strcpy(acpi_device_name(device), "Topstar Laptop ACPI");
+	strcpy(acpi_device_class(device), TOPSTAR_LAPTOP_CLASS);
+	device->driver_data = topstar;
+	topstar->device = device;
 
-	if (acpi_topstar_fncx_switch(device, true))
-		goto add_err;
+	err = topstar_acpi_init(topstar);
+	if (err)
+		goto err_acpi;
 
-	if (acpi_topstar_init_hkey(tps_hkey))
-		goto add_err;
+	err = topstar_input_init(topstar);
+	if (err)
+		goto err_acpi_input;
 
-	device->driver_data = tps_hkey;
 	return 0;
 
-add_err:
-	kfree(tps_hkey);
-	return -ENODEV;
+err_acpi_input:
+	topstar_acpi_exit(topstar);
+err_acpi:
+	kfree(topstar);
+	return err;
 }
 
-static int acpi_topstar_remove(struct acpi_device *device)
+static int topstar_acpi_remove(struct acpi_device *device)
 {
-	struct topstar_hkey *tps_hkey = acpi_driver_data(device);
-
-	acpi_topstar_fncx_switch(device, false);
+	struct topstar_laptop *topstar = acpi_driver_data(device);
 
-	input_unregister_device(tps_hkey->inputdev);
-	kfree(tps_hkey);
+	topstar_input_exit(topstar);
+	topstar_acpi_exit(topstar);
 
+	kfree(topstar);
 	return 0;
 }
 
@@ -168,18 +203,18 @@ static const struct acpi_device_id topstar_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, topstar_device_ids);
 
-static struct acpi_driver acpi_topstar_driver = {
-	.name = "Topstar laptop ACPI driver",
-	.class = ACPI_TOPSTAR_CLASS,
+static struct acpi_driver topstar_acpi_driver = {
+	.name = "Topstar Laptop ACPI Driver",
+	.class = TOPSTAR_LAPTOP_CLASS,
 	.ids = topstar_device_ids,
 	.ops = {
-		.add = acpi_topstar_add,
-		.remove = acpi_topstar_remove,
-		.notify = acpi_topstar_notify,
+		.add = topstar_acpi_add,
+		.remove = topstar_acpi_remove,
+		.notify = topstar_acpi_notify,
 	},
 };
-module_acpi_driver(acpi_topstar_driver);
+module_acpi_driver(topstar_acpi_driver);
 
-MODULE_AUTHOR("Herton Ronaldo Krzesinski");
-MODULE_DESCRIPTION("Topstar Laptop ACPI Extras driver");
+MODULE_AUTHOR("Herton Ronaldo Krzesinski, Guillaume Douézan-Grard");
+MODULE_DESCRIPTION("Topstar Laptop ACPI Extras");
 MODULE_LICENSE("GPL");
-- 
2.14.1

[PATCH 2/4] platform/x86: topstar-laptop: change to generic module

[Guillaume Douézan-Grard]

Signed-off-by: Guillaume Douézan-Grard <gdouezangrard@gmail.com>
---
 drivers/platform/x86/topstar-laptop.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index d3197302aa91..ce33754c1f29 100644
--- a/drivers/platform/x86/topstar-laptop.c
+++ b/drivers/platform/x86/topstar-laptop.c
@@ -213,7 +213,25 @@ static struct acpi_driver topstar_acpi_driver = {
 		.notify = topstar_acpi_notify,
 	},
 };
-module_acpi_driver(topstar_acpi_driver);
+
+static int __init topstar_laptop_init(void)
+{
+	int res;
+
+	res = acpi_bus_register_driver(&topstar_acpi_driver);
+	if (res < 0)
+		return res;
+
+	return 0;
+}
+
+static void __exit topstar_laptop_exit(void)
+{
+	acpi_bus_unregister_driver(&topstar_acpi_driver);
+}
+
+module_init(topstar_laptop_init);
+module_exit(topstar_laptop_exit);
 
 MODULE_AUTHOR("Herton Ronaldo Krzesinski, Guillaume Douézan-Grard");
 MODULE_DESCRIPTION("Topstar Laptop ACPI Extras");
-- 
2.14.1

[PATCH 3/4] platform/x86: topstar-laptop: add platform device

[Guillaume Douézan-Grard]

Signed-off-by: Guillaume Douézan-Grard <gdouezangrard@gmail.com>
---
 drivers/platform/x86/topstar-laptop.c | 57 ++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index ce33754c1f29..e2ee1e5cc734 100644
--- a/drivers/platform/x86/topstar-laptop.c
+++ b/drivers/platform/x86/topstar-laptop.c
@@ -19,6 +19,7 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/platform_device.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 
@@ -26,6 +27,7 @@
 
 struct topstar_laptop {
 	struct acpi_device *device;
+	struct platform_device *platform;
 	struct input_dev *input;
 };
 
@@ -81,6 +83,7 @@ static int topstar_input_init(struct topstar_laptop *topstar)
 	input->name = "Topstar Laptop Extra Buttons";
 	input->phys = TOPSTAR_LAPTOP_CLASS "/input0";
 	input->id.bustype = BUS_HOST;
+	input->dev.parent = &topstar->platform->dev;
 
 	err = sparse_keymap_setup(input, topstar_keymap, NULL);
 	if (err) {
@@ -107,6 +110,41 @@ static void topstar_input_exit(struct topstar_laptop *topstar)
 	input_unregister_device(topstar->input);
 }
 
+/*
+ * Platform
+ */
+
+static struct platform_driver topstar_platform_driver = {
+	.driver = {
+		.name = TOPSTAR_LAPTOP_CLASS,
+	},
+};
+
+static int topstar_platform_init(struct topstar_laptop *topstar)
+{
+	int err;
+
+	topstar->platform = platform_device_alloc(TOPSTAR_LAPTOP_CLASS, -1);
+	if (!topstar->platform)
+		return -ENOMEM;
+	platform_set_drvdata(topstar->platform, topstar);
+
+	err = platform_device_add(topstar->platform);
+	if (err)
+		goto err_platform;
+
+	return 0;
+
+err_platform:
+	platform_device_put(topstar->platform);
+	return err;
+}
+
+static void topstar_platform_exit(struct topstar_laptop *topstar)
+{
+	platform_device_unregister(topstar->platform);
+}
+
 /*
  * ACPI
  */
@@ -172,6 +210,10 @@ static int topstar_acpi_add(struct acpi_device *device)
 	if (err)
 		goto err_acpi;
 
+	err = topstar_platform_init(topstar);
+	if (err)
+		goto err_acpi_platform;
+
 	err = topstar_input_init(topstar);
 	if (err)
 		goto err_acpi_input;
@@ -179,6 +221,8 @@ static int topstar_acpi_add(struct acpi_device *device)
 	return 0;
 
 err_acpi_input:
+	topstar_platform_exit(topstar);
+err_acpi_platform:
 	topstar_acpi_exit(topstar);
 err_acpi:
 	kfree(topstar);
@@ -190,6 +234,7 @@ static int topstar_acpi_remove(struct acpi_device *device)
 	struct topstar_laptop *topstar = acpi_driver_data(device);
 
 	topstar_input_exit(topstar);
+	topstar_platform_exit(topstar);
 	topstar_acpi_exit(topstar);
 
 	kfree(topstar);
@@ -218,16 +263,26 @@ static int __init topstar_laptop_init(void)
 {
 	int res;
 
+	res = platform_driver_register(&topstar_platform_driver);
+	if (res < 0)
+		goto err_laptop_platform;
+
 	res = acpi_bus_register_driver(&topstar_acpi_driver);
 	if (res < 0)
-		return res;
+		goto err_laptop_acpi;
 
 	return 0;
+
+err_laptop_acpi:
+	platform_driver_unregister(&topstar_platform_driver);
+err_laptop_platform:
+	return res;
 }
 
 static void __exit topstar_laptop_exit(void)
 {
 	acpi_bus_unregister_driver(&topstar_acpi_driver);
+	platform_driver_unregister(&topstar_platform_driver);
 }
 
 module_init(topstar_laptop_init);
-- 
2.14.1

[PATCH 4/4] platform/x86: topstar-laptop: add optional WLAN LED workaround

[Guillaume Douézan-Grard]

Topstar U931 laptops provide an LED synced with the WLAN adapter
hard-blocking state. Unfortunately, some models seem to be defective,
making impossible to hard-block the adapter with the WLAN switch and
thus the LED is useless.

An ACPI method is available to programmatically control this switch and
it indirectly allows to control the LED.

This commit registers the LED within the corresponding subsystem, making
possible for instance to use an rfkill-based trigger to synchronize the
LED with the soft-blocking state.

This feature is disabled by default and can be enabled with the
`led_workaround` module parameter.

Signed-off-by: Guillaume Douézan-Grard <gdouezangrard@gmail.com>
---
 drivers/platform/x86/Kconfig          |  2 +
 drivers/platform/x86/topstar-laptop.c | 83 +++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 80b87954f6dd..392f09066a80 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -684,6 +684,8 @@ config TOPSTAR_LAPTOP
 	depends on ACPI
 	depends on INPUT
 	select INPUT_SPARSEKMAP
+	select LEDS_CLASS
+	select NEW_LEDS
 	---help---
 	  This driver adds support for hotkeys found on Topstar laptops.
 
diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index e2ee1e5cc734..f57a082fb75f 100644
--- a/drivers/platform/x86/topstar-laptop.c
+++ b/drivers/platform/x86/topstar-laptop.c
@@ -22,15 +22,87 @@
 #include <linux/platform_device.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
+#include <linux/leds.h>
 
 #define TOPSTAR_LAPTOP_CLASS "topstar"
 
+static bool led_workaround;
+module_param_named(led_workaround, led_workaround, bool, 0444);
+MODULE_PARM_DESC(led_workaround,
+		"Enables software-based WLAN LED control on systems with defective hardware switch");
+
 struct topstar_laptop {
 	struct acpi_device *device;
 	struct platform_device *platform;
 	struct input_dev *input;
+	struct led_classdev led;
 };
 
+/*
+ * LED
+ */
+
+static enum led_brightness topstar_led_get(struct led_classdev *led)
+{
+	return led->brightness;
+}
+
+static int topstar_led_set(struct led_classdev *led,
+		enum led_brightness state)
+{
+	struct topstar_laptop *topstar = container_of(led,
+			struct topstar_laptop, led);
+
+	struct acpi_object_list params;
+	union acpi_object in_obj;
+	unsigned long long int ret;
+	acpi_status status;
+
+	params.count = 1;
+	params.pointer = &in_obj;
+	in_obj.type = ACPI_TYPE_INTEGER;
+	in_obj.integer.value = 0x83;
+
+	/*
+	 * Topstar ACPI returns 0x30001 when the LED is ON and 0x30000 when it
+	 * is OFF.
+	 */
+	status = acpi_evaluate_integer(topstar->device->handle,
+			"GETX", &params, &ret);
+	if (ACPI_FAILURE(status))
+		return -1;
+
+	/*
+	 * FNCX(0x83) toggles the LED (more precisely, it is supposed to
+	 * act as an hardware switch and disconnect the WLAN adapter but
+	 * it seems to be faulty on some models like the Topstar U931
+	 * Notebook).
+	 */
+	if ((ret == 0x30001 && state == LED_OFF)
+			|| (ret == 0x30000 && state != LED_OFF)) {
+		status = acpi_execute_simple_method(topstar->device->handle,
+				"FNCX", 0x83);
+		if (ACPI_FAILURE(status))
+			return -1;
+	}
+
+	return 0;
+}
+
+static int topstar_led_init(struct topstar_laptop *topstar)
+{
+	topstar->led.name = "topstar::wlan";
+	topstar->led.brightness_get = topstar_led_get;
+	topstar->led.brightness_set_blocking = topstar_led_set;
+	topstar->led.default_trigger = "rfkill0";
+	return led_classdev_register(&topstar->platform->dev, &topstar->led);
+}
+
+static void topstar_led_exit(struct topstar_laptop *topstar)
+{
+	led_classdev_unregister(&topstar->led);
+}
+
 /*
  * Input
  */
@@ -218,8 +290,16 @@ static int topstar_acpi_add(struct acpi_device *device)
 	if (err)
 		goto err_acpi_input;
 
+	if (led_workaround) {
+		err = topstar_led_init(topstar);
+		if (err)
+			goto err_acpi_led;
+	}
+
 	return 0;
 
+err_acpi_led:
+	topstar_input_exit(topstar);
 err_acpi_input:
 	topstar_platform_exit(topstar);
 err_acpi_platform:
@@ -233,6 +313,9 @@ static int topstar_acpi_remove(struct acpi_device *device)
 {
 	struct topstar_laptop *topstar = acpi_driver_data(device);
 
+	if (led_workaround)
+		topstar_led_exit(topstar);
+
 	topstar_input_exit(topstar);
 	topstar_platform_exit(topstar);
 	topstar_acpi_exit(topstar);
-- 
2.14.1

Re: [PATCH 0/4] Topstar ACPI LED Workaround

[Andy Shevchenko]

On Tue, Oct 17, 2017 at 7:17 PM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> Hi Darren,
>
> On Topstar U931 Notebooks, an issue prevents the WLAN toggle key from
> being properly managed by the Embedded Controller and successfully
> disconnect the adapter. A specific ACPI method allows to toggle the WLAN
> LED state regardless.
>
> These are barebone laptops, sold under quite a lot of brands and
> configurations, with different firmwares and so on. I can only say for sure
> that this issue is present for all the models sold under a specific brand,
> that's why I'm reluctant to enable this by default with a DMI check.
>
> Thus, the new `led_workaround` option registers this LED with the
> corresponding subsystem, making possible to use a software-based trigger
> (rfkill for instance to synchronize the LED with the softkill state).
>
> Thank you for your time,

I have applied to my review and testing queue with some changes:
- device names are left unchanged (it's ABI, we can't just change
device name to whatever we want if we can't guarantee that doesn't
break anything, at least I can't)
- labels should mark what is expected to be executed when code chooses that path
- I split module authors to two macro calls (this makes slightly
better readability in the sources and when one calls modinfo)

Can you check if it's okay with your devices and works as expected?

If everything okay, take my them from review-andy branch, add commit
message to where it's missed and send a new version.

Thanks.

>
> --
> Guillaume Douézan-Grard
>
> Guillaume Douézan-Grard (4):
>   platform/x86: topstar-laptop: non-functional changes
>   platform/x86: topstar-laptop: change to generic module
>   platform/x86: topstar-laptop: add platform device
>   platform/x86: topstar-laptop: add optional WLAN LED workaround
>
>  drivers/platform/x86/Kconfig          |   2 +
>  drivers/platform/x86/topstar-laptop.c | 339 ++++++++++++++++++++++++++--------
>  2 files changed, 267 insertions(+), 74 deletions(-)
>
> --
> 2.14.1
>



-- 
With Best Regards,
Andy Shevchenko