On Thu, Apr 25, 2024, at 2:12 PM, Pali Rohár wrote:
> On Thursday 25 April 2024 11:27:57 Lyndon Sanche wrote:
>> +int thermal_init(void)
>> +{
>> + int ret;
>> + int supported_modes;
>> +
>> + ret = thermal_get_supported_modes(&supported_modes);
>> +
>> + if (ret != 0 || supported_modes == 0)
>> + return -ENXIO;
>> +
>> + 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);
>> +
>> + platform_profile_register(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,
>> @@ -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");
>
> I think that -ENOMEM error should be failure of the loading the driver.
> It does not make sense to continue of memory allocation failed.
>
> On the other hand when the thermal modes are not support (e.g. old
> Latitude models) then there should not be a warning message. It is
> expected that on systems without thermal modes the setup fails.
Thank you for your feedback.
I agree with your suggestion. -ENOMEM would indicate something bigger is amiss. I can add a check:
If -ENOMEM, fail driver.
If anything other error, skip, but do not show a message.
Lyndon
