Received-SPF: pass (google.com: domain of linux-kernel+bounces-159136-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: i1719461a:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-386-g4cb8e397f9-fm-20240415.001-g4cb8e397
Precedence: bulk
MIME-Version: 1.0
Message-Id: <d31486b6-d71d-4985-adb1-2425f19b0b5c@app.fastmail.com>
In-Reply-To: <20240425201213.apwdxazbdbjddtpr@pali>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
 <20240425201213.apwdxazbdbjddtpr@pali>
Date: Thu, 25 Apr 2024 14:27:32 -0600
From: "Lyndon Sanche" <lsanche@lyndeno.ca>
To: =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Cc: "Matthew Garrett" <mjg59@srcf.ucam.org>,
 "Hans de Goede" <hdegoede@redhat.com>,
 =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <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
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 25, 2024, at 2:12 PM, Pali Roh=C3=A1r wrote:
> On Thursday 25 April 2024 11:27:57 Lyndon Sanche wrote:
>> +int thermal_init(void)
>> +{
>> +	int ret;
>> +	int supported_modes;
>> +
>> +	ret =3D thermal_get_supported_modes(&supported_modes);
>> +
>> +	if (ret !=3D 0 || supported_modes =3D=3D 0)
>> +		return -ENXIO;
>> +
>> +	thermal_handler =3D kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
>> +	if (!thermal_handler)
>> +		return -ENOMEM;
>> +	thermal_handler->profile_get =3D thermal_platform_profile_get;
>> +	thermal_handler->profile_set =3D 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 =3D {
>>  	.name =3D "platform::mute",
>>  	.max_brightness =3D 1,
>> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
>>  		mute_led_registered =3D true;
>>  	}
>> =20
>> +	// Do not fail module if thermal modes not supported,
>> +	// just skip
>> +	if (thermal_init() !=3D 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