diff mbox series

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

Message ID 20240426020448.10862-1-lsanche@lyndeno.ca (mailing list archive)
State Changes Requested, archived
Headers show
Series [v2] platform/x86: dell-laptop: Implement platform_profile | expand

Commit Message

Lyndon Sanche April 26, 2024, 2:04 a.m. UTC
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@lyndeno.ca>
---
 drivers/platform/x86/dell/dell-laptop.c | 223 ++++++++++++++++++++++++
 drivers/platform/x86/dell/dell-smbios.h |   1 +
 2 files changed, 224 insertions(+)

Comments

Ilpo Järvinen April 26, 2024, 9:23 a.m. UTC | #1
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@lyndeno.ca>
> ---

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.
Lyndon Sanche April 26, 2024, 6:05 p.m. UTC | #2
On Fri, Apr 26 2024 at 12:23:00 PM +03:00:00, Ilpo Järvinen 
<ilpo.jarvinen@linux.intel.com> 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@lyndeno.ca>
>>  ---
> 
> 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
kernel test robot May 13, 2024, 8:09 p.m. UTC | #3
Hi Lyndon,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.9 next-20240513]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lyndon-Sanche/platform-x86-dell-laptop-Implement-platform_profile/20240511-144534
base:   linus/master
patch link:    https://lore.kernel.org/r/20240426020448.10862-1-lsanche%40lyndeno.ca
patch subject: [PATCH v2] platform/x86: dell-laptop: Implement platform_profile
config: i386-randconfig-005-20240513 (https://download.01.org/0day-ci/archive/20240514/202405140348.Wz7gOmdP-lkp@intel.com/config)
compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240514/202405140348.Wz7gOmdP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405140348.Wz7gOmdP-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/platform/x86/dell/dell-laptop.o: in function `thermal_init':
>> drivers/platform/x86/dell/dell-laptop.c:2404:(.text+0x21a3): undefined reference to `platform_profile_register'
   ld: drivers/platform/x86/dell/dell-laptop.o: in function `thermal_cleanup':
>> drivers/platform/x86/dell/dell-laptop.c:2412:(.text+0x21cc): undefined reference to `platform_profile_remove'
>> ld: drivers/platform/x86/dell/dell-laptop.c:2412:(.init.text+0xb44): undefined reference to `platform_profile_remove'
>> ld: drivers/platform/x86/dell/dell-laptop.c:2412:(.exit.text+0x7c): undefined reference to `platform_profile_remove'


vim +2404 drivers/platform/x86/dell/dell-laptop.c

  2377	
  2378	int thermal_init(void)
  2379	{
  2380		int ret;
  2381		int supported_modes;
  2382	
  2383		ret = thermal_get_supported_modes(&supported_modes);
  2384	
  2385		if (ret || !supported_modes)
  2386			return 0;
  2387	
  2388		thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
  2389		if (!thermal_handler)
  2390			return -ENOMEM;
  2391		thermal_handler->profile_get = thermal_platform_profile_get;
  2392		thermal_handler->profile_set = thermal_platform_profile_set;
  2393	
  2394		if ((supported_modes >> DELL_QUIET) & 1)
  2395			set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
  2396		if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
  2397			set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
  2398		if ((supported_modes >> DELL_BALANCED) & 1)
  2399			set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
  2400		if ((supported_modes >> DELL_PERFORMANCE) & 1)
  2401			set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
  2402	
  2403		// Clean up but do not fail
> 2404		if (platform_profile_register(thermal_handler))
  2405			kfree(thermal_handler);
  2406	
  2407		return 0;
  2408	}
  2409	
  2410	void thermal_cleanup(void)
  2411	{
> 2412		platform_profile_remove();
  2413		kfree(thermal_handler);
  2414	}
  2415
diff mbox series

Patch

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 42f7de2b4522..ff67bc7697b1 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -27,6 +27,7 @@ 
 #include <linux/i8042.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/platform_profile.h>
 #include <acpi/video.h>
 #include "dell-rbtn.h"
 #include "dell-smbios.h"
@@ -95,10 +96,18 @@  static struct backlight_device *dell_backlight_device;
 static struct rfkill *wifi_rfkill;
 static struct rfkill *bluetooth_rfkill;
 static struct rfkill *wwan_rfkill;
+static struct platform_profile_handler *thermal_handler;
 static bool force_rfkill;
 static bool micmute_led_registered;
 static bool mute_led_registered;
 
+enum thermal_mode_bits {
+	DELL_BALANCED = 0,
+	DELL_COOL_BOTTOM = 1,
+	DELL_QUIET = 2,
+	DELL_PERFORMANCE = 3,
+};
+
 module_param(force_rfkill, bool, 0444);
 MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
 
@@ -2199,6 +2208,211 @@  static int mute_led_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
+// Derived from smbios-thermal-ctl
+//
+// cbClass 17
+// cbSelect 19
+// User Selectable Thermal Tables(USTT)
+// cbArg1 determines the function to be performed
+// cbArg1 0x0 = Get Thermal Information
+//  cbRES1         Standard return codes (0, -1, -2)
+//  cbRES2, byte 0  Bitmap of supported thermal modes. A mode is supported if its bit is set to 1
+//     Bit 0 Balanced
+//     Bit 1 Cool Bottom
+//     Bit 2 Quiet
+//     Bit 3 Performance
+//  cbRES2, byte 1 Bitmap of supported Active Acoustic Controller (AAC) modes. Each mode
+//                 corresponds to the supported thermal modes in byte 0. A mode is supported if
+//                 its bit is set to 1.
+//     Bit 0 AAC (Balanced)
+//     Bit 1 AAC (Cool Bottom
+//     Bit 2 AAC (Quiet)
+//     Bit 3 AAC (Performance)
+//  cbRes3, byte 0 Current Thermal Mode
+//     Bit 0 Balanced
+//     Bit 1 Cool Bottom
+//     Bit 2 Quiet
+//     Bit 3 Performanc
+//  cbRes3, byte 1  AAC Configuration type
+//          0       Global (AAC enable/disable applies to all supported USTT modes)
+//          1       USTT mode specific
+//  cbRes3, byte 2  Current Active Acoustic Controller (AAC) Mode
+//     If AAC Configuration Type is Global,
+//          0       AAC mode disabled
+//          1       AAC mode enabled
+//     If AAC Configuration Type is USTT mode specific (multiple bits may be set),
+//          Bit 0 AAC (Balanced)
+//          Bit 1 AAC (Cool Bottom
+//          Bit 2 AAC (Quiet)
+//          Bit 3 AAC (Performance)
+//  cbRes3, byte 3  Current Fan Failure Mode
+//     Bit 0 Minimal Fan Failure (at least one fan has failed, one fan working)
+//     Bit 1 Catastrophic Fan Failure (all fans have failed)
+//  cbArg1 0x1   (Set Thermal Information), both desired thermal mode and
+//               desired AAC mode shall be applied
+//  cbArg2, byte 0  Desired Thermal Mode to set (only one bit may be set for this parameter)
+//     Bit 0 Balanced
+//     Bit 1 Cool Bottom
+//     Bit 2 Quiet
+//     Bit 3 Performance
+//  cbArg2, byte 1  Desired Active Acoustic Controller (AAC) Mode to set
+//     If AAC Configuration Type is Global,
+//         0  AAC mode disabled
+//         1  AAC mode enabled
+//
+//     If AAC Configuration Type is USTT mode specific
+//     (multiple bits may be set for this parameter),
+//         Bit 0 AAC (Balanced)
+//         Bit 1 AAC (Cool Bottom
+//         Bit 2 AAC (Quiet)
+//         Bit 3 AAC (Performance)
+static int thermal_get_mode(void)
+{
+	struct calling_interface_buffer buffer;
+	int state;
+	int ret;
+
+	dell_fill_request(&buffer, 0x0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	if (ret)
+		return ret;
+	state = buffer.output[2];
+	if ((state >> DELL_BALANCED) & 1)
+		return DELL_BALANCED;
+	else if ((state >> DELL_COOL_BOTTOM) & 1)
+		return DELL_COOL_BOTTOM;
+	else if ((state >> DELL_QUIET) & 1)
+		return DELL_QUIET;
+	else if ((state >> DELL_PERFORMANCE) & 1)
+		return DELL_PERFORMANCE;
+	else
+		return -ENXIO;
+}
+
+static int thermal_get_supported_modes(int *supported_bits)
+{
+	struct calling_interface_buffer buffer;
+	int ret;
+
+	dell_fill_request(&buffer, 0x0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	if (ret)
+		return ret;
+	*supported_bits = buffer.output[1] & 0xF;
+	return 0;
+}
+
+static int thermal_get_acc_mode(int *acc_mode)
+{
+	struct calling_interface_buffer buffer;
+	int ret;
+
+	dell_fill_request(&buffer, 0x0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	if (ret)
+		return ret;
+	*acc_mode = ((buffer.output[3] >> 8) & 0xFF);
+	return 0;
+}
+
+static int thermal_set_mode(enum thermal_mode_bits state)
+{
+	struct calling_interface_buffer buffer;
+	int ret;
+	int acc_mode;
+
+	ret = thermal_get_acc_mode(&acc_mode);
+	if (ret)
+		return ret;
+
+	dell_fill_request(&buffer, 0x1, (acc_mode << 8) | BIT(state), 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	return ret;
+}
+
+static int thermal_platform_profile_set(struct platform_profile_handler *pprof,
+					enum platform_profile_option profile)
+{
+	switch (profile) {
+	case PLATFORM_PROFILE_BALANCED:
+		return thermal_set_mode(DELL_BALANCED);
+	case PLATFORM_PROFILE_PERFORMANCE:
+		return thermal_set_mode(DELL_PERFORMANCE);
+	case PLATFORM_PROFILE_QUIET:
+		return thermal_set_mode(DELL_QUIET);
+	case PLATFORM_PROFILE_COOL:
+		return thermal_set_mode(DELL_COOL_BOTTOM);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
+					enum platform_profile_option *profile)
+{
+	int ret = thermal_get_mode();
+
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case DELL_BALANCED:
+		*profile = PLATFORM_PROFILE_BALANCED;
+		break;
+	case DELL_PERFORMANCE:
+		*profile = PLATFORM_PROFILE_PERFORMANCE;
+		break;
+	case DELL_COOL_BOTTOM:
+		*profile = PLATFORM_PROFILE_COOL;
+		break;
+	case DELL_QUIET:
+		*profile = PLATFORM_PROFILE_QUIET;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int thermal_init(void)
+{
+	int ret;
+	int supported_modes;
+
+	ret = thermal_get_supported_modes(&supported_modes);
+
+	if (ret || !supported_modes)
+		return 0;
+
+	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
+	if (!thermal_handler)
+		return -ENOMEM;
+	thermal_handler->profile_get = thermal_platform_profile_get;
+	thermal_handler->profile_set = thermal_platform_profile_set;
+
+	if ((supported_modes >> DELL_QUIET) & 1)
+		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
+	if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
+		set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
+	if ((supported_modes >> DELL_BALANCED) & 1)
+		set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
+	if ((supported_modes >> DELL_PERFORMANCE) & 1)
+		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
+
+	// Clean up but do not fail
+	if (platform_profile_register(thermal_handler))
+		kfree(thermal_handler);
+
+	return 0;
+}
+
+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();
 }
 
 /* dell-rbtn.c driver export functions which will not work correctly (and could
diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h
index eb341bf000c6..585d042f1779 100644
--- a/drivers/platform/x86/dell/dell-smbios.h
+++ b/drivers/platform/x86/dell/dell-smbios.h
@@ -19,6 +19,7 @@ 
 /* Classes and selects used only in kernel drivers */
 #define CLASS_KBD_BACKLIGHT 4
 #define SELECT_KBD_BACKLIGHT 11
+#define SELECT_THERMAL_MANAGEMENT 19
 
 /* Tokens used in kernel drivers, any of these
  * should be filtered from userspace access