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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
---
 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