[PATCH v2 0/4] Topstar ACPI LED Workaround

[PATCH v2 0/4] Topstar ACPI LED Workaround

[Guillaume Douézan-Grard]

Hi Andy,

Thank you for your review. Changes since the previous submission are
highlighted below.

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


Changes since v1:

* leave devices names unchanged for ABI compatibility,
* fix label names,
* separate module authors definition,
* add commit description to Patch 3.


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 | 334 ++++++++++++++++++++++++++--------
 2 files changed, 265 insertions(+), 71 deletions(-)

-- 
2.14.3

[PATCH v2 0/4] Topstar ACPI LED Workaround

[Guillaume Douézan-Grard]

Hi Andy,

Thank you for your review. Changes since the previous submission are
highlighted below.

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,

[PATCH v2 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 | 178 ++++++++++++++++++++--------------
 1 file changed, 107 insertions(+), 71 deletions(-)

diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index 1032c00b907b..106537fdc4e6 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,107 +63,136 @@ static const struct key_entry topstar_keymap[] = {
 	{ KE_END, 0 }
 };
 
-static void acpi_topstar_notify(struct acpi_device *device, u32 event)
-{
-	static bool dup_evnt[2];
-	bool *dup;
-	struct topstar_hkey *hkey = acpi_driver_data(device);
-
-	/* 0x83 and 0x84 key events comes duplicated... */
-	if (event == 0x83 || event == 0x84) {
-		dup = &dup_evnt[event - 0x83];
-		if (*dup) {
-			*dup = false;
-			return;
-		}
-		*dup = true;
-	}
-
-	if (!sparse_keymap_report_event(hkey->inputdev, event, 1, true))
-		pr_info("unknown event = 0x%02x\n", event);
-}
-
-static int acpi_topstar_fncx_switch(struct acpi_device *device, bool state)
+static void topstar_input_notify(struct topstar_laptop *topstar, int event)
 {
-	acpi_status status;
-
-	status = acpi_execute_simple_method(device->handle, "FNCX",
-						state ? 0x86 : 0x87);
-	if (ACPI_FAILURE(status)) {
-		pr_err("Unable to switch FNCX notifications\n");
-		return -ENODEV;
-	}
-
-	return 0;
+	if (!sparse_keymap_report_event(topstar->input, event, 1, true))
+		pr_info("Unknown key %x pressed\n", event);
 }
 
-static int acpi_topstar_init_hkey(struct topstar_hkey *hkey)
+static int topstar_input_init(struct topstar_laptop *topstar)
 {
 	struct input_dev *input;
-	int error;
+	int err;
 
 	input = input_allocate_device();
 	if (!input)
 		return -ENOMEM;
 
 	input->name = "Topstar Laptop extra buttons";
-	input->phys = "topstar/input0";
+	input->phys = TOPSTAR_LAPTOP_CLASS "/input0";
 	input->id.bustype = BUS_HOST;
 
-	error = sparse_keymap_setup(input, topstar_keymap, NULL);
-	if (error) {
+	err = sparse_keymap_setup(input, topstar_keymap, NULL);
+	if (err) {
 		pr_err("Unable to setup input device keymap\n");
 		goto err_free_dev;
 	}
 
-	error = input_register_device(input);
-	if (error) {
+	err = input_register_device(input);
+	if (err) {
 		pr_err("Unable to register input device\n");
 		goto err_free_dev;
 	}
 
-	hkey->inputdev = input;
+	topstar->input = input;
 	return 0;
 
- err_free_dev:
+err_free_dev:
 	input_free_device(input);
-	return error;
+	return err;
+}
+
+static void topstar_input_exit(struct topstar_laptop *topstar)
+{
+	input_unregister_device(topstar->input);
+}
+
+/*
+ * ACPI
+ */
+
+static int topstar_acpi_fncx_switch(struct acpi_device *device, bool state)
+{
+	acpi_status status;
+	u64 arg = state ? 0x86 : 0x87;
+
+	status = acpi_execute_simple_method(device->handle, "FNCX", arg);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Unable to switch FNCX notifications\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int topstar_acpi_init(struct topstar_laptop *topstar)
+{
+	return topstar_acpi_fncx_switch(topstar->device, true);
+}
+
+static void topstar_acpi_exit(struct topstar_laptop *topstar)
+{
+	topstar_acpi_fncx_switch(topstar->device, false);
 }
 
-static int acpi_topstar_add(struct acpi_device *device)
+static void topstar_acpi_notify(struct acpi_device *device, u32 event)
 {
-	struct topstar_hkey *tps_hkey;
+	struct topstar_laptop *topstar = acpi_driver_data(device);
+	static bool dup_evnt[2];
+	bool *dup;
 
-	tps_hkey = kzalloc(sizeof(struct topstar_hkey), GFP_KERNEL);
-	if (!tps_hkey)
+	/* 0x83 and 0x84 key events comes duplicated... */
+	if (event == 0x83 || event == 0x84) {
+		dup = &dup_evnt[event - 0x83];
+		if (*dup) {
+			*dup = false;
+			return;
+		}
+		*dup = true;
+	}
+
+	topstar_input_notify(topstar, event);
+}
+
+static int topstar_acpi_add(struct acpi_device *device)
+{
+	struct topstar_laptop *topstar;
+	int err;
+
+	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_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_free;
 
-	if (acpi_topstar_init_hkey(tps_hkey))
-		goto add_err;
+	err = topstar_input_init(topstar);
+	if (err)
+		goto err_acpi_exit;
 
-	device->driver_data = tps_hkey;
 	return 0;
 
-add_err:
-	kfree(tps_hkey);
-	return -ENODEV;
+err_acpi_exit:
+	topstar_acpi_exit(topstar);
+err_free:
+	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,19 @@ static const struct acpi_device_id topstar_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, topstar_device_ids);
 
-static struct acpi_driver acpi_topstar_driver = {
+static struct acpi_driver topstar_acpi_driver = {
 	.name = "Topstar laptop ACPI driver",
-	.class = ACPI_TOPSTAR_CLASS,
+	.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_AUTHOR("Guillaume Douézan-Grard");
 MODULE_DESCRIPTION("Topstar Laptop ACPI Extras driver");
 MODULE_LICENSE("GPL");
-- 
2.14.3

[PATCH v2 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 106537fdc4e6..714d9d56c79f 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");
 MODULE_AUTHOR("Guillaume Douézan-Grard");
-- 
2.14.3

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

[Guillaume Douézan-Grard]

* add platform device to support further addition of a led subsystem,

* add existing input device to the new platform device.

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

diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index 714d9d56c79f..d4f9ee35c520 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,42 @@ 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_device_put;
+
+	return 0;
+
+err_device_put:
+	platform_device_put(topstar->platform);
+	return err;
+}
+
+static void topstar_platform_exit(struct topstar_laptop *topstar)
+{
+	platform_device_unregister(topstar->platform);
+}
+
 /*
  * ACPI
  */
@@ -172,12 +211,18 @@ static int topstar_acpi_add(struct acpi_device *device)
 	if (err)
 		goto err_free;
 
-	err = topstar_input_init(topstar);
+	err = topstar_platform_init(topstar);
 	if (err)
 		goto err_acpi_exit;
 
+	err = topstar_input_init(topstar);
+	if (err)
+		goto err_platform_exit;
+
 	return 0;
 
+err_platform_exit:
+	topstar_platform_exit(topstar);
 err_acpi_exit:
 	topstar_acpi_exit(topstar);
 err_free:
@@ -190,6 +235,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 +264,25 @@ static int __init topstar_laptop_init(void)
 {
 	int res;
 
-	res = acpi_bus_register_driver(&topstar_acpi_driver);
+	res = platform_driver_register(&topstar_platform_driver);
 	if (res < 0)
 		return res;
 
+	res = acpi_bus_register_driver(&topstar_acpi_driver);
+	if (res < 0)
+		goto err_driver_unreg;
+
 	return 0;
+
+err_driver_unreg:
+	platform_driver_unregister(&topstar_platform_driver);
+	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.3

[PATCH v2 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 0fdf68865c5e..be42cde94cdf 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -697,6 +697,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 d4f9ee35c520..40fffaa43326 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
  */
@@ -219,8 +291,16 @@ static int topstar_acpi_add(struct acpi_device *device)
 	if (err)
 		goto err_platform_exit;
 
+	if (led_workaround) {
+		err = topstar_led_init(topstar);
+		if (err)
+			goto err_input_exit;
+	}
+
 	return 0;
 
+err_input_exit:
+	topstar_input_exit(topstar);
 err_platform_exit:
 	topstar_platform_exit(topstar);
 err_acpi_exit:
@@ -234,6 +314,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.3

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

[Andy Shevchenko]

On Sun, Oct 29, 2017 at 1:49 AM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> Hi Andy,
>
> Thank you for your review. Changes since the previous submission are
> highlighted below.

Thanks for an update.

Unfortunately patch series needs more work (and we have time, since
it's -rc7 already, to improve).

I'm going to comment per patch slightly later.

>
> 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
>
>
> Changes since v1:
>
> * leave devices names unchanged for ABI compatibility,
> * fix label names,
> * separate module authors definition,
> * add commit description to Patch 3.
>
>
> 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 | 334 ++++++++++++++++++++++++++--------
>  2 files changed, 265 insertions(+), 71 deletions(-)
>
> --
> 2.14.3
>



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 29, 2017 at 1:50 AM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> 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.

So, since I would anticipated a new version of the patch series, I
would suggest to split this one to few patches:

1. Only renaming
2. Only moving functions around (separation part?)
3. Only indentation / style fixes + your author ship (should be last
in the entire series)


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 29, 2017 at 1:51 AM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:

I can't take it without commit message.

Basically it reverts the commit 15165594da65 ("topstar-laptop: convert
to module_acpi_driver()")

So, can you just try git revert and explain in a commit message "why
do we need it"?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 29, 2017 at 1:52 AM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> * add platform device to support further addition of a led subsystem,
>
> * add existing input device to the new platform device.

>  #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>

Please, keep it ordered as much as possible, i.e. add the line after
sparse-keymap.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 29, 2017 at 1:53 AM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> 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.

>  #include <linux/platform_device.h>
>  #include <linux/input.h>
>  #include <linux/input/sparse-keymap.h>
> +#include <linux/leds.h>

Yep, exact place, esp. after moving platform_device to the right place.

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

So, this is most problematic piece in the series.

We are not encouraging module parameters. Why do we need one? Can't be
detected automatically (perhaps based on DMI strings)?

-- 
With Best Regards,
Andy Shevchenko

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

[Guillaume Douézan-Grard]

On Fri, Nov 03, 2017 at 02:50:52PM +0200, Andy Shevchenko wrote:
> On Sun, Oct 29, 2017 at 1:53 AM, Guillaume Douézan-Grard
> <gdouezangrard@gmail.com> wrote:
> > 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.
> 
> >  #include <linux/platform_device.h>
> >  #include <linux/input.h>
> >  #include <linux/input/sparse-keymap.h>
> > +#include <linux/leds.h>
> 
> Yep, exact place, esp. after moving platform_device to the right place.
> 
> > +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");
> 
> So, this is most problematic piece in the series.
> 
> We are not encouraging module parameters. Why do we need one? Can't be
> detected automatically (perhaps based on DMI strings)?

Darren told me that. I tried to answer this question in the cover letter:

"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."

In my case for instance, the DMI info has not been filled in by the retailler
since I only have the ODM base board information to identify a model.

> -- 
> With Best Regards,
> Andy Shevchenko

For now, I will prepare a new version containing the other needed changes you
pointed at for the other patches.

Thanks for your time,

--
Guillaume Douézan-Grard

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

[Guillaume Douézan-Grard]

On Fri, Nov 03, 2017 at 02:50:52PM +0200, Andy Shevchenko wrote:
> On Sun, Oct 29, 2017 at 1:53 AM, Guillaume Douézan-Grard
> <gdouezangrard@gmail.com> wrote:
> > 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.
> 
> >  #include <linux/platform_device.h>
> >  #include <linux/input.h>
> >  #include <linux/input/sparse-keymap.h>
> > +#include <linux/leds.h>
> 
> Yep, exact place, esp. after moving platform_device to the right place.
> 
> > +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");
> 
> So, this is most problematic piece in the series.
> 
> We are not encouraging module parameters. Why do we need one? Can't be
> detected automatically (perhaps based on DMI strings)?

Darren told me that. I tried to answer this question in the cover letter:

"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."

In my case for instance, the DMI info has not been filled in by the retailler
since I only have the ODM base board information to identify a model.

> -- 
> With Best Regards,
> Andy Shevchenko

For now, I will prepare a new version containing the other needed changes you
pointed at for the other patches.

Thanks for your time,

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

[Andy Shevchenko]

On Fri, Nov 3, 2017 at 5:07 PM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> On Fri, Nov 03, 2017 at 02:50:52PM +0200, Andy Shevchenko wrote:
>> On Sun, Oct 29, 2017 at 1:53 AM, Guillaume Douézan-Grard
>> <gdouezangrard@gmail.com> wrote:
>> > 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.
>>
>> >  #include <linux/platform_device.h>
>> >  #include <linux/input.h>
>> >  #include <linux/input/sparse-keymap.h>
>> > +#include <linux/leds.h>
>>
>> Yep, exact place, esp. after moving platform_device to the right place.
>>
>> > +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");
>>
>> So, this is most problematic piece in the series.
>>
>> We are not encouraging module parameters. Why do we need one? Can't be
>> detected automatically (perhaps based on DMI strings)?
>
> Darren told me that.

> I tried to answer this question in the cover letter:

Perhaps it makes sense to put this explanation in the commit message.

> "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."
>
> In my case for instance, the DMI info has not been filled in by the retailler
> since I only have the ODM base board information to identify a model.

I see. I would like to have a consensus on this one with Darren, the
rest (after addressing comments) looks good to me.

> For now, I will prepare a new version containing the other needed changes you
> pointed at for the other patches.

OK.

-- 
With Best Regards,
Andy Shevchenko

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

[Darren Hart]

On Sun, Nov 05, 2017 at 03:28:09PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 3, 2017 at 5:07 PM, Guillaume Douézan-Grard
> <gdouezangrard@gmail.com> wrote:
> > On Fri, Nov 03, 2017 at 02:50:52PM +0200, Andy Shevchenko wrote:
> >> On Sun, Oct 29, 2017 at 1:53 AM, Guillaume Douézan-Grard
> >> <gdouezangrard@gmail.com> wrote:
> >> > 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.
> >>
> >> >  #include <linux/platform_device.h>
> >> >  #include <linux/input.h>
> >> >  #include <linux/input/sparse-keymap.h>
> >> > +#include <linux/leds.h>
> >>
> >> Yep, exact place, esp. after moving platform_device to the right place.
> >>
> >> > +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");
> >>
> >> So, this is most problematic piece in the series.
> >>
> >> We are not encouraging module parameters. Why do we need one? Can't be
> >> detected automatically (perhaps based on DMI strings)?
> >
> > Darren told me that.
> 
> > I tried to answer this question in the cover letter:
> 
> Perhaps it makes sense to put this explanation in the commit message.
> 
> > "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."
> >
> > In my case for instance, the DMI info has not been filled in by the retailler
> > since I only have the ODM base board information to identify a model.
> 
> I see. I would like to have a consensus on this one with Darren, the
> rest (after addressing comments) looks good to me.

If you can definitively say that all models of brand X and this HID have this
quirk, that is considerably better than a lot of quirks we deal with today. I
suggest doing that and holding off on the option. If it gets to the point where
a number otherwise unidentifiable systems have this problem, we can add the
option then.

-- 
Darren Hart
VMware Open Source Technology Center

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

[Guillaume Douézan-Grard]

On Sun, Nov 05, 2017 at 02:34:43PM -0800, Darren Hart wrote:
> On Sun, Nov 05, 2017 at 03:28:09PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 3, 2017 at 5:07 PM, Guillaume Douézan-Grard
> > <gdouezangrard@gmail.com> wrote:
> > > On Fri, Nov 03, 2017 at 02:50:52PM +0200, Andy Shevchenko wrote:
> > >> On Sun, Oct 29, 2017 at 1:53 AM, Guillaume Douézan-Grard
> > >> <gdouezangrard@gmail.com> wrote:
> > >> > 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.
> > >>
> > >> >  #include <linux/platform_device.h>
> > >> >  #include <linux/input.h>
> > >> >  #include <linux/input/sparse-keymap.h>
> > >> > +#include <linux/leds.h>
> > >>
> > >> Yep, exact place, esp. after moving platform_device to the right place.
> > >>
> > >> > +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");
> > >>
> > >> So, this is most problematic piece in the series.
> > >>
> > >> We are not encouraging module parameters. Why do we need one? Can't be
> > >> detected automatically (perhaps based on DMI strings)?
> > >
> > > Darren told me that.
> > 
> > > I tried to answer this question in the cover letter:
> > 
> > Perhaps it makes sense to put this explanation in the commit message.
> > 
> > > "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."
> > >
> > > In my case for instance, the DMI info has not been filled in by the retailler
> > > since I only have the ODM base board information to identify a model.
> > 
> > I see. I would like to have a consensus on this one with Darren, the
> > rest (after addressing comments) looks good to me.
> 
> If you can definitively say that all models of brand X and this HID have this
> quirk, that is considerably better than a lot of quirks we deal with today. I
> suggest doing that and holding off on the option. If it gets to the point where
> a number otherwise unidentifiable systems have this problem, we can add the
> option then.

Thanks for your reviews.

I replaced the module parameter with a board name/version DMI check,
that will be included for the new patch serie.

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

diff --git a/drivers/platform/x86/topstar-laptop.c b/drivers/platform/x86/topstar-laptop.c
index 0ed1ce404ea1..72433cfdf231 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/dmi.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/leds.h>
@@ -26,15 +27,12 @@
 
 #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;
+
+	bool led_registered;
 	struct led_classdev led;
 };
 
@@ -291,10 +289,16 @@ static int topstar_acpi_add(struct acpi_device *device)
 	if (err)
 		goto err_platform_exit;
 
-	if (led_workaround) {
+	/*
+	 * Enable software-based WLAN LED control on systems with defective
+	 * hardware switch.
+	 */
+	if (dmi_match(DMI_BOARD_NAME, "U931")
+			&& dmi_match(DMI_BOARD_VERSION, "RVP7")) {
 		err = topstar_led_init(topstar);
 		if (err)
 			goto err_input_exit;
+		topstar->led_registered = true;
 	}
 
 	return 0;
@@ -314,7 +318,7 @@ static int topstar_acpi_remove(struct acpi_device *device)
 {
 	struct topstar_laptop *topstar = acpi_driver_data(device);
 
-	if (led_workaround)
+	if (topstar->led_registered)
 		topstar_led_exit(topstar);
 
 	topstar_input_exit(topstar);
-- 
2.15.0

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

[Andy Shevchenko]

On Wed, Nov 8, 2017 at 2:25 AM, Guillaume Douézan-Grard
<gdouezangrard@gmail.com> wrote:
> On Sun, Nov 05, 2017 at 02:34:43PM -0800, Darren Hart wrote:
>> On Sun, Nov 05, 2017 at 03:28:09PM +0200, Andy Shevchenko wrote:

> I replaced the module parameter with a board name/version DMI check,
> that will be included for the new patch serie.

Below is (as PoC) fine for me.
Though one comment.

> @@ -291,10 +289,16 @@ static int topstar_acpi_add(struct acpi_device *device)
>         if (err)
>                 goto err_platform_exit;
>
> -       if (led_workaround) {
> +       /*
> +        * Enable software-based WLAN LED control on systems with defective
> +        * hardware switch.
> +        */

> +       if (dmi_match(DMI_BOARD_NAME, "U931")
> +                       && dmi_match(DMI_BOARD_VERSION, "RVP7")) {

I would better to see dmi_system_id table with callback that sets this
boolean variable inside topstar structure.

Something like below (adapt it to better suit your needs):

static const struct dmi_system_id topstar_quirks[] = {
       {
               .callback = dmi_led_workaround,
               .ident = "Topstar ... BLA BLA BLA",
               .matches = {
                      DMI_MATCH(DMI_BOARD_NAME, "U931"),
                      DMI_MATCH(DMI_BOARD_VERSION, "RVP7"),
               },
               .driver_data = topstar,
       },
       {}
};

-- 
With Best Regards,
Andy Shevchenko