Am 07.03.25 um 01:16 schrieb Kurt Borja:
Hi Armin, On Thu Mar 6, 2025 at 5:35 PM -05, Armin Wolf wrote:Am 06.03.25 um 01:56 schrieb Kurt Borja:All models with the "AWCC" WMAX device support a way of manually controlling fans. The PWM duty cycle of a fan can't be controlled directly. Instead the AWCC interface let's us tune a PWM `boost` value, which has the following empirically discovered, aproximate behavior over the PWM value: pwm = pwm_base + (pwm_boost / 255) * (pwm_max - pwm_base) Where the pwm_base is the locked PWM value controlled by the FW and pwm_boost is a value between 0 and 255. Expose this pwm_boost knob as a custom HWMON attribute. Cc: Guenter Roeck <linux@xxxxxxxxxxxx> Cc: Jean Delvare <jdelvare@xxxxxxxx> Cc: linux-hwmon@xxxxxxxxxxxxxxx Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx> --- drivers/platform/x86/dell/alienware-wmi-wmax.c | 223 ++++++++++++++++++++++++- 1 file changed, 220 insertions(+), 3 deletions(-) diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c index 20cf3371ee3c0e1ea038b3ca517e831f3b30dc29..de4e8f177aadc9552b05cc732e41ee458b761143 100644 --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c @@ -13,8 +13,11 @@ #include <linux/bits.h> #include <linux/dmi.h> #include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/minmax.h> #include <linux/moduleparam.h> #include <linux/platform_profile.h> +#include <linux/pm.h> #include <linux/units.h> #include <linux/wmi.h> #include "alienware-wmi.h" @@ -179,10 +182,12 @@ enum AWCC_THERMAL_INFORMATION_OPERATIONS { AWCC_OP_GET_FAN_MIN_RPM = 0x08, AWCC_OP_GET_FAN_MAX_RPM = 0x09, AWCC_OP_GET_CURRENT_PROFILE = 0x0B, + AWCC_OP_GET_FAN_BOOST = 0x0C, }; enum AWCC_THERMAL_CONTROL_OPERATIONS { AWCC_OP_ACTIVATE_PROFILE = 0x01, + AWCC_OP_SET_FAN_BOOST = 0x02, }; enum AWCC_GAME_SHIFT_STATUS_OPERATIONS { @@ -248,6 +253,7 @@ struct awcc_fan_data { u32 total_temps; u32 min_rpm; u32 max_rpm; + u8 suspend_cache; u8 id; }; @@ -627,6 +633,17 @@ static inline int awcc_op_get_temperature(struct wmi_device *wdev, u8 temp_id, u .arg3 = 0, }; + return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out); +} + +static inline int awcc_op_get_fan_boost(struct wmi_device *wdev, u8 fan_id, u32 *out) +{ + struct wmax_u32_args args = { + .operation = AWCC_OP_GET_FAN_BOOST, + .arg1 = fan_id, + .arg2 = 0, + .arg3 = 0, + }; return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out); } @@ -656,6 +673,19 @@ static inline int awcc_op_activate_profile(struct wmi_device *wdev, u8 profile) return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_CONTROL, &args, &out); } +static int awcc_op_set_fan_boost(struct wmi_device *wdev, u8 fan_id, u8 boost) +{ + struct wmax_u32_args args = { + .operation = AWCC_OP_SET_FAN_BOOST, + .arg1 = fan_id, + .arg2 = boost, + .arg3 = 0, + }; + u32 out; + + return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_CONTROL, &args, &out); +} + static int awcc_profile_id_to_pprof(u32 id, enum platform_profile_option *profile) { switch (id) { @@ -717,6 +747,7 @@ 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); + enum platform_profile_option profile; struct awcc_fan_data *fan; u32 state; int ret; @@ -765,6 +796,28 @@ static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, fan = priv->fan_data[channel]; switch (attr) { + case hwmon_pwm_enable: + ret = awcc_op_get_current_profile(priv->wdev, &state); + if (ret) + return ret; + + ret = awcc_profile_id_to_pprof(state, &profile); + if (ret) + return ret; + + switch (profile) { + case PLATFORM_PROFILE_PERFORMANCE:The hwmon sysfs docs say that 0 means that the fan is spinning at maximum speed. Does PLATFORM_PROFILE_PERFORMANCE guarantee that all fans are always spinning at maximum speed?Yes PERFORMANCE is full-speed for all devices I know. Manual fan control is completely disabled for that profile too. In fact I'm thinking about adding a module parameter to suppress this behavior. Not everyone may like that. That's outside the scope of these series tho.
I see no problem in exposing PLATFORM_PROFILE_PERFORMANCE as a normal platform profile, if a user does not want to use it then they can simply not select it.
If no then i suggest to drop support for 0.+ *val = 0; + break; + case PLATFORM_PROFILE_CUSTOM: + *val = 1; + break; + default: + *val = 2; + break; + } + + break; case hwmon_pwm_auto_channels_temp: bitmap_copy(val, fan->auto_channels_temp, BITS_PER_LONG); break; @@ -840,10 +893,48 @@ static int awcc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types ty return 0; } + +static int awcc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + struct awcc_priv *priv = dev_get_drvdata(dev); + int ret; + + switch (type) { + case hwmon_pwm: + switch (attr) { + case hwmon_pwm_enable: + /* + * We don't want to duplicate platform profile logic, so + * we only allow enabling manual fan control + */I do not think that having pwm1_enable brings any benefit, as the pwmX_boost attributes behave differently than pwmX attributes. I think it would be enough to document that pwmX_boost settings will only reliably work when the custom platform profile is selected.Now I realise I completely forgot about the admin-guide documentation! I'll include it in the next revision. Is this path ok? Documentation/admin-guide/laptops/alienware-wmi.rst Or should I add driver specific ABI documentation? (or both ofc) I don't want to name the file alienware-laptop because this driver is compatible with Dell G-Series too.
The name "alienware-wmi.rst" is OK. I suggest that you create driver specific ABI documentation for the boost attributes and reference them inside the laptop documentation.
+ if (val != 1) + return -EINVAL; + + ret = awcc_op_activate_profile(priv->wdev, AWCC_SPECIAL_PROFILE_CUSTOM); + if (ret) + return ret; + + if (priv->ppdev) + platform_profile_notify(priv->ppdev); + break; + default: + return -EOPNOTSUPP; + } + + break; + default: + return -EOPNOTSUPP; + } + + return 0; +} + static const struct hwmon_ops awcc_hwmon_ops = { .is_visible = awcc_hwmon_is_visible, .read = awcc_hwmon_read, .read_string = awcc_hwmon_read_string, + .write = awcc_hwmon_write, }; static const struct hwmon_channel_info * const awcc_hwmon_info[] = { @@ -864,7 +955,7 @@ static const struct hwmon_channel_info * const awcc_hwmon_info[] = { HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX ), HWMON_CHANNEL_INFO(pwm, - HWMON_PWM_AUTO_CHANNELS_TEMP, + HWMON_PWM_AUTO_CHANNELS_TEMP | HWMON_PWM_ENABLE, HWMON_PWM_AUTO_CHANNELS_TEMP, HWMON_PWM_AUTO_CHANNELS_TEMP, HWMON_PWM_AUTO_CHANNELS_TEMP, @@ -879,6 +970,75 @@ static const struct hwmon_chip_info awcc_hwmon_chip_info = { .info = awcc_hwmon_info, }; +static ssize_t pwm_boost_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + int ret, index = to_sensor_dev_attr(attr)->index;Please initialize "index" on a separate line, can remember the reverse xmas-tree order for variables.Ack.+ struct awcc_priv *priv = dev_get_drvdata(dev); + struct awcc_fan_data *fan = priv->fan_data[index]; + u32 boost; + + ret = awcc_op_get_fan_boost(priv->wdev, fan->id, &boost); + if (ret) + return ret; + + return sysfs_emit(buf, "%u\n", boost); +} + +static ssize_t pwm_boost_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + int ret, index = to_sensor_dev_attr(attr)->index; + struct awcc_priv *priv = dev_get_drvdata(dev); + struct awcc_fan_data *fan = priv->fan_data[index]; + unsigned long val; + + ret = kstrtoul(buf, 0, &val); + if (ret) + return ret; + + ret = awcc_op_set_fan_boost(priv->wdev, fan->id, clamp_val(val, 0, 255)); + + return ret ? ret : count; +} + +static SENSOR_DEVICE_ATTR_RW(pwm1_boost, pwm_boost, 0); +static SENSOR_DEVICE_ATTR_RW(pwm2_boost, pwm_boost, 1); +static SENSOR_DEVICE_ATTR_RW(pwm3_boost, pwm_boost, 2); +static SENSOR_DEVICE_ATTR_RW(pwm4_boost, pwm_boost, 3);Since those attributes are working differently than the standard pwm attributes, i suggest to instead name them fanX_boost.I went for pwm*_boost because we also export pwm*_auto_channels_temp, but I'm ok with fan*_boost too.
Having pwmX_auto_channels_temp as the only pwm attribute is fine, the fsteutates driver does something similar. Naming the attributes fanX_boost would better fir inside the fan-centric hwmon API of this driver.
+ +static umode_t pwm_boost_attr_visible(struct kobject *kobj, struct attribute *attr, int n) +{ + struct awcc_priv *priv = dev_get_drvdata(kobj_to_dev(kobj)); + + return n < priv->fan_count ? attr->mode : 0; +} + +static bool pwm_boost_group_visible(struct kobject *kobj) +{ + return true; +} + +DEFINE_SYSFS_GROUP_VISIBLE(pwm_boost); + +static struct attribute *fan_boost_attrs[] = { + &sensor_dev_attr_pwm1_boost.dev_attr.attr, + &sensor_dev_attr_pwm2_boost.dev_attr.attr, + &sensor_dev_attr_pwm3_boost.dev_attr.attr, + &sensor_dev_attr_pwm4_boost.dev_attr.attr, + NULL +}; + +static const struct attribute_group pwm_boost_group = { + .attrs = fan_boost_attrs, + .is_visible = SYSFS_GROUP_VISIBLE(pwm_boost), +}; + +static const struct attribute_group *awcc_hwmon_groups[] = { + &pwm_boost_group, + NULL +}; + static int awcc_hwmon_temps_init(struct wmi_device *wdev) { struct awcc_priv *priv = dev_get_drvdata(&wdev->dev); @@ -1011,12 +1171,50 @@ static int awcc_hwmon_init(struct wmi_device *wdev) if (ret) return ret; - priv->hwdev = devm_hwmon_device_register_with_info(&wdev->dev, "alienware_wmi", priv, - &awcc_hwmon_chip_info, NULL); + priv->hwdev = devm_hwmon_device_register_with_info(&wdev->dev, "alienware_wmi", + priv, &awcc_hwmon_chip_info, + awcc_hwmon_groups); return PTR_ERR_OR_ZERO(priv->hwdev); } +static void awcc_hwmon_suspend(struct device *dev) +{ + struct awcc_priv *priv = dev_get_drvdata(dev); + struct awcc_fan_data *fan; + unsigned int i; + u32 boost; + int ret; + + for (i = 0; i < priv->fan_count; i++) { + fan = priv->fan_data[i]; + + ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_FAN_BOOST, + fan->id, &boost); + if (ret) + fan->suspend_cache = 0;Please at least log a warning here that the fan boost value can not be restored properly.Ack. Is not propagating errors a good approach here? My idea was to try to turn off fans no matter what.
In this case it should be OK, failing to restore the fan boost value is not critical.
+ else + fan->suspend_cache = clamp_val(boost, 0, 255); + + awcc_op_set_fan_boost(priv->wdev, fan->id, 0); + } +} + +static void awcc_hwmon_resume(struct device *dev) +{ + + struct awcc_priv *priv = dev_get_drvdata(dev); + struct awcc_fan_data *fan; + unsigned int i; + + for (i = 0; i < priv->fan_count; i++) { + fan = priv->fan_data[i]; + + if (fan->suspend_cache)How does the driver restore fan boost settings with a value of 0?We set to 0 when suspending so I don't think it's necessary to restore to 0 again when resuming.
I understand, in this case everything should be fine. Thanks, Armin Wolf
Thanks, Armin Wolf+ awcc_op_set_fan_boost(priv->wdev, fan->id, fan->suspend_cache); + } +} + /* * Thermal Profile control * - Provides thermal profile control through the Platform Profile API @@ -1233,6 +1431,24 @@ static int wmax_wmi_probe(struct wmi_device *wdev, const void *context) return ret; } +static int wmax_wmi_suspend(struct device *dev) +{ + if (awcc->hwmon) + awcc_hwmon_suspend(dev); + + return 0; +} + +static int wmax_wmi_resume(struct device *dev) +{ + if (awcc->hwmon) + awcc_hwmon_resume(dev); + + return 0; +} + +DEFINE_SIMPLE_DEV_PM_OPS(wmax_wmi_pm_ops, wmax_wmi_suspend, wmax_wmi_resume); + static const struct wmi_device_id alienware_wmax_device_id_table[] = { { WMAX_CONTROL_GUID, NULL }, { }, @@ -1243,6 +1459,7 @@ static struct wmi_driver alienware_wmax_wmi_driver = { .driver = { .name = "alienware-wmi-wmax", .probe_type = PROBE_PREFER_ASYNCHRONOUS, + .pm = pm_sleep_ptr(&wmax_wmi_pm_ops), }, .id_table = alienware_wmax_device_id_table, .probe = wmax_wmi_probe,
