Re: [PATCH 08/10] platform/x86: alienware-wmi-wmax: Add support for manual fan control

[Armin Wolf <W_Armin@gmx.de>]

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)
>