Re: [PATCH 1/3] platform/x86: asus: cleanup AGFN fan handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 15, 2019 at 9:54 AM Daniel Drake <drake@xxxxxxxxxxxx> wrote:
>
> The asus-wmi driver currently uses the "AGFN" interface and
> the FAN_CTRL device for fan control. According to the spec, this
> interface is very dated and marked as pending removal from products
> currently in development.
>
> Clean up the way that the AGFN fan is detected and handled, also
> preparing the driver for the introduction of an alternate fan
> control method needed to support recent Asus products.
>
> Not anticipating further development of this interface, simplify
> the code by dropping any notion of being able to control multiple
> AGFN fans (this was already limited to just a single fan through only
> exposing a single fan in sysfs).
>
> Check for the presence of AGFN fans at probe time, simplifying the code
> flow in asus_hwmon_sysfs_is_visible().
>

Thanks for the patch series!

Unfortunately it's not applicable.

Also, please use full driver name in the title, i.e. asus-wmi in this case.

> Signed-off-by: Daniel Drake <drake@xxxxxxxxxxxx>
> ---
>  drivers/platform/x86/asus-wmi.c            | 238 +++++++++------------
>  include/linux/platform_data/x86/asus-wmi.h |   4 +-
>  2 files changed, 109 insertions(+), 133 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 65ce96dffdb1..0a1410ea2250 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -78,6 +78,8 @@ MODULE_LICENSE("GPL");
>  #define ASUS_FAN_MFUN                  0x13
>  #define ASUS_FAN_SFUN_READ             0x06
>  #define ASUS_FAN_SFUN_WRITE            0x07
> +
> +/* Based on standard hwmon pwmX_enable values */
>  #define ASUS_FAN_CTRL_MANUAL           1
>  #define ASUS_FAN_CTRL_AUTO             2
>
> @@ -133,7 +135,7 @@ struct agfn_args {
>  } __packed;
>
>  /* struct used for calling fan read and write methods */
> -struct fan_args {
> +struct agfn_fan_args {
>         struct agfn_args agfn;  /* common fields */
>         u8 fan;                 /* fan number: 0: set auto mode 1: 1st fan */
>         u32 speed;              /* read: RPM/100 - write: 0-255 */
> @@ -161,6 +163,11 @@ struct asus_rfkill {
>         u32 dev_id;
>  };
>
> +enum fan_type {
> +       FAN_TYPE_NONE = 0,
> +       FAN_TYPE_AGFN,          /* deprecated on newer platforms */
> +};
> +
>  struct asus_wmi {
>         int dsts_id;
>         int spec;
> @@ -191,9 +198,9 @@ struct asus_wmi {
>         struct asus_rfkill gps;
>         struct asus_rfkill uwb;
>
> -       bool asus_hwmon_fan_manual_mode;
> -       int asus_hwmon_num_fans;
> -       int asus_hwmon_pwm;
> +       enum fan_type fan_type;
> +       int fan_pwm_mode;
> +       int agfn_pwm;
>
>         bool fan_boost_mode_available;
>         u8 fan_boost_mode_mask;
> @@ -1139,10 +1146,10 @@ static void asus_wmi_set_als(void)
>
>  /* Hwmon device ***************************************************************/
>
> -static int asus_hwmon_agfn_fan_speed_read(struct asus_wmi *asus, int fan,
> +static int asus_agfn_fan_speed_read(struct asus_wmi *asus, int fan,
>                                           int *speed)
>  {
> -       struct fan_args args = {
> +       struct agfn_fan_args args = {
>                 .agfn.len = sizeof(args),
>                 .agfn.mfun = ASUS_FAN_MFUN,
>                 .agfn.sfun = ASUS_FAN_SFUN_READ,
> @@ -1166,10 +1173,10 @@ static int asus_hwmon_agfn_fan_speed_read(struct asus_wmi *asus, int fan,
>         return 0;
>  }
>
> -static int asus_hwmon_agfn_fan_speed_write(struct asus_wmi *asus, int fan,
> +static int asus_agfn_fan_speed_write(struct asus_wmi *asus, int fan,
>                                      int *speed)
>  {
> -       struct fan_args args = {
> +       struct agfn_fan_args args = {
>                 .agfn.len = sizeof(args),
>                 .agfn.mfun = ASUS_FAN_MFUN,
>                 .agfn.sfun = ASUS_FAN_SFUN_WRITE,
> @@ -1189,7 +1196,7 @@ static int asus_hwmon_agfn_fan_speed_write(struct asus_wmi *asus, int fan,
>                 return -ENXIO;
>
>         if (speed && fan == 1)
> -               asus->asus_hwmon_pwm = *speed;
> +               asus->agfn_pwm = *speed;
>
>         return 0;
>  }
> @@ -1198,87 +1205,75 @@ static int asus_hwmon_agfn_fan_speed_write(struct asus_wmi *asus, int fan,
>   * Check if we can read the speed of one fan. If true we assume we can also
>   * control it.
>   */
> -static int asus_hwmon_get_fan_number(struct asus_wmi *asus, int *num_fans)
> +static bool asus_wmi_has_agfn_fan(struct asus_wmi *asus)
>  {
>         int status;
> -       int speed = 0;
> +       int speed;
> +       u32 value;
>
> -       *num_fans = 0;
> +       status = asus_agfn_fan_speed_read(asus, 1, &speed);
> +       if (status != 0)
> +               return false;
>
> -       status = asus_hwmon_agfn_fan_speed_read(asus, 1, &speed);
> -       if (!status)
> -               *num_fans = 1;
> +       status = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, &value);
> +       if (status != 0)
> +               return false;
>
> -       return 0;
> +       /*
> +        * We need to find a better way, probably using sfun,
> +        * bits or spec ...
> +        * Currently we disable it if:
> +        * - ASUS_WMI_UNSUPPORTED_METHOD is returned
> +        * - reverved bits are non-zero
> +        * - sfun and presence bit are not set
> +        */
> +       return !(value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000
> +                || (!asus->sfun && !(value & ASUS_WMI_DSTS_PRESENCE_BIT)));
>  }
>
> -static int asus_hwmon_fan_set_auto(struct asus_wmi *asus)
> +static int asus_fan_set_auto(struct asus_wmi *asus)
>  {
>         int status;
>
> -       status = asus_hwmon_agfn_fan_speed_write(asus, 0, NULL);
> +       status = asus_agfn_fan_speed_write(asus, 0, NULL);
>         if (status)
>                 return -ENXIO;
>
> -       asus->asus_hwmon_fan_manual_mode = false;
> -
>         return 0;
>  }
>
> -static int asus_hwmon_fan_rpm_show(struct device *dev, int fan)
> +static ssize_t pwm1_show(struct device *dev,
> +                              struct device_attribute *attr,
> +                              char *buf)
>  {
>         struct asus_wmi *asus = dev_get_drvdata(dev);
> -       int value;
> -       int ret;
> -
> -       /* no speed readable on manual mode */
> -       if (asus->asus_hwmon_fan_manual_mode)
> -               return -ENXIO;
> -
> -       ret = asus_hwmon_agfn_fan_speed_read(asus, fan+1, &value);
> -       if (ret) {
> -               pr_warn("reading fan speed failed: %d\n", ret);
> -               return -ENXIO;
> -       }
> -
> -       return value;
> -}
> -
> -static void asus_hwmon_pwm_show(struct asus_wmi *asus, int fan, int *value)
> -{
>         int err;
> +       int value;
>
> -       if (asus->asus_hwmon_pwm >= 0) {
> -               *value = asus->asus_hwmon_pwm;
> -               return;
> -       }
> +       /* If we already set a value then just return it */
> +       if (asus->agfn_pwm >= 0)
> +               return sprintf(buf, "%d\n", asus->agfn_pwm);
>
> -       err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, value);
> +       /*
> +        * If we haven't set already set a value through the AGFN interface,
> +        * we read a current value through the (now-deprecated) FAN_CTRL device.
> +        */
> +       err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, &value);
>         if (err < 0)
> -               return;
> +               return err;
>
> -       *value &= 0xFF;
> -
> -       if (*value == 1) /* Low Speed */
> -               *value = 85;
> -       else if (*value == 2)
> -               *value = 170;
> -       else if (*value == 3)
> -               *value = 255;
> -       else if (*value) {
> -               pr_err("Unknown fan speed %#x\n", *value);
> -               *value = -1;
> +       value &= 0xFF;
> +
> +       if (value == 1) /* Low Speed */
> +               value = 85;
> +       else if (value == 2)
> +               value = 170;
> +       else if (value == 3)
> +               value = 255;
> +       else if (value) {
> +               pr_err("Unknown fan speed %#x\n", value);
> +               value = -1;
>         }
> -}
> -
> -static ssize_t pwm1_show(struct device *dev,
> -                              struct device_attribute *attr,
> -                              char *buf)
> -{
> -       struct asus_wmi *asus = dev_get_drvdata(dev);
> -       int value;
> -
> -       asus_hwmon_pwm_show(asus, 0, &value);
>
>         return sprintf(buf, "%d\n", value);
>  }
> @@ -1298,11 +1293,11 @@ static ssize_t pwm1_store(struct device *dev,
>
>         value = clamp(value, 0, 255);
>
> -       state = asus_hwmon_agfn_fan_speed_write(asus, 1, &value);
> +       state = asus_agfn_fan_speed_write(asus, 1, &value);
>         if (state)
>                 pr_warn("Setting fan speed failed: %d\n", state);
>         else
> -               asus->asus_hwmon_fan_manual_mode = true;
> +               asus->fan_pwm_mode = ASUS_FAN_CTRL_MANUAL;
>
>         return count;
>  }
> @@ -1311,10 +1306,21 @@ static ssize_t fan1_input_show(struct device *dev,
>                                         struct device_attribute *attr,
>                                         char *buf)
>  {
> -       int value = asus_hwmon_fan_rpm_show(dev, 0);
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +       int value;
> +       int ret;
>
> -       return sprintf(buf, "%d\n", value < 0 ? -1 : value*100);
> +       /* no speed readable on manual mode */
> +       if (asus->fan_pwm_mode == ASUS_FAN_CTRL_MANUAL)
> +               return -ENXIO;
> +
> +       ret = asus_agfn_fan_speed_read(asus, 1, &value);
> +       if (ret) {
> +               pr_warn("reading fan speed failed: %d\n", ret);
> +               return -ENXIO;
> +       }
>
> +       return sprintf(buf, "%d\n", value < 0 ? -1 : value*100);
>  }
>
>  static ssize_t pwm1_enable_show(struct device *dev,
> @@ -1323,10 +1329,7 @@ static ssize_t pwm1_enable_show(struct device *dev,
>  {
>         struct asus_wmi *asus = dev_get_drvdata(dev);
>
> -       if (asus->asus_hwmon_fan_manual_mode)
> -               return sprintf(buf, "%d\n", ASUS_FAN_CTRL_MANUAL);
> -
> -       return sprintf(buf, "%d\n", ASUS_FAN_CTRL_AUTO);
> +       return sprintf(buf, "%d\n", asus->fan_pwm_mode);
>  }
>
>  static ssize_t pwm1_enable_store(struct device *dev,
> @@ -1343,14 +1346,21 @@ static ssize_t pwm1_enable_store(struct device *dev,
>         if (ret)
>                 return ret;
>
> -       if (state == ASUS_FAN_CTRL_MANUAL)
> -               asus->asus_hwmon_fan_manual_mode = true;
> -       else
> -               status = asus_hwmon_fan_set_auto(asus);
> +       switch (state) {
> +       case ASUS_FAN_CTRL_MANUAL:
> +               break;
>
> -       if (status)
> -               return status;
> +       case ASUS_FAN_CTRL_AUTO:
> +               status = asus_fan_set_auto(asus);
> +               if (status)
> +                       return status;
> +               break;
>
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       asus->fan_pwm_mode = state;
>         return count;
>  }
>
> @@ -1404,59 +1414,31 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>         struct device *dev = container_of(kobj, struct device, kobj);
>         struct platform_device *pdev = to_platform_device(dev->parent);
>         struct asus_wmi *asus = platform_get_drvdata(pdev);
> -       int dev_id = -1;
> -       int fan_attr = -1;
>         u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
> -       bool ok = true;
> -
> -       if (attr == &dev_attr_pwm1.attr)
> -               dev_id = ASUS_WMI_DEVID_FAN_CTRL;
> -       else if (attr == &dev_attr_temp1_input.attr)
> -               dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;
> -
>
>         if (attr == &dev_attr_fan1_input.attr
>             || attr == &dev_attr_fan1_label.attr
>             || attr == &dev_attr_pwm1.attr
>             || attr == &dev_attr_pwm1_enable.attr) {
> -               fan_attr = 1;
> -       }
> -
> -       if (dev_id != -1) {
> -               int err = asus_wmi_get_devstate(asus, dev_id, &value);
> +               if (asus->fan_type == FAN_TYPE_NONE)
> +                       return 0;
> +       } else if (attr == &dev_attr_temp1_input.attr) {
> +               int err = asus_wmi_get_devstate(asus,
> +                                               ASUS_WMI_DEVID_THERMAL_CTRL,
> +                                               &value);
>
> -               if (err < 0 && fan_attr == -1)
> +               if (err < 0)
>                         return 0; /* can't return negative here */
> -       }
>
> -       if (dev_id == ASUS_WMI_DEVID_FAN_CTRL) {
> -               /*
> -                * We need to find a better way, probably using sfun,
> -                * bits or spec ...
> -                * Currently we disable it if:
> -                * - ASUS_WMI_UNSUPPORTED_METHOD is returned
> -                * - reverved bits are non-zero
> -                * - sfun and presence bit are not set
> -                */
> -               if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000
> -                   || (!asus->sfun && !(value & ASUS_WMI_DSTS_PRESENCE_BIT)))
> -                       ok = false;
> -               else
> -                       ok = fan_attr <= asus->asus_hwmon_num_fans;
> -       } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) {
>                 /*
>                  * If the temperature value in deci-Kelvin is near the absolute
>                  * zero temperature, something is clearly wrong
>                  */
>                 if (value == 0 || value == 1)
> -                       ok = false;
> -       } else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) {
> -               ok = true;
> -       } else {
> -               ok = false;
> +                       return 0;
>         }
>
> -       return ok ? attr->mode : 0;
> +       return attr->mode;
>  }
>
>  static const struct attribute_group hwmon_attribute_group = {
> @@ -1482,21 +1464,16 @@ static int asus_wmi_hwmon_init(struct asus_wmi *asus)
>
>  static int asus_wmi_fan_init(struct asus_wmi *asus)
>  {
> -       int status;
> -
> -       asus->asus_hwmon_pwm = -1;
> -       asus->asus_hwmon_num_fans = -1;
> -       asus->asus_hwmon_fan_manual_mode = false;
> +       asus->fan_type = FAN_TYPE_NONE;
> +       asus->agfn_pwm = -1;
>
> -       status = asus_hwmon_get_fan_number(asus, &asus->asus_hwmon_num_fans);
> -       if (status) {
> -               asus->asus_hwmon_num_fans = 0;
> -               pr_warn("Could not determine number of fans: %d\n", status);
> -               return -ENXIO;
> +       if (asus_wmi_has_agfn_fan(asus)) {
> +               asus->fan_type = FAN_TYPE_AGFN;
> +               asus_fan_set_auto(asus);
> +               asus->fan_pwm_mode = ASUS_FAN_CTRL_AUTO;
>         }
>
> -       pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
> -       return 0;
> +       return asus->fan_type != FAN_TYPE_NONE;
>  }
>
>  /* Fan mode *******************************************************************/
> @@ -2348,7 +2325,6 @@ static int asus_wmi_add(struct platform_device *pdev)
>                 goto fail_input;
>
>         err = asus_wmi_fan_init(asus); /* probably no problems on error */
> -       asus_hwmon_fan_set_auto(asus);
>
>         err = asus_wmi_hwmon_init(asus);
>         if (err)
> @@ -2440,7 +2416,7 @@ static int asus_wmi_remove(struct platform_device *device)
>         asus_wmi_rfkill_exit(asus);
>         asus_wmi_debugfs_exit(asus);
>         asus_wmi_sysfs_exit(asus->platform_device);
> -       asus_hwmon_fan_set_auto(asus);
> +       asus_fan_set_auto(asus);
>
>         kfree(asus);
>         return 0;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 4802cd2c7309..5ae9c062a1f6 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -12,7 +12,7 @@
>  #define ASUS_WMI_METHODID_GPID         0x44495047 /* Get Panel ID?? (Resol) */
>  #define ASUS_WMI_METHODID_QMOD         0x444F4D51 /* Quiet MODe */
>  #define ASUS_WMI_METHODID_SPLV         0x4C425053 /* Set Panel Light Value */
> -#define ASUS_WMI_METHODID_AGFN         0x4E464741 /* FaN? */
> +#define ASUS_WMI_METHODID_AGFN         0x4E464741 /* Atk Generic FuNction */
>  #define ASUS_WMI_METHODID_SFUN         0x4E554653 /* FUNCtionalities */
>  #define ASUS_WMI_METHODID_SDSP         0x50534453 /* Set DiSPlay output */
>  #define ASUS_WMI_METHODID_GDSP         0x50534447 /* Get DiSPlay output */
> @@ -72,7 +72,7 @@
>
>  /* Fan, Thermal */
>  #define ASUS_WMI_DEVID_THERMAL_CTRL    0x00110011
> -#define ASUS_WMI_DEVID_FAN_CTRL                0x00110012
> +#define ASUS_WMI_DEVID_FAN_CTRL                0x00110012 /* deprecated */
>
>  /* Power */
>  #define ASUS_WMI_DEVID_PROCESSOR_STATE 0x00120012
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux