Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile

[Lyndon Sanche]

On Tue, Apr 30, 2024, at 4:31 AM, Ilpo Järvinen 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 = BIT(0),
>> +	DELL_COOL_BOTTOM = BIT(1),
>> +	DELL_QUIET = BIT(2),
>> +	DELL_PERFORMANCE = BIT(3),
>
> You need #include <linux/bits.h> for BIT().
>

Agreed.

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

I missed this.

>> +		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;
>
> I think you should propagate the error code differently from nothing 
> supported:
>
> 	if (ret < 0)
> 		return ret;
> 	if (!supported_modes)
> 		return 0;
>

Agreed.

Thank you for your feedback.

Lyndon