| Message ID | 20250208051614.10644-9-kuurtb@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | HWMON support + DebugFS + Improvements | expand |
Am 08.02.25 um 06:16 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 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 EC and > pwm_boost is a value between 0 and 255. > > This pwm_boost knob is exposed as a standard `pwm` attribute. I am not sure if exposing this over the standard "pwm" attribute is correct here, since userspace applications expect to have full access to the fan when using the "pwm" attribute. Maybe using a custom attribute like "fanX_boost" would make sense here? Either way documenting this special behavior would be nice for future users, maybe you can write this down under Documentation/admin-guide/laptops? Thanks, Armin Wolf > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Kurt Borja <kuurtb@gmail.com> > --- > .../platform/x86/dell/alienware-wmi-wmax.c | 55 +++++++++++++++++-- > 1 file changed, 49 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c > index 5f02da7ff25f..06d6f88ea54b 100644 > --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c > +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c > @@ -13,6 +13,7 @@ > #include <linux/dmi.h> > #include <linux/hwmon.h> > #include <linux/jiffies.h> > +#include <linux/minmax.h> > #include <linux/moduleparam.h> > #include <linux/mutex.h> > #include <linux/overflow.h> > @@ -176,10 +177,12 @@ enum AWCC_THERMAL_INFORMATION_OPERATIONS { > AWCC_OP_GET_MIN_RPM = 0x08, > AWCC_OP_GET_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 { > @@ -563,12 +566,13 @@ static inline int awcc_thermal_information(struct wmi_device *wdev, u8 operation > return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out); > } > > -static inline int awcc_thermal_control(struct wmi_device *wdev, u8 profile) > +static inline int awcc_thermal_control(struct wmi_device *wdev, u8 operation, > + u8 arg1, u8 arg2) > { > struct wmax_u32_args args = { > - .operation = AWCC_OP_ACTIVATE_PROFILE, > - .arg1 = profile, > - .arg2 = 0, > + .operation = operation, > + .arg1 = arg1, > + .arg2 = arg2, > .arg3 = 0, > }; > u32 out; > @@ -684,6 +688,11 @@ static umode_t awcc_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_type > if (channel < priv->fan_count) > return 0444; > > + break; > + case hwmon_pwm: > + if (channel < priv->fan_count) > + return 0644; > + > break; > default: > break; > @@ -698,6 +707,7 @@ static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > struct awcc_priv *priv = dev_get_drvdata(dev); > struct awcc_temp_channel_data *temp; > struct awcc_fan_channel_data *fan; > + u32 fan_boost; > int ret; > > switch (type) { > @@ -742,6 +752,16 @@ static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > return -EOPNOTSUPP; > } > > + break; > + case hwmon_pwm: > + fan = &priv->fan_data[channel]; > + > + ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_FAN_BOOST, > + fan->id, &fan_boost); > + if (ret) > + return ret; > + > + *val = fan_boost; > break; > default: > return -EOPNOTSUPP; > @@ -796,10 +816,27 @@ 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); > + u8 fan_id = priv->fan_data[channel].id; > + > + switch (type) { > + case hwmon_pwm: > + return awcc_thermal_control(priv->wdev, AWCC_OP_SET_FAN_BOOST, > + fan_id, (u8)clamp_val(val, 0, 255)); > + default: > + return -EOPNOTSUPP; > + } > +} > + > 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[] = { > @@ -814,6 +851,12 @@ static const struct hwmon_channel_info * const awcc_hwmon_info[] = { > HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX, > HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX > ), > + HWMON_CHANNEL_INFO(pwm, > + HWMON_PWM_INPUT, > + HWMON_PWM_INPUT, > + HWMON_PWM_INPUT, > + HWMON_PWM_INPUT > + ), > NULL > }; > > @@ -954,8 +997,8 @@ static int awcc_platform_profile_set(struct device *dev, > } > } > > - return awcc_thermal_control(priv->wdev, > - priv->supported_profiles[profile]); > + return awcc_thermal_control(priv->wdev, AWCC_OP_ACTIVATE_PROFILE, > + priv->supported_profiles[profile], 0); > } > > static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
Am 16.02.25 um 07:12 schrieb Armin Wolf: > Am 08.02.25 um 06:16 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 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 EC and >> pwm_boost is a value between 0 and 255. >> >> This pwm_boost knob is exposed as a standard `pwm` attribute. > > I am not sure if exposing this over the standard "pwm" attribute is > correct here, > since userspace applications expect to have full access to the fan > when using the > "pwm" attribute. > > Maybe using a custom attribute like "fanX_boost" would make sense > here? Either way > documenting this special behavior would be nice for future users, > maybe you can write > this down under Documentation/admin-guide/laptops? > > Thanks, > Armin Wolf > >> Cc: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Kurt Borja <kuurtb@gmail.com> >> --- >> .../platform/x86/dell/alienware-wmi-wmax.c | 55 +++++++++++++++++-- >> 1 file changed, 49 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c >> b/drivers/platform/x86/dell/alienware-wmi-wmax.c >> index 5f02da7ff25f..06d6f88ea54b 100644 >> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c >> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c >> @@ -13,6 +13,7 @@ >> #include <linux/dmi.h> >> #include <linux/hwmon.h> >> #include <linux/jiffies.h> >> +#include <linux/minmax.h> >> #include <linux/moduleparam.h> >> #include <linux/mutex.h> >> #include <linux/overflow.h> >> @@ -176,10 +177,12 @@ enum AWCC_THERMAL_INFORMATION_OPERATIONS { >> AWCC_OP_GET_MIN_RPM = 0x08, >> AWCC_OP_GET_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 { >> @@ -563,12 +566,13 @@ static inline int >> awcc_thermal_information(struct wmi_device *wdev, u8 operation >> return __awcc_wmi_command(wdev, >> AWCC_METHOD_THERMAL_INFORMATION, &args, out); >> } >> >> -static inline int awcc_thermal_control(struct wmi_device *wdev, u8 >> profile) >> +static inline int awcc_thermal_control(struct wmi_device *wdev, u8 >> operation, >> + u8 arg1, u8 arg2) >> { >> struct wmax_u32_args args = { >> - .operation = AWCC_OP_ACTIVATE_PROFILE, >> - .arg1 = profile, >> - .arg2 = 0, >> + .operation = operation, >> + .arg1 = arg1, >> + .arg2 = arg2, >> .arg3 = 0, >> }; >> u32 out; >> @@ -684,6 +688,11 @@ static umode_t awcc_hwmon_is_visible(const void >> *drvdata, enum hwmon_sensor_type >> if (channel < priv->fan_count) >> return 0444; >> >> + break; >> + case hwmon_pwm: >> + if (channel < priv->fan_count) >> + return 0644; >> + >> break; >> default: >> break; >> @@ -698,6 +707,7 @@ static int awcc_hwmon_read(struct device *dev, >> enum hwmon_sensor_types type, >> struct awcc_priv *priv = dev_get_drvdata(dev); >> struct awcc_temp_channel_data *temp; >> struct awcc_fan_channel_data *fan; >> + u32 fan_boost; >> int ret; >> >> switch (type) { >> @@ -742,6 +752,16 @@ static int awcc_hwmon_read(struct device *dev, >> enum hwmon_sensor_types type, >> return -EOPNOTSUPP; >> } >> >> + break; >> + case hwmon_pwm: >> + fan = &priv->fan_data[channel]; >> + >> + ret = awcc_thermal_information(priv->wdev, >> AWCC_OP_GET_FAN_BOOST, >> + fan->id, &fan_boost); >> + if (ret) >> + return ret; >> + >> + *val = fan_boost; >> break; >> default: >> return -EOPNOTSUPP; >> @@ -796,10 +816,27 @@ static int awcc_hwmon_read_string(struct device >> *dev, enum hwmon_sensor_types ty >> return 0; >> } >> >> + I nearly forgot: Please don't use multiple blank lines. Thanks, Armin Wolf >> >> +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); >> + u8 fan_id = priv->fan_data[channel].id; >> + >> + switch (type) { >> + case hwmon_pwm: >> + return awcc_thermal_control(priv->wdev, AWCC_OP_SET_FAN_BOOST, >> + fan_id, (u8)clamp_val(val, 0, 255)); >> + default: >> + return -EOPNOTSUPP; >> + } >> +} >> + >> 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[] = { >> @@ -814,6 +851,12 @@ static const struct hwmon_channel_info * const >> awcc_hwmon_info[] = { >> HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | >> HWMON_F_MAX, >> HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | >> HWMON_F_MAX >> ), >> + HWMON_CHANNEL_INFO(pwm, >> + HWMON_PWM_INPUT, >> + HWMON_PWM_INPUT, >> + HWMON_PWM_INPUT, >> + HWMON_PWM_INPUT >> + ), >> NULL >> }; >> >> @@ -954,8 +997,8 @@ static int awcc_platform_profile_set(struct >> device *dev, >> } >> } >> >> - return awcc_thermal_control(priv->wdev, >> - priv->supported_profiles[profile]); >> + return awcc_thermal_control(priv->wdev, AWCC_OP_ACTIVATE_PROFILE, >> + priv->supported_profiles[profile], 0); >> } >> >> static int awcc_platform_profile_probe(void *drvdata, unsigned long >> *choices) >
On Sun Feb 16, 2025 at 1:12 AM -05, Armin Wolf wrote: > Am 08.02.25 um 06:16 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 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 EC and >> pwm_boost is a value between 0 and 255. >> >> This pwm_boost knob is exposed as a standard `pwm` attribute. > > I am not sure if exposing this over the standard "pwm" attribute is correct here, > since userspace applications expect to have full access to the fan when using the > "pwm" attribute. > > Maybe using a custom attribute like "fanX_boost" would make sense here? Either way Actually, in the first draft of this patch I actually did this. I exposed it as pwmX_boost because it behaved like a "percentage" kind of thing. I'm ok with fanX_boost tho, it's more explicit in it's intention. > documenting this special behavior would be nice for future users, maybe you can write > this down under Documentation/admin-guide/laptops? Sure! I will. I also want to document the legacy driver features, just like an historical archive, although some people still use these features.
diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c index 5f02da7ff25f..06d6f88ea54b 100644 --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c @@ -13,6 +13,7 @@ #include <linux/dmi.h> #include <linux/hwmon.h> #include <linux/jiffies.h> +#include <linux/minmax.h> #include <linux/moduleparam.h> #include <linux/mutex.h> #include <linux/overflow.h> @@ -176,10 +177,12 @@ enum AWCC_THERMAL_INFORMATION_OPERATIONS { AWCC_OP_GET_MIN_RPM = 0x08, AWCC_OP_GET_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 { @@ -563,12 +566,13 @@ static inline int awcc_thermal_information(struct wmi_device *wdev, u8 operation return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out); } -static inline int awcc_thermal_control(struct wmi_device *wdev, u8 profile) +static inline int awcc_thermal_control(struct wmi_device *wdev, u8 operation, + u8 arg1, u8 arg2) { struct wmax_u32_args args = { - .operation = AWCC_OP_ACTIVATE_PROFILE, - .arg1 = profile, - .arg2 = 0, + .operation = operation, + .arg1 = arg1, + .arg2 = arg2, .arg3 = 0, }; u32 out; @@ -684,6 +688,11 @@ static umode_t awcc_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_type if (channel < priv->fan_count) return 0444; + break; + case hwmon_pwm: + if (channel < priv->fan_count) + return 0644; + break; default: break; @@ -698,6 +707,7 @@ static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, struct awcc_priv *priv = dev_get_drvdata(dev); struct awcc_temp_channel_data *temp; struct awcc_fan_channel_data *fan; + u32 fan_boost; int ret; switch (type) { @@ -742,6 +752,16 @@ static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, return -EOPNOTSUPP; } + break; + case hwmon_pwm: + fan = &priv->fan_data[channel]; + + ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_FAN_BOOST, + fan->id, &fan_boost); + if (ret) + return ret; + + *val = fan_boost; break; default: return -EOPNOTSUPP; @@ -796,10 +816,27 @@ 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); + u8 fan_id = priv->fan_data[channel].id; + + switch (type) { + case hwmon_pwm: + return awcc_thermal_control(priv->wdev, AWCC_OP_SET_FAN_BOOST, + fan_id, (u8)clamp_val(val, 0, 255)); + default: + return -EOPNOTSUPP; + } +} + 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[] = { @@ -814,6 +851,12 @@ static const struct hwmon_channel_info * const awcc_hwmon_info[] = { HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX, HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX ), + HWMON_CHANNEL_INFO(pwm, + HWMON_PWM_INPUT, + HWMON_PWM_INPUT, + HWMON_PWM_INPUT, + HWMON_PWM_INPUT + ), NULL }; @@ -954,8 +997,8 @@ static int awcc_platform_profile_set(struct device *dev, } } - return awcc_thermal_control(priv->wdev, - priv->supported_profiles[profile]); + return awcc_thermal_control(priv->wdev, AWCC_OP_ACTIVATE_PROFILE, + priv->supported_profiles[profile], 0); } static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
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 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 EC and pwm_boost is a value between 0 and 255. This pwm_boost knob is exposed as a standard `pwm` attribute. Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Kurt Borja <kuurtb@gmail.com> --- .../platform/x86/dell/alienware-wmi-wmax.c | 55 +++++++++++++++++-- 1 file changed, 49 insertions(+), 6 deletions(-)