Re: [PATCH v2] platform/x86: dell-laptop: Implement platform_profile

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Fri, Apr 26 2024 at 12:23:00 PM +03:00:00, Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
On Thu, 25 Apr 2024, Lyndon Sanche wrote:

 Some Dell laptops support configuration of preset
 fan modes through smbios tables.

 If the platform supports these fan modes, set up
 platform_profile to change these modes. If not
 supported, skip enabling platform_profile.

 Signed-off-by: Lyndon Sanche <lsanche@xxxxxxxxxx>
 ---

Two things:
- You're missing patch version history (put it below the --- line)
- Don't send updates so soon, give people time to comment. When I saw v1
  for the first time, you had already posted the next version.

 +void thermal_cleanup(void)
 +{
 +	platform_profile_remove();
 +	kfree(thermal_handler);
 +}
 +
  static struct led_classdev mute_led_cdev = {
  	.name = "platform::mute",
  	.max_brightness = 1,
 @@ -2238,6 +2452,12 @@ static int __init dell_init(void)
  		goto fail_rfkill;
  	}

 +	// Do not fail module if thermal modes not supported,
 +	// just skip
 +	ret = thermal_init();
 +	if (ret)
 +		goto fail_thermal;
 +
  	if (quirks && quirks->touchpad_led)
  		touchpad_led_init(&platform_device->dev);

 @@ -2317,6 +2537,8 @@ static int __init dell_init(void)
  		led_classdev_unregister(&mute_led_cdev);
  fail_led:
  	dell_cleanup_rfkill();
 +fail_thermal:
 +	thermal_cleanup();
  fail_rfkill:
  	platform_device_del(platform_device);
  fail_platform_device2:
 @@ -2344,6 +2566,7 @@ static void __exit dell_exit(void)
  		platform_device_unregister(platform_device);
  		platform_driver_unregister(&platform_driver);
  	}
 +	thermal_cleanup();

This is still not right, you'll still platform_profile_remove() even if
the init side call failed.

--
 i.


Thank you for your feedback. I agree with your comments and will add more checking on whether certain cleanup actions are necessary.

Lyndon







[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux