platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lyndon Sanche <lsanche@lyndeno.ca>
To: Armin Wolf <W_Armin@gmx.de>
Cc: "Matthew Garrett" <mjg59@srcf.ucam.org>,
	"Pali Rohár" <pali@kernel.org>,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, Dell.Client.Kernel@dell.com
Subject: Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile
Date: Thu, 25 Apr 2024 18:54:03 -0600	[thread overview]
Message-ID: <3IYICS.H9INGKTE6BW73@lyndeno.ca> (raw)
In-Reply-To: <7a215986-4702-443a-9e1d-a2fb1172276a@gmx.de>



On Thu, Apr 25 2024 at 11:07:22 PM +02:00:00, Armin Wolf 
<W_Armin@gmx.de> wrote:
> Am 25.04.24 um 19:27 schrieb Lyndon Sanche:
>> 
>> +//         1  AAC mode enabled
>> +//
>> +//     If AAC Configuration Type is USTT mode specific (multiple 
>> bits may be set for this parameter),
> 
> Hi,
> 
> checkpatch reports: WARNING: line length of 101 exceeds 100 columns

I can wrap this last line.

> 
>> +//         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 0;
> 
> This would return DELL_BALANCED if no option is set. Please return an 
> appropriate error code.

Thanks for catching this, I will return a proper error code.

> 
>> +}
>> +
>> +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)
>> +{
>> +	int ret;
>> +
>> +	switch (profile) {
>> +	case PLATFORM_PROFILE_BALANCED:
>> +		ret = thermal_set_mode(DELL_BALANCED);
>> +		break;
> 
> Maybe using "return thermal_set_mode()" would be better in this cases.

Good idea, I can clean up the code with this suggestion.

> 
>> +	case PLATFORM_PROFILE_PERFORMANCE:
>> +		ret = thermal_set_mode(DELL_PERFORMANCE);
>> +		break;
>> +	case PLATFORM_PROFILE_QUIET:
>> +		ret = thermal_set_mode(DELL_QUIET);
>> +		break;
>> +	case PLATFORM_PROFILE_COOL:
>> +		ret = thermal_set_mode(DELL_COOL_BOTTOM);
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int thermal_platform_profile_get(struct 
>> platform_profile_handler *pprof,
>> +					enum platform_profile_option *profile)
>> +{
>> +	switch (thermal_get_mode()) {
> 
> Please check if thermal_get_mode() returned an error code and return 
> it in this case.

Will add error checking.

>> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
>> +
>> +	platform_profile_register(thermal_handler);
>> +
>> +	return 0;
> 
> Please check the return value of platform_profile_register() and 
> return the error if this function fails,
> see commit fe0e04cf66a1 ("platform/surface: platform_profile: 
> Propagate error if profile registration fails")
> for an explanation.

Thank you for catching this. I forgot to handle the return value.

> 
>> +}
>> +
>> +void thermal_cleanup(void)
>> +{
>> +	platform_profile_remove();
>> +	kfree(thermal_handler);
>> +}
>> +
>>   static struct led_classdev mute_led_cdev = {
>>   	.name = "platform::mute",
>>   	.max_brightness = 1,
>> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
>>   		mute_led_registered = true;
>>   	}
>> 
>> +	// Do not fail module if thermal modes not supported,
>> +	// just skip
>> +	if (thermal_init() != 0)
>> +		pr_warn("Unable to setup platform_profile, skipping");
>> +
>>   	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>>   		return 0;
>> 
>> @@ -2344,6 +2563,7 @@ static void __exit dell_exit(void)
>>   		platform_device_unregister(platform_device);
>>   		platform_driver_unregister(&platform_driver);
>>   	}
>> +	thermal_cleanup();
> 
> Should only be called when thermal_init() was successful.

I do not believe it is incorrect to skip checking for this.

platform_profile_remove() does not return anything and does not panic 
when a profile is not currently registered. My understanding from 
reading the source is it handles the case of no profile gracefully. 
Please let me know if my understanding is incorrect however.

kfree does not care of the thermal handler is allocated or not. Please 
let me know if calling kfree on NULL pointers is poor form for this 
application.

Thank you for your feedback, I do appreciate it.

Lyndon



  reply	other threads:[~2024-04-26  0:54 UTC|newest]

Thread overview: 80+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-25 17:27 [PATCH] platform/x86: dell-laptop: Implement platform_profile Lyndon Sanche
2024-04-25 20:07 ` Mario Limonciello
2024-04-25 20:24   ` Lyndon Sanche
2024-04-25 20:28     ` Mario Limonciello
2024-04-25 21:51       ` Srinivas Pandruvada
2024-04-26  0:38         ` Lyndon Sanche
2024-04-26 16:14     ` srinivas pandruvada
2024-04-26 18:23       ` Lyndon Sanche
2024-04-26 18:24         ` srinivas pandruvada
2024-04-25 20:12 ` Pali Rohár
2024-04-25 20:27   ` Lyndon Sanche
2024-04-25 20:31     ` Pali Rohár
2024-04-25 21:07 ` Armin Wolf
2024-04-26  0:54   ` Lyndon Sanche [this message]
2024-04-26  2:04 ` [PATCH v2] " Lyndon Sanche
2024-04-26  9:23   ` Ilpo Järvinen
2024-04-26 18:05     ` Lyndon Sanche
2024-05-13 20:09   ` kernel test robot
2024-04-26  6:57 ` [PATCH] " Ilpo Järvinen
2024-04-29 16:48 ` [PATCH v3] " Lyndon Sanche
2024-04-29 17:45   ` Mario Limonciello
2024-04-29 17:51     ` Mario Limonciello
2024-04-29 21:25       ` Lyndon Sanche
2024-04-29 21:21     ` Lyndon Sanche
2024-04-30 10:31   ` Ilpo Järvinen
2024-04-30 18:38     ` Lyndon Sanche
2024-04-30 15:36   ` kernel test robot
2024-05-01  8:16   ` kernel test robot
2024-05-01 16:35   ` kernel test robot
2024-05-01 17:07   ` kernel test robot
2024-05-01  1:14 ` [PATCH v4] " Lyndon Sanche
2024-05-01  1:36   ` Pali Rohár
2024-05-01  1:42     ` Lyndon Sanche
2024-05-01 21:58 ` [PATCH v5] " Lyndon Sanche
2024-05-03 10:19   ` kernel test robot
2024-05-04  1:03     ` Lyndon Sanche
2024-05-06 10:18       ` Hans de Goede
2024-05-07 16:00         ` Lyndon Sanche
2024-05-03 21:19   ` Armin Wolf
2024-05-04  0:59     ` Lyndon Sanche
2024-05-08 14:24   ` Shen, Yijun
2024-05-08 15:53     ` Mario Limonciello
2024-05-11 15:05       ` Shen, Yijun
2024-05-11 15:12         ` Limonciello, Mario
2024-05-11 15:56           ` Shen, Yijun
2024-05-12 17:53             ` Armin Wolf
2024-05-12 17:58               ` Limonciello, Mario
2024-05-12 18:47                 ` Armin Wolf
2024-05-12 22:14                   ` Limonciello, Mario
2024-05-11 16:02         ` Lyndon Sanche
2024-05-09 15:10     ` Lyndon Sanche
2024-05-11  1:49       ` Lyndon Sanche
2024-05-11 15:22         ` Shen, Yijun
2024-05-11 15:54           ` Lyndon Sanche
2024-05-11 16:12             ` Shen, Yijun
2024-05-11  2:36 ` [PATCH v6 0/2] " Lyndon Sanche
2024-05-11  2:36 ` [PATCH v6 1/2] platform/x86: dell-smbios: Add helper for checking supported commands Lyndon Sanche
2024-05-11 15:13   ` Limonciello, Mario
2024-05-12 18:00   ` Armin Wolf
2024-05-11  2:36 ` [PATCH v6 2/2] platform/x86: dell-laptop: Implement platform_profile Lyndon Sanche
2024-05-11 15:16   ` Limonciello, Mario
2024-05-11 15:59     ` Lyndon Sanche
     [not found]       ` <48JCDS.E4RT1F9DTKFU1@lyndeno.ca>
2024-05-12  1:43         ` Limonciello, Mario
2024-05-12 15:25           ` Hans de Goede
2024-05-12  0:14     ` Lyndon Sanche
2024-05-12 18:05   ` Armin Wolf
2024-05-15 17:06     ` Lyndon Sanche
2024-05-17 22:42 ` [PATCH v7 0/3] platform/x86: dell: " Lyndon Sanche
2024-05-17 22:42   ` [PATCH v7 1/3] platform/x86: dell-smbios: Add helper for checking supported class Lyndon Sanche
2024-05-17 22:42   ` [PATCH v7 2/3] platform/x86: dell-smbios: Move request functions for reuse Lyndon Sanche
2024-05-17 22:42   ` [PATCH v7 3/3] platform/x86: dell-pc: Implement platform_profile Lyndon Sanche
2024-05-27  9:39     ` Ilpo Järvinen
2024-05-28 16:16       ` Lyndon Sanche
2024-05-21 20:50   ` [PATCH v7 0/3] platform/x86: dell: " Mario Limonciello
2024-05-29 17:47 ` [PATCH v8 " Lyndon Sanche
2024-05-29 17:47   ` [PATCH v8 1/3] platform/x86: dell-smbios: Add helper for checking supported class Lyndon Sanche
2024-05-29 17:47   ` [PATCH v8 2/3] platform/x86: dell-smbios: Move request functions for reuse Lyndon Sanche
2024-05-29 17:47   ` [PATCH v8 3/3] platform/x86: dell-pc: Implement platform_profile Lyndon Sanche
2024-05-30  8:39     ` Ilpo Järvinen
2024-05-31  1:19       ` Lyndon Sanche

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3IYICS.H9INGKTE6BW73@lyndeno.ca \
    --to=lsanche@lyndeno.ca \
    --cc=Dell.Client.Kernel@dell.com \
    --cc=W_Armin@gmx.de \
    --cc=hdegoede@redhat.com \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mjg59@srcf.ucam.org \
    --cc=pali@kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).