fujitsu_init() cleanup
Subsystems (2)
v4 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 v3:
- Do not use KBUILD_MODNAME as backlight device name as it breaks userspace interface ("fujitsu-laptop" vs. "fujitsu_laptop").
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 v4 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.
--- 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 ("fujitsu-laptop" ,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
PATCH v4 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 v4 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 v4 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
Quoted 29 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 v3:
- Do not use
KBUILD_MODNAMEas backlight device name as it breaks userspace interface ("fujitsu-laptop" vs. "fujitsu_laptop").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(-)
As noted, this V4 patch series is identical to V3 except for the backlight device name fix. No regressions are evident on the hardware I have (S7020). I am therefore 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
Quoted 12 lines · Jonathan Woithe
Quoted 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.As noted, this V4 patch series is identical to V3 except for the backlight device name fix. No regressions are evident on the hardware I have (S7020). I am therefore 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.
Quoted 2 lines
Tested-by Jonathan Woithe <[email protected]>Reviewed-by Jonathan Woithe <[email protected]>
Pushed to testing, thanks!
Signature · 2 lines
With Best Regards, Andy Shevchenko