fujitsu_init() cleanup
Subsystems (2)
v3 cover fujitsu_init() cleanup
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(-)
Signature · 1 line
2.12.0
PATCH v3 1/4 platform/x86: fujitsu-laptop: register backlight device in a separate function
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.
--- drivers/platform/x86/fujitsu-laptop.c | 38 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 16 deletions(-)
drivers/platform/x86/fujitsu-laptop.c +22 -16
@@ -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);
Signature · 1 line
2.12.0
Re: [PATCH v3 1/4] platform/x86: fujitsu-laptop: register backlight device in a separate function
Quoted 20 lines
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. UseKBUILD_MODNAMEas 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(-)drivers/platform/x86/fujitsu-laptop.c +1
-0@@ -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 )
drivers/platform/x86/fujitsu-laptop.c static const struct dmi_system_id fujitsu_dmi_table[] __initconst = { · line 698
Quoted 10 lines
drivers/platform/x86/fujitsu-laptop.c +10
-0+{ +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?
Signature · 2 lines
Best regards, Michał Kępień
Jonathan Woithe
maintainerRe: [PATCH v3 1/4] platform/x86: fujitsu-laptop: register backlight device in a separate function
Hi Michael
Quoted 19 lines · Micha?? K??pie??
Quoted 19 lines
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. UseKBUILD_MODNAMEas 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(-)drivers/platform/x86/fujitsu-laptop.c +0
-0@@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {/* ACPI device for LCD brightness control */
Quoted 15 lines
drivers/platform/x86/fujitsu-laptop.c static const struct dmi_system_id fujitsu_dmi_table[] __initconst = { · line 698
Quoted 11 lines
drivers/platform/x86/fujitsu-laptop.c +11
-0+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_MODNAMEis "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.
Quoted 6 lines
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
Re: [PATCH v3 1/4] platform/x86: fujitsu-laptop: register backlight device in a separate function
Quoted 49 lines
Hi Michael
Quoted 19 lines · Micha?? K??pie??
Quoted 19 lines
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. UseKBUILD_MODNAMEas 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(-)drivers/platform/x86/fujitsu-laptop.c +0
-0@@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {/* ACPI device for LCD brightness control */ Quoted 15 lines
drivers/platform/x86/fujitsu-laptop.c static const struct dmi_system_id fujitsu_dmi_table[] __initconst = { · line 698
Quoted 11 lines
drivers/platform/x86/fujitsu-laptop.c +11
-0+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_MODNAMEis "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.
Quoted 6 lines
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?
Quoted 5 lines
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.
Signature · 2 lines
Best regards, Michał Kępień
PATCH v3 2/4 platform/x86: fujitsu-laptop: sync backlight power status in acpi_fujitsu_laptop_add()
In reply to Michał Kępień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.
--- drivers/platform/x86/fujitsu-laptop.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/platform/x86/fujitsu-laptop.c +8 -8
@@ -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 ;
Signature · 1 line
2.12.0
PATCH v3 3/4 platform/x86: fujitsu-laptop: only register backlight device if FUJ02B1 is present
In reply to Michał Kępień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.
--- drivers/platform/x86/fujitsu-laptop.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
drivers/platform/x86/fujitsu-laptop.c +10 -14
@@ -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);
Signature · 1 line
2.12.0
PATCH v3 4/4 platform/x86: fujitsu-laptop: cleanup error labels in fujitsu_init()
In reply to Michał Kępień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.
--- drivers/platform/x86/fujitsu-laptop.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/platform/x86/fujitsu-laptop.c +14 -14
@@ -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;
Signature · 1 line
2.12.0
Jonathan Woithe
maintainerQuoted 3 lines · Micha?? K??pie??
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
Quoted 24 lines · Micha?? K??pie??
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).
Quoted 8 lines
- 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.
Regards
jonathan