[PATCH 0/6] fujitsu-laptop: LED cleanup

[PATCH 0/6] fujitsu-laptop: LED cleanup

[Michał Kępień]

This patch series cleans up parts of fujitsu-laptop responsible for
handling LED class devices.  It was tested on a Lifebook E744.  It
depends on the previous patch series I submitted for fujitsu-laptop and
should be applied on top of the backlight cleanup series.

 drivers/platform/x86/Kconfig          |   2 +-
 drivers/platform/x86/fujitsu-laptop.c | 402 +++++++++++++++-------------------
 2 files changed, 176 insertions(+), 228 deletions(-)

-- 
2.12.2

[PATCH 1/6] platform/x86: fujitsu-laptop: select LEDS_CLASS

[Michał Kępień]

Follow common subsystem practice of selecting LEDS_CLASS instead of
riddling module code with #ifdefs.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/Kconfig          |  2 +-
 drivers/platform/x86/fujitsu-laptop.c | 14 +-------------
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 5602bdfcad65..d18acb1105a5 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -182,8 +182,8 @@ config FUJITSU_LAPTOP
 	depends on INPUT
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
-	depends on LEDS_CLASS || LEDS_CLASS=n
 	select INPUT_SPARSEKMAP
+	select LEDS_CLASS
 	---help---
 	  This is a driver for laptops built by Fujitsu:
 
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 9fd5b98aeef8..555de246b994 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -59,11 +59,9 @@
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/kfifo.h>
+#include <linux/leds.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
-#include <linux/leds.h>
-#endif
 #include <acpi/video.h>
 
 #define FUJITSU_DRIVER_VERSION "0.6.0"
@@ -94,7 +92,6 @@
 #define FLAG_LID	0x100
 #define FLAG_DOCK	0x200
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
 /* FUNC interface - LED control */
 #define FUNC_LED_OFF	0x1
 #define FUNC_LED_ON	0x30001
@@ -104,7 +101,6 @@
 #define RADIO_LED_ON	0x20
 #define ECO_LED	0x10000
 #define ECO_LED_ON	0x80000
-#endif
 
 /* Hotkey details */
 #define KEY1_CODE	0x410	/* codes for the keys in the GIRB register */
@@ -165,7 +161,6 @@ struct fujitsu_laptop {
 
 static struct fujitsu_laptop *fujitsu_laptop;
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
 static enum led_brightness logolamp_get(struct led_classdev *cdev);
 static int logolamp_set(struct led_classdev *cdev,
 			       enum led_brightness brightness);
@@ -206,7 +201,6 @@ static struct led_classdev eco_led = {
  .brightness_get = eco_led_get,
  .brightness_set_blocking = eco_led_set
 };
-#endif
 
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
 static u32 dbg_level = 0x03;
@@ -253,7 +247,6 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 	return value;
 }
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
 /* LED class callbacks */
 
 static int logolamp_set(struct led_classdev *cdev,
@@ -349,7 +342,6 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 
 	return brightness;
 }
-#endif
 
 /* Hardware access for LCD brightness control */
 
@@ -845,7 +837,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		goto err_free_fifo;
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
 						&logolamp_led);
@@ -902,7 +893,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 			       result);
 		}
 	}
-#endif
 
 	return result;
 
@@ -916,7 +906,6 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
 	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
 	if (fujitsu_laptop->logolamp_registered)
 		led_classdev_unregister(&logolamp_led);
 
@@ -928,7 +917,6 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 
 	if (fujitsu_laptop->eco_led_registered)
 		led_classdev_unregister(&eco_led);
-#endif
 
 	fujitsu_laptop_platform_remove();
 
-- 
2.12.2

[PATCH 2/6] platform/x86: fujitsu-laptop: refactor LED registration

[Michał Kępień]

Move a long section of code responsible for registering LEDs out of
acpi_fujitsu_laptop_add() to improve readability and ensure proper
cleanup of platform device and kfifo e.g. when two supported LEDs are
detected, the first one gets registered successfully but the second one
does not.  This makes the result variable in acpi_fujitsu_laptop_add()
redundant, so remove it.  Adjust whitespace to make checkpatch happy.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 128 +++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 555de246b994..4d13cc03d3b1 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -754,9 +754,72 @@ static void fujitsu_laptop_platform_remove(void)
 	platform_device_unregister(fujitsu_laptop->pf_device);
 }
 
-static int acpi_fujitsu_laptop_add(struct acpi_device *device)
+static int acpi_fujitsu_laptop_leds_register(void)
 {
 	int result = 0;
+
+	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
+		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
+					       &logolamp_led);
+		if (result == 0) {
+			fujitsu_laptop->logolamp_registered = 1;
+		} else {
+			pr_err("Could not register LED handler for logo lamp, error %i\n",
+			       result);
+		}
+	}
+
+	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
+	    (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
+		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
+					       &kblamps_led);
+		if (result == 0) {
+			fujitsu_laptop->kblamps_registered = 1;
+		} else {
+			pr_err("Could not register LED handler for keyboard lamps, error %i\n",
+			       result);
+		}
+	}
+
+	/*
+	 * BTNI bit 24 seems to indicate the presence of a radio toggle
+	 * button in place of a slide switch, and all such machines appear
+	 * to also have an RF LED.  Therefore use bit 24 as an indicator
+	 * that an RF LED is present.
+	 */
+	if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
+		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
+					       &radio_led);
+		if (result == 0) {
+			fujitsu_laptop->radio_led_registered = 1;
+		} else {
+			pr_err("Could not register LED handler for radio LED, error %i\n",
+			       result);
+		}
+	}
+
+	/* Support for eco led is not always signaled in bit corresponding
+	 * to the bit used to control the led. According to the DSDT table,
+	 * bit 14 seems to indicate presence of said led as well.
+	 * Confirm by testing the status.
+	 */
+	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
+	    (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
+		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
+					       &eco_led);
+		if (result == 0) {
+			fujitsu_laptop->eco_led_registered = 1;
+		} else {
+			pr_err("Could not register LED handler for eco LED, error %i\n",
+			       result);
+		}
+	}
+
+	return result;
+}
+
+static int acpi_fujitsu_laptop_add(struct acpi_device *device)
+{
 	int state = 0;
 	int error;
 	int i;
@@ -837,65 +900,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		goto err_free_fifo;
 
-	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-						&logolamp_led);
-		if (result == 0) {
-			fujitsu_laptop->logolamp_registered = 1;
-		} else {
-			pr_err("Could not register LED handler for logo lamp, error %i\n",
-			       result);
-		}
-	}
-
-	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
-	   (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-						&kblamps_led);
-		if (result == 0) {
-			fujitsu_laptop->kblamps_registered = 1;
-		} else {
-			pr_err("Could not register LED handler for keyboard lamps, error %i\n",
-			       result);
-		}
-	}
-
-	/*
-	 * BTNI bit 24 seems to indicate the presence of a radio toggle
-	 * button in place of a slide switch, and all such machines appear
-	 * to also have an RF LED.  Therefore use bit 24 as an indicator
-	 * that an RF LED is present.
-	 */
-	if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-						&radio_led);
-		if (result == 0) {
-			fujitsu_laptop->radio_led_registered = 1;
-		} else {
-			pr_err("Could not register LED handler for radio LED, error %i\n",
-			       result);
-		}
-	}
-
-	/* Support for eco led is not always signaled in bit corresponding
-	 * to the bit used to control the led. According to the DSDT table,
-	 * bit 14 seems to indicate presence of said led as well.
-	 * Confirm by testing the status.
-	*/
-	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
-	   (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-						&eco_led);
-		if (result == 0) {
-			fujitsu_laptop->eco_led_registered = 1;
-		} else {
-			pr_err("Could not register LED handler for eco LED, error %i\n",
-			       result);
-		}
-	}
+	error = acpi_fujitsu_laptop_leds_register();
+	if (error)
+		goto err_remove_platform_device;
 
-	return result;
+	return 0;
 
+err_remove_platform_device:
+	fujitsu_laptop_platform_remove();
 err_free_fifo:
 	kfifo_free(&fujitsu_laptop->fifo);
 err_stop:
-- 
2.12.2

[PATCH 3/6] platform/x86: fujitsu-laptop: reorganize LED-related code

[Michał Kępień]

Move around LED definitions and callbacks to eliminate the need for
forward declarations and ensure code is organized LED-wise, not
action-wise.  Reorder assignments inside designated initializers so that
they are in the same order as struct led_classdev fields in
include/linux/leds.h.  Adjust whitespace to make checkpatch happy.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 261 ++++++++++++++++------------------
 1 file changed, 124 insertions(+), 137 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 4d13cc03d3b1..da0bd556b0bb 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -161,47 +161,6 @@ struct fujitsu_laptop {
 
 static struct fujitsu_laptop *fujitsu_laptop;
 
-static enum led_brightness logolamp_get(struct led_classdev *cdev);
-static int logolamp_set(struct led_classdev *cdev,
-			       enum led_brightness brightness);
-
-static struct led_classdev logolamp_led = {
- .name = "fujitsu::logolamp",
- .brightness_get = logolamp_get,
- .brightness_set_blocking = logolamp_set
-};
-
-static enum led_brightness kblamps_get(struct led_classdev *cdev);
-static int kblamps_set(struct led_classdev *cdev,
-			       enum led_brightness brightness);
-
-static struct led_classdev kblamps_led = {
- .name = "fujitsu::kblamps",
- .brightness_get = kblamps_get,
- .brightness_set_blocking = kblamps_set
-};
-
-static enum led_brightness radio_led_get(struct led_classdev *cdev);
-static int radio_led_set(struct led_classdev *cdev,
-			       enum led_brightness brightness);
-
-static struct led_classdev radio_led = {
- .name = "fujitsu::radio_led",
- .default_trigger = "rfkill-any",
- .brightness_get = radio_led_get,
- .brightness_set_blocking = radio_led_set
-};
-
-static enum led_brightness eco_led_get(struct led_classdev *cdev);
-static int eco_led_set(struct led_classdev *cdev,
-			       enum led_brightness brightness);
-
-static struct led_classdev eco_led = {
- .name = "fujitsu::eco_led",
- .brightness_get = eco_led_get,
- .brightness_set_blocking = eco_led_set
-};
-
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
 static u32 dbg_level = 0x03;
 #endif
@@ -247,102 +206,6 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 	return value;
 }
 
-/* LED class callbacks */
-
-static int logolamp_set(struct led_classdev *cdev,
-			       enum led_brightness brightness)
-{
-	int poweron = FUNC_LED_ON, always = FUNC_LED_ON;
-	int ret;
-
-	if (brightness < LED_HALF)
-		poweron = FUNC_LED_OFF;
-
-	if (brightness < LED_FULL)
-		always = FUNC_LED_OFF;
-
-	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
-	if (ret < 0)
-		return ret;
-
-	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
-}
-
-static int kblamps_set(struct led_classdev *cdev,
-			       enum led_brightness brightness)
-{
-	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
-	else
-		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
-}
-
-static int radio_led_set(struct led_classdev *cdev,
-				enum led_brightness brightness)
-{
-	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, RADIO_LED_ON);
-	else
-		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, 0x0);
-}
-
-static int eco_led_set(struct led_classdev *cdev,
-				enum led_brightness brightness)
-{
-	int curr;
-
-	curr = call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0);
-	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED, curr | ECO_LED_ON);
-	else
-		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED, curr & ~ECO_LED_ON);
-}
-
-static enum led_brightness logolamp_get(struct led_classdev *cdev)
-{
-	int ret;
-
-	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0);
-	if (ret == FUNC_LED_ON)
-		return LED_FULL;
-
-	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
-	if (ret == FUNC_LED_ON)
-		return LED_HALF;
-
-	return LED_OFF;
-}
-
-static enum led_brightness kblamps_get(struct led_classdev *cdev)
-{
-	enum led_brightness brightness = LED_OFF;
-
-	if (call_fext_func(FUNC_LEDS, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
-		brightness = LED_FULL;
-
-	return brightness;
-}
-
-static enum led_brightness radio_led_get(struct led_classdev *cdev)
-{
-	enum led_brightness brightness = LED_OFF;
-
-	if (call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
-		brightness = LED_FULL;
-
-	return brightness;
-}
-
-static enum led_brightness eco_led_get(struct led_classdev *cdev)
-{
-	enum led_brightness brightness = LED_OFF;
-
-	if (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
-		brightness = LED_FULL;
-
-	return brightness;
-}
-
 /* Hardware access for LCD brightness control */
 
 static int set_lcd_level(int level)
@@ -754,6 +617,130 @@ static void fujitsu_laptop_platform_remove(void)
 	platform_device_unregister(fujitsu_laptop->pf_device);
 }
 
+static int logolamp_set(struct led_classdev *cdev,
+			enum led_brightness brightness)
+{
+	int poweron = FUNC_LED_ON, always = FUNC_LED_ON;
+	int ret;
+
+	if (brightness < LED_HALF)
+		poweron = FUNC_LED_OFF;
+
+	if (brightness < LED_FULL)
+		always = FUNC_LED_OFF;
+
+	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
+	if (ret < 0)
+		return ret;
+
+	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
+}
+
+static enum led_brightness logolamp_get(struct led_classdev *cdev)
+{
+	int ret;
+
+	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0);
+	if (ret == FUNC_LED_ON)
+		return LED_FULL;
+
+	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
+	if (ret == FUNC_LED_ON)
+		return LED_HALF;
+
+	return LED_OFF;
+}
+
+static struct led_classdev logolamp_led = {
+	.name = "fujitsu::logolamp",
+	.brightness_set_blocking = logolamp_set,
+	.brightness_get = logolamp_get
+};
+
+static int kblamps_set(struct led_classdev *cdev,
+		       enum led_brightness brightness)
+{
+	if (brightness >= LED_FULL)
+		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
+				      FUNC_LED_ON);
+	else
+		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
+				      FUNC_LED_OFF);
+}
+
+static enum led_brightness kblamps_get(struct led_classdev *cdev)
+{
+	enum led_brightness brightness = LED_OFF;
+
+	if (call_fext_func(FUNC_LEDS, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+		brightness = LED_FULL;
+
+	return brightness;
+}
+
+static struct led_classdev kblamps_led = {
+	.name = "fujitsu::kblamps",
+	.brightness_set_blocking = kblamps_set,
+	.brightness_get = kblamps_get
+};
+
+static int radio_led_set(struct led_classdev *cdev,
+			 enum led_brightness brightness)
+{
+	if (brightness >= LED_FULL)
+		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON,
+				      RADIO_LED_ON);
+	else
+		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, 0x0);
+}
+
+static enum led_brightness radio_led_get(struct led_classdev *cdev)
+{
+	enum led_brightness brightness = LED_OFF;
+
+	if (call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
+		brightness = LED_FULL;
+
+	return brightness;
+}
+
+static struct led_classdev radio_led = {
+	.name = "fujitsu::radio_led",
+	.brightness_set_blocking = radio_led_set,
+	.brightness_get = radio_led_get,
+	.default_trigger = "rfkill-any"
+};
+
+static int eco_led_set(struct led_classdev *cdev,
+		       enum led_brightness brightness)
+{
+	int curr;
+
+	curr = call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0);
+	if (brightness >= LED_FULL)
+		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED,
+				      curr | ECO_LED_ON);
+	else
+		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED,
+				      curr & ~ECO_LED_ON);
+}
+
+static enum led_brightness eco_led_get(struct led_classdev *cdev)
+{
+	enum led_brightness brightness = LED_OFF;
+
+	if (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
+		brightness = LED_FULL;
+
+	return brightness;
+}
+
+static struct led_classdev eco_led = {
+	.name = "fujitsu::eco_led",
+	.brightness_set_blocking = eco_led_set,
+	.brightness_get = eco_led_get
+};
+
 static int acpi_fujitsu_laptop_leds_register(void)
 {
 	int result = 0;
-- 
2.12.2

[PATCH 4/6] platform/x86: fujitsu-laptop: switch to managed LED class devices

[Michał Kępień]

Use devm_led_classdev_register() for registering LED class devices in
order to simplify cleanup and remove LED-related fields with the
"_registered" suffix from struct fujitsu_laptop.  This also fixes a
cleanup bug: with non-managed LED class devices, if e.g. two supported
LEDs are detected, the first one gets registered successfully but the
second one does not, acpi_fujitsu_laptop_add() will return an error, but
the successfully registered LED will never get unregistered.

Change the parent device for LED class devices to the FUJ02E3 ACPI
device due to this being the logically correct relationship as LED class
devices do not depend on any facility provided by the platform device
registered by fujitsu-laptop, which was their parent until now.

Each managed LED class device is automatically unregistered when the
last reference to its parent device is dropped.  Taking the parent
change described above into account, LED class devices registered by
fujitsu-laptop will be unregistered after acpi_fujitsu_laptop_remove()
is called.  During unregistration, LED brightness is reset to LED_OFF by
LED core, so do not set the acpi_handle field of struct fujitsu_laptop
to NULL inside acpi_fujitsu_laptop_remove() to prevent call_fext_func()
from generating errors upon module removal.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 55 +++++++----------------------------
 1 file changed, 11 insertions(+), 44 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index da0bd556b0bb..ce658789e748 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -153,10 +153,6 @@ struct fujitsu_laptop {
 	spinlock_t fifo_lock;
 	int flags_supported;
 	int flags_state;
-	int logolamp_registered;
-	int kblamps_registered;
-	int radio_led_registered;
-	int eco_led_registered;
 };
 
 static struct fujitsu_laptop *fujitsu_laptop;
@@ -741,31 +737,24 @@ static struct led_classdev eco_led = {
 	.brightness_get = eco_led_get
 };
 
-static int acpi_fujitsu_laptop_leds_register(void)
+static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
 	int result = 0;
 
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-					       &logolamp_led);
-		if (result == 0) {
-			fujitsu_laptop->logolamp_registered = 1;
-		} else {
+		result = devm_led_classdev_register(&device->dev,
+						    &logolamp_led);
+		if (result)
 			pr_err("Could not register LED handler for logo lamp, error %i\n",
 			       result);
-		}
 	}
 
 	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
 	    (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-					       &kblamps_led);
-		if (result == 0) {
-			fujitsu_laptop->kblamps_registered = 1;
-		} else {
+		result = devm_led_classdev_register(&device->dev, &kblamps_led);
+		if (result)
 			pr_err("Could not register LED handler for keyboard lamps, error %i\n",
 			       result);
-		}
 	}
 
 	/*
@@ -775,14 +764,10 @@ static int acpi_fujitsu_laptop_leds_register(void)
 	 * that an RF LED is present.
 	 */
 	if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-					       &radio_led);
-		if (result == 0) {
-			fujitsu_laptop->radio_led_registered = 1;
-		} else {
+		result = devm_led_classdev_register(&device->dev, &radio_led);
+		if (result)
 			pr_err("Could not register LED handler for radio LED, error %i\n",
 			       result);
-		}
 	}
 
 	/* Support for eco led is not always signaled in bit corresponding
@@ -792,14 +777,10 @@ static int acpi_fujitsu_laptop_leds_register(void)
 	 */
 	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
 	    (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
-		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
-					       &eco_led);
-		if (result == 0) {
-			fujitsu_laptop->eco_led_registered = 1;
-		} else {
+		result = devm_led_classdev_register(&device->dev, &eco_led);
+		if (result)
 			pr_err("Could not register LED handler for eco LED, error %i\n",
 			       result);
-		}
 	}
 
 	return result;
@@ -887,7 +868,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		goto err_free_fifo;
 
-	error = acpi_fujitsu_laptop_leds_register();
+	error = acpi_fujitsu_laptop_leds_register(device);
 	if (error)
 		goto err_remove_platform_device;
 
@@ -905,24 +886,10 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
 	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
 
-	if (fujitsu_laptop->logolamp_registered)
-		led_classdev_unregister(&logolamp_led);
-
-	if (fujitsu_laptop->kblamps_registered)
-		led_classdev_unregister(&kblamps_led);
-
-	if (fujitsu_laptop->radio_led_registered)
-		led_classdev_unregister(&radio_led);
-
-	if (fujitsu_laptop->eco_led_registered)
-		led_classdev_unregister(&eco_led);
-
 	fujitsu_laptop_platform_remove();
 
 	kfifo_free(&fujitsu_laptop->fifo);
 
-	fujitsu_laptop->acpi_handle = NULL;
-
 	return 0;
 }
 
-- 
2.12.2

[PATCH 5/6] platform/x86: fujitsu-laptop: do not log LED registration failures

[Michał Kępień]

If acpi_fujitsu_laptop_leds_register() returns an error, the latter will
become the return value of acpi_fujitsu_laptop_add(), which in turn will
be reported by driver core.  Simplify code by replacing pr_err() calls
with return statements.  Return 0 instead of result when no errors occur
in order to make the code easier to read.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index ce658789e748..177b9b57ac2f 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -739,22 +739,20 @@ static struct led_classdev eco_led = {
 
 static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
-	int result = 0;
+	int result;
 
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = devm_led_classdev_register(&device->dev,
 						    &logolamp_led);
 		if (result)
-			pr_err("Could not register LED handler for logo lamp, error %i\n",
-			       result);
+			return result;
 	}
 
 	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
 	    (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
 		result = devm_led_classdev_register(&device->dev, &kblamps_led);
 		if (result)
-			pr_err("Could not register LED handler for keyboard lamps, error %i\n",
-			       result);
+			return result;
 	}
 
 	/*
@@ -766,8 +764,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
 		result = devm_led_classdev_register(&device->dev, &radio_led);
 		if (result)
-			pr_err("Could not register LED handler for radio LED, error %i\n",
-			       result);
+			return result;
 	}
 
 	/* Support for eco led is not always signaled in bit corresponding
@@ -779,11 +776,10 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	    (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
 		result = devm_led_classdev_register(&device->dev, &eco_led);
 		if (result)
-			pr_err("Could not register LED handler for eco LED, error %i\n",
-			       result);
+			return result;
 	}
 
-	return result;
+	return 0;
 }
 
 static int acpi_fujitsu_laptop_add(struct acpi_device *device)
-- 
2.12.2

Re: [PATCH 5/6] platform/x86: fujitsu-laptop: do not log LED registration failures

[Darren Hart]

On Fri, Apr 07, 2017 at 03:07:12PM +0200, Michał Kępień wrote:
> If acpi_fujitsu_laptop_leds_register() returns an error, the latter will
> become the return value of acpi_fujitsu_laptop_add(), which in turn will
> be reported by driver core.  Simplify code by replacing pr_err() calls
> with return statements.  Return 0 instead of result when no errors occur
> in order to make the code easier to read.

Hi Michał,

Jonathan's comment regarding the information loss of removing the pr_err
statements seems valid to me. Based on the outer if block, I take it each
registration only fails in true error scenarios and not because some laptop
might have one but not another LED in the list. If so, then the pr_err messages
would only appear when there was a legitimate problem. I think they're worth

This seems to introduce a behavior change as well. Previously only the last
LED registered would determine the result - which is wrong of course and I
believe you noted a related bug in an early patch. Previously, however, if
LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted.

So the question really comes down to this: Is there a legitimate situation in
which one LEDs registration fails and another succeeds? If so, then this would
constitute a regression for such systems.

> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index ce658789e748..177b9b57ac2f 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -739,22 +739,20 @@ static struct led_classdev eco_led = {
>  
>  static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>  {
> -	int result = 0;
> +	int result;
>  
>  	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
>  		result = devm_led_classdev_register(&device->dev,
>  						    &logolamp_led);
>  		if (result)
> -			pr_err("Could not register LED handler for logo lamp, error %i\n",
> -			       result);
> +			return result;
>  	}
>  
>  	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
>  	    (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
>  		result = devm_led_classdev_register(&device->dev, &kblamps_led);
>  		if (result)
> -			pr_err("Could not register LED handler for keyboard lamps, error %i\n",
> -			       result);
> +			return result;
>  	}
>  
>  	/*
> @@ -766,8 +764,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>  	if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
>  		result = devm_led_classdev_register(&device->dev, &radio_led);
>  		if (result)
> -			pr_err("Could not register LED handler for radio LED, error %i\n",
> -			       result);
> +			return result;
>  	}
>  
>  	/* Support for eco led is not always signaled in bit corresponding
> @@ -779,11 +776,10 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>  	    (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
>  		result = devm_led_classdev_register(&device->dev, &eco_led);
>  		if (result)
> -			pr_err("Could not register LED handler for eco LED, error %i\n",
> -			       result);
> +			return result;
>  	}
>  
> -	return result;
> +	return 0;
>  }
>  
>  static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> -- 
> 2.12.2
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 5/6] platform/x86: fujitsu-laptop: do not log LED registration failures

[Michał Kępień]

Jonathan, I hope this response to Darren's message also addresses your
concerns.  Feel free to let me know if it does not.

> On Fri, Apr 07, 2017 at 03:07:12PM +0200, Michał Kępień wrote:
> > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will
> > become the return value of acpi_fujitsu_laptop_add(), which in turn will
> > be reported by driver core.  Simplify code by replacing pr_err() calls
> > with return statements.  Return 0 instead of result when no errors occur
> > in order to make the code easier to read.
> 
> Hi Michał,
> 
> Jonathan's comment regarding the information loss of removing the pr_err
> statements seems valid to me. Based on the outer if block, I take it each
> registration only fails in true error scenarios and not because some laptop
> might have one but not another LED in the list.

Correct.

> If so, then the pr_err messages
> would only appear when there was a legitimate problem. I think they're worth

I am not hell-bent on removing these pr_err() calls, but allow me to
briefly walk you through my thought process.

devm_led_classdev_register() is basically a managed wrapper for
led_classdev_register(), so let's see under what circumstances the
latter may fail.  While it does quite a bit, its return value can only
be different than zero for one of two reasons:

  - there is already a LED with the same name present in the system, so
    the kernel automatically renames the one we are registering and the
    length of the generated name exceeds LED_MAX_NAME_SIZE,

  - device_create_with_groups() fails, either because we are out of
    memory or the device hierarchy is screwed up.

The first case will never happen, given that the longest LED name that
fujitsu-laptop tries to register is 18 bytes long, the counter used for
auto-incrementation is an unsigned int and LED_MAX_NAME_SIZE is 64.

In the second case, we are likely to be notified by driver core about
the exact nature of the failure, but more importantly, logging which LED
"caused" the failure makes us none the wiser.  Actions taken by the
kernel in response to each of the devm_led_classdev_register() calls are
virtually identical and if any of these fails, we are more than likely
to have problems way more severe than non-functioning LEDs.

Have I missed anything or perhaps assumed something I should have not?

> This seems to introduce a behavior change as well. Previously only the last
> LED registered would determine the result - which is wrong of course and I
> believe you noted a related bug in an early patch. Previously, however, if
> LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted.
> 
> So the question really comes down to this: Is there a legitimate situation in
> which one LEDs registration fails and another succeeds? If so, then this would
> constitute a regression for such systems.

The behavior change you mentioned is intentional.  As pointed out above,
if any devm_led_classdev_register() call fails, it means we have reached
some inconsistent state which is really unlikely to be improved by
further attempts to register even more devices.

What do you guys think?

-- 
Best regards,
Michał Kępień

Re: [PATCH 5/6] platform/x86: fujitsu-laptop: do not log LED registration failures

[Darren Hart]

On Tue, Apr 18, 2017 at 10:10:01AM +0200, Michał Kępień wrote:
> Jonathan, I hope this response to Darren's message also addresses your
> concerns.  Feel free to let me know if it does not.
> 
> > On Fri, Apr 07, 2017 at 03:07:12PM +0200, Michał Kępień wrote:
> > > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will
> > > become the return value of acpi_fujitsu_laptop_add(), which in turn will
> > > be reported by driver core.  Simplify code by replacing pr_err() calls
> > > with return statements.  Return 0 instead of result when no errors occur
> > > in order to make the code easier to read.
> > 
> > Hi Michał,
> > 
> > Jonathan's comment regarding the information loss of removing the pr_err
> > statements seems valid to me. Based on the outer if block, I take it each
> > registration only fails in true error scenarios and not because some laptop
> > might have one but not another LED in the list.
> 
> Correct.
> 
> > If so, then the pr_err messages
> > would only appear when there was a legitimate problem. I think they're worth
> 
> I am not hell-bent on removing these pr_err() calls, but allow me to
> briefly walk you through my thought process.
> 
> devm_led_classdev_register() is basically a managed wrapper for
> led_classdev_register(), so let's see under what circumstances the
> latter may fail.  While it does quite a bit, its return value can only
> be different than zero for one of two reasons:
> 
>   - there is already a LED with the same name present in the system, so
>     the kernel automatically renames the one we are registering and the
>     length of the generated name exceeds LED_MAX_NAME_SIZE,
> 
>   - device_create_with_groups() fails, either because we are out of
>     memory or the device hierarchy is screwed up.
> 
> The first case will never happen, given that the longest LED name that
> fujitsu-laptop tries to register is 18 bytes long, the counter used for
> auto-incrementation is an unsigned int and LED_MAX_NAME_SIZE is 64.
> 
> In the second case, we are likely to be notified by driver core about
> the exact nature of the failure, but more importantly, logging which LED
> "caused" the failure makes us none the wiser.  Actions taken by the
> kernel in response to each of the devm_led_classdev_register() calls are
> virtually identical and if any of these fails, we are more than likely
> to have problems way more severe than non-functioning LEDs.
> 
> Have I missed anything or perhaps assumed something I should have not?
> 
> > This seems to introduce a behavior change as well. Previously only the last
> > LED registered would determine the result - which is wrong of course and I
> > believe you noted a related bug in an early patch. Previously, however, if
> > LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted.
> > 
> > So the question really comes down to this: Is there a legitimate situation in
> > which one LEDs registration fails and another succeeds? If so, then this would
> > constitute a regression for such systems.
> 
> The behavior change you mentioned is intentional.  As pointed out above,
> if any devm_led_classdev_register() call fails, it means we have reached
> some inconsistent state which is really unlikely to be improved by
> further attempts to register even more devices.
> 
> What do you guys think?

Excellent rationale, I withdraw the concern.

Jonathan?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 5/6] platform/x86: fujitsu-laptop: do not log LED registration failures

[Jonathan Woithe]

On Tue, Apr 18, 2017 at 09:01:12AM -0700, Darren Hart wrote:
> On Tue, Apr 18, 2017 at 10:10:01AM +0200, Micha?? K??pie?? wrote:
> > Jonathan, I hope this response to Darren's message also addresses your
> > concerns.  Feel free to let me know if it does not.
> > 
> > > On Fri, Apr 07, 2017 at 03:07:12PM +0200, Micha?? K??pie?? wrote:
> > > > If acpi_fujitsu_laptop_leds_register() returns an error, the latter will
> > > > become the return value of acpi_fujitsu_laptop_add(), which in turn will
> > > > be reported by driver core.  Simplify code by replacing pr_err() calls
> > > > with return statements.  Return 0 instead of result when no errors occur
> > > > in order to make the code easier to read.
> > > 
> > > Hi Micha??,
> > > 
> > > Jonathan's comment regarding the information loss of removing the pr_err
> > > statements seems valid to me. Based on the outer if block, I take it each
> > > registration only fails in true error scenarios and not because some laptop
> > > might have one but not another LED in the list.
> > 
> > Correct.
> > 
> > > If so, then the pr_err messages
> > > would only appear when there was a legitimate problem. I think they're worth
> > 
> > I am not hell-bent on removing these pr_err() calls, but allow me to
> > briefly walk you through my thought process.
> > :

Thanks Michael for your detailed explanation of your rationale and the
background to these changes.

> > > This seems to introduce a behavior change as well. Previously only the last
> > > LED registered would determine the result - which is wrong of course and I
> > > believe you noted a related bug in an early patch. Previously, however, if
> > > LOGOLAMP_POWERON failed, for example, the KEYBOARD_LAMPS would still be attempted.
> > > 
> > > So the question really comes down to this: Is there a legitimate situation in
> > > which one LEDs registration fails and another succeeds? If so, then this would
> > > constitute a regression for such systems.
> > 
> > The behavior change you mentioned is intentional.  As pointed out above,
> > if any devm_led_classdev_register() call fails, it means we have reached
> > some inconsistent state which is really unlikely to be improved by
> > further attempts to register even more devices.
> > 
> > What do you guys think?
> 
> Excellent rationale, I withdraw the concern.
> Jonathan?

I am happy to proceed based on Michael's subsequent explanation.

The changes in this patch series are reasonably extensive but should not
result in any observable changes in behaviour.  They represent a significant
modernisation of the code, taking advantage of the current approach to
module architecture.  Unfortunately I do not have hardware which includes
the LEDs which these changes affect, so I cannot confirm the absence of
regressions.  I note however that it has been tested on a Lifebook E744 and
I am therefore happy to see the patch series merged on this basis.

Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Regards
  jonathan

[PATCH 6/6] platform/x86: fujitsu-laptop: simplify error handling in acpi_fujitsu_laptop_add()

[Michał Kępień]

As LED class devices registered by fujitsu-laptop no longer depend on
the platform device, two function calls inside acpi_fujitsu_laptop_add()
can be rearranged in order to simplify error handling.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 177b9b57ac2f..2d3d0e4083dc 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -860,18 +860,16 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
 	}
 
-	error = fujitsu_laptop_platform_add();
+	error = acpi_fujitsu_laptop_leds_register(device);
 	if (error)
 		goto err_free_fifo;
 
-	error = acpi_fujitsu_laptop_leds_register(device);
+	error = fujitsu_laptop_platform_add();
 	if (error)
-		goto err_remove_platform_device;
+		goto err_free_fifo;
 
 	return 0;
 
-err_remove_platform_device:
-	fujitsu_laptop_platform_remove();
 err_free_fifo:
 	kfifo_free(&fujitsu_laptop->fifo);
 err_stop:
-- 
2.12.2

Re: [PATCH 0/6] fujitsu-laptop: LED cleanup

[Jonathan Woithe]

On Fri, Apr 07, 2017 at 03:07:07PM +0200, Micha?? K??pie?? wrote:
> This patch series cleans up parts of fujitsu-laptop responsible for
> handling LED class devices.  It was tested on a Lifebook E744.  It
> depends on the previous patch series I submitted for fujitsu-laptop and
> should be applied on top of the backlight cleanup series.
> 
>  drivers/platform/x86/Kconfig          |   2 +-
>  drivers/platform/x86/fujitsu-laptop.c | 402 +++++++++++++++-------------------
>  2 files changed, 176 insertions(+), 228 deletions(-)

FYI I should be able to complete my review of this series over the Easter
weekend.  This week has been hectic to say the least.  Apologies for the
delay.

Regards
  jonathan

Re: [PATCH 0/6] fujitsu-laptop: LED cleanup

[Michał Kępień]

> On Fri, Apr 07, 2017 at 03:07:07PM +0200, Micha?? K??pie?? wrote:
> > This patch series cleans up parts of fujitsu-laptop responsible for
> > handling LED class devices.  It was tested on a Lifebook E744.  It
> > depends on the previous patch series I submitted for fujitsu-laptop and
> > should be applied on top of the backlight cleanup series.
> > 
> >  drivers/platform/x86/Kconfig          |   2 +-
> >  drivers/platform/x86/fujitsu-laptop.c | 402 +++++++++++++++-------------------
> >  2 files changed, 176 insertions(+), 228 deletions(-)
> 
> FYI I should be able to complete my review of this series over the Easter
> weekend.  This week has been hectic to say the least.  Apologies for the
> delay.

As usual, no worries.  This buys me time to polish the next series.  By
the way, thank you for your patience in reviewing my submissions.  We
are getting there, I only have three more series queued after this one.

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/6] fujitsu-laptop: LED cleanup

[Jonathan Woithe]

On Fri, Apr 07, 2017 at 03:07:07PM +0200, Micha?? K??pie?? wrote:
> This patch series cleans up parts of fujitsu-laptop responsible for
> handling LED class devices.  It was tested on a Lifebook E744.  It
> depends on the previous patch series I submitted for fujitsu-laptop and
> should be applied on top of the backlight cleanup series.

I have completed my review.  The changes in this patch series are reasonably
extensive but should not result in any observable changes in behaviour. 
They represent a significant modernisation of the code, taking advantage of
the current approach to module architecture.  Unfortunately I do not have
hardware which includes the LEDs which these changes affect, so I cannot
confirm the absence of regressions.  I note however that it has been tested
on a Lifebook E744.

My only question about this patch set relates to patch 5/6 ("fujitsu-laptop:
do not log LED registration failures").  While it is true that driver core
will log an error due to the error flagged by the return value from
acpi_fujitsu_laptop_add() (as explained in the commit message), the
elimination of the pr_err() statements means that we loose the ability to
see which LED caused the problem.  All we know is that there has been an
error flagged by something within (or called by) acpi_fujitsu_laptop_add(). 
This may be related to LEDs or anything else initialised by
acpi_fujitsu_laptop_add().

Is the resulting simplification of code worth the loss of this finer grained
information in the event of a LED registration error?

Regards
  jonathan