[v3] alienware-wmi: Dell AWCC platform_profile support

Message ID	20241008195642.36677-2-kuurtb@gmail.com (mailing list archive)
State	Changes Requested, archived
Headers	show Received: from mail-pf1-f170.google.com (mail-pf1-f170.google.com [209.85.210.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 16921136982; Tue, 8 Oct 2024 19:58:15 +0000 (UTC) From: Kurt Borja <kuurtb@gmail.com> To: kuurtb@gmail.com Cc: hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, Dell.Client.Kernel@dell.com Subject: [PATCH v3] alienware-wmi: Dell AWCC platform_profile support Date: Tue, 8 Oct 2024 16:56:43 -0300 Message-ID: <20241008195642.36677-2-kuurtb@gmail.com> In-Reply-To: <20241007093324.49631-3-kuurtb@gmail.com> References: <20241007093324.49631-3-kuurtb@gmail.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	[v3] alienware-wmi: Dell AWCC platform_profile support \| expand [v3] alienware-wmi: Dell AWCC platform_profile support

Kurt Borja Oct. 8, 2024, 7:56 p.m. UTC

This patch adds platform_profile support for Dell devices which implement
User Selectable Thermal Tables (USTT) that are meant to be controlled by
Alienware Command Center (AWCC). These devices may include newer Alienware
M-Series, Alienware X-Series and Dell's G-Series. This patch, was tested
by me on an Alienware x15 R1.

It is suspected that Alienware Command Center manages thermal profiles
through the WMI interface, specifically through a device with identifier
\_SB_.AMW1.WMAX. This device was reverse engineered and the relevant
functionality is documented here [1]. This driver interacts with this
WMI device and thus is able to mimic AWCC's thermal profiles functionality
through the platform_profile API. In consequence the user would be able
to set and retrieve thermal profiles, which are just fan speed profiles.

This driver was heavily inspired on inspur_platform_profile, special
thanks.

Notes:
 - Performance (FullSpeed) profile is a special profile which has it's own
   entry in the Firmware Settings of the Alienware x15 R1. It also changes
   the color of the F1 key. I suspect this behavior would be replicated in
   other X-Series or M-Series laptops.
 - G-Mode is a profile documented on [1] which mimics the behavior of
   FullSpeed mode but it does not have an entry on the Firmware Settings of
   the Alienware x15 R1, this may correspond to the G-Mode functionality on
   G-Series laptops (activated by a special button) but I cannot test it. I
   did not include this code in the driver as G-Mode causes unexpected
   behavior on X-Series laptops.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>

---
v3:
 - Removed extra empty line
 - 0x0B named WMAX_ARG_GET_CURRENT_PROF
 - Removed casts to the same type on functions added in this patch
 - Thermal profile to WMAX argument is now an static function and makes
   use of in-built kernel macros
 - Platform profile is now removed only if it was created first
 - create_platform_profile is now create_thermal_profile to avoid
   confusion
 - profile_get and profile_set functions renamed too to match the above
v2:
 - Moved functionality to alienware-wmi driver
 - Added thermal and gmode quirks to add support based on dmi match
 - Performance profile is now GMODE for devices that support it
 - alienware_wmax_command now is insize agnostic to support new thermal
   methods
---
 drivers/platform/x86/dell/Kconfig         |   1 +
 drivers/platform/x86/dell/alienware-wmi.c | 238 ++++++++++++++++++++--
 2 files changed, 226 insertions(+), 13 deletions(-)

Armin Wolf Oct. 9, 2024, 8:42 a.m. UTC | #1

Am 08.10.24 um 21:56 schrieb Kurt Borja:

> This patch adds platform_profile support for Dell devices which implement
> User Selectable Thermal Tables (USTT) that are meant to be controlled by
> Alienware Command Center (AWCC). These devices may include newer Alienware
> M-Series, Alienware X-Series and Dell's G-Series. This patch, was tested
> by me on an Alienware x15 R1.
>
> It is suspected that Alienware Command Center manages thermal profiles
> through the WMI interface, specifically through a device with identifier
> \_SB_.AMW1.WMAX. This device was reverse engineered and the relevant
> functionality is documented here [1]. This driver interacts with this
> WMI device and thus is able to mimic AWCC's thermal profiles functionality
> through the platform_profile API. In consequence the user would be able
> to set and retrieve thermal profiles, which are just fan speed profiles.

Can you write a short piece of documentation at Documentation/wmi/devices/
to describe the Alienware WMI interface? This would be helpful for future developers.

> This driver was heavily inspired on inspur_platform_profile, special
> thanks.
>
> Notes:
>   - Performance (FullSpeed) profile is a special profile which has it's own
>     entry in the Firmware Settings of the Alienware x15 R1. It also changes
>     the color of the F1 key. I suspect this behavior would be replicated in
>     other X-Series or M-Series laptops.
>   - G-Mode is a profile documented on [1] which mimics the behavior of
>     FullSpeed mode but it does not have an entry on the Firmware Settings of
>     the Alienware x15 R1, this may correspond to the G-Mode functionality on
>     G-Series laptops (activated by a special button) but I cannot test it. I
>     did not include this code in the driver as G-Mode causes unexpected
>     behavior on X-Series laptops.
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>
> ---
> v3:
>   - Removed extra empty line
>   - 0x0B named WMAX_ARG_GET_CURRENT_PROF
>   - Removed casts to the same type on functions added in this patch
>   - Thermal profile to WMAX argument is now an static function and makes
>     use of in-built kernel macros
>   - Platform profile is now removed only if it was created first
>   - create_platform_profile is now create_thermal_profile to avoid
>     confusion
>   - profile_get and profile_set functions renamed too to match the above
> v2:
>   - Moved functionality to alienware-wmi driver
>   - Added thermal and gmode quirks to add support based on dmi match
>   - Performance profile is now GMODE for devices that support it
>   - alienware_wmax_command now is insize agnostic to support new thermal
>     methods
> ---
>   drivers/platform/x86/dell/Kconfig         |   1 +
>   drivers/platform/x86/dell/alienware-wmi.c | 238 ++++++++++++++++++++--
>   2 files changed, 226 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index 68a49788a..b06d634cd 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -21,6 +21,7 @@ config ALIENWARE_WMI
>   	depends on LEDS_CLASS
>   	depends on NEW_LEDS
>   	depends on ACPI_WMI
> +	select ACPI_PLATFORM_PROFILE
>   	help
>   	 This is a driver for controlling Alienware BIOS driven
>   	 features.  It exposes an interface for controlling the AlienFX
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index f5ee62ce1..e3ef4b10b 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -10,6 +10,7 @@
>   #include <linux/acpi.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> +#include <linux/platform_profile.h>
>   #include <linux/dmi.h>
>   #include <linux/leds.h>
>
> @@ -25,6 +26,10 @@
>   #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>   #define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
>   #define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
> +#define WMAX_METHOD_THERMAL_INFORMATION	0x14
> +#define WMAX_METHOD_THERMAL_CONTROL	0x15
> +
> +#define WMAX_ARG_GET_CURRENT_PROF	0x0B
>
>   MODULE_AUTHOR("Mario Limonciello <mario.limonciello@outlook.com>");
>   MODULE_DESCRIPTION("Alienware special feature control");
> @@ -49,11 +54,22 @@ enum WMAX_CONTROL_STATES {
>   	WMAX_SUSPEND = 3,
>   };
>
> +enum WMAX_THERMAL_PROFILE {
> +	WMAX_THERMAL_QUIET = 0xA3,
> +	WMAX_THERMAL_BALANCED = 0xA0,
> +	WMAX_THERMAL_BALANCED_PERFORMANCE = 0xA1,
> +	WMAX_THERMAL_PERFORMANCE = 0xA4,
> +	WMAX_THERMAL_GMODE = 0xAB,
> +	WMAX_THERMAL_LOW_POWER	= 0xA5,
> +};
> +
>   struct quirk_entry {
>   	u8 num_zones;
>   	u8 hdmi_mux;
>   	u8 amplifier;
>   	u8 deepslp;
> +	u8 thermal;
> +	u8 gmode;
>   };
>
>   static struct quirk_entry *quirks;
> @@ -64,6 +80,8 @@ static struct quirk_entry quirk_inspiron5675 = {
>   	.hdmi_mux = 0,
>   	.amplifier = 0,
>   	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>   };
>
>   static struct quirk_entry quirk_unknown = {
> @@ -71,6 +89,8 @@ static struct quirk_entry quirk_unknown = {
>   	.hdmi_mux = 0,
>   	.amplifier = 0,
>   	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>   };
>
>   static struct quirk_entry quirk_x51_r1_r2 = {
> @@ -78,6 +98,8 @@ static struct quirk_entry quirk_x51_r1_r2 = {
>   	.hdmi_mux = 0,
>   	.amplifier = 0,
>   	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>   };
>
>   static struct quirk_entry quirk_x51_r3 = {
> @@ -85,6 +107,8 @@ static struct quirk_entry quirk_x51_r3 = {
>   	.hdmi_mux = 0,
>   	.amplifier = 1,
>   	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>   };
>
>   static struct quirk_entry quirk_asm100 = {
> @@ -92,6 +116,8 @@ static struct quirk_entry quirk_asm100 = {
>   	.hdmi_mux = 1,
>   	.amplifier = 0,
>   	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>   };
>
>   static struct quirk_entry quirk_asm200 = {
> @@ -99,6 +125,8 @@ static struct quirk_entry quirk_asm200 = {
>   	.hdmi_mux = 1,
>   	.amplifier = 0,
>   	.deepslp = 1,
> +	.thermal = 0,
> +	.gmode = 0,
>   };
>
>   static struct quirk_entry quirk_asm201 = {
> @@ -106,6 +134,17 @@ static struct quirk_entry quirk_asm201 = {
>   	.hdmi_mux = 1,
>   	.amplifier = 1,
>   	.deepslp = 1,
> +	.thermal = 0,
> +	.gmode = 0,
> +};
> +
> +static struct quirk_entry quirk_x15_r1 = {
> +	.num_zones = 2,
> +	.hdmi_mux = 0,
> +	.amplifier = 0,
> +	.deepslp = 0,
> +	.thermal = 1,
> +	.gmode = 0,
>   };
>
>   static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -169,6 +208,15 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
>   		     },
>   	 .driver_data = &quirk_asm201,
>   	 },
> +	 {
> +	 .callback = dmi_matched,
> +	 .ident = "Alienware x15 R1",
> +	 .matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
> +		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware x15 R1")
> +		    },
> +	 .driver_data = &quirk_x15_r1,
> +	},
>   	 {
>   	 .callback = dmi_matched,
>   	 .ident = "Dell Inc. Inspiron 5675",
> @@ -218,6 +266,7 @@ static struct platform_device *platform_device;
>   static struct device_attribute *zone_dev_attrs;
>   static struct attribute **zone_attrs;
>   static struct platform_zone *zone_data;
> +static struct platform_profile_handler pp_handler;
>
>   static struct platform_driver platform_driver = {
>   	.driver = {
> @@ -500,7 +549,7 @@ static void alienware_zone_exit(struct platform_device *dev)
>   	kfree(zone_attrs);
>   }
>
> -static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
> +static acpi_status alienware_wmax_command(void *in_args, size_t insize,
>   					  u32 command, int *out_data)
>   {

Can you split this change into a separate patch? This would make review a bit easier.

>   	acpi_status status;
> @@ -508,7 +557,7 @@ static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
>   	struct acpi_buffer input;
>   	struct acpi_buffer output;
>
> -	input.length = (acpi_size) sizeof(*in_args);
> +	input.length = (acpi_size) insize;

Please drop the cast to acpi_size.

>   	input.pointer = in_args;
>   	if (out_data) {
>   		output.length = ACPI_ALLOCATE_BUFFER;
> @@ -541,8 +590,8 @@ static ssize_t show_hdmi_cable(struct device *dev,
>   		.arg = 0,
>   	};
>   	status =
> -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
> -				   (u32 *) &out_data);
> +	    alienware_wmax_command(&in_args, sizeof(in_args),
> +				   WMAX_METHOD_HDMI_CABLE, (u32 *) &out_data);
>   	if (ACPI_SUCCESS(status)) {
>   		if (out_data == 0)
>   			return sysfs_emit(buf, "[unconnected] connected unknown\n");
> @@ -562,8 +611,8 @@ static ssize_t show_hdmi_source(struct device *dev,
>   		.arg = 0,
>   	};
>   	status =
> -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
> -				   (u32 *) &out_data);
> +	    alienware_wmax_command(&in_args, sizeof(in_args),
> +				   WMAX_METHOD_HDMI_STATUS, (u32 *) &out_data);
>
>   	if (ACPI_SUCCESS(status)) {
>   		if (out_data == 1)
> @@ -589,7 +638,8 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>   		args.arg = 3;
>   	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
>
> -	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
> +	status = alienware_wmax_command(&args, sizeof(args),
> +					WMAX_METHOD_HDMI_SOURCE, NULL);
>
>   	if (ACPI_FAILURE(status))
>   		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
> @@ -642,8 +692,8 @@ static ssize_t show_amplifier_status(struct device *dev,
>   		.arg = 0,
>   	};
>   	status =
> -	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
> -				   (u32 *) &out_data);
> +	    alienware_wmax_command(&in_args, sizeof(in_args),
> +				   WMAX_METHOD_AMPLIFIER_CABLE, (u32 *) &out_data);
>   	if (ACPI_SUCCESS(status)) {
>   		if (out_data == 0)
>   			return sysfs_emit(buf, "[unconnected] connected unknown\n");
> @@ -694,8 +744,8 @@ static ssize_t show_deepsleep_status(struct device *dev,
>   	struct wmax_basic_args in_args = {
>   		.arg = 0,
>   	};
> -	status = alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
> -					(u32 *) &out_data);
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_DEEP_SLEEP_STATUS, (u32 *) &out_data);
>   	if (ACPI_SUCCESS(status)) {
>   		if (out_data == 0)
>   			return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
> @@ -723,8 +773,8 @@ static ssize_t toggle_deepsleep(struct device *dev,
>   		args.arg = 2;
>   	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
>
> -	status = alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL,
> -					NULL);
> +	status = alienware_wmax_command(&args, sizeof(args),
> +					WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
>
>   	if (ACPI_FAILURE(status))
>   		pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
> @@ -760,6 +810,160 @@ static int create_deepsleep(struct platform_device *dev)
>   	return ret;
>   }
>
> +/*
> + * Thermal Profile control
> + *  - Provides thermal profile control through the Platform Profile API
> + */
> +#define PROFILE_MASK		GENMASK(15,8)
> +#define PROFILE_ACTIVATE	BIT(0)
> +
> +static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
> +{
> +	return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
> +}
> +
> +static int thermal_profile_get(struct platform_profile_handler *pprof,
> +				enum platform_profile_option *profile)
> +{
> +	acpi_status status;
> +	u32 in_args = WMAX_ARG_GET_CURRENT_PROF;
> +	u32 out_data;
> +
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_THERMAL_INFORMATION, &out_data);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EOPNOTSUPP;

Please return -EIO.

> +
> +	if (out_data == 0xFFFFFFFF)
> +		return -EBADRQC;
> +
> +	switch (out_data) {
> +	case WMAX_THERMAL_LOW_POWER:
> +		*profile = PLATFORM_PROFILE_LOW_POWER;
> +		break;
> +	case WMAX_THERMAL_QUIET:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	case WMAX_THERMAL_BALANCED:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case WMAX_THERMAL_BALANCED_PERFORMANCE:
> +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> +		break;
> +	case WMAX_THERMAL_PERFORMANCE:
> +	case WMAX_THERMAL_GMODE:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	default:
> +		return -ENODATA;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_profile_set(struct platform_profile_handler *pprof,
> +				enum platform_profile_option profile)
> +{
> +	acpi_status status;
> +	u32 in_args;
> +	u32 out_data;
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> +		break;
> +	case PLATFORM_PROFILE_QUIET:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> +		break;
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_PERFORMANCE);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EOPNOTSUPP;

Return -EIO.

> +
> +	if (out_data == 0xFFFFFFFF)
> +		return -EBADRQC;
> +
> +	return 0;
> +}
> +
> +static int gmode_thermal_profile_set(struct platform_profile_handler *pprof,
> +				     enum platform_profile_option profile)
> +{
> +	acpi_status status;
> +	u32 in_args;
> +	u32 out_data;
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> +		break;
> +	case PLATFORM_PROFILE_QUIET:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> +		break;
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_GMODE);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EOPNOTSUPP;

Return -EIO.

Otherwise the patch looks quite good.

Thanks,
Armin Wolf

> +
> +	if (out_data == 0xFFFFFFFF)
> +		return -EBADRQC;
> +
> +	return 0;
> +}
> +
> +static int create_thermal_profile(void)
> +{
> +	pp_handler.profile_get = thermal_profile_get;
> +
> +	if (quirks->gmode > 0)
> +		pp_handler.profile_set = gmode_thermal_profile_set;
> +	else
> +		pp_handler.profile_set = thermal_profile_set;
> +
> +	set_bit(PLATFORM_PROFILE_LOW_POWER, pp_handler.choices);
> +	set_bit(PLATFORM_PROFILE_QUIET, pp_handler.choices);
> +	set_bit(PLATFORM_PROFILE_BALANCED, pp_handler.choices);
> +	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, pp_handler.choices);
> +	set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
> +
> +	return platform_profile_register(&pp_handler);
> +}
> +
> +static void remove_thermal_profile(void)
> +{
> +	if (quirks->thermal > 0)
> +		platform_profile_remove();
> +}
> +
>   static int __init alienware_wmi_init(void)
>   {
>   	int ret;
> @@ -807,6 +1011,12 @@ static int __init alienware_wmi_init(void)
>   			goto fail_prep_deepsleep;
>   	}
>
> +	if (quirks->thermal > 0) {
> +		ret = create_thermal_profile();
> +		if (ret)
> +			goto fail_prep_thermal_profile;
> +	}
> +
>   	ret = alienware_zone_init(platform_device);
>   	if (ret)
>   		goto fail_prep_zones;
> @@ -817,6 +1027,7 @@ static int __init alienware_wmi_init(void)
>   	alienware_zone_exit(platform_device);
>   fail_prep_deepsleep:
>   fail_prep_amplifier:
> +fail_prep_thermal_profile:
>   fail_prep_hdmi:
>   	platform_device_del(platform_device);
>   fail_platform_device2:
> @@ -834,6 +1045,7 @@ static void __exit alienware_wmi_exit(void)
>   	if (platform_device) {
>   		alienware_zone_exit(platform_device);
>   		remove_hdmi(platform_device);
> +		remove_thermal_profile();
>   		platform_device_unregister(platform_device);
>   		platform_driver_unregister(&platform_driver);
>   	}

Ilpo Järvinen Oct. 9, 2024, 8:56 a.m. UTC | #2

On Tue, 8 Oct 2024, Kurt Borja wrote:

> This patch adds platform_profile support for Dell devices which implement
> User Selectable Thermal Tables (USTT) that are meant to be controlled by
> Alienware Command Center (AWCC). These devices may include newer Alienware
> M-Series, Alienware X-Series and Dell's G-Series. This patch, was tested
> by me on an Alienware x15 R1.
> 
> It is suspected that Alienware Command Center manages thermal profiles
> through the WMI interface, specifically through a device with identifier
> \_SB_.AMW1.WMAX. This device was reverse engineered and the relevant
> functionality is documented here [1]. This driver interacts with this
> WMI device and thus is able to mimic AWCC's thermal profiles functionality
> through the platform_profile API. In consequence the user would be able
> to set and retrieve thermal profiles, which are just fan speed profiles.
> 
> This driver was heavily inspired on inspur_platform_profile, special
> thanks.
> 
> Notes:
>  - Performance (FullSpeed) profile is a special profile which has it's own
>    entry in the Firmware Settings of the Alienware x15 R1. It also changes
>    the color of the F1 key. I suspect this behavior would be replicated in
>    other X-Series or M-Series laptops.
>  - G-Mode is a profile documented on [1] which mimics the behavior of
>    FullSpeed mode but it does not have an entry on the Firmware Settings of
>    the Alienware x15 R1, this may correspond to the G-Mode functionality on
>    G-Series laptops (activated by a special button) but I cannot test it. I
>    did not include this code in the driver as G-Mode causes unexpected
>    behavior on X-Series laptops.
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> 
> ---
> v3:
>  - Removed extra empty line
>  - 0x0B named WMAX_ARG_GET_CURRENT_PROF
>  - Removed casts to the same type on functions added in this patch
>  - Thermal profile to WMAX argument is now an static function and makes
>    use of in-built kernel macros
>  - Platform profile is now removed only if it was created first
>  - create_platform_profile is now create_thermal_profile to avoid
>    confusion
>  - profile_get and profile_set functions renamed too to match the above
> v2:
>  - Moved functionality to alienware-wmi driver
>  - Added thermal and gmode quirks to add support based on dmi match
>  - Performance profile is now GMODE for devices that support it
>  - alienware_wmax_command now is insize agnostic to support new thermal
>    methods
> ---
>  drivers/platform/x86/dell/Kconfig         |   1 +
>  drivers/platform/x86/dell/alienware-wmi.c | 238 ++++++++++++++++++++--
>  2 files changed, 226 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index 68a49788a..b06d634cd 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -21,6 +21,7 @@ config ALIENWARE_WMI
>  	depends on LEDS_CLASS
>  	depends on NEW_LEDS
>  	depends on ACPI_WMI
> +	select ACPI_PLATFORM_PROFILE
>  	help
>  	 This is a driver for controlling Alienware BIOS driven
>  	 features.  It exposes an interface for controlling the AlienFX
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index f5ee62ce1..e3ef4b10b 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -10,6 +10,7 @@
>  #include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/platform_profile.h>
>  #include <linux/dmi.h>
>  #include <linux/leds.h>
>  
> @@ -25,6 +26,10 @@
>  #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>  #define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
>  #define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
> +#define WMAX_METHOD_THERMAL_INFORMATION	0x14
> +#define WMAX_METHOD_THERMAL_CONTROL	0x15
> +
> +#define WMAX_ARG_GET_CURRENT_PROF	0x0B
>  
>  MODULE_AUTHOR("Mario Limonciello <mario.limonciello@outlook.com>");
>  MODULE_DESCRIPTION("Alienware special feature control");
> @@ -49,11 +54,22 @@ enum WMAX_CONTROL_STATES {
>  	WMAX_SUSPEND = 3,
>  };
>  
> +enum WMAX_THERMAL_PROFILE {
> +	WMAX_THERMAL_QUIET = 0xA3,
> +	WMAX_THERMAL_BALANCED = 0xA0,
> +	WMAX_THERMAL_BALANCED_PERFORMANCE = 0xA1,
> +	WMAX_THERMAL_PERFORMANCE = 0xA4,
> +	WMAX_THERMAL_GMODE = 0xAB,
> +	WMAX_THERMAL_LOW_POWER	= 0xA5,
> +};
> +
>  struct quirk_entry {
>  	u8 num_zones;
>  	u8 hdmi_mux;
>  	u8 amplifier;
>  	u8 deepslp;
> +	u8 thermal;
> +	u8 gmode;
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -64,6 +80,8 @@ static struct quirk_entry quirk_inspiron5675 = {
>  	.hdmi_mux = 0,
>  	.amplifier = 0,
>  	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>  };
>  
>  static struct quirk_entry quirk_unknown = {
> @@ -71,6 +89,8 @@ static struct quirk_entry quirk_unknown = {
>  	.hdmi_mux = 0,
>  	.amplifier = 0,
>  	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>  };
>  
>  static struct quirk_entry quirk_x51_r1_r2 = {
> @@ -78,6 +98,8 @@ static struct quirk_entry quirk_x51_r1_r2 = {
>  	.hdmi_mux = 0,
>  	.amplifier = 0,
>  	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>  };
>  
>  static struct quirk_entry quirk_x51_r3 = {
> @@ -85,6 +107,8 @@ static struct quirk_entry quirk_x51_r3 = {
>  	.hdmi_mux = 0,
>  	.amplifier = 1,
>  	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>  };
>  
>  static struct quirk_entry quirk_asm100 = {
> @@ -92,6 +116,8 @@ static struct quirk_entry quirk_asm100 = {
>  	.hdmi_mux = 1,
>  	.amplifier = 0,
>  	.deepslp = 0,
> +	.thermal = 0,
> +	.gmode = 0,
>  };
>  
>  static struct quirk_entry quirk_asm200 = {
> @@ -99,6 +125,8 @@ static struct quirk_entry quirk_asm200 = {
>  	.hdmi_mux = 1,
>  	.amplifier = 0,
>  	.deepslp = 1,
> +	.thermal = 0,
> +	.gmode = 0,
>  };
>  
>  static struct quirk_entry quirk_asm201 = {
> @@ -106,6 +134,17 @@ static struct quirk_entry quirk_asm201 = {
>  	.hdmi_mux = 1,
>  	.amplifier = 1,
>  	.deepslp = 1,
> +	.thermal = 0,
> +	.gmode = 0,
> +};
> +
> +static struct quirk_entry quirk_x15_r1 = {
> +	.num_zones = 2,
> +	.hdmi_mux = 0,
> +	.amplifier = 0,
> +	.deepslp = 0,
> +	.thermal = 1,
> +	.gmode = 0,
>  };
>  
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -169,6 +208,15 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
>  		     },
>  	 .driver_data = &quirk_asm201,
>  	 },
> +	 {
> +	 .callback = dmi_matched,
> +	 .ident = "Alienware x15 R1",
> +	 .matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
> +		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware x15 R1")
> +		    },
> +	 .driver_data = &quirk_x15_r1,
> +	},
>  	 {
>  	 .callback = dmi_matched,
>  	 .ident = "Dell Inc. Inspiron 5675",
> @@ -218,6 +266,7 @@ static struct platform_device *platform_device;
>  static struct device_attribute *zone_dev_attrs;
>  static struct attribute **zone_attrs;
>  static struct platform_zone *zone_data;
> +static struct platform_profile_handler pp_handler;
>  
>  static struct platform_driver platform_driver = {
>  	.driver = {
> @@ -500,7 +549,7 @@ static void alienware_zone_exit(struct platform_device *dev)
>  	kfree(zone_attrs);
>  }
>  
> -static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
> +static acpi_status alienware_wmax_command(void *in_args, size_t insize,
>  					  u32 command, int *out_data)
>  {
>  	acpi_status status;
> @@ -508,7 +557,7 @@ static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
>  	struct acpi_buffer input;
>  	struct acpi_buffer output;
>  
> -	input.length = (acpi_size) sizeof(*in_args);
> +	input.length = (acpi_size) insize;
>  	input.pointer = in_args;
>  	if (out_data) {
>  		output.length = ACPI_ALLOCATE_BUFFER;
> @@ -541,8 +590,8 @@ static ssize_t show_hdmi_cable(struct device *dev,
>  		.arg = 0,
>  	};
>  	status =
> -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
> -				   (u32 *) &out_data);
> +	    alienware_wmax_command(&in_args, sizeof(in_args),
> +				   WMAX_METHOD_HDMI_CABLE, (u32 *) &out_data);
>  	if (ACPI_SUCCESS(status)) {
>  		if (out_data == 0)
>  			return sysfs_emit(buf, "[unconnected] connected unknown\n");
> @@ -562,8 +611,8 @@ static ssize_t show_hdmi_source(struct device *dev,
>  		.arg = 0,
>  	};
>  	status =
> -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
> -				   (u32 *) &out_data);
> +	    alienware_wmax_command(&in_args, sizeof(in_args),
> +				   WMAX_METHOD_HDMI_STATUS, (u32 *) &out_data);
>  
>  	if (ACPI_SUCCESS(status)) {
>  		if (out_data == 1)
> @@ -589,7 +638,8 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>  		args.arg = 3;
>  	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
>  
> -	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
> +	status = alienware_wmax_command(&args, sizeof(args),
> +					WMAX_METHOD_HDMI_SOURCE, NULL);
>  
>  	if (ACPI_FAILURE(status))
>  		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
> @@ -642,8 +692,8 @@ static ssize_t show_amplifier_status(struct device *dev,
>  		.arg = 0,
>  	};
>  	status =
> -	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
> -				   (u32 *) &out_data);
> +	    alienware_wmax_command(&in_args, sizeof(in_args),
> +				   WMAX_METHOD_AMPLIFIER_CABLE, (u32 *) &out_data);
>  	if (ACPI_SUCCESS(status)) {
>  		if (out_data == 0)
>  			return sysfs_emit(buf, "[unconnected] connected unknown\n");
> @@ -694,8 +744,8 @@ static ssize_t show_deepsleep_status(struct device *dev,
>  	struct wmax_basic_args in_args = {
>  		.arg = 0,
>  	};
> -	status = alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
> -					(u32 *) &out_data);
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_DEEP_SLEEP_STATUS, (u32 *) &out_data);
>  	if (ACPI_SUCCESS(status)) {
>  		if (out_data == 0)
>  			return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
> @@ -723,8 +773,8 @@ static ssize_t toggle_deepsleep(struct device *dev,
>  		args.arg = 2;
>  	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
>  
> -	status = alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL,
> -					NULL);
> +	status = alienware_wmax_command(&args, sizeof(args),
> +					WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
>  
>  	if (ACPI_FAILURE(status))
>  		pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
> @@ -760,6 +810,160 @@ static int create_deepsleep(struct platform_device *dev)
>  	return ret;
>  }
>  
> +/*
> + * Thermal Profile control
> + *  - Provides thermal profile control through the Platform Profile API
> + */
> +#define PROFILE_MASK		GENMASK(15,8)

Space after comma.

> +#define PROFILE_ACTIVATE	BIT(0)

Add driver specific prefix to the defines please.

> +
> +static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
> +{
> +	return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
> +}
> +
> +static int thermal_profile_get(struct platform_profile_handler *pprof,
> +				enum platform_profile_option *profile)
> +{
> +	acpi_status status;
> +	u32 in_args = WMAX_ARG_GET_CURRENT_PROF;
> +	u32 out_data;
> +
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_THERMAL_INFORMATION, &out_data);

Are you sure you want to keep out_data as u32? I'm not very happy how
alienware_wmax_command() takes int * but all callers seem to prefer u32 *
(or pass NULL). Should the out_data parameter for alienware_wmax_command() 
be u32 * or int *?

In general, if you find something that doesn't make sense, it often just
is an indication that some cleanup is in order. We're more than happy to
consider such patches along with the feature patches as then things are
moving into the correct direction even if the progress would be slow.

> +	if (ACPI_FAILURE(status))
> +		return -EOPNOTSUPP;
> +
> +	if (out_data == 0xFFFFFFFF)

This constant too should be named if the data really is u32 and not a 
negative error code in which case I'd be fine with < 0 (without naming
the error code) like in the original but it would need int as the type for 
the compare to work.

> +		return -EBADRQC;
> +
> +	switch (out_data) {
> +	case WMAX_THERMAL_LOW_POWER:
> +		*profile = PLATFORM_PROFILE_LOW_POWER;
> +		break;
> +	case WMAX_THERMAL_QUIET:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	case WMAX_THERMAL_BALANCED:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case WMAX_THERMAL_BALANCED_PERFORMANCE:
> +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> +		break;
> +	case WMAX_THERMAL_PERFORMANCE:
> +	case WMAX_THERMAL_GMODE:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	default:
> +		return -ENODATA;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_profile_set(struct platform_profile_handler *pprof,
> +				enum platform_profile_option profile)
> +{
> +	acpi_status status;
> +	u32 in_args;
> +	u32 out_data;
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> +		break;
> +	case PLATFORM_PROFILE_QUIET:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> +		break;
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_PERFORMANCE);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EOPNOTSUPP;
> +
> +	if (out_data == 0xFFFFFFFF)

Ditto.

> +		return -EBADRQC;
> +
> +	return 0;
> +}
> +
> +static int gmode_thermal_profile_set(struct platform_profile_handler *pprof,
> +				     enum platform_profile_option profile)
> +{
> +	acpi_status status;
> +	u32 in_args;
> +	u32 out_data;
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> +		break;
> +	case PLATFORM_PROFILE_QUIET:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> +		break;
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> +		break;
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_GMODE);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EOPNOTSUPP;
> +
> +	if (out_data == 0xFFFFFFFF)

Ditto.

Kurt Borja Oct. 9, 2024, 2:48 p.m. UTC | #3

Yes, of course.

> > This driver was heavily inspired on inspur_platform_profile, special
> > thanks.
> > 
> > Notes:
> >   - Performance (FullSpeed) profile is a special profile which has it's own
> >     entry in the Firmware Settings of the Alienware x15 R1. It also changes
> >     the color of the F1 key. I suspect this behavior would be replicated in
> >     other X-Series or M-Series laptops.
> >   - G-Mode is a profile documented on [1] which mimics the behavior of
> >     FullSpeed mode but it does not have an entry on the Firmware Settings of
> >     the Alienware x15 R1, this may correspond to the G-Mode functionality on
> >     G-Series laptops (activated by a special button) but I cannot test it. I
> >     did not include this code in the driver as G-Mode causes unexpected
> >     behavior on X-Series laptops.
> > 
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > 
> > ---
> > v3:
> >   - Removed extra empty line
> >   - 0x0B named WMAX_ARG_GET_CURRENT_PROF
> >   - Removed casts to the same type on functions added in this patch
> >   - Thermal profile to WMAX argument is now an static function and makes
> >     use of in-built kernel macros
> >   - Platform profile is now removed only if it was created first
> >   - create_platform_profile is now create_thermal_profile to avoid
> >     confusion
> >   - profile_get and profile_set functions renamed too to match the above
> > v2:
> >   - Moved functionality to alienware-wmi driver
> >   - Added thermal and gmode quirks to add support based on dmi match
> >   - Performance profile is now GMODE for devices that support it
> >   - alienware_wmax_command now is insize agnostic to support new thermal
> >     methods
> > ---
> >   drivers/platform/x86/dell/Kconfig         |   1 +
> >   drivers/platform/x86/dell/alienware-wmi.c | 238 ++++++++++++++++++++--
> >   2 files changed, 226 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> > index 68a49788a..b06d634cd 100644
> > --- a/drivers/platform/x86/dell/Kconfig
> > +++ b/drivers/platform/x86/dell/Kconfig
> > @@ -21,6 +21,7 @@ config ALIENWARE_WMI
> >   	depends on LEDS_CLASS
> >   	depends on NEW_LEDS
> >   	depends on ACPI_WMI
> > +	select ACPI_PLATFORM_PROFILE
> >   	help
> >   	 This is a driver for controlling Alienware BIOS driven
> >   	 features.  It exposes an interface for controlling the AlienFX
> > diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> > index f5ee62ce1..e3ef4b10b 100644
> > --- a/drivers/platform/x86/dell/alienware-wmi.c
> > +++ b/drivers/platform/x86/dell/alienware-wmi.c
> > @@ -10,6 +10,7 @@
> >   #include <linux/acpi.h>
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/platform_profile.h>
> >   #include <linux/dmi.h>
> >   #include <linux/leds.h>
> > 
> > @@ -25,6 +26,10 @@
> >   #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
> >   #define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
> >   #define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
> > +#define WMAX_METHOD_THERMAL_INFORMATION	0x14
> > +#define WMAX_METHOD_THERMAL_CONTROL	0x15
> > +
> > +#define WMAX_ARG_GET_CURRENT_PROF	0x0B
> > 
> >   MODULE_AUTHOR("Mario Limonciello <mario.limonciello@outlook.com>");
> >   MODULE_DESCRIPTION("Alienware special feature control");
> > @@ -49,11 +54,22 @@ enum WMAX_CONTROL_STATES {
> >   	WMAX_SUSPEND = 3,
> >   };
> > 
> > +enum WMAX_THERMAL_PROFILE {
> > +	WMAX_THERMAL_QUIET = 0xA3,
> > +	WMAX_THERMAL_BALANCED = 0xA0,
> > +	WMAX_THERMAL_BALANCED_PERFORMANCE = 0xA1,
> > +	WMAX_THERMAL_PERFORMANCE = 0xA4,
> > +	WMAX_THERMAL_GMODE = 0xAB,
> > +	WMAX_THERMAL_LOW_POWER	= 0xA5,
> > +};
> > +
> >   struct quirk_entry {
> >   	u8 num_zones;
> >   	u8 hdmi_mux;
> >   	u8 amplifier;
> >   	u8 deepslp;
> > +	u8 thermal;
> > +	u8 gmode;
> >   };
> > 
> >   static struct quirk_entry *quirks;
> > @@ -64,6 +80,8 @@ static struct quirk_entry quirk_inspiron5675 = {
> >   	.hdmi_mux = 0,
> >   	.amplifier = 0,
> >   	.deepslp = 0,
> > +	.thermal = 0,
> > +	.gmode = 0,
> >   };
> > 
> >   static struct quirk_entry quirk_unknown = {
> > @@ -71,6 +89,8 @@ static struct quirk_entry quirk_unknown = {
> >   	.hdmi_mux = 0,
> >   	.amplifier = 0,
> >   	.deepslp = 0,
> > +	.thermal = 0,
> > +	.gmode = 0,
> >   };
> > 
> >   static struct quirk_entry quirk_x51_r1_r2 = {
> > @@ -78,6 +98,8 @@ static struct quirk_entry quirk_x51_r1_r2 = {
> >   	.hdmi_mux = 0,
> >   	.amplifier = 0,
> >   	.deepslp = 0,
> > +	.thermal = 0,
> > +	.gmode = 0,
> >   };
> > 
> >   static struct quirk_entry quirk_x51_r3 = {
> > @@ -85,6 +107,8 @@ static struct quirk_entry quirk_x51_r3 = {
> >   	.hdmi_mux = 0,
> >   	.amplifier = 1,
> >   	.deepslp = 0,
> > +	.thermal = 0,
> > +	.gmode = 0,
> >   };
> > 
> >   static struct quirk_entry quirk_asm100 = {
> > @@ -92,6 +116,8 @@ static struct quirk_entry quirk_asm100 = {
> >   	.hdmi_mux = 1,
> >   	.amplifier = 0,
> >   	.deepslp = 0,
> > +	.thermal = 0,
> > +	.gmode = 0,
> >   };
> > 
> >   static struct quirk_entry quirk_asm200 = {
> > @@ -99,6 +125,8 @@ static struct quirk_entry quirk_asm200 = {
> >   	.hdmi_mux = 1,
> >   	.amplifier = 0,
> >   	.deepslp = 1,
> > +	.thermal = 0,
> > +	.gmode = 0,
> >   };
> > 
> >   static struct quirk_entry quirk_asm201 = {
> > @@ -106,6 +134,17 @@ static struct quirk_entry quirk_asm201 = {
> >   	.hdmi_mux = 1,
> >   	.amplifier = 1,
> >   	.deepslp = 1,
> > +	.thermal = 0,
> > +	.gmode = 0,
> > +};
> > +
> > +static struct quirk_entry quirk_x15_r1 = {
> > +	.num_zones = 2,
> > +	.hdmi_mux = 0,
> > +	.amplifier = 0,
> > +	.deepslp = 0,
> > +	.thermal = 1,
> > +	.gmode = 0,
> >   };
> > 
> >   static int __init dmi_matched(const struct dmi_system_id *dmi)
> > @@ -169,6 +208,15 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
> >   		     },
> >   	 .driver_data = &quirk_asm201,
> >   	 },
> > +	 {
> > +	 .callback = dmi_matched,
> > +	 .ident = "Alienware x15 R1",
> > +	 .matches = {
> > +		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
> > +		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware x15 R1")
> > +		    },
> > +	 .driver_data = &quirk_x15_r1,
> > +	},
> >   	 {
> >   	 .callback = dmi_matched,
> >   	 .ident = "Dell Inc. Inspiron 5675",
> > @@ -218,6 +266,7 @@ static struct platform_device *platform_device;
> >   static struct device_attribute *zone_dev_attrs;
> >   static struct attribute **zone_attrs;
> >   static struct platform_zone *zone_data;
> > +static struct platform_profile_handler pp_handler;
> > 
> >   static struct platform_driver platform_driver = {
> >   	.driver = {
> > @@ -500,7 +549,7 @@ static void alienware_zone_exit(struct platform_device *dev)
> >   	kfree(zone_attrs);
> >   }
> > 
> > -static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
> > +static acpi_status alienware_wmax_command(void *in_args, size_t insize,
> >   					  u32 command, int *out_data)
> >   {
> 
> Can you split this change into a separate patch? This would make review a bit easier.
> 

Yes, sure. In that case should I mention this patch depends on that one
in v4?


> >   	acpi_status status;
> > @@ -508,7 +557,7 @@ static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
> >   	struct acpi_buffer input;
> >   	struct acpi_buffer output;
> > 
> > -	input.length = (acpi_size) sizeof(*in_args);
> > +	input.length = (acpi_size) insize;
> 
> Please drop the cast to acpi_size.
> 

Ok.

> >   	input.pointer = in_args;
> >   	if (out_data) {
> >   		output.length = ACPI_ALLOCATE_BUFFER;
> > @@ -541,8 +590,8 @@ static ssize_t show_hdmi_cable(struct device *dev,
> >   		.arg = 0,
> >   	};
> >   	status =
> > -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
> > -				   (u32 *) &out_data);
> > +	    alienware_wmax_command(&in_args, sizeof(in_args),
> > +				   WMAX_METHOD_HDMI_CABLE, (u32 *) &out_data);
> >   	if (ACPI_SUCCESS(status)) {
> >   		if (out_data == 0)
> >   			return sysfs_emit(buf, "[unconnected] connected unknown\n");
> > @@ -562,8 +611,8 @@ static ssize_t show_hdmi_source(struct device *dev,
> >   		.arg = 0,
> >   	};
> >   	status =
> > -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
> > -				   (u32 *) &out_data);
> > +	    alienware_wmax_command(&in_args, sizeof(in_args),
> > +				   WMAX_METHOD_HDMI_STATUS, (u32 *) &out_data);
> > 
> >   	if (ACPI_SUCCESS(status)) {
> >   		if (out_data == 1)
> > @@ -589,7 +638,8 @@ static ssize_t toggle_hdmi_source(struct device *dev,
> >   		args.arg = 3;
> >   	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
> > 
> > -	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
> > +	status = alienware_wmax_command(&args, sizeof(args),
> > +					WMAX_METHOD_HDMI_SOURCE, NULL);
> > 
> >   	if (ACPI_FAILURE(status))
> >   		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
> > @@ -642,8 +692,8 @@ static ssize_t show_amplifier_status(struct device *dev,
> >   		.arg = 0,
> >   	};
> >   	status =
> > -	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
> > -				   (u32 *) &out_data);
> > +	    alienware_wmax_command(&in_args, sizeof(in_args),
> > +				   WMAX_METHOD_AMPLIFIER_CABLE, (u32 *) &out_data);
> >   	if (ACPI_SUCCESS(status)) {
> >   		if (out_data == 0)
> >   			return sysfs_emit(buf, "[unconnected] connected unknown\n");
> > @@ -694,8 +744,8 @@ static ssize_t show_deepsleep_status(struct device *dev,
> >   	struct wmax_basic_args in_args = {
> >   		.arg = 0,
> >   	};
> > -	status = alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
> > -					(u32 *) &out_data);
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_DEEP_SLEEP_STATUS, (u32 *) &out_data);
> >   	if (ACPI_SUCCESS(status)) {
> >   		if (out_data == 0)
> >   			return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
> > @@ -723,8 +773,8 @@ static ssize_t toggle_deepsleep(struct device *dev,
> >   		args.arg = 2;
> >   	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
> > 
> > -	status = alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL,
> > -					NULL);
> > +	status = alienware_wmax_command(&args, sizeof(args),
> > +					WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
> > 
> >   	if (ACPI_FAILURE(status))
> >   		pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
> > @@ -760,6 +810,160 @@ static int create_deepsleep(struct platform_device *dev)
> >   	return ret;
> >   }
> > 
> > +/*
> > + * Thermal Profile control
> > + *  - Provides thermal profile control through the Platform Profile API
> > + */
> > +#define PROFILE_MASK		GENMASK(15,8)
> > +#define PROFILE_ACTIVATE	BIT(0)
> > +
> > +static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
> > +{
> > +	return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
> > +}
> > +
> > +static int thermal_profile_get(struct platform_profile_handler *pprof,
> > +				enum platform_profile_option *profile)
> > +{
> > +	acpi_status status;
> > +	u32 in_args = WMAX_ARG_GET_CURRENT_PROF;
> > +	u32 out_data;
> > +
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_THERMAL_INFORMATION, &out_data);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return -EOPNOTSUPP;
> 
> Please return -EIO.
> 

Ok.

> > +
> > +	if (out_data == 0xFFFFFFFF)
> > +		return -EBADRQC;
> > +
> > +	switch (out_data) {
> > +	case WMAX_THERMAL_LOW_POWER:
> > +		*profile = PLATFORM_PROFILE_LOW_POWER;
> > +		break;
> > +	case WMAX_THERMAL_QUIET:
> > +		*profile = PLATFORM_PROFILE_QUIET;
> > +		break;
> > +	case WMAX_THERMAL_BALANCED:
> > +		*profile = PLATFORM_PROFILE_BALANCED;
> > +		break;
> > +	case WMAX_THERMAL_BALANCED_PERFORMANCE:
> > +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> > +		break;
> > +	case WMAX_THERMAL_PERFORMANCE:
> > +	case WMAX_THERMAL_GMODE:
> > +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> > +		break;
> > +	default:
> > +		return -ENODATA;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int thermal_profile_set(struct platform_profile_handler *pprof,
> > +				enum platform_profile_option profile)
> > +{
> > +	acpi_status status;
> > +	u32 in_args;
> > +	u32 out_data;
> > +
> > +	switch (profile) {
> > +	case PLATFORM_PROFILE_LOW_POWER:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> > +		break;
> > +	case PLATFORM_PROFILE_QUIET:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> > +		break;
> > +	case PLATFORM_PROFILE_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_PERFORMANCE);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return -EOPNOTSUPP;
> 
> Return -EIO.
> 
> > +
> > +	if (out_data == 0xFFFFFFFF)
> > +		return -EBADRQC;
> > +
> > +	return 0;
> > +}
> > +
> > +static int gmode_thermal_profile_set(struct platform_profile_handler *pprof,
> > +				     enum platform_profile_option profile)
> > +{
> > +	acpi_status status;
> > +	u32 in_args;
> > +	u32 out_data;
> > +
> > +	switch (profile) {
> > +	case PLATFORM_PROFILE_LOW_POWER:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> > +		break;
> > +	case PLATFORM_PROFILE_QUIET:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> > +		break;
> > +	case PLATFORM_PROFILE_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_GMODE);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return -EOPNOTSUPP;
> 
> Return -EIO.
> 
> Otherwise the patch looks quite good.
> 

Thank you! It's my first time working on the kernel.

> Thanks,
> Armin Wolf

Your feedback is appreciated.

Kurt

> > +
> > +	if (out_data == 0xFFFFFFFF)
> > +		return -EBADRQC;
> > +
> > +	return 0;
> > +}
> > +
> > +static int create_thermal_profile(void)
> > +{
> > +	pp_handler.profile_get = thermal_profile_get;
> > +
> > +	if (quirks->gmode > 0)
> > +		pp_handler.profile_set = gmode_thermal_profile_set;
> > +	else
> > +		pp_handler.profile_set = thermal_profile_set;
> > +
> > +	set_bit(PLATFORM_PROFILE_LOW_POWER, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_QUIET, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_BALANCED, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
> > +
> > +	return platform_profile_register(&pp_handler);
> > +}
> > +
> > +static void remove_thermal_profile(void)
> > +{
> > +	if (quirks->thermal > 0)
> > +		platform_profile_remove();
> > +}
> > +
> >   static int __init alienware_wmi_init(void)
> >   {
> >   	int ret;
> > @@ -807,6 +1011,12 @@ static int __init alienware_wmi_init(void)
> >   			goto fail_prep_deepsleep;
> >   	}
> > 
> > +	if (quirks->thermal > 0) {
> > +		ret = create_thermal_profile();
> > +		if (ret)
> > +			goto fail_prep_thermal_profile;
> > +	}
> > +
> >   	ret = alienware_zone_init(platform_device);
> >   	if (ret)
> >   		goto fail_prep_zones;
> > @@ -817,6 +1027,7 @@ static int __init alienware_wmi_init(void)
> >   	alienware_zone_exit(platform_device);
> >   fail_prep_deepsleep:
> >   fail_prep_amplifier:
> > +fail_prep_thermal_profile:
> >   fail_prep_hdmi:
> >   	platform_device_del(platform_device);
> >   fail_platform_device2:
> > @@ -834,6 +1045,7 @@ static void __exit alienware_wmi_exit(void)
> >   	if (platform_device) {
> >   		alienware_zone_exit(platform_device);
> >   		remove_hdmi(platform_device);
> > +		remove_thermal_profile();
> >   		platform_device_unregister(platform_device);
> >   		platform_driver_unregister(&platform_driver);
> >   	}

Kurt Borja Oct. 9, 2024, 3:39 p.m. UTC | #4

Ok.

> > +#define PROFILE_ACTIVATE	BIT(0)
> 
> Add driver specific prefix to the defines please.
> 

Ok.

> > +
> > +static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
> > +{
> > +	return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
> > +}
> > +
> > +static int thermal_profile_get(struct platform_profile_handler *pprof,
> > +				enum platform_profile_option *profile)
> > +{
> > +	acpi_status status;
> > +	u32 in_args = WMAX_ARG_GET_CURRENT_PROF;
> > +	u32 out_data;
> > +
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_THERMAL_INFORMATION, &out_data);
> 
> Are you sure you want to keep out_data as u32? I'm not very happy how
> alienware_wmax_command() takes int * but all callers seem to prefer u32 *
> (or pass NULL). 
>

I don't have the old WMAX interface but the new one specifies an [out]
uint32 argument for all methods, I guess it's the same for the old one.

> Should the out_data parameter for alienware_wmax_command() 
> be u32 * or int *?
 
I'd say we go with u32 * for everything to resemble as much as possible
the WMAX interface description. That way there won't be confusion in the 
future.

I will name 0xFFFFFFFF appropriately.

> In general, if you find something that doesn't make sense, it often just
> is an indication that some cleanup is in order. We're more than happy to
> consider such patches along with the feature patches as then things are
> moving into the correct direction even if the progress would be slow.

All right. I will keep it in mind.

I will send a clean up patch together with the split Armin requested
together with v4.

> > +	if (ACPI_FAILURE(status))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (out_data == 0xFFFFFFFF)
> 
> This constant too should be named if the data really is u32 and not a 
> negative error code in which case I'd be fine with < 0 (without naming
> the error code) like in the original but it would need int as the type for 
> the compare to work.
> 
> > +		return -EBADRQC;
> > +
> > +	switch (out_data) {
> > +	case WMAX_THERMAL_LOW_POWER:
> > +		*profile = PLATFORM_PROFILE_LOW_POWER;
> > +		break;
> > +	case WMAX_THERMAL_QUIET:
> > +		*profile = PLATFORM_PROFILE_QUIET;
> > +		break;
> > +	case WMAX_THERMAL_BALANCED:
> > +		*profile = PLATFORM_PROFILE_BALANCED;
> > +		break;
> > +	case WMAX_THERMAL_BALANCED_PERFORMANCE:
> > +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> > +		break;
> > +	case WMAX_THERMAL_PERFORMANCE:
> > +	case WMAX_THERMAL_GMODE:
> > +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> > +		break;
> > +	default:
> > +		return -ENODATA;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int thermal_profile_set(struct platform_profile_handler *pprof,
> > +				enum platform_profile_option profile)
> > +{
> > +	acpi_status status;
> > +	u32 in_args;
> > +	u32 out_data;
> > +
> > +	switch (profile) {
> > +	case PLATFORM_PROFILE_LOW_POWER:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> > +		break;
> > +	case PLATFORM_PROFILE_QUIET:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> > +		break;
> > +	case PLATFORM_PROFILE_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_PERFORMANCE);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (out_data == 0xFFFFFFFF)
> 
> Ditto.
> 
> > +		return -EBADRQC;
> > +
> > +	return 0;
> > +}
> > +
> > +static int gmode_thermal_profile_set(struct platform_profile_handler *pprof,
> > +				     enum platform_profile_option profile)
> > +{
> > +	acpi_status status;
> > +	u32 in_args;
> > +	u32 out_data;
> > +
> > +	switch (profile) {
> > +	case PLATFORM_PROFILE_LOW_POWER:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
> > +		break;
> > +	case PLATFORM_PROFILE_QUIET:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
> > +		break;
> > +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
> > +		break;
> > +	case PLATFORM_PROFILE_PERFORMANCE:
> > +		in_args = profile_to_wmax_arg(WMAX_THERMAL_GMODE);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	status = alienware_wmax_command(&in_args, sizeof(in_args),
> > +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (out_data == 0xFFFFFFFF)
> 
> Ditto.
> 
> -- 
>  i.
> 

Thank you for your feedback!

Kurt

> > +		return -EBADRQC;
> > +
> > +	return 0;
> > +}
> > +
> > +static int create_thermal_profile(void)
> > +{
> > +	pp_handler.profile_get = thermal_profile_get;
> > +
> > +	if (quirks->gmode > 0)
> > +		pp_handler.profile_set = gmode_thermal_profile_set;
> > +	else
> > +		pp_handler.profile_set = thermal_profile_set;
> > +
> > +	set_bit(PLATFORM_PROFILE_LOW_POWER, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_QUIET, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_BALANCED, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, pp_handler.choices);
> > +	set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
> > +
> > +	return platform_profile_register(&pp_handler);
> > +}
> > +
> > +static void remove_thermal_profile(void)
> > +{
> > +	if (quirks->thermal > 0)
> > +		platform_profile_remove();
> > +}
> > +
> >  static int __init alienware_wmi_init(void)
> >  {
> >  	int ret;
> > @@ -807,6 +1011,12 @@ static int __init alienware_wmi_init(void)
> >  			goto fail_prep_deepsleep;
> >  	}
> >  
> > +	if (quirks->thermal > 0) {
> > +		ret = create_thermal_profile();
> > +		if (ret)
> > +			goto fail_prep_thermal_profile;
> > +	}
> > +
> >  	ret = alienware_zone_init(platform_device);
> >  	if (ret)
> >  		goto fail_prep_zones;
> > @@ -817,6 +1027,7 @@ static int __init alienware_wmi_init(void)
> >  	alienware_zone_exit(platform_device);
> >  fail_prep_deepsleep:
> >  fail_prep_amplifier:
> > +fail_prep_thermal_profile:
> >  fail_prep_hdmi:
> >  	platform_device_del(platform_device);
> >  fail_platform_device2:
> > @@ -834,6 +1045,7 @@ static void __exit alienware_wmi_exit(void)
> >  	if (platform_device) {
> >  		alienware_zone_exit(platform_device);
> >  		remove_hdmi(platform_device);
> > +		remove_thermal_profile();
> >  		platform_device_unregister(platform_device);
> >  		platform_driver_unregister(&platform_driver);
> >  	}
> >

Armin Wolf Oct. 9, 2024, 3:48 p.m. UTC | #5

Am 09.10.24 um 16:48 schrieb Kurt Borja:

> Yes, of course.
>
>>> This driver was heavily inspired on inspur_platform_profile, special
>>> thanks.
>>>
>>> Notes:
>>>    - Performance (FullSpeed) profile is a special profile which has it's own
>>>      entry in the Firmware Settings of the Alienware x15 R1. It also changes
>>>      the color of the F1 key. I suspect this behavior would be replicated in
>>>      other X-Series or M-Series laptops.
>>>    - G-Mode is a profile documented on [1] which mimics the behavior of
>>>      FullSpeed mode but it does not have an entry on the Firmware Settings of
>>>      the Alienware x15 R1, this may correspond to the G-Mode functionality on
>>>      G-Series laptops (activated by a special button) but I cannot test it. I
>>>      did not include this code in the driver as G-Mode causes unexpected
>>>      behavior on X-Series laptops.
>>>
>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>>
>>> ---
>>> v3:
>>>    - Removed extra empty line
>>>    - 0x0B named WMAX_ARG_GET_CURRENT_PROF
>>>    - Removed casts to the same type on functions added in this patch
>>>    - Thermal profile to WMAX argument is now an static function and makes
>>>      use of in-built kernel macros
>>>    - Platform profile is now removed only if it was created first
>>>    - create_platform_profile is now create_thermal_profile to avoid
>>>      confusion
>>>    - profile_get and profile_set functions renamed too to match the above
>>> v2:
>>>    - Moved functionality to alienware-wmi driver
>>>    - Added thermal and gmode quirks to add support based on dmi match
>>>    - Performance profile is now GMODE for devices that support it
>>>    - alienware_wmax_command now is insize agnostic to support new thermal
>>>      methods
>>> ---
>>>    drivers/platform/x86/dell/Kconfig         |   1 +
>>>    drivers/platform/x86/dell/alienware-wmi.c | 238 ++++++++++++++++++++--
>>>    2 files changed, 226 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>>> index 68a49788a..b06d634cd 100644
>>> --- a/drivers/platform/x86/dell/Kconfig
>>> +++ b/drivers/platform/x86/dell/Kconfig
>>> @@ -21,6 +21,7 @@ config ALIENWARE_WMI
>>>    	depends on LEDS_CLASS
>>>    	depends on NEW_LEDS
>>>    	depends on ACPI_WMI
>>> +	select ACPI_PLATFORM_PROFILE
>>>    	help
>>>    	 This is a driver for controlling Alienware BIOS driven
>>>    	 features.  It exposes an interface for controlling the AlienFX
>>> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
>>> index f5ee62ce1..e3ef4b10b 100644
>>> --- a/drivers/platform/x86/dell/alienware-wmi.c
>>> +++ b/drivers/platform/x86/dell/alienware-wmi.c
>>> @@ -10,6 +10,7 @@
>>>    #include <linux/acpi.h>
>>>    #include <linux/module.h>
>>>    #include <linux/platform_device.h>
>>> +#include <linux/platform_profile.h>
>>>    #include <linux/dmi.h>
>>>    #include <linux/leds.h>
>>>
>>> @@ -25,6 +26,10 @@
>>>    #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>>>    #define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
>>>    #define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
>>> +#define WMAX_METHOD_THERMAL_INFORMATION	0x14
>>> +#define WMAX_METHOD_THERMAL_CONTROL	0x15
>>> +
>>> +#define WMAX_ARG_GET_CURRENT_PROF	0x0B
>>>
>>>    MODULE_AUTHOR("Mario Limonciello <mario.limonciello@outlook.com>");
>>>    MODULE_DESCRIPTION("Alienware special feature control");
>>> @@ -49,11 +54,22 @@ enum WMAX_CONTROL_STATES {
>>>    	WMAX_SUSPEND = 3,
>>>    };
>>>
>>> +enum WMAX_THERMAL_PROFILE {
>>> +	WMAX_THERMAL_QUIET = 0xA3,
>>> +	WMAX_THERMAL_BALANCED = 0xA0,
>>> +	WMAX_THERMAL_BALANCED_PERFORMANCE = 0xA1,
>>> +	WMAX_THERMAL_PERFORMANCE = 0xA4,
>>> +	WMAX_THERMAL_GMODE = 0xAB,
>>> +	WMAX_THERMAL_LOW_POWER	= 0xA5,
>>> +};
>>> +
>>>    struct quirk_entry {
>>>    	u8 num_zones;
>>>    	u8 hdmi_mux;
>>>    	u8 amplifier;
>>>    	u8 deepslp;
>>> +	u8 thermal;
>>> +	u8 gmode;
>>>    };
>>>
>>>    static struct quirk_entry *quirks;
>>> @@ -64,6 +80,8 @@ static struct quirk_entry quirk_inspiron5675 = {
>>>    	.hdmi_mux = 0,
>>>    	.amplifier = 0,
>>>    	.deepslp = 0,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static struct quirk_entry quirk_unknown = {
>>> @@ -71,6 +89,8 @@ static struct quirk_entry quirk_unknown = {
>>>    	.hdmi_mux = 0,
>>>    	.amplifier = 0,
>>>    	.deepslp = 0,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static struct quirk_entry quirk_x51_r1_r2 = {
>>> @@ -78,6 +98,8 @@ static struct quirk_entry quirk_x51_r1_r2 = {
>>>    	.hdmi_mux = 0,
>>>    	.amplifier = 0,
>>>    	.deepslp = 0,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static struct quirk_entry quirk_x51_r3 = {
>>> @@ -85,6 +107,8 @@ static struct quirk_entry quirk_x51_r3 = {
>>>    	.hdmi_mux = 0,
>>>    	.amplifier = 1,
>>>    	.deepslp = 0,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static struct quirk_entry quirk_asm100 = {
>>> @@ -92,6 +116,8 @@ static struct quirk_entry quirk_asm100 = {
>>>    	.hdmi_mux = 1,
>>>    	.amplifier = 0,
>>>    	.deepslp = 0,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static struct quirk_entry quirk_asm200 = {
>>> @@ -99,6 +125,8 @@ static struct quirk_entry quirk_asm200 = {
>>>    	.hdmi_mux = 1,
>>>    	.amplifier = 0,
>>>    	.deepslp = 1,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static struct quirk_entry quirk_asm201 = {
>>> @@ -106,6 +134,17 @@ static struct quirk_entry quirk_asm201 = {
>>>    	.hdmi_mux = 1,
>>>    	.amplifier = 1,
>>>    	.deepslp = 1,
>>> +	.thermal = 0,
>>> +	.gmode = 0,
>>> +};
>>> +
>>> +static struct quirk_entry quirk_x15_r1 = {
>>> +	.num_zones = 2,
>>> +	.hdmi_mux = 0,
>>> +	.amplifier = 0,
>>> +	.deepslp = 0,
>>> +	.thermal = 1,
>>> +	.gmode = 0,
>>>    };
>>>
>>>    static int __init dmi_matched(const struct dmi_system_id *dmi)
>>> @@ -169,6 +208,15 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
>>>    		     },
>>>    	 .driver_data = &quirk_asm201,
>>>    	 },
>>> +	 {
>>> +	 .callback = dmi_matched,
>>> +	 .ident = "Alienware x15 R1",
>>> +	 .matches = {
>>> +		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
>>> +		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware x15 R1")
>>> +		    },
>>> +	 .driver_data = &quirk_x15_r1,
>>> +	},
>>>    	 {
>>>    	 .callback = dmi_matched,
>>>    	 .ident = "Dell Inc. Inspiron 5675",
>>> @@ -218,6 +266,7 @@ static struct platform_device *platform_device;
>>>    static struct device_attribute *zone_dev_attrs;
>>>    static struct attribute **zone_attrs;
>>>    static struct platform_zone *zone_data;
>>> +static struct platform_profile_handler pp_handler;
>>>
>>>    static struct platform_driver platform_driver = {
>>>    	.driver = {
>>> @@ -500,7 +549,7 @@ static void alienware_zone_exit(struct platform_device *dev)
>>>    	kfree(zone_attrs);
>>>    }
>>>
>>> -static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
>>> +static acpi_status alienware_wmax_command(void *in_args, size_t insize,
>>>    					  u32 command, int *out_data)
>>>    {
>> Can you split this change into a separate patch? This would make review a bit easier.
>>
> Yes, sure. In that case should I mention this patch depends on that one
> in v4?

Yes, but only in the description of the resulting patch series.

Thanks,
Armin Wolf

>>>    	acpi_status status;
>>> @@ -508,7 +557,7 @@ static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
>>>    	struct acpi_buffer input;
>>>    	struct acpi_buffer output;
>>>
>>> -	input.length = (acpi_size) sizeof(*in_args);
>>> +	input.length = (acpi_size) insize;
>> Please drop the cast to acpi_size.
>>
> Ok.
>
>>>    	input.pointer = in_args;
>>>    	if (out_data) {
>>>    		output.length = ACPI_ALLOCATE_BUFFER;
>>> @@ -541,8 +590,8 @@ static ssize_t show_hdmi_cable(struct device *dev,
>>>    		.arg = 0,
>>>    	};
>>>    	status =
>>> -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
>>> -				   (u32 *) &out_data);
>>> +	    alienware_wmax_command(&in_args, sizeof(in_args),
>>> +				   WMAX_METHOD_HDMI_CABLE, (u32 *) &out_data);
>>>    	if (ACPI_SUCCESS(status)) {
>>>    		if (out_data == 0)
>>>    			return sysfs_emit(buf, "[unconnected] connected unknown\n");
>>> @@ -562,8 +611,8 @@ static ssize_t show_hdmi_source(struct device *dev,
>>>    		.arg = 0,
>>>    	};
>>>    	status =
>>> -	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
>>> -				   (u32 *) &out_data);
>>> +	    alienware_wmax_command(&in_args, sizeof(in_args),
>>> +				   WMAX_METHOD_HDMI_STATUS, (u32 *) &out_data);
>>>
>>>    	if (ACPI_SUCCESS(status)) {
>>>    		if (out_data == 1)
>>> @@ -589,7 +638,8 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>>>    		args.arg = 3;
>>>    	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
>>>
>>> -	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
>>> +	status = alienware_wmax_command(&args, sizeof(args),
>>> +					WMAX_METHOD_HDMI_SOURCE, NULL);
>>>
>>>    	if (ACPI_FAILURE(status))
>>>    		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
>>> @@ -642,8 +692,8 @@ static ssize_t show_amplifier_status(struct device *dev,
>>>    		.arg = 0,
>>>    	};
>>>    	status =
>>> -	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
>>> -				   (u32 *) &out_data);
>>> +	    alienware_wmax_command(&in_args, sizeof(in_args),
>>> +				   WMAX_METHOD_AMPLIFIER_CABLE, (u32 *) &out_data);
>>>    	if (ACPI_SUCCESS(status)) {
>>>    		if (out_data == 0)
>>>    			return sysfs_emit(buf, "[unconnected] connected unknown\n");
>>> @@ -694,8 +744,8 @@ static ssize_t show_deepsleep_status(struct device *dev,
>>>    	struct wmax_basic_args in_args = {
>>>    		.arg = 0,
>>>    	};
>>> -	status = alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
>>> -					(u32 *) &out_data);
>>> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
>>> +					WMAX_METHOD_DEEP_SLEEP_STATUS, (u32 *) &out_data);
>>>    	if (ACPI_SUCCESS(status)) {
>>>    		if (out_data == 0)
>>>    			return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
>>> @@ -723,8 +773,8 @@ static ssize_t toggle_deepsleep(struct device *dev,
>>>    		args.arg = 2;
>>>    	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
>>>
>>> -	status = alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL,
>>> -					NULL);
>>> +	status = alienware_wmax_command(&args, sizeof(args),
>>> +					WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
>>>
>>>    	if (ACPI_FAILURE(status))
>>>    		pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
>>> @@ -760,6 +810,160 @@ static int create_deepsleep(struct platform_device *dev)
>>>    	return ret;
>>>    }
>>>
>>> +/*
>>> + * Thermal Profile control
>>> + *  - Provides thermal profile control through the Platform Profile API
>>> + */
>>> +#define PROFILE_MASK		GENMASK(15,8)
>>> +#define PROFILE_ACTIVATE	BIT(0)
>>> +
>>> +static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
>>> +{
>>> +	return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
>>> +}
>>> +
>>> +static int thermal_profile_get(struct platform_profile_handler *pprof,
>>> +				enum platform_profile_option *profile)
>>> +{
>>> +	acpi_status status;
>>> +	u32 in_args = WMAX_ARG_GET_CURRENT_PROF;
>>> +	u32 out_data;
>>> +
>>> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
>>> +					WMAX_METHOD_THERMAL_INFORMATION, &out_data);
>>> +
>>> +	if (ACPI_FAILURE(status))
>>> +		return -EOPNOTSUPP;
>> Please return -EIO.
>>
> Ok.
>
>>> +
>>> +	if (out_data == 0xFFFFFFFF)
>>> +		return -EBADRQC;
>>> +
>>> +	switch (out_data) {
>>> +	case WMAX_THERMAL_LOW_POWER:
>>> +		*profile = PLATFORM_PROFILE_LOW_POWER;
>>> +		break;
>>> +	case WMAX_THERMAL_QUIET:
>>> +		*profile = PLATFORM_PROFILE_QUIET;
>>> +		break;
>>> +	case WMAX_THERMAL_BALANCED:
>>> +		*profile = PLATFORM_PROFILE_BALANCED;
>>> +		break;
>>> +	case WMAX_THERMAL_BALANCED_PERFORMANCE:
>>> +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
>>> +		break;
>>> +	case WMAX_THERMAL_PERFORMANCE:
>>> +	case WMAX_THERMAL_GMODE:
>>> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
>>> +		break;
>>> +	default:
>>> +		return -ENODATA;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int thermal_profile_set(struct platform_profile_handler *pprof,
>>> +				enum platform_profile_option profile)
>>> +{
>>> +	acpi_status status;
>>> +	u32 in_args;
>>> +	u32 out_data;
>>> +
>>> +	switch (profile) {
>>> +	case PLATFORM_PROFILE_LOW_POWER:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
>>> +		break;
>>> +	case PLATFORM_PROFILE_QUIET:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
>>> +		break;
>>> +	case PLATFORM_PROFILE_BALANCED:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
>>> +		break;
>>> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
>>> +		break;
>>> +	case PLATFORM_PROFILE_PERFORMANCE:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_PERFORMANCE);
>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
>>> +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
>>> +
>>> +	if (ACPI_FAILURE(status))
>>> +		return -EOPNOTSUPP;
>> Return -EIO.
>>
>>> +
>>> +	if (out_data == 0xFFFFFFFF)
>>> +		return -EBADRQC;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int gmode_thermal_profile_set(struct platform_profile_handler *pprof,
>>> +				     enum platform_profile_option profile)
>>> +{
>>> +	acpi_status status;
>>> +	u32 in_args;
>>> +	u32 out_data;
>>> +
>>> +	switch (profile) {
>>> +	case PLATFORM_PROFILE_LOW_POWER:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_LOW_POWER);
>>> +		break;
>>> +	case PLATFORM_PROFILE_QUIET:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_QUIET);
>>> +		break;
>>> +	case PLATFORM_PROFILE_BALANCED:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED);
>>> +		break;
>>> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_BALANCED_PERFORMANCE);
>>> +		break;
>>> +	case PLATFORM_PROFILE_PERFORMANCE:
>>> +		in_args = profile_to_wmax_arg(WMAX_THERMAL_GMODE);
>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	status = alienware_wmax_command(&in_args, sizeof(in_args),
>>> +					WMAX_METHOD_THERMAL_CONTROL, &out_data);
>>> +
>>> +	if (ACPI_FAILURE(status))
>>> +		return -EOPNOTSUPP;
>> Return -EIO.
>>
>> Otherwise the patch looks quite good.
>>
> Thank you! It's my first time working on the kernel.
>
>> Thanks,
>> Armin Wolf
> Your feedback is appreciated.
>
> Kurt
>
>>> +
>>> +	if (out_data == 0xFFFFFFFF)
>>> +		return -EBADRQC;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int create_thermal_profile(void)
>>> +{
>>> +	pp_handler.profile_get = thermal_profile_get;
>>> +
>>> +	if (quirks->gmode > 0)
>>> +		pp_handler.profile_set = gmode_thermal_profile_set;
>>> +	else
>>> +		pp_handler.profile_set = thermal_profile_set;
>>> +
>>> +	set_bit(PLATFORM_PROFILE_LOW_POWER, pp_handler.choices);
>>> +	set_bit(PLATFORM_PROFILE_QUIET, pp_handler.choices);
>>> +	set_bit(PLATFORM_PROFILE_BALANCED, pp_handler.choices);
>>> +	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, pp_handler.choices);
>>> +	set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
>>> +
>>> +	return platform_profile_register(&pp_handler);
>>> +}
>>> +
>>> +static void remove_thermal_profile(void)
>>> +{
>>> +	if (quirks->thermal > 0)
>>> +		platform_profile_remove();
>>> +}
>>> +
>>>    static int __init alienware_wmi_init(void)
>>>    {
>>>    	int ret;
>>> @@ -807,6 +1011,12 @@ static int __init alienware_wmi_init(void)
>>>    			goto fail_prep_deepsleep;
>>>    	}
>>>
>>> +	if (quirks->thermal > 0) {
>>> +		ret = create_thermal_profile();
>>> +		if (ret)
>>> +			goto fail_prep_thermal_profile;
>>> +	}
>>> +
>>>    	ret = alienware_zone_init(platform_device);
>>>    	if (ret)
>>>    		goto fail_prep_zones;
>>> @@ -817,6 +1027,7 @@ static int __init alienware_wmi_init(void)
>>>    	alienware_zone_exit(platform_device);
>>>    fail_prep_deepsleep:
>>>    fail_prep_amplifier:
>>> +fail_prep_thermal_profile:
>>>    fail_prep_hdmi:
>>>    	platform_device_del(platform_device);
>>>    fail_platform_device2:
>>> @@ -834,6 +1045,7 @@ static void __exit alienware_wmi_exit(void)
>>>    	if (platform_device) {
>>>    		alienware_zone_exit(platform_device);
>>>    		remove_hdmi(platform_device);
>>> +		remove_thermal_profile();
>>>    		platform_device_unregister(platform_device);
>>>    		platform_driver_unregister(&platform_driver);
>>>    	}

kernel test robot Oct. 10, 2024, 3:44 a.m. UTC | #6

Hi Kurt,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.12-rc2 next-20241009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kurt-Borja/alienware-wmi-Dell-AWCC-platform_profile-support/20241009-040025
base:   linus/master
patch link:    https://lore.kernel.org/r/20241008195642.36677-2-kuurtb%40gmail.com
patch subject: [PATCH v3] alienware-wmi: Dell AWCC platform_profile support
config: i386-randconfig-r051-20241010 (https://download.01.org/0day-ci/archive/20241010/202410101120.w4OLAnaI-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410101120.w4OLAnaI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410101120.w4OLAnaI-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/platform/x86/dell/alienware-wmi.c:822:9: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     822 |         return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
         |                ^
   1 error generated.


vim +/FIELD_PREP +822 drivers/platform/x86/dell/alienware-wmi.c

   819	
   820	static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
   821	{
 > 822		return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
   823	}
   824

Ilpo Järvinen Oct. 10, 2024, 9:46 a.m. UTC | #7

On Thu, 10 Oct 2024, kernel test robot wrote:

> Hi Kurt,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.12-rc2 next-20241009]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kurt-Borja/alienware-wmi-Dell-AWCC-platform_profile-support/20241009-040025
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20241008195642.36677-2-kuurtb%40gmail.com
> patch subject: [PATCH v3] alienware-wmi: Dell AWCC platform_profile support
> config: i386-randconfig-r051-20241010 (https://download.01.org/0day-ci/archive/20241010/202410101120.w4OLAnaI-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410101120.w4OLAnaI-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410101120.w4OLAnaI-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/platform/x86/dell/alienware-wmi.c:822:9: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      822 |         return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
>          |                ^
>    1 error generated.
> 
> 
> vim +/FIELD_PREP +822 drivers/platform/x86/dell/alienware-wmi.c
> 
>    819	
>    820	static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
>    821	{
>  > 822		return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
>    823	}
>    824	

For the one time I forget to mention that please add the necessary 
headers, it immediately bites (that was in my mind at one point but in 
the end I forgot to add it).

So please, add the headers both for FIELD_PREP() and GENMASK().

Kurt Borja Oct. 10, 2024, 6:39 p.m. UTC | #8

Thank you! I noticed the kernel thanks to the kernel robot too. I will
be sure to add them in v4.

Kurt

kernel test robot Oct. 15, 2024, 1:35 a.m. UTC | #9

Hi Kurt,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.12-rc3 next-20241014]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kurt-Borja/alienware-wmi-Dell-AWCC-platform_profile-support/20241011-184337
base:   linus/master
patch link:    https://lore.kernel.org/r/20241008195642.36677-2-kuurtb%40gmail.com
patch subject: [PATCH v3] alienware-wmi: Dell AWCC platform_profile support
config: x86_64-buildonly-randconfig-004-20241015 (https://download.01.org/0day-ci/archive/20241015/202410150939.BgH8WpE3-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241015/202410150939.BgH8WpE3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410150939.BgH8WpE3-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/platform/x86/dell/alienware-wmi.c: In function 'profile_to_wmax_arg':
>> drivers/platform/x86/dell/alienware-wmi.c:822:16: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
     822 |         return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
         |                ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_PREP +822 drivers/platform/x86/dell/alienware-wmi.c

   819	
   820	static u32 profile_to_wmax_arg(enum WMAX_THERMAL_PROFILE prof)
   821	{
 > 822		return FIELD_PREP(PROFILE_MASK, prof) | PROFILE_ACTIVATE;
   823	}
   824

[v3] alienware-wmi: Dell AWCC platform_profile support

Commit Message

Comments

Patch