Re: [PATCH v2 07/10] platform/x86: alienware-wmi-wmax: Add HWMON support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Tue, 4 Mar 2025, Kurt Borja wrote:

> On Tue Mar 4, 2025 at 10:53 AM -05, Ilpo Järvinen wrote:
> > On Tue, 25 Feb 2025, Kurt Borja wrote:
> >
> >> All models with the "AWCC" WMAX device support monitoring fan speed and
> >> temperature sensors. Expose this feature through the HWMON interface.
> >> 
> >> Sensor readings are cached for 1 second before refreshing them to
> >> mitigate the performance cost of calling WMI methods.
> >> 
> >> Cc: Guenter Roeck <linux@roeck-us.net>
> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> >> ---
> >>  drivers/platform/x86/dell/Kconfig             |   1 +
> >>  .../platform/x86/dell/alienware-wmi-wmax.c    | 403 ++++++++++++++++++
> >>  2 files changed, 404 insertions(+)
> >> 
> >> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> >> index f8a0dffcaab7..85a57c01aaad 100644
> >> --- a/drivers/platform/x86/dell/Kconfig
> >> +++ b/drivers/platform/x86/dell/Kconfig
> >> @@ -43,6 +43,7 @@ config ALIENWARE_WMI_WMAX
> >>  	bool "Alienware WMAX WMI device driver"
> >>  	default y
> >>  	depends on ALIENWARE_WMI
> >> +	depends on HWMON
> >>  	select ACPI_PLATFORM_PROFILE
> >>  	help
> >>  	 Alienware WMI driver with AlienFX LED, HDMI, amplifier, deep sleep and
> >> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> >> index bbe87f91fcb6..818023a5b205 100644
> >> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
> >> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> >> @@ -9,10 +9,13 @@
> >>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >>  
> >>  #include <linux/bitfield.h>
> >> +#include <linux/bitmap.h>
> >>  #include <linux/bits.h>
> >>  #include <linux/dmi.h>
> >> +#include <linux/hwmon.h>
> >>  #include <linux/moduleparam.h>
> >>  #include <linux/platform_profile.h>
> >> +#include <linux/units.h>
> >>  #include <linux/wmi.h>
> >>  #include "alienware-wmi.h"
> >>  
> >> @@ -25,6 +28,7 @@
> >>  #define WMAX_METHOD_BRIGHTNESS			0x3
> >>  #define WMAX_METHOD_ZONE_CONTROL		0x4
> >>  
> >> +#define AWCC_METHOD_GET_FAN_SENSORS		0x13
> >>  #define AWCC_METHOD_THERMAL_INFORMATION		0x14
> >>  #define AWCC_METHOD_THERMAL_CONTROL		0x15
> >>  #define AWCC_METHOD_GAME_SHIFT_STATUS		0x25
> >> @@ -38,6 +42,10 @@
> >>  /* Arbitrary limit based on supported models */
> >>  #define AWCC_MAX_RES_COUNT			16
> >>  
> >> +static bool force_hwmon;
> >> +module_param_unsafe(force_hwmon, bool, 0);
> >> +MODULE_PARM_DESC(force_hwmon, "Force probing for HWMON support without checking if the WMI backend is available");
> >> +
> >>  static bool force_platform_profile;
> >>  module_param_unsafe(force_platform_profile, bool, 0);
> >>  MODULE_PARM_DESC(force_platform_profile, "Forces auto-detecting thermal profiles without checking if WMI thermal backend is available");
> >> @@ -47,16 +55,19 @@ module_param_unsafe(force_gmode, bool, 0);
> >>  MODULE_PARM_DESC(force_gmode, "Forces G-Mode when performance profile is selected");
> >>  
> >>  struct awcc_quirks {
> >> +	bool hwmon;
> >>  	bool pprof;
> >>  	bool gmode;
> >>  };
> >>  
> >>  static struct awcc_quirks g_series_quirks = {
> >> +	.hwmon = true,
> >>  	.pprof = true,
> >>  	.gmode = true,
> >>  };
> >>  
> >>  static struct awcc_quirks generic_quirks = {
> >> +	.hwmon = true,
> >>  	.pprof = true,
> >>  	.gmode = false,
> >>  };
> >> @@ -154,9 +165,18 @@ static const struct dmi_system_id awcc_dmi_table[] __initconst = {
> >>  	},
> >>  };
> >>  
> >> +enum AWCC_GET_FAN_SENSORS_OPERATIONS {
> >> +	AWCC_OP_GET_TOTAL_FAN_TEMPS		= 0x01,
> >> +	AWCC_OP_GET_FAN_TEMP_ID			= 0x02,
> >> +};
> >> +
> >>  enum AWCC_THERMAL_INFORMATION_OPERATIONS {
> >>  	AWCC_OP_GET_SYSTEM_DESCRIPTION		= 0x02,
> >>  	AWCC_OP_GET_RESOURCE_ID			= 0x03,
> >> +	AWCC_OP_GET_TEMPERATURE			= 0x04,
> >> +	AWCC_OP_GET_FAN_RPM			= 0x05,
> >> +	AWCC_OP_GET_FAN_MIN_RPM			= 0x08,
> >> +	AWCC_OP_GET_FAN_MAX_RPM			= 0x09,
> >>  	AWCC_OP_GET_CURRENT_PROFILE		= 0x0B,
> >>  };
> >>  
> >> @@ -179,6 +199,12 @@ enum AWCC_SPECIAL_THERMAL_CODES {
> >>  	AWCC_SPECIAL_PROFILE_GMODE		= 0xAB,
> >>  };
> >>  
> >> +enum AWCC_TEMP_SENSOR_TYPES {
> >> +	AWCC_TEMP_SENSOR_CPU			= 0x01,
> >> +	AWCC_TEMP_SENSOR_GPU			= 0x06,
> >> +	AWCC_TEMP_SENSOR_LAST
> >> +};
> >> +
> >>  enum awcc_thermal_profile {
> >>  	AWCC_PROFILE_USTT_BALANCED,
> >>  	AWCC_PROFILE_USTT_BALANCED_PERFORMANCE,
> >> @@ -215,6 +241,15 @@ struct wmax_u32_args {
> >>  	u8 arg3;
> >>  } __packed;
> >>  
> >> +struct awcc_fan_data {
> >> +	unsigned long *related_temps;
> >> +	unsigned long *auto_channels_temp;
> >> +	u32 total_temps;
> >> +	u32 min_rpm;
> >> +	u32 max_rpm;
> >> +	u8 id;
> >> +};
> >> +
> >>  struct awcc_priv {
> >>  	struct wmi_device *wdev;
> >>  	union {
> >> @@ -230,6 +265,11 @@ struct awcc_priv {
> >>  
> >>  	struct device *ppdev;
> >>  	u8 supported_profiles[PLATFORM_PROFILE_LAST];
> >> +
> >> +	struct device *hwdev;
> >> +	struct awcc_fan_data **fan_data;
> >> +	unsigned int temp_sensors_size;
> >> +	unsigned long *temp_sensors;
> >>  };
> >>  
> >>  static const enum platform_profile_option awcc_mode_to_platform_profile[AWCC_PROFILE_LAST] = {
> >> @@ -494,6 +534,19 @@ static int __awcc_wmi_command(struct wmi_device *wdev, u32 method_id,
> >>  	return 0;
> >>  }
> >>  
> >> +static inline int awcc_get_fan_sensors(struct wmi_device *wdev, u8 operation,
> >> +				       u8 fan_id, u8 index, u32 *out)
> >> +{
> >> +	struct wmax_u32_args args = {
> >> +		.operation = operation,
> >> +		.arg1 = fan_id,
> >> +		.arg2 = index,
> >> +		.arg3 = 0,
> >> +	};
> >> +
> >> +	return __awcc_wmi_command(wdev, AWCC_METHOD_GET_FAN_SENSORS, &args, out);
> >> +}
> >> +
> >>  static inline int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
> >>  					   u8 arg, u32 *out)
> >>  {
> >> @@ -564,6 +617,343 @@ static inline int awcc_op_get_resource_id(struct wmi_device *wdev, u8 index, u32
> >>  	return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
> >>  }
> >>  
> >> +/*
> >> + * HWMON
> >> + *  - Provides temperature and fan speed monitoring as well as manual fan
> >> + *    control
> >> + */
> >> +static umode_t awcc_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> >> +				     u32 attr, int channel)
> >> +{
> >> +	const struct awcc_priv *priv = drvdata;
> >> +
> >> +	switch (type) {
> >> +	case hwmon_temp:
> >> +		if (channel < priv->temp_count)
> >> +			return 0444;
> >> +
> >> +		break;
> >
> > IMO, these could be written as:
> > 		return channel < priv->temp_count ? 0444 : 0;
> 
> Ack.
> 
> >
> >> +	case hwmon_fan:
> >> +		if (channel < priv->fan_count)
> >> +			return 0444;
> >> +
> >> +		break;
> >> +	case hwmon_pwm:
> >> +		if (channel < priv->fan_count)
> >> +			return 0444;
> >> +
> >> +		break;
> >> +	default:
> >> +		break;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> >> +			   u32 attr, int channel, long *val)
> >> +{
> >> +	struct awcc_priv *priv = dev_get_drvdata(dev);
> >> +	struct awcc_fan_data *fan;
> >> +	u32 state;
> >> +	int ret;
> >> +	u8 temp;
> >> +
> >> +	switch (type) {
> >> +	case hwmon_temp:
> >> +		temp = find_nth_bit(priv->temp_sensors, U8_MAX, channel);
> >> +		if (temp >= U8_MAX)
> >
> > It cannot be larger than as its type is u8??
> 
> Thanks, I forgot to change this! U8_MAX should be replaced with
> priv->temp_sensors_size.

I recall seeing another similar line somewhere else in the file so please 
take a look at that as well.

> Thank you for reviewing this set! :)
> 
> 

-- 
 i.