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.
Changes from v1:
- Update debug message logged by set_lcd_level() to reflect code flow
changes introduced by patch 04/11.
drivers/platform/x86/fujitsu-laptop.c | 157 ++++++++++------------------------
1 file changed, 46 insertions(+), 111 deletions(-)
--
2.12.2
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ń <[email protected]>
---
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 cd2da9a02592..a3e9efd1ac88 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 {
@@ -612,11 +612,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();
@@ -655,7 +650,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);
@@ -1150,8 +1145,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.2
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ń <[email protected]>
---
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 a797e7890773..f58a33d70be3 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -438,7 +438,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.2
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ń <[email protected]>
---
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 f58a33d70be3..9fd5b98aeef8 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;
};
@@ -409,11 +406,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;
}
@@ -592,8 +584,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
@@ -633,9 +623,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.2
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ń <[email protected]>
---
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 59107a599d22..2f563aa00592 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.2
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ń <[email protected]>
---
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 b019060d6dc4..cd2da9a02592 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;
@@ -606,21 +612,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;
@@ -1153,9 +1148,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.2
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ń <[email protected]>
---
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 a3e9efd1ac88..0cd58c96938c 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -445,13 +445,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.2
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ń <[email protected]>
---
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 0cd58c96938c..a797e7890773 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -443,18 +443,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.2
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ń <[email protected]>
---
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 6d4a2a36716b..b019060d6dc4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -384,6 +384,8 @@ static int set_lcd_level(int level)
return -ENODEV;
}
+ fujitsu_bl->brightness_level = level;
+
return 0;
}
--
2.12.2
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ń <[email protected]>
---
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
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ń <[email protected]>
---
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 722250d1aa20..59107a599d22 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.2
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ń <[email protected]>
---
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 f66da4b0c31a..722250d1aa20 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.2
On Wed, Apr 05, 2017 at 08:48:59AM +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.
>
> Changes from v1:
>
> - Update debug message logged by set_lcd_level() to reflect code flow
> changes introduced by patch 04/11.
Queued to testing, thanks!
>
> drivers/platform/x86/fujitsu-laptop.c | 157 ++++++++++------------------------
> 1 file changed, 46 insertions(+), 111 deletions(-)
>
> --
> 2.12.2
>
>
--
Darren Hart
VMware Open Source Technology Center
> On Wed, Apr 05, 2017 at 08:48:59AM +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.
> >
> > Changes from v1:
> >
> > - Update debug message logged by set_lcd_level() to reflect code flow
> > changes introduced by patch 04/11.
>
> Queued to testing, thanks!
Jonathan, I would still love to hear your opinion on this series. Take
your time, though, I do not see any reason to rush things. I will only
send the next series once you ack this one.
--
Best regards,
Michał Kępień
Hi Michael
On Wed, Apr 05, 2017 at 09:55:34PM +0200, Micha?? K??pie?? wrote:
> > On Wed, Apr 05, 2017 at 08:48:59AM +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.
> > >
> > > Changes from v1:
> > >
> > > - Update debug message logged by set_lcd_level() to reflect code flow
> > > changes introduced by patch 04/11.
> >
> > Queued to testing, thanks!
>
> Jonathan, I would still love to hear your opinion on this series. Take
> your time, though, I do not see any reason to rush things. I will only
> send the next series once you ack this one.
Sure, no problem. As mentioned earlier, this week has been busy. I am
hoping I might find the time to complete my review this evening. If not, it
will be some time over the weekend.
Regards
jonathan
On Thu, Apr 06, 2017 at 10:15:14AM +0930, Jonathan Woithe wrote:
> Hi Michael
>
> On Wed, Apr 05, 2017 at 09:55:34PM +0200, Micha?? K??pie?? wrote:
> > > On Wed, Apr 05, 2017 at 08:48:59AM +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.
> > > >
> > > > Changes from v1:
> > > >
> > > > - Update debug message logged by set_lcd_level() to reflect code flow
> > > > changes introduced by patch 04/11.
> > >
> > > Queued to testing, thanks!
> >
> > Jonathan, I would still love to hear your opinion on this series. Take
> > your time, though, I do not see any reason to rush things. I will only
> > send the next series once you ack this one.
>
> Sure, no problem. As mentioned earlier, this week has been busy. I am
> hoping I might find the time to complete my review this evening. If not, it
> will be some time over the weekend.
Eeek, I jumped the gun on that. I've moved this from testing to fujitsu, and
it'll wait there until Jonathan gets a chance to review. Apologies Jonathan.
--
Darren Hart
VMware Open Source Technology Center
On Wed, Apr 05, 2017 at 08:49:02AM +0200, Micha?? K??pie?? wrote:
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 59107a599d22..2f563aa00592 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;
> }
This is not necessary something actionable, but I am wondering about the
rationale of using a switch statement here given that there really are only
two options. In my mind at least a simple "if" clause would be clearer and
easier to read (with or without the braces):
if (use_alt_lcd_levels) {
method = "SBL2";
} else {
method = "SBLL";
}
Regards
jonathan
On Fri, Apr 07, 2017 at 09:23:47PM +0930, I wrote:
> On Wed, Apr 05, 2017 at 08:49:02AM +0200, Micha?? K??pie?? wrote:
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index 59107a599d22..2f563aa00592 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;
> > }
>
> This is not necessary something actionable, but I am wondering about the
> rationale of using a switch statement here given that there really are only
> two options. In my mind at least a simple "if" clause would be clearer and
> easier to read (with or without the braces):
>
> if (use_alt_lcd_levels) {
> method = "SBL2";
> } else {
> method = "SBLL";
> }
Ah, the reason for the use of the switch was to prepare the way for patch
06/11 which adds an autodetection value to the definition of
use_alt_lcd_levels. All good.
Regards
jonathan
On Wed, Apr 05, 2017 at 08:07:54PM -0700, Darren Hart wrote:
> On Thu, Apr 06, 2017 at 10:15:14AM +0930, Jonathan Woithe wrote:
> > On Wed, Apr 05, 2017 at 09:55:34PM +0200, Micha?? K??pie?? wrote:
> > > > On Wed, Apr 05, 2017 at 08:48:59AM +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.
> > > > >
> > > > > Changes from v1:
> > > > >
> > > > > - Update debug message logged by set_lcd_level() to reflect code flow
> > > > > changes introduced by patch 04/11.
> > > >
> > > > Queued to testing, thanks!
> > >
> > > Jonathan, I would still love to hear your opinion on this series. Take
> > > your time, though, I do not see any reason to rush things. I will only
> > > send the next series once you ack this one.
> >
> > Sure, no problem. As mentioned earlier, this week has been busy. I am
> > hoping I might find the time to complete my review this evening. If not, it
> > will be some time over the weekend.
>
> Eeek, I jumped the gun on that. I've moved this from testing to fujitsu, and
> it'll wait there until Jonathan gets a chance to review. Apologies Jonathan.
That's fine, these things happen.
I've completed a review of this series. It represents another worthwhile
evolutionary clean up of the fujitsu-laptop driver, this time concentrating
on the backlight functions. Importantly, it makes adjustments for changes
in related kernel infrastructure as it has evolved over the last few
years (the handling of module parameters, method return values and so on).
I am happy for this series to be merged. Thanks Michael for your continued
clean up efforts.
Reviewed-by: Jonathan Woithe <[email protected]>
Regards
jonathan
On Fri, Apr 07, 2017 at 09:41:35PM +0930, Jonathan Woithe wrote:
> On Wed, Apr 05, 2017 at 08:07:54PM -0700, Darren Hart wrote:
> > On Thu, Apr 06, 2017 at 10:15:14AM +0930, Jonathan Woithe wrote:
> > > On Wed, Apr 05, 2017 at 09:55:34PM +0200, Micha?? K??pie?? wrote:
> > > > > On Wed, Apr 05, 2017 at 08:48:59AM +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.
> > > > > >
> > > > > > Changes from v1:
> > > > > >
> > > > > > - Update debug message logged by set_lcd_level() to reflect code flow
> > > > > > changes introduced by patch 04/11.
> > > > >
> > > > > Queued to testing, thanks!
> > > >
> > > > Jonathan, I would still love to hear your opinion on this series. Take
> > > > your time, though, I do not see any reason to rush things. I will only
> > > > send the next series once you ack this one.
> > >
> > > Sure, no problem. As mentioned earlier, this week has been busy. I am
> > > hoping I might find the time to complete my review this evening. If not, it
> > > will be some time over the weekend.
> >
> > Eeek, I jumped the gun on that. I've moved this from testing to fujitsu, and
> > it'll wait there until Jonathan gets a chance to review. Apologies Jonathan.
>
> That's fine, these things happen.
>
> I've completed a review of this series. It represents another worthwhile
> evolutionary clean up of the fujitsu-laptop driver, this time concentrating
> on the backlight functions. Importantly, it makes adjustments for changes
> in related kernel infrastructure as it has evolved over the last few
> years (the handling of module parameters, method return values and so on).
>
> I am happy for this series to be merged. Thanks Michael for your continued
> clean up efforts.
>
> Reviewed-by: Jonathan Woithe <[email protected]>
Queued to testing, thanks!
--
Darren Hart
VMware Open Source Technology Center