[RFC PATCH 0/5] platform/x86: acer-wmi: Various improvements

[RFC PATCH 0/5] platform/x86: acer-wmi: Various improvements

[Armin Wolf]

This patch series contains various improvements to the acer-wmi
driver based on user reports:

- adds support for the Acer PH14-51
- improves hwmon support
- ignores function 8 events

The changes are compile-tested only, so i would like to have them
tested on a real machine.

Armin Wolf (5):
  platform/x86: acer-wmi: Add support for Acer PH14-51
  platform/x86: acer-wmi: Rename ACER_CAP_FAN_SPEED_READ
  platform/x86: acer-wmi: Add support for temperature sensors
  platform/x86: acer-wmi: Improve error handling when reading AC status
  platform/x86: acer-wmi: Ignore AC events

 drivers/platform/x86/acer-wmi.c | 164 ++++++++++++++++++++++++--------
 1 file changed, 122 insertions(+), 42 deletions(-)

--
2.39.5

[RFC PATCH 1/5] platform/x86: acer-wmi: Add support for Acer PH14-51

[Armin Wolf]

Add the Acer Predator PT14-51 to acer_quirks to provide support
for the turbo button and predator_v4 hwmon interface.

Reported-by: Rayan Margham <rayanmargham4@gmail.com>
Closes: https://lore.kernel.org/platform-driver-x86/CACzB==6tUsCnr5musVMz-EymjTUCJfNtKzhMFYqMRU_h=kydXA@mail.gmail.com
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index d09baa3d3d90..5cff538ee67f 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -393,6 +393,13 @@ static struct quirk_entry quirk_acer_predator_ph315_53 = {
 	.gpu_fans = 1,
 };

+static struct quirk_entry quirk_acer_predator_pt14_51 = {
+	.turbo = 1,
+	.cpu_fans = 1,
+	.gpu_fans = 1,
+	.predator_v4 = 1,
+};
+
 static struct quirk_entry quirk_acer_predator_v4 = {
 	.predator_v4 = 1,
 };
@@ -600,6 +607,15 @@ static const struct dmi_system_id acer_quirks[] __initconst = {
 		},
 		.driver_data = &quirk_acer_predator_v4,
 	},
+	{
+		.callback = dmi_matched,
+		.ident = "Acer Predator PT14-51",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Predator PT14-51"),
+		},
+		.driver_data = &quirk_acer_predator_pt14_51,
+	},
 	{
 		.callback = set_force_caps,
 		.ident = "Acer Aspire Switch 10E SW3-016",
--
2.39.5

[RFC PATCH 2/5] platform/x86: acer-wmi: Rename ACER_CAP_FAN_SPEED_READ

[Armin Wolf]

Rename ACER_CAP_FAN_SPEED_READ to ACER_CAP_HWMON to prepare for
upcomming changes in the hwmon handling code.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 5cff538ee67f..dd57787466b9 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -246,7 +246,7 @@ struct hotkey_function_type_aa {
 #define ACER_CAP_TURBO_LED		BIT(8)
 #define ACER_CAP_TURBO_FAN		BIT(9)
 #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
-#define ACER_CAP_FAN_SPEED_READ		BIT(11)
+#define ACER_CAP_HWMON			BIT(11)

 /*
  * Interface type flags
@@ -358,7 +358,7 @@ static void __init set_quirks(void)

 	if (quirks->predator_v4)
 		interface->capability |= ACER_CAP_PLATFORM_PROFILE |
-					 ACER_CAP_FAN_SPEED_READ;
+					 ACER_CAP_HWMON;
 }

 static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -2551,7 +2551,7 @@ static int acer_platform_probe(struct platform_device *device)
 			goto error_platform_profile;
 	}

-	if (has_cap(ACER_CAP_FAN_SPEED_READ)) {
+	if (has_cap(ACER_CAP_HWMON)) {
 		err = acer_wmi_hwmon_init();
 		if (err)
 			goto error_hwmon;
--
2.39.5

[RFC PATCH 3/5] platform/x86: acer-wmi: Add support for temperature sensors

[Armin Wolf]

After looking at the ACPI AML code, it seems that the command 0x0000
used with ACER_WMID_GET_GAMING_SYS_INFO_METHODID returns a bitmap of
all supported sensors available through the 0x0001 command.

Add support for this command and the additional temperature sensors.
This also fixed detection of fan sensors should the fan not spin
during device initialization.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 129 ++++++++++++++++++++++++--------
 1 file changed, 96 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index dd57787466b9..7b549920eba7 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -30,6 +30,7 @@
 #include <linux/input/sparse-keymap.h>
 #include <acpi/video.h>
 #include <linux/hwmon.h>
+#include <linux/units.h>
 #include <linux/bitfield.h>

 MODULE_AUTHOR("Carlos Corbacho");
@@ -70,7 +71,8 @@ MODULE_LICENSE("GPL");

 #define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54

-#define ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK GENMASK(20, 8)
+#define ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK GENMASK(24, 8)
+#define ACER_PREDATOR_V4_SUPPORTED_SENSORS_BIT_MASK GENMASK(40, 24)

 /*
  * Acer ACPI method GUIDs
@@ -98,9 +100,17 @@ enum acer_wmi_event_ids {
 };

 enum acer_wmi_predator_v4_sys_info_command {
-	ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS = 0x02,
-	ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED = 0x0201,
-	ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED = 0x0601,
+	ACER_WMID_CMD_GET_PREDATOR_V4_SUPPORTED_SENSORS = 0x0000,
+	ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING = 0x0001,
+	ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS = 0x0002,
+};
+
+enum acer_wmi_predator_v4_sensor_id {
+	ACER_WMID_SENSOR_CPU_TEMPERATURE = 0x01,
+	ACER_WMID_SENSOR_CPU_FAN_SPEED = 0x02,
+	ACER_WMID_SENSOR_EXTERNAL_TEMPERATURE_2 = 0x03,
+	ACER_WMID_SENSOR_GPU_FAN_SPEED = 0x06,
+	ACER_WMID_SENSOR_GPU_TEMPERATURE = 0x0A,
 };

 static const struct key_entry acer_wmi_keymap[] __initconst = {
@@ -271,6 +281,7 @@ static u16 commun_func_bitmap;
 static u8 commun_fn_key_number;
 static bool cycle_gaming_thermal_profile = true;
 static bool predator_v4;
+static u64 supported_sensors;

 module_param(mailled, int, 0444);
 module_param(brightness, int, 0444);
@@ -1513,6 +1524,24 @@ static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
 	return status;
 }

+static int WMID_gaming_get_sys_info(u32 command, u64 *out)
+{
+	acpi_status status;
+	u64 result;
+
+	status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_SYS_INFO_METHODID, command, &result);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	/* The lower 8 bits must be zero for the operation to have succeeded */
+	if (result & 0xff)
+		return -EIO;
+
+	*out = result;
+
+	return 0;
+}
+
 static void WMID_gaming_set_fan_mode(u8 fan_mode)
 {
 	/* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
@@ -1760,26 +1789,6 @@ static int acer_gsensor_event(void)
 	return 0;
 }

-static int acer_get_fan_speed(int fan)
-{
-	if (quirks->predator_v4) {
-		acpi_status status;
-		u64 fanspeed;
-
-		status = WMI_gaming_execute_u64(
-			ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
-			fan == 0 ? ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED :
-				   ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED,
-			&fanspeed);
-
-		if (ACPI_FAILURE(status))
-			return -EIO;
-
-		return FIELD_GET(ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK, fanspeed);
-	}
-	return -EOPNOTSUPP;
-}
-
 /*
  *  Predator series turbo button
  */
@@ -2671,43 +2680,86 @@ static void __init create_debugfs(void)
 			   &interface->debug.wmid_devices);
 }

+static const enum acer_wmi_predator_v4_sensor_id acer_wmi_temp_channel_to_sensor_id[] = {
+	[0] = ACER_WMID_SENSOR_CPU_TEMPERATURE,
+	[1] = ACER_WMID_SENSOR_GPU_TEMPERATURE,
+	[2] = ACER_WMID_SENSOR_EXTERNAL_TEMPERATURE_2,
+};
+
+static const enum acer_wmi_predator_v4_sensor_id acer_wmi_fan_channel_to_sensor_id[] = {
+	[0] = ACER_WMID_SENSOR_CPU_FAN_SPEED,
+	[1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
+};
+
 static umode_t acer_wmi_hwmon_is_visible(const void *data,
 					 enum hwmon_sensor_types type, u32 attr,
 					 int channel)
 {
+	enum acer_wmi_predator_v4_sensor_id sensor_id;
+	const u64 *supported_sensors = data;
+
 	switch (type) {
+	case hwmon_temp:
+		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
+		break;
 	case hwmon_fan:
-		if (acer_get_fan_speed(channel) >= 0)
-			return 0444;
+		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
 		break;
 	default:
 		return 0;
 	}

+	if (*supported_sensors & BIT(sensor_id - 1))
+		return 0444;
+
 	return 0;
 }

 static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			       u32 attr, int channel, long *val)
 {
+	enum acer_wmi_predator_v4_sensor_id sensor_id;
+	u64 command, result;
 	int ret;

 	switch (type) {
+	case hwmon_temp:
+		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
+		command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING | (sensor_id << 8);
+
+		ret = WMID_gaming_get_sys_info(command, &result);
+		if (ret < 0)
+			return ret;
+
+		result = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
+		*val = result * MILLIDEGREE_PER_DEGREE;
+		return 0;
 	case hwmon_fan:
-		ret = acer_get_fan_speed(channel);
+		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
+		command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING | (sensor_id << 8);
+
+		ret = WMID_gaming_get_sys_info(command, &result);
 		if (ret < 0)
 			return ret;
-		*val = ret;
-		break;
+
+		*val = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
-
-	return 0;
 }

 static const struct hwmon_channel_info *const acer_wmi_hwmon_info[] = {
-	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT), NULL
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT,
+			   HWMON_T_INPUT,
+			   HWMON_T_INPUT
+			   ),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT
+			   ),
+	NULL
 };

 static const struct hwmon_ops acer_wmi_hwmon_ops = {
@@ -2724,9 +2776,20 @@ static int acer_wmi_hwmon_init(void)
 {
 	struct device *dev = &acer_platform_device->dev;
 	struct device *hwmon;
+	u64 result;
+	int ret;
+
+	ret = WMID_gaming_get_sys_info(ACER_WMID_CMD_GET_PREDATOR_V4_SUPPORTED_SENSORS, &result);
+	if (ret < 0)
+		return ret;
+
+	/* Return early if no sensors are available */
+	supported_sensors = FIELD_GET(ACER_PREDATOR_V4_SUPPORTED_SENSORS_BIT_MASK, result);
+	if (!supported_sensors)
+		return 0;

 	hwmon = devm_hwmon_device_register_with_info(dev, "acer",
-						     &acer_platform_driver,
+						     &supported_sensors,
 						     &acer_wmi_hwmon_chip_info,
 						     NULL);

--
2.39.5

[RFC PATCH 4/5] platform/x86: acer-wmi: Improve error handling when reading AC status

[Armin Wolf]

If a call to ACER_WMID_GET_GAMING_SYS_INFO_METHODID fails, the lower
8 bits will be non-zero. Use WMID_gaming_get_sys_info() to check for
this when reading the AC status.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 7b549920eba7..6964fea84fa1 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1951,12 +1951,9 @@ static int acer_thermal_profile_change(void)
 			return err;

 		/* Check power source */
-		status = WMI_gaming_execute_u64(
-			ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
-			ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
-
-		if (ACPI_FAILURE(status))
-			return -EIO;
+		err = WMID_gaming_get_sys_info(ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
+		if (err < 0)
+			return err;

 		switch (current_tp) {
 		case ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO:
--
2.39.5

[RFC PATCH 5/5] platform/x86: acer-wmi: Ignore AC events

[Armin Wolf]

On the Acer Swift SFG14-41, the events 8 - 1 and 8 - 0 are printed on
AC connect/disconnect. Ignore those events to avoid spamming the
kernel log with error messages.

Reported-by: Farhan Anwar <farhan.anwar8@gmail.com>
Closes: https://lore.kernel.org/platform-driver-x86/2ffb529d-e7c8-4026-a3b8-120c8e7afec8@gmail.com
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/acer-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 6964fea84fa1..73243090242d 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -97,6 +97,7 @@ enum acer_wmi_event_ids {
 	WMID_HOTKEY_EVENT = 0x1,
 	WMID_ACCEL_OR_KBD_DOCK_EVENT = 0x5,
 	WMID_GAMING_TURBO_KEY_EVENT = 0x7,
+	WMID_AC_EVENT = 0x8,
 };

 enum acer_wmi_predator_v4_sys_info_command {
@@ -2302,6 +2303,9 @@ static void acer_wmi_notify(union acpi_object *obj, void *context)
 		if (return_value.key_num == 0x5 && has_cap(ACER_CAP_PLATFORM_PROFILE))
 			acer_thermal_profile_change();
 		break;
+	case WMID_AC_EVENT:
+		/* We ignore AC events here */
+		break;
 	default:
 		pr_warn("Unknown function number - %d - %d\n",
 			return_value.function, return_value.key_num);
--
2.39.5

Re: [RFC PATCH 2/5] platform/x86: acer-wmi: Rename ACER_CAP_FAN_SPEED_READ

[Ilpo Järvinen]

On Sun, 24 Nov 2024, Armin Wolf wrote:

> Rename ACER_CAP_FAN_SPEED_READ to ACER_CAP_HWMON to prepare for
> upcomming changes in the hwmon handling code.

upcoming

-- 
 i.

> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/acer-wmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 5cff538ee67f..dd57787466b9 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -246,7 +246,7 @@ struct hotkey_function_type_aa {
>  #define ACER_CAP_TURBO_LED		BIT(8)
>  #define ACER_CAP_TURBO_FAN		BIT(9)
>  #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
> -#define ACER_CAP_FAN_SPEED_READ		BIT(11)
> +#define ACER_CAP_HWMON			BIT(11)
> 
>  /*
>   * Interface type flags
> @@ -358,7 +358,7 @@ static void __init set_quirks(void)
> 
>  	if (quirks->predator_v4)
>  		interface->capability |= ACER_CAP_PLATFORM_PROFILE |
> -					 ACER_CAP_FAN_SPEED_READ;
> +					 ACER_CAP_HWMON;
>  }
> 
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -2551,7 +2551,7 @@ static int acer_platform_probe(struct platform_device *device)
>  			goto error_platform_profile;
>  	}
> 
> -	if (has_cap(ACER_CAP_FAN_SPEED_READ)) {
> +	if (has_cap(ACER_CAP_HWMON)) {
>  		err = acer_wmi_hwmon_init();
>  		if (err)
>  			goto error_hwmon;
> --
> 2.39.5
> 

Re: [RFC PATCH 3/5] platform/x86: acer-wmi: Add support for temperature sensors

[Ilpo Järvinen]

On Sun, 24 Nov 2024, Armin Wolf wrote:

> After looking at the ACPI AML code, it seems that the command 0x0000
> used with ACER_WMID_GET_GAMING_SYS_INFO_METHODID returns a bitmap of
> all supported sensors available through the 0x0001 command.
> 
> Add support for this command and the additional temperature sensors.

I had to read quite a bit of code before understanding how this changed
what was a few hard-coded sensor IDs embedded inside the old wmi commands 
into a more generalized approach using what turned out to be a field of 
sensors. Perhaps you could explain more here.

> This also fixed detection of fan sensors should the fan not spin
> during device initialization.

This too comes out of the woods because the old way of doing things is 
entirely overlooked by this commit message.

> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/acer-wmi.c | 129 ++++++++++++++++++++++++--------
>  1 file changed, 96 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index dd57787466b9..7b549920eba7 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -30,6 +30,7 @@
>  #include <linux/input/sparse-keymap.h>
>  #include <acpi/video.h>
>  #include <linux/hwmon.h>
> +#include <linux/units.h>
>  #include <linux/bitfield.h>
> 
>  MODULE_AUTHOR("Carlos Corbacho");
> @@ -70,7 +71,8 @@ MODULE_LICENSE("GPL");
> 
>  #define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54
> 
> -#define ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK GENMASK(20, 8)
> +#define ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK GENMASK(24, 8)
> +#define ACER_PREDATOR_V4_SUPPORTED_SENSORS_BIT_MASK GENMASK(40, 24)
> 
>  /*
>   * Acer ACPI method GUIDs
> @@ -98,9 +100,17 @@ enum acer_wmi_event_ids {
>  };
> 
>  enum acer_wmi_predator_v4_sys_info_command {
> -	ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS = 0x02,
> -	ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED = 0x0201,
> -	ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED = 0x0601,
> +	ACER_WMID_CMD_GET_PREDATOR_V4_SUPPORTED_SENSORS = 0x0000,
> +	ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING = 0x0001,
> +	ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS = 0x0002,
> +};
> +
> +enum acer_wmi_predator_v4_sensor_id {
> +	ACER_WMID_SENSOR_CPU_TEMPERATURE = 0x01,
> +	ACER_WMID_SENSOR_CPU_FAN_SPEED = 0x02,
> +	ACER_WMID_SENSOR_EXTERNAL_TEMPERATURE_2 = 0x03,
> +	ACER_WMID_SENSOR_GPU_FAN_SPEED = 0x06,
> +	ACER_WMID_SENSOR_GPU_TEMPERATURE = 0x0A,

Please align values in both enums.

>  };
> 
>  static const struct key_entry acer_wmi_keymap[] __initconst = {
> @@ -271,6 +281,7 @@ static u16 commun_func_bitmap;
>  static u8 commun_fn_key_number;
>  static bool cycle_gaming_thermal_profile = true;
>  static bool predator_v4;
> +static u64 supported_sensors;
> 
>  module_param(mailled, int, 0444);
>  module_param(brightness, int, 0444);
> @@ -1513,6 +1524,24 @@ static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
>  	return status;
>  }
> 
> +static int WMID_gaming_get_sys_info(u32 command, u64 *out)
> +{
> +	acpi_status status;
> +	u64 result;
> +
> +	status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_SYS_INFO_METHODID, command, &result);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	/* The lower 8 bits must be zero for the operation to have succeeded */
> +	if (result & 0xff)

Could we name this field and use FIELD_GET()?

> +		return -EIO;
> +
> +	*out = result;
> +
> +	return 0;
> +}
> +
>  static void WMID_gaming_set_fan_mode(u8 fan_mode)
>  {
>  	/* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
> @@ -1760,26 +1789,6 @@ static int acer_gsensor_event(void)
>  	return 0;
>  }
> 
> -static int acer_get_fan_speed(int fan)
> -{
> -	if (quirks->predator_v4) {
> -		acpi_status status;
> -		u64 fanspeed;
> -
> -		status = WMI_gaming_execute_u64(
> -			ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
> -			fan == 0 ? ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED :
> -				   ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED,
> -			&fanspeed);
> -
> -		if (ACPI_FAILURE(status))
> -			return -EIO;
> -
> -		return FIELD_GET(ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK, fanspeed);
> -	}
> -	return -EOPNOTSUPP;
> -}
> -
>  /*
>   *  Predator series turbo button
>   */
> @@ -2671,43 +2680,86 @@ static void __init create_debugfs(void)
>  			   &interface->debug.wmid_devices);
>  }
> 
> +static const enum acer_wmi_predator_v4_sensor_id acer_wmi_temp_channel_to_sensor_id[] = {
> +	[0] = ACER_WMID_SENSOR_CPU_TEMPERATURE,
> +	[1] = ACER_WMID_SENSOR_GPU_TEMPERATURE,
> +	[2] = ACER_WMID_SENSOR_EXTERNAL_TEMPERATURE_2,
> +};
> +
> +static const enum acer_wmi_predator_v4_sensor_id acer_wmi_fan_channel_to_sensor_id[] = {
> +	[0] = ACER_WMID_SENSOR_CPU_FAN_SPEED,
> +	[1] = ACER_WMID_SENSOR_GPU_FAN_SPEED,
> +};
> +
>  static umode_t acer_wmi_hwmon_is_visible(const void *data,
>  					 enum hwmon_sensor_types type, u32 attr,
>  					 int channel)
>  {
> +	enum acer_wmi_predator_v4_sensor_id sensor_id;
> +	const u64 *supported_sensors = data;
> +
>  	switch (type) {
> +	case hwmon_temp:
> +		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
> +		break;
>  	case hwmon_fan:
> -		if (acer_get_fan_speed(channel) >= 0)
> -			return 0444;
> +		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
>  		break;
>  	default:
>  		return 0;
>  	}
> 
> +	if (*supported_sensors & BIT(sensor_id - 1))
> +		return 0444;
> +
>  	return 0;
>  }
> 
>  static int acer_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			       u32 attr, int channel, long *val)
>  {
> +	enum acer_wmi_predator_v4_sensor_id sensor_id;
> +	u64 command, result;
>  	int ret;
> 
>  	switch (type) {
> +	case hwmon_temp:
> +		sensor_id = acer_wmi_temp_channel_to_sensor_id[channel];
> +		command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING | (sensor_id << 8);

Please use FIELD_PREP() and a named define.

> +
> +		ret = WMID_gaming_get_sys_info(command, &result);
> +		if (ret < 0)
> +			return ret;
> +
> +		result = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
> +		*val = result * MILLIDEGREE_PER_DEGREE;
> +		return 0;
>  	case hwmon_fan:
> -		ret = acer_get_fan_speed(channel);
> +		sensor_id = acer_wmi_fan_channel_to_sensor_id[channel];
> +		command = ACER_WMID_CMD_GET_PREDATOR_V4_SENSOR_READING | (sensor_id << 8);

Ditto.

-- 
 i.

> +
> +		ret = WMID_gaming_get_sys_info(command, &result);
>  		if (ret < 0)
>  			return ret;
> -		*val = ret;
> -		break;
> +
> +		*val = FIELD_GET(ACER_PREDATOR_V4_SENSOR_READING_BIT_MASK, result);
> +		return 0;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> -
> -	return 0;
>  }
> 
>  static const struct hwmon_channel_info *const acer_wmi_hwmon_info[] = {
> -	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT), NULL
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT,
> +			   HWMON_T_INPUT,
> +			   HWMON_T_INPUT
> +			   ),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT
> +			   ),
> +	NULL
>  };
> 
>  static const struct hwmon_ops acer_wmi_hwmon_ops = {
> @@ -2724,9 +2776,20 @@ static int acer_wmi_hwmon_init(void)
>  {
>  	struct device *dev = &acer_platform_device->dev;
>  	struct device *hwmon;
> +	u64 result;
> +	int ret;
> +
> +	ret = WMID_gaming_get_sys_info(ACER_WMID_CMD_GET_PREDATOR_V4_SUPPORTED_SENSORS, &result);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Return early if no sensors are available */
> +	supported_sensors = FIELD_GET(ACER_PREDATOR_V4_SUPPORTED_SENSORS_BIT_MASK, result);
> +	if (!supported_sensors)
> +		return 0;
> 
>  	hwmon = devm_hwmon_device_register_with_info(dev, "acer",
> -						     &acer_platform_driver,
> +						     &supported_sensors,
>  						     &acer_wmi_hwmon_chip_info,
>  						     NULL);
> 
> --
> 2.39.5
> 

Re: [RFC PATCH 4/5] platform/x86: acer-wmi: Improve error handling when reading AC status

[Ilpo Järvinen]

On Sun, 24 Nov 2024, Armin Wolf wrote:

> If a call to ACER_WMID_GET_GAMING_SYS_INFO_METHODID fails, the lower
> 8 bits will be non-zero. Use WMID_gaming_get_sys_info() to check for
> this when reading the AC status.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/acer-wmi.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 7b549920eba7..6964fea84fa1 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1951,12 +1951,9 @@ static int acer_thermal_profile_change(void)
>  			return err;
> 
>  		/* Check power source */
> -		status = WMI_gaming_execute_u64(
> -			ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
> -			ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
> -
> -		if (ACPI_FAILURE(status))
> -			return -EIO;
> +		err = WMID_gaming_get_sys_info(ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
> +		if (err < 0)
> +			return err;
> 
>  		switch (current_tp) {
>  		case ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO:
> --
> 2.39.5
> 

I'd add WMID_gaming_get_sys_info() in this change and reorder the series
so that this comes before the fan/temp/hwmon changes.

-- 
 i.

Re: [RFC PATCH 5/5] platform/x86: acer-wmi: Ignore AC events

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On Sun, 24 Nov 2024, Armin Wolf wrote:

> On the Acer Swift SFG14-41, the events 8 - 1 and 8 - 0 are printed on
> AC connect/disconnect. Ignore those events to avoid spamming the
> kernel log with error messages.
> 
> Reported-by: Farhan Anwar <farhan.anwar8@gmail.com>
> Closes: https://lore.kernel.org/platform-driver-x86/2ffb529d-e7c8-4026-a3b8-120c8e7afec8@gmail.com
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

--
 i.

> ---
>  drivers/platform/x86/acer-wmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 6964fea84fa1..73243090242d 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -97,6 +97,7 @@ enum acer_wmi_event_ids {
>  	WMID_HOTKEY_EVENT = 0x1,
>  	WMID_ACCEL_OR_KBD_DOCK_EVENT = 0x5,
>  	WMID_GAMING_TURBO_KEY_EVENT = 0x7,
> +	WMID_AC_EVENT = 0x8,
>  };
> 
>  enum acer_wmi_predator_v4_sys_info_command {
> @@ -2302,6 +2303,9 @@ static void acer_wmi_notify(union acpi_object *obj, void *context)
>  		if (return_value.key_num == 0x5 && has_cap(ACER_CAP_PLATFORM_PROFILE))
>  			acer_thermal_profile_change();
>  		break;
> +	case WMID_AC_EVENT:
> +		/* We ignore AC events here */
> +		break;
>  	default:
>  		pr_warn("Unknown function number - %d - %d\n",
>  			return_value.function, return_value.key_num);
> --
> 2.39.5
>