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

[Armin Wolf <W_Armin@xxxxxx>]

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@xxxxxxxxxxxx>
> > Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
> > ---
> >   .../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)