These patches should make fujitsu_init() a bit more palatable. No
changes are made to platform device code yet, for clarity these will be
posted in a separate series after this one gets applied.
Changes from v2:
- Patch 2/4 from v2 did not work as expected and was thus replaced
with a rebased version of patch 3/4 from v1.
- Added a check in patch 3/4 to prevent a NULL dereference when
ACPI device FUJ02B1 is not present and ACPI device FUJ02E3 is.
Changes from v1:
- Rebase on top of reworked Alan Jenkins' cleanup patch series.
- Drop patch 1/4 from v1 as it has already been applied in reworked
Alan Jenkins' cleanup patch series.
- Patch 3/4 from v1 has been replaced with a completely different one
(patch 2/4). It needs to be tested on a relevant machine as it is
based purely on a dump of a DSDT table (further details can be found
in the patch itself).
- Patch 3/4 in v2 is a rebased version of patch 8/10 from the reworked
Alan Jenkins' cleanup patch series. Patch 2/4 from v2 (the one
mentioned in the previous bullet point) ensures this one can be
safely applied without causing a NULL dereference under any
circumstances.
drivers/platform/x86/fujitsu-laptop.c | 98 ++++++++++++++++++-----------------
1 file changed, 50 insertions(+), 48 deletions(-)
--
2.12.0
Error labels currently used in fujitsu_init() are really hard to follow:
some (fail_laptop) indicate which operation has failed, others
(fail_sysfs_group) indicate where unrolling should start and the rest
(fail_platform_driver) is simply confusing. Change them to follow the
pattern used throughout the rest of the module, i.e. make every label
indicate the first unrolling operation it leads to.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 8b2701da4a4e..5bd3f8f8e800 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1244,60 +1244,60 @@ static int __init fujitsu_init(void)
ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
if (ret)
- goto fail_acpi;
+ goto err_free_fujitsu_bl;
/* Register platform stuff */
fujitsu_bl->pf_device = platform_device_alloc("fujitsu-laptop", -1);
if (!fujitsu_bl->pf_device) {
ret = -ENOMEM;
- goto fail_platform_driver;
+ goto err_unregister_acpi;
}
ret = platform_device_add(fujitsu_bl->pf_device);
if (ret)
- goto fail_platform_device1;
+ goto err_put_platform_device;
ret =
sysfs_create_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsu_pf_attribute_group);
if (ret)
- goto fail_platform_device2;
+ goto err_del_platform_device;
ret = platform_driver_register(&fujitsu_pf_driver);
if (ret)
- goto fail_sysfs_group;
+ goto err_remove_sysfs_group;
/* Register laptop driver */
fujitsu_laptop = kzalloc(sizeof(struct fujitsu_laptop), GFP_KERNEL);
if (!fujitsu_laptop) {
ret = -ENOMEM;
- goto fail_laptop;
+ goto err_unregister_platform_driver;
}
ret = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
if (ret)
- goto fail_laptop1;
+ goto err_free_fujitsu_laptop;
pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
return 0;
-fail_laptop1:
+err_free_fujitsu_laptop:
kfree(fujitsu_laptop);
-fail_laptop:
+err_unregister_platform_driver:
platform_driver_unregister(&fujitsu_pf_driver);
-fail_sysfs_group:
+err_remove_sysfs_group:
sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsu_pf_attribute_group);
-fail_platform_device2:
+err_del_platform_device:
platform_device_del(fujitsu_bl->pf_device);
-fail_platform_device1:
+err_put_platform_device:
platform_device_put(fujitsu_bl->pf_device);
-fail_platform_driver:
+err_unregister_acpi:
acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
-fail_acpi:
+err_free_fujitsu_bl:
kfree(fujitsu_bl);
return ret;
--
2.12.0
As the backlight device registered by fujitsu-laptop relies on the
FUJ02B1 ACPI device being present, only register the backlight device
once that ACPI device is detected.
Suggested-by: Alan Jenkins <[email protected]>
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 348af17cdc94..8b2701da4a4e 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -781,6 +781,12 @@ 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)
+ goto err_unregister_input_dev;
+ }
+
return 0;
err_unregister_input_dev:
@@ -797,6 +803,7 @@ static int acpi_fujitsu_bl_remove(struct acpi_device *device)
struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
struct input_dev *input = fujitsu_bl->input;
+ backlight_device_unregister(fujitsu_bl->bl_device);
input_unregister_device(input);
fujitsu_bl->acpi_handle = NULL;
@@ -956,7 +963,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
pr_info("BTNI: [0x%x]\n", call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
/* Sync backlight power status */
- if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+ if (fujitsu_bl->bl_device &&
+ acpi_video_get_backlight_type() == acpi_backlight_vendor) {
if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
else
@@ -1256,17 +1264,9 @@ static int __init fujitsu_init(void)
if (ret)
goto fail_platform_device2;
- /* Register backlight stuff */
-
- if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
- ret = fujitsu_backlight_register();
- if (ret)
- goto fail_sysfs_group;
- }
-
ret = platform_driver_register(&fujitsu_pf_driver);
if (ret)
- goto fail_backlight;
+ goto fail_sysfs_group;
/* Register laptop driver */
@@ -1288,8 +1288,6 @@ static int __init fujitsu_init(void)
kfree(fujitsu_laptop);
fail_laptop:
platform_driver_unregister(&fujitsu_pf_driver);
-fail_backlight:
- backlight_device_unregister(fujitsu_bl->bl_device);
fail_sysfs_group:
sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsu_pf_attribute_group);
@@ -1313,8 +1311,6 @@ static void __exit fujitsu_cleanup(void)
platform_driver_unregister(&fujitsu_pf_driver);
- backlight_device_unregister(fujitsu_bl->bl_device);
-
sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsu_pf_attribute_group);
--
2.12.0
Registering an ACPI driver does not mean the device it handles has to
exist. As the code which syncs backlight power status uses
call_fext_func(), it needs the FUJ02E3 ACPI device to be present, so
ensure that code is only run once the FUJ02E3 device is detected.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 185c929898d9..348af17cdc94 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -955,6 +955,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
/* Suspect this is a keymap of the application panel, print it */
pr_info("BTNI: [0x%x]\n", call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
+ /* Sync backlight power status */
+ if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+ if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
+ fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
+ else
+ fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
+ }
+
#if IS_ENABLED(CONFIG_LEDS_CLASS)
if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
result = led_classdev_register(&fujitsu_bl->pf_device->dev,
@@ -1272,14 +1280,6 @@ static int __init fujitsu_init(void)
if (ret)
goto fail_laptop1;
- /* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
- if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
- if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
- fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
- else
- fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
- }
-
pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
return 0;
--
2.12.0
Move code responsible for backlight device registration to a separate
function in order to simplify error handling and decrease indentation.
Simplify initialization of struct backlight_properties. Use
KBUILD_MODNAME as device name to avoid repeating the same string literal
throughout the module.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 38 ++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index e12cc3504d48..185c929898d9 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
/* ACPI device for LCD brightness control */
+static int fujitsu_backlight_register(void)
+{
+ 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(KBUILD_MODNAME, NULL, NULL,
+ &fujitsu_bl_ops, &props);
+ if (IS_ERR(bd))
+ return PTR_ERR(bd);
+
+ fujitsu_bl->bl_device = bd;
+
+ return 0;
+}
+
static int acpi_fujitsu_bl_add(struct acpi_device *device)
{
int state = 0;
@@ -1192,7 +1211,7 @@ MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
static int __init fujitsu_init(void)
{
- int ret, max_brightness;
+ int ret;
if (acpi_disabled)
return -ENODEV;
@@ -1232,22 +1251,9 @@ static int __init fujitsu_init(void)
/* Register backlight stuff */
if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
- struct backlight_properties props;
-
- memset(&props, 0, sizeof(struct backlight_properties));
- max_brightness = fujitsu_bl->max_brightness;
- props.type = BACKLIGHT_PLATFORM;
- props.max_brightness = max_brightness - 1;
- fujitsu_bl->bl_device = backlight_device_register("fujitsu-laptop",
- NULL, NULL,
- &fujitsu_bl_ops,
- &props);
- if (IS_ERR(fujitsu_bl->bl_device)) {
- ret = PTR_ERR(fujitsu_bl->bl_device);
- fujitsu_bl->bl_device = NULL;
+ ret = fujitsu_backlight_register();
+ if (ret)
goto fail_sysfs_group;
- }
- fujitsu_bl->bl_device->props.brightness = fujitsu_bl->brightness_level;
}
ret = platform_driver_register(&fujitsu_pf_driver);
--
2.12.0
On Tue, Mar 07, 2017 at 11:15:12AM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable. No
> changes are made to platform device code yet, for clarity these will be
> posted in a separate series after this one gets applied.
I will test and review these as soon as possible. It is likely to be later
this week or over the weekend. I suspect this version will be good to go
but we should confirm this. :-)
Regards
jonathan
> Move code responsible for backlight device registration to a separate
> function in order to simplify error handling and decrease indentation.
> Simplify initialization of struct backlight_properties. Use
> KBUILD_MODNAME as device name to avoid repeating the same string literal
> throughout the module.
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> drivers/platform/x86/fujitsu-laptop.c | 38 ++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index e12cc3504d48..185c929898d9 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
>
> /* ACPI device for LCD brightness control */
>
> +static int fujitsu_backlight_register(void)
> +{
> + 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(KBUILD_MODNAME, NULL, NULL,
> + &fujitsu_bl_ops, &props);
I have only just now noticed that this effectively breaks userspace
interface as KBUILD_MODNAME is "fujitsu_laptop" while the previously
used device name was "fujitsu-laptop" (underscore vs. hyphen).
Jonathan, as this series is already long overdue, I suggest the
following course of action: please review these patches anyway and if
you find no other issues, please provide your Reviewed-by etc. as you
normally would. Once you do that, I will post v4 which will use
"fujitsu-laptop" for backlight device name and will contain your v3
review tags. Does that sound reasonable?
--
Best regards,
Michał Kępień
On Tue, Mar 07, 2017 at 11:15:12AM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable. No
> changes are made to platform device code yet, for clarity these will be
> posted in a separate series after this one gets applied.
>
> Changes from v2:
>
> - Patch 2/4 from v2 did not work as expected and was thus replaced
> with a rebased version of patch 3/4 from v1.
>
> - Added a check in patch 3/4 to prevent a NULL dereference when
> ACPI device FUJ02B1 is not present and ACPI device FUJ02E3 is.
>
> Changes from v1:
>
> - Rebase on top of reworked Alan Jenkins' cleanup patch series.
>
> - Drop patch 1/4 from v1 as it has already been applied in reworked
> Alan Jenkins' cleanup patch series.
>
> - Patch 3/4 from v1 has been replaced with a completely different one
> (patch 2/4). It needs to be tested on a relevant machine as it is
> based purely on a dump of a DSDT table (further details can be found
> in the patch itself).
>
> - Patch 3/4 in v2 is a rebased version of patch 8/10 from the reworked
> Alan Jenkins' cleanup patch series. Patch 2/4 from v2 (the one
> mentioned in the previous bullet point) ensures this one can be
> safely applied without causing a NULL dereference under any
> circumstances.
>
> drivers/platform/x86/fujitsu-laptop.c | 98 ++++++++++++++++++-----------------
> 1 file changed, 50 insertions(+), 48 deletions(-)
Having tested this V3 patch series on an S7020 I can confirm that there are
no key functional regressions with respect to the module components utilised
by this model: the bl_power and brightness controls work in the same way as
they have in the past.
My backlight control scripts failed because the attribute path they expected
(/sys/devices/virtual/backlight/fujitsu-laptop/) did not exist. Instead
this patch creates /sys/devices/virtual/backlight/fujitsu_laptop/, the
difference being an underscore instead of a hyphen. This API regression was
caused by the introduction of KBUILD_MODNAME to replace the literal
"fujitsu-laptop" in patch 1/4. While it would be nice to use KBUILD_MODNAME
instead of the string literal, doing so does constitute a userspace API
breakage which is best avoided. I therefore suggest KBUILD_MODNAME be
replaced with "fujitsu-laptop" in patch 1/4.
If the above substitution is made in patch 1/4 I am happy to see this series
applied. It represents a further worthwhile improvement to the
fujitsu-laptop driver which will facilitate future maintenance and provide a
more consistent basis for upcoming improvements.
Tested-by: Jonathan Woithe <[email protected]>
Reviewed-by: Jonathan Woithe <[email protected]>
Regards
jonathan
Hi Michael
On Fri, Mar 10, 2017 at 10:08:49AM +0100, Micha?? K??pie?? wrote:
> > Move code responsible for backlight device registration to a separate
> > function in order to simplify error handling and decrease indentation.
> > Simplify initialization of struct backlight_properties. Use
> > KBUILD_MODNAME as device name to avoid repeating the same string literal
> > throughout the module.
> >
> > Signed-off-by: Micha?? K??pie?? <[email protected]>
> > ---
> > drivers/platform/x86/fujitsu-laptop.c | 38 ++++++++++++++++++++---------------
> > 1 file changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index e12cc3504d48..185c929898d9 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
> >
> > /* ACPI device for LCD brightness control */
> >
> > +static int fujitsu_backlight_register(void)
> > +{
> > + 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(KBUILD_MODNAME, NULL, NULL,
> > + &fujitsu_bl_ops, &props);
>
> I have only just now noticed that this effectively breaks userspace
> interface as KBUILD_MODNAME is "fujitsu_laptop" while the previously
> used device name was "fujitsu-laptop" (underscore vs. hyphen).
Ah yes, I noticed that too, as you see from the review I just sent through.
Sorry, I didn't see the above post until after I'd sent the review in. It's
no big deal though.
> Jonathan, as this series is already long overdue, I suggest the
> following course of action: please review these patches anyway and if
> you find no other issues, please provide your Reviewed-by etc. as you
> normally would. Once you do that, I will post v4 which will use
> "fujitsu-laptop" for backlight device name and will contain your v3
> review tags. Does that sound reasonable?
As per my review comments, I am happy with the patch series so long as the
backlight device name reverts to "fujitsu-laptop". Your suggested course of
action sounds fine to me. Alternatively, I could quickly and easily add
tags to a v4 post if that is seen to be more appropriate by Darren/Andy.
I'm easy either way.
Regards
jonathan
> Hi Michael
>
> On Fri, Mar 10, 2017 at 10:08:49AM +0100, Micha?? K??pie?? wrote:
> > > Move code responsible for backlight device registration to a separate
> > > function in order to simplify error handling and decrease indentation.
> > > Simplify initialization of struct backlight_properties. Use
> > > KBUILD_MODNAME as device name to avoid repeating the same string literal
> > > throughout the module.
> > >
> > > Signed-off-by: Micha?? K??pie?? <[email protected]>
> > > ---
> > > drivers/platform/x86/fujitsu-laptop.c | 38 ++++++++++++++++++++---------------
> > > 1 file changed, 22 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > > index e12cc3504d48..185c929898d9 100644
> > > --- a/drivers/platform/x86/fujitsu-laptop.c
> > > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > > @@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
> > >
> > > /* ACPI device for LCD brightness control */
> > >
> > > +static int fujitsu_backlight_register(void)
> > > +{
> > > + 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(KBUILD_MODNAME, NULL, NULL,
> > > + &fujitsu_bl_ops, &props);
> >
> > I have only just now noticed that this effectively breaks userspace
> > interface as KBUILD_MODNAME is "fujitsu_laptop" while the previously
> > used device name was "fujitsu-laptop" (underscore vs. hyphen).
>
> Ah yes, I noticed that too, as you see from the review I just sent through.
> Sorry, I didn't see the above post until after I'd sent the review in. It's
> no big deal though.
>
> > Jonathan, as this series is already long overdue, I suggest the
> > following course of action: please review these patches anyway and if
> > you find no other issues, please provide your Reviewed-by etc. as you
> > normally would. Once you do that, I will post v4 which will use
> > "fujitsu-laptop" for backlight device name and will contain your v3
> > review tags. Does that sound reasonable?
>
> As per my review comments, I am happy with the patch series so long as the
> backlight device name reverts to "fujitsu-laptop". Your suggested course of
> action sounds fine to me. Alternatively, I could quickly and easily add
> tags to a v4 post if that is seen to be more appropriate by Darren/Andy.
> I'm easy either way.
Sure, we can do it this way as well. I will post v4 shortly.
--
Best regards,
Michał Kępień