Received-SPF: pass (google.com: domain of linux-kernel+bounces-164603-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Feedback-ID: i1719461a:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-416-g2c1796742e-fm-20240424.001-g2c179674
Precedence: bulk
MIME-Version: 1.0
Message-Id: <992f3688-fff9-4970-a5f4-103fd868551d@app.fastmail.com>
In-Reply-To: <6075bdd1-324c-ced7-4424-826b50767afc@linux.intel.com>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
 <20240429164844.7544-2-lsanche@lyndeno.ca>
 <6075bdd1-324c-ced7-4424-826b50767afc@linux.intel.com>
Date: Tue, 30 Apr 2024 12:38:35 -0600
From: "Lyndon Sanche" <lsanche@lyndeno.ca>
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: mario.limonciello@amd.com, pali@kernel.org, W_Armin@gmx.de,
 srinivas.pandruvada@linux.intel.com, "Matthew Garrett" <mjg59@srcf.ucam.org>,
 "Hans de Goede" <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org,
 LKML <linux-kernel@vger.kernel.org>, Dell.Client.Kernel@dell.com
Subject: Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 30, 2024, at 4:31 AM, Ilpo J=C3=A4rvinen wrote:
> On Mon, 29 Apr 2024, Lyndon Sanche wrote:
>> + */
>> +
>> +#define DELL_ACC_GET_FIELD GENMASK(19, 16)
>> +#define DELL_ACC_SET_FIELD GENMASK(11, 8)
>> +#define DELL_THERMAL_SUPPORTED GENMASK(3, 0)
>
> Please align these with tabs.
>

Agreed.

>> +enum thermal_mode_bits {
>> +	DELL_BALANCED =3D BIT(0),
>> +	DELL_COOL_BOTTOM =3D BIT(1),
>> +	DELL_QUIET =3D BIT(2),
>> +	DELL_PERFORMANCE =3D BIT(3),
>
> You need #include <linux/bits.h> for BIT().
>

Agreed.

>> +	}
>> +}
>> +
>> +static int thermal_platform_profile_get(struct platform_profile_hand=
ler *pprof,
>> +					enum platform_profile_option *profile)
>> +{
>> +	int ret =3D thermal_get_mode();
>> +
>> +	if (ret < 0)
>
> I think I already mentioned about this, change to:
>
> 	int ret;
>
> 	ret =3D thermal_get_mode();
> 	if (ret < 0)
>

I missed this.

>> +		return ret;
>> +
>> +	switch (ret) {
>> +	case DELL_BALANCED:
>> +		*profile =3D PLATFORM_PROFILE_BALANCED;
>> +		break;
>> +	case DELL_PERFORMANCE:
>> +		*profile =3D PLATFORM_PROFILE_PERFORMANCE;
>> +		break;
>> +	case DELL_COOL_BOTTOM:
>> +		*profile =3D PLATFORM_PROFILE_COOL;
>> +		break;
>> +	case DELL_QUIET:
>> +		*profile =3D PLATFORM_PROFILE_QUIET;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int thermal_init(void)
>> +{
>> +	int ret;
>> +	int supported_modes;
>> +
>> +	ret =3D thermal_get_supported_modes(&supported_modes);
>> +	if (ret || !supported_modes)
>> +		return 0;
>
> I think you should propagate the error code differently from nothing=20
> supported:
>
> 	if (ret < 0)
> 		return ret;
> 	if (!supported_modes)
> 		return 0;
>

Agreed.

Thank you for your feedback.

Lyndon