[PATCH 00/11] fujitsu-laptop: backlight cleanup

[PATCH 00/11] fujitsu-laptop: backlight cleanup

[Michał Kępień]

This series introduces further changes to the way LCD backlight is
handled by fujitsu-laptop.  These changes include fixing a bug in code
responsible for generating brightness-related input events, cleaning up
handling of module parameters, reducing code duplication, removing
superfluous debug messages and other fixes.

This series was tested on a Lifebook S7020 and a Lifebook E744.

This series is based on the testing branch as it requires earlier patch
series I submitted in order to apply cleanly.

 drivers/platform/x86/fujitsu-laptop.c | 156 ++++++++++------------------------
 1 file changed, 45 insertions(+), 111 deletions(-)

-- 
2.12.1

[PATCH 01/11] platform/x86: fujitsu-laptop: only handle backlight when appropriate

[Michał Kępień]

The backlight part of fujitsu-laptop is only used by laptops which are
incapable of using the standard ACPI video interface for handling
brightness changes.  Conversely, on laptops which are capable of using
the latter, no vendor-specific ACPI calls should be made unless
explicitly requested by the user.  Bail out immediately from
acpi_fujitsu_bl_add() unless using the vendor-specific interface was
either explicitly requested by the user or automatically selected by the
kernel.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 52d6d2105f99..a428da3b591d 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -589,6 +589,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	int state = 0;
 	int error;
 
+	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+		return -ENODEV;
+
 	if (!device)
 		return -EINVAL;
 
@@ -641,11 +644,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
 	get_lcd_level();
 
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		error = fujitsu_backlight_register();
-		if (error)
-			return error;
-	}
+	error = fujitsu_backlight_register();
+	if (error)
+		return error;
 
 	return 0;
 }
-- 
2.12.1

[PATCH 02/11] platform/x86: fujitsu-laptop: switch to a managed backlight device

[Michał Kępień]

Use a managed backlight device to get rid of acpi_fujitsu_bl_remove().
Change the parent of the backlight device from NULL to the FUJ02B1 ACPI
device as the latter is required for the backlight device to work.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index a428da3b591d..a9520e732c8b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -565,17 +565,18 @@ static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
 	return input_register_device(fujitsu_bl->input);
 }
 
-static int fujitsu_backlight_register(void)
+static int fujitsu_backlight_register(struct acpi_device *device)
 {
-	struct backlight_properties props = {
+	const struct backlight_properties props = {
 		.brightness = fujitsu_bl->brightness_level,
 		.max_brightness = fujitsu_bl->max_brightness - 1,
 		.type = BACKLIGHT_PLATFORM
 	};
 	struct backlight_device *bd;
 
-	bd = backlight_device_register("fujitsu-laptop", NULL, NULL,
-				       &fujitsu_bl_ops, &props);
+	bd = devm_backlight_device_register(&device->dev, "fujitsu-laptop",
+					    &device->dev, NULL,
+					    &fujitsu_bl_ops, &props);
 	if (IS_ERR(bd))
 		return PTR_ERR(bd);
 
@@ -644,24 +645,13 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
 	get_lcd_level();
 
-	error = fujitsu_backlight_register();
+	error = fujitsu_backlight_register(device);
 	if (error)
 		return error;
 
 	return 0;
 }
 
-static int acpi_fujitsu_bl_remove(struct acpi_device *device)
-{
-	struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
-
-	backlight_device_unregister(fujitsu_bl->bl_device);
-
-	fujitsu_bl->acpi_handle = NULL;
-
-	return 0;
-}
-
 /* Brightness notify */
 
 static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
@@ -1092,7 +1082,6 @@ static struct acpi_driver acpi_fujitsu_bl_driver = {
 	.ids = fujitsu_bl_device_ids,
 	.ops = {
 		.add = acpi_fujitsu_bl_add,
-		.remove = acpi_fujitsu_bl_remove,
 		.notify = acpi_fujitsu_bl_notify,
 		},
 };
-- 
2.12.1

[PATCH 03/11] platform/x86: fujitsu-laptop: merge set_lcd_level_alt() into set_lcd_level()

[Michał Kępień]

Depending on the value of the use_alt_lcd_levels module parameter, one
of two functions is used for setting LCD brightness level.  These
functions are almost identical and only differ in the name of the ACPI
method they call.  Instead of checking the value of use_alt_lcd_levels
at each call site, move that check to set_lcd_level() and get rid of
set_lcd_level_alt().

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index a9520e732c8b..5005400b57bb 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -360,41 +360,26 @@ static int set_lcd_level(int level)
 {
 	acpi_status status = AE_OK;
 	acpi_handle handle = NULL;
-
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n",
-		    level);
-
-	if (level < 0 || level >= fujitsu_bl->max_brightness)
-		return -EINVAL;
-
-	status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBLL", &handle);
-	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n");
-		return -ENODEV;
+	char *method;
+
+	switch (use_alt_lcd_levels) {
+	case 1:
+		method = "SBL2";
+		break;
+	default:
+		method = "SBLL";
+		break;
 	}
 
-
-	status = acpi_execute_simple_method(handle, NULL, level);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	return 0;
-}
-
-static int set_lcd_level_alt(int level)
-{
-	acpi_status status = AE_OK;
-	acpi_handle handle = NULL;
-
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n",
-		    level);
+	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via %s [%d]\n",
+		    method, level);
 
 	if (level < 0 || level >= fujitsu_bl->max_brightness)
 		return -EINVAL;
 
-	status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBL2", &handle);
+	status = acpi_get_handle(fujitsu_bl->acpi_handle, method, &handle);
 	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n");
+		vdbg_printk(FUJLAPTOP_DBG_ERROR, "%s not present\n", method);
 		return -ENODEV;
 	}
 
@@ -463,10 +448,7 @@ static int bl_update_status(struct backlight_device *b)
 			"Unable to adjust backlight power, error code %i\n",
 			ret);
 
-	if (use_alt_lcd_levels)
-		ret = set_lcd_level_alt(b->props.brightness);
-	else
-		ret = set_lcd_level(b->props.brightness);
+	ret = set_lcd_level(b->props.brightness);
 	if (ret != 0)
 		vdbg_printk(FUJLAPTOP_DBG_ERROR,
 			"Unable to adjust LCD brightness, error code %i\n",
@@ -679,12 +661,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 	if (oldb == newb)
 		return;
 
-	if (disable_brightness_adjust != 1) {
-		if (use_alt_lcd_levels)
-			set_lcd_level_alt(newb);
-		else
-			set_lcd_level(newb);
-	}
+	if (disable_brightness_adjust != 1)
+		set_lcd_level(newb);
 
 	sparse_keymap_report_event(input, oldb < newb, 1, true);
 }
-- 
2.12.1

[PATCH 04/11] platform/x86: fujitsu-laptop: simplify set_lcd_level()

[Michał Kępień]

acpi_execute_simple_method() takes a method parameter which tells it to
look for the given method underneath the given handle, so calling
acpi_get_handle() beforehand is redundant.  Replace the call to
acpi_get_handle() with a call to acpi_execute_simple_method(), thus
eliminating the need for a local variable storing the handle.  Also do
not assign a default value to status as it has no influence on execution
flow.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 5005400b57bb..b888ba190fb2 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -358,8 +358,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 
 static int set_lcd_level(int level)
 {
-	acpi_status status = AE_OK;
-	acpi_handle handle = NULL;
+	acpi_status status;
 	char *method;
 
 	switch (use_alt_lcd_levels) {
@@ -377,16 +376,13 @@ static int set_lcd_level(int level)
 	if (level < 0 || level >= fujitsu_bl->max_brightness)
 		return -EINVAL;
 
-	status = acpi_get_handle(fujitsu_bl->acpi_handle, method, &handle);
+	status = acpi_execute_simple_method(fujitsu_bl->acpi_handle, method,
+					    level);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "%s not present\n", method);
 		return -ENODEV;
 	}
 
-	status = acpi_execute_simple_method(handle, NULL, level);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
 	return 0;
 }
 
-- 
2.12.1

Re: [PATCH 04/11] platform/x86: fujitsu-laptop: simplify set_lcd_level()

[Darren Hart]

On Thu, Mar 30, 2017 at 12:35:02PM +0200, Michał Kępień wrote:
> acpi_execute_simple_method() takes a method parameter which tells it to
> look for the given method underneath the given handle, so calling
> acpi_get_handle() beforehand is redundant.  Replace the call to
> acpi_get_handle() with a call to acpi_execute_simple_method(), thus
> eliminating the need for a local variable storing the handle.  Also do
> not assign a default value to status as it has no influence on execution
> flow.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 5005400b57bb..b888ba190fb2 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -358,8 +358,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
>  
>  static int set_lcd_level(int level)
>  {
> -	acpi_status status = AE_OK;
> -	acpi_handle handle = NULL;
> +	acpi_status status;
>  	char *method;
>  
>  	switch (use_alt_lcd_levels) {
> @@ -377,16 +376,13 @@ static int set_lcd_level(int level)
>  	if (level < 0 || level >= fujitsu_bl->max_brightness)
>  		return -EINVAL;
>  
> -	status = acpi_get_handle(fujitsu_bl->acpi_handle, method, &handle);
> +	status = acpi_execute_simple_method(fujitsu_bl->acpi_handle, method,
> +					    level);
>  	if (ACPI_FAILURE(status)) {
>  		vdbg_printk(FUJLAPTOP_DBG_ERROR, "%s not present\n", method);

So this changes behavior a little bit for debug messages. Previously, this
message only triggered if acpi_get_handle failed, now it will trigger if the
method exists but fails, whereas that was silent previously (see below). I think
printing in either case is fine, but we should adjust the message to reflect the
new code flow.

>  		return -ENODEV;
>  	}
>  
> -	status = acpi_execute_simple_method(handle, NULL, level);
> -	if (ACPI_FAILURE(status))
> -		return -ENODEV;
> -
>  	return 0;
>  }
>  
> -- 
> 2.12.1
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

[PATCH 04+/11] platform/x86: fujitsu-laptop: simplify set_lcd_level()

[Michał Kępień]

acpi_execute_simple_method() takes a method parameter which tells it to
look for the given method underneath the given handle, so calling
acpi_get_handle() beforehand is redundant.  Replace the call to
acpi_get_handle() with a call to acpi_execute_simple_method(), thus
eliminating the need for a local variable storing the handle.  Update
debug message to reflect this change.  Also do not assign a default
value to status as it has no influence on execution flow.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
Darren,

This is patch 04/11 with updated debug and commit messages.  Now that
you mentioned this issue, I think we should do the same for patch 2/3 in
the call_fext_func() cleanup series [1].  Would you like me to just send
an updated version of that specific patch (similarly to what I did in
this case) or rather send v2 of the entire call_fext_func() cleanup
series?

[1] https://www.spinics.net/lists/platform-driver-x86/msg11058.html

 drivers/platform/x86/fujitsu-laptop.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 2f563aa00592..6d4a2a36716b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -358,8 +358,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 
 static int set_lcd_level(int level)
 {
-	acpi_status status = AE_OK;
-	acpi_handle handle = NULL;
+	acpi_status status;
 	char *method;
 
 	switch (use_alt_lcd_levels) {
@@ -377,16 +376,14 @@ static int set_lcd_level(int level)
 	if (level < 0 || level >= fujitsu_bl->max_brightness)
 		return -EINVAL;
 
-	status = acpi_get_handle(fujitsu_bl->acpi_handle, method, &handle);
+	status = acpi_execute_simple_method(fujitsu_bl->acpi_handle, method,
+					    level);
 	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_ERROR, "%s not present\n", method);
+		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate %s\n",
+			    method);
 		return -ENODEV;
 	}
 
-	status = acpi_execute_simple_method(handle, NULL, level);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
 	return 0;
 }
 
-- 
2.12.2

Re: [PATCH 04+/11] platform/x86: fujitsu-laptop: simplify set_lcd_level()

[Darren Hart]

On Tue, Apr 04, 2017 at 11:39:03AM +0200, Michał Kępień wrote:
> acpi_execute_simple_method() takes a method parameter which tells it to
> look for the given method underneath the given handle, so calling
> acpi_get_handle() beforehand is redundant.  Replace the call to
> acpi_get_handle() with a call to acpi_execute_simple_method(), thus
> eliminating the need for a local variable storing the handle.  Update
> debug message to reflect this change.  Also do not assign a default
> value to status as it has no influence on execution flow.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
> Darren,
> 
> This is patch 04/11 with updated debug and commit messages.  Now that
> you mentioned this issue, I think we should do the same for patch 2/3 in
> the call_fext_func() cleanup series [1].  Would you like me to just send
> an updated version of that specific patch (similarly to what I did in
> this case) or rather send v2 of the entire call_fext_func() cleanup
> series?

Thanks, looks good.

Now that there is more than one cleanup, please do just send a v2 of the entire
series. Less error prone that way.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

[PATCH 05/11] platform/x86: fujitsu-laptop: sync brightness in set_lcd_level()

[Michał Kępień]

When using brightness keys and backlight device's sysfs interface
alternately, an incorrect input event might be generated for a
brightness key press.  Consider the following sequence of events:

 1. Set backlight brightness to 6 using brightness keys.
 2. Write 4 to /sys/class/backlight/fujitsu-laptop/brightness.
 3. Press the "brightness up" key.

The input event generated in this scenario would be KEY_BRIGHTNESSDOWN,
because before step 3 brightness_level would still be at 6.  As the new
brightness level is established using GBLL, it would evaluate to 5
(SBLL/SBL2 sets it to 4 in step 2 and pressing the "brightness up" key
increases it by 1).  This in turn would cause acpi_fujitsu_bl_notify()
to observe a 6 -> 5 change, i.e. a decrease in brightness, while screen
brightness would in fact be increased.

Fix this by updating brightness_level in set_lcd_level.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b888ba190fb2..9981988f82d2 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -383,6 +383,8 @@ static int set_lcd_level(int level)
 		return -ENODEV;
 	}
 
+	fujitsu_bl->brightness_level = level;
+
 	return 0;
 }
 
-- 
2.12.1

[PATCH 06/11] platform/x86: fujitsu-laptop: clean up use_alt_lcd_levels handling

[Michał Kępień]

The value of each module parameter can be changed on the fly via sysfs.
However, the current way of handling use_alt_lcd_levels prevents the
user from dynamically switching from a value of 0 or 1 back to
autodetection as the latter is only performed upon ACPI device
instantiation.  Fix this by moving autodetection (in a simplified form)
to set_lcd_level() and changing module parameter type to int.  Do not
announce autodetection results in a debug message as current value of
use_alt_lcd_levels can simply be read from sysfs.  Clarify module
parameter description.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 9981988f82d2..d10fbb0ed95b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -362,6 +362,12 @@ static int set_lcd_level(int level)
 	char *method;
 
 	switch (use_alt_lcd_levels) {
+	case -1:
+		if (acpi_has_method(fujitsu_bl->acpi_handle, "SBL2"))
+			method = "SBL2";
+		else
+			method = "SBLL";
+		break;
 	case 1:
 		method = "SBL2";
 		break;
@@ -605,21 +611,10 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 			pr_err("_INI Method failed\n");
 	}
 
-	if (use_alt_lcd_levels == -1) {
-		if (acpi_has_method(NULL, "\\_SB.PCI0.LPCB.FJEX.SBL2"))
-			use_alt_lcd_levels = 1;
-		else
-			use_alt_lcd_levels = 0;
-		vdbg_printk(FUJLAPTOP_DBG_TRACE, "auto-detected usealt as %i\n",
-			    use_alt_lcd_levels);
-	}
-
 	/* do config (detect defaults) */
-	use_alt_lcd_levels = use_alt_lcd_levels == 1 ? 1 : 0;
 	disable_brightness_adjust = disable_brightness_adjust == 1 ? 1 : 0;
-	vdbg_printk(FUJLAPTOP_DBG_INFO,
-		    "config: [alt interface: %d], [adjust disable: %d]\n",
-		    use_alt_lcd_levels, disable_brightness_adjust);
+	vdbg_printk(FUJLAPTOP_DBG_INFO, "config: [adjust disable: %d]\n",
+		    disable_brightness_adjust);
 
 	if (get_max_brightness() <= 0)
 		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
@@ -1152,9 +1147,8 @@ static void __exit fujitsu_cleanup(void)
 module_init(fujitsu_init);
 module_exit(fujitsu_cleanup);
 
-module_param(use_alt_lcd_levels, uint, 0644);
-MODULE_PARM_DESC(use_alt_lcd_levels,
-		 "Use alternative interface for lcd_levels (needed for Lifebook s6410).");
+module_param(use_alt_lcd_levels, int, 0644);
+MODULE_PARM_DESC(use_alt_lcd_levels, "Interface used for setting LCD brightness level (-1 = auto, 0 = force SBLL, 1 = force SBL2)");
 module_param(disable_brightness_adjust, uint, 0644);
 MODULE_PARM_DESC(disable_brightness_adjust, "Disable brightness adjustment .");
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-- 
2.12.1

[PATCH 07/11] platform/x86: fujitsu-laptop: make disable_brightness_adjust a boolean

[Michał Kępień]

Due to the way the disable_brightness_adjust module parameter is
currently handled in acpi_fujitsu_bl_add(), it can only be set to either
0 or 1, which effectively makes it a boolean.  Emphasize that by
changing module parameter type to bool.  Do not announce parameter value
in a debug message as it can be dynamically changed via sysfs and its
current value can also be read from there.  Clean up module parameter
description.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index d10fbb0ed95b..84c2bd55526f 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -147,7 +147,7 @@ struct fujitsu_bl {
 
 static struct fujitsu_bl *fujitsu_bl;
 static int use_alt_lcd_levels = -1;
-static int disable_brightness_adjust = -1;
+static bool disable_brightness_adjust;
 
 /* Device used to access hotkeys and other features on the laptop */
 struct fujitsu_laptop {
@@ -611,11 +611,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 			pr_err("_INI Method failed\n");
 	}
 
-	/* do config (detect defaults) */
-	disable_brightness_adjust = disable_brightness_adjust == 1 ? 1 : 0;
-	vdbg_printk(FUJLAPTOP_DBG_INFO, "config: [adjust disable: %d]\n",
-		    disable_brightness_adjust);
-
 	if (get_max_brightness() <= 0)
 		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
 	get_lcd_level();
@@ -654,7 +649,7 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 	if (oldb == newb)
 		return;
 
-	if (disable_brightness_adjust != 1)
+	if (!disable_brightness_adjust)
 		set_lcd_level(newb);
 
 	sparse_keymap_report_event(input, oldb < newb, 1, true);
@@ -1149,8 +1144,8 @@ module_exit(fujitsu_cleanup);
 
 module_param(use_alt_lcd_levels, int, 0644);
 MODULE_PARM_DESC(use_alt_lcd_levels, "Interface used for setting LCD brightness level (-1 = auto, 0 = force SBLL, 1 = force SBL2)");
-module_param(disable_brightness_adjust, uint, 0644);
-MODULE_PARM_DESC(disable_brightness_adjust, "Disable brightness adjustment .");
+module_param(disable_brightness_adjust, bool, 0644);
+MODULE_PARM_DESC(disable_brightness_adjust, "Disable LCD brightness adjustment");
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
 module_param_named(debug, dbg_level, uint, 0644);
 MODULE_PARM_DESC(debug, "Sets debug level bit-mask");
-- 
2.12.1

[PATCH 08/11] platform/x86: fujitsu-laptop: ignore errors when setting backlight power

[Michał Kępień]

Not all Fujitsu laptops support controlling backlight power through the
FUJ02E3 ACPI device.  Remove the debug message informing about a failed
attempt to set backlight power as it may be confusing and the return
value of call_fext_func() is logged anyway.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 84c2bd55526f..ed9c37e0591d 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -444,13 +444,9 @@ static int bl_update_status(struct backlight_device *b)
 {
 	int ret;
 	if (b->props.power == FB_BLANK_POWERDOWN)
-		ret = call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
+		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
 	else
-		ret = call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
-	if (ret != 0)
-		vdbg_printk(FUJLAPTOP_DBG_ERROR,
-			"Unable to adjust backlight power, error code %i\n",
-			ret);
+		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
 
 	ret = set_lcd_level(b->props.brightness);
 	if (ret != 0)
-- 
2.12.1

[PATCH 09/11] platform/x86: fujitsu-laptop: do not log set_lcd_level() failures in bl_update_status()

[Michał Kępień]

Any set_lcd_level() call can fail for one of two reasons: either
requested brightness is outside the allowed range or the ACPI method
used for setting brightness level is not available.  For
bl_update_status(), the first case is handled by backlight core, which
means bl_update_status() will not even be called if requested brightness
is outside the allowed range.  The second case will be logged anyway by
set_lcd_level() itself, so there is no point in generating another
message in bl_update_status().  Remove the superfluous debug message
along with a local variable that is now redundant.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index ed9c37e0591d..78a39b143200 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -442,18 +442,12 @@ static int bl_get_brightness(struct backlight_device *b)
 
 static int bl_update_status(struct backlight_device *b)
 {
-	int ret;
 	if (b->props.power == FB_BLANK_POWERDOWN)
 		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
 	else
 		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
 
-	ret = set_lcd_level(b->props.brightness);
-	if (ret != 0)
-		vdbg_printk(FUJLAPTOP_DBG_ERROR,
-			"Unable to adjust LCD brightness, error code %i\n",
-			ret);
-	return ret;
+	return set_lcd_level(b->props.brightness);
 }
 
 static const struct backlight_ops fujitsu_bl_ops = {
-- 
2.12.1

[PATCH 10/11] platform/x86: fujitsu-laptop: account for backlight power when determining brightness

[Michał Kępień]

The comment for the get_brightness backlight device callback in
include/linux/backlight.h states that it should "return the current
backlight brightness (accounting for power, fb_blank etc.)".
fujitsu-laptop violates that premise by simply returning the value to
which ACPI function GBLL evaluates to.  Make sure 0 is returned when
backlight power is turned off.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 78a39b143200..99befc1bc4f6 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -437,7 +437,7 @@ static int get_max_brightness(void)
 
 static int bl_get_brightness(struct backlight_device *b)
 {
-	return get_lcd_level();
+	return b->props.power == FB_BLANK_POWERDOWN ? 0 : get_lcd_level();
 }
 
 static int bl_update_status(struct backlight_device *b)
-- 
2.12.1

[PATCH 11/11] platform/x86: fujitsu-laptop: remove redundant fields from struct fujitsu_bl

[Michał Kępień]

The dev field of struct fujitsu_bl is assigned in acpi_fujitsu_bl_add(),
but never used afterwards.  brightness_changed is set in get_lcd_level()
and then its value is only printed in a debug message, so it does not
influence execution flow.  Remove both fields as they are redundant.
Update the aforementioned debug message.  Adjust whitespace to make
checkpatch happy.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 99befc1bc4f6..976baceea018 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -135,13 +135,10 @@
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
 	acpi_handle acpi_handle;
-	struct acpi_device *dev;
 	struct input_dev *input;
 	char phys[32];
 	struct backlight_device *bl_device;
-
 	unsigned int max_brightness;
-	unsigned int brightness_changed;
 	unsigned int brightness_level;
 };
 
@@ -408,11 +405,6 @@ static int get_lcd_level(void)
 
 	fujitsu_bl->brightness_level = state & 0x0fffffff;
 
-	if (state & 0x80000000)
-		fujitsu_bl->brightness_changed = 1;
-	else
-		fujitsu_bl->brightness_changed = 0;
-
 	return fujitsu_bl->brightness_level;
 }
 
@@ -591,8 +583,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	       acpi_device_name(device), acpi_device_bid(device),
 	       !device->power.state ? "on" : "off");
 
-	fujitsu_bl->dev = device;
-
 	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
 		vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
 		if (ACPI_FAILURE
@@ -632,9 +622,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 	get_lcd_level();
 	newb = fujitsu_bl->brightness_level;
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE,
-		    "brightness button event [%i -> %i (%i)]\n",
-		    oldb, newb, fujitsu_bl->brightness_changed);
+	vdbg_printk(FUJLAPTOP_DBG_TRACE, "brightness button event [%i -> %i]\n",
+		    oldb, newb);
 
 	if (oldb == newb)
 		return;
-- 
2.12.1

Re: [PATCH 11/11] platform/x86: fujitsu-laptop: remove redundant fields from struct fujitsu_bl

[Darren Hart]

On Thu, Mar 30, 2017 at 12:35:09PM +0200, Michał Kępień wrote:
> The dev field of struct fujitsu_bl is assigned in acpi_fujitsu_bl_add(),
> but never used afterwards.  brightness_changed is set in get_lcd_level()
> and then its value is only printed in a debug message, so it does not
> influence execution flow.  Remove both fields as they are redundant.

Indeed, once that was removed from the platform device and the corresponding
sysfs entries were eliminated, we can drop it:

78b2602 platform/x86: fujitsu-laptop: remove backlight-related attributes from the platform device

:-)

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 00/11] fujitsu-laptop: backlight cleanup

[Darren Hart]

On Thu, Mar 30, 2017 at 12:34:58PM +0200, Michał Kępień wrote:
> This series introduces further changes to the way LCD backlight is
> handled by fujitsu-laptop.  These changes include fixing a bug in code
> responsible for generating brightness-related input events, cleaning up
> handling of module parameters, reducing code duplication, removing
> superfluous debug messages and other fixes.
> 
> This series was tested on a Lifebook S7020 and a Lifebook E744.
> 
> This series is based on the testing branch as it requires earlier patch
> series I submitted in order to apply cleanly.

Other than the rather minor nit to 4/11, I've reviewed this series and feel it's
ready to go. If you would like to resend 4/11 in response to that specific
patch, I can just pick it up and get it queued and tested while we await a
review from Jonathan.

Again, nice work.

-- 
Darren Hart
VMware Open Source Technology Center