diff mbox series

asus-wmi: Expand support of GPU fan to read RPM and label

Message ID 20220916004623.10992-1-luke@ljones.dev (mailing list archive)
State Handled Elsewhere
Headers show
Series asus-wmi: Expand support of GPU fan to read RPM and label | expand

Commit Message

Luke D. Jones Sept. 16, 2022, 12:46 a.m. UTC 
The previously added patch to add support for pwm change for TUF laptops
also is usuable for more than TUF. The same method `0x00110014` is
used to read the fan RPM.

Add two extra attributes for reading fan2 plus fan2 label.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 36 +++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

Comments

Barnabás Pőcze Sept. 17, 2022, 5:11 p.m. UTC | #1 
Hi


2022. szeptember 16., péntek 2:46 keltezéssel, Luke D. Jones írta:

> The previously added patch to add support for pwm change for TUF laptops
> also is usuable for more than TUF. The same method `0x00110014` is
> used to read the fan RPM.
> 
> Add two extra attributes for reading fan2 plus fan2 label.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 36 +++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ae46af731de9..7fe6ce25da0a 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -72,6 +72,7 @@ module_param(fnlock_default, bool, 0444);
> [...]
> +static ssize_t fan2_input_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int value;
> +	int ret;
> +
> +	ret = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_GPU_FAN_CTRL, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	value &= 0xffff;
> +
> +	return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value * 100);

Can `value` can be negative here? I am not sure because of the `value &= 0xffff` part.
And maybe it would be better to return -ENODATA or something similar instead of printing "-1".


> +}
> [...]


Regards,
Barnabás Pőcze
Hans de Goede Sept. 19, 2022, 12:18 p.m. UTC | #2 
Hi Luke,

On 9/16/22 01:46, Luke D. Jones wrote:
> The previously added patch to add support for pwm change for TUF laptops
> also is usuable for more than TUF. The same method `0x00110014` is
> used to read the fan RPM.
> 
> Add two extra attributes for reading fan2 plus fan2 label.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 36 +++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ae46af731de9..7fe6ce25da0a 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -72,6 +72,7 @@ module_param(fnlock_default, bool, 0444);
>  
>  #define ASUS_WMI_FNLOCK_BIOS_DISABLED	BIT(0)
>  
> +#define ASUS_GPU_FAN_DESC		"gpu_fan"
>  #define ASUS_FAN_DESC			"cpu_fan"
>  #define ASUS_FAN_MFUN			0x13
>  #define ASUS_FAN_SFUN_READ		0x06
> @@ -2078,6 +2079,30 @@ static ssize_t asus_hwmon_temp1(struct device *dev,
>  }
>  
>  /* GPU fan on modern ROG laptops */
> +static ssize_t fan2_input_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int value;
> +	int ret;
> +
> +	ret = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_GPU_FAN_CTRL, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	value &= 0xffff;
> +
> +	return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value * 100);

As already mentioned since you & with 0xffff above the sign bit can never be
set, so the value is never less then < 0, so I have simplified this to:

	return sysfs_emit(buf, "%d\n", value * 100);

while merging.

> +}
> +
> +static ssize_t fan2_label_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return sprintf(buf, "%s\n", ASUS_GPU_FAN_DESC);
> +}

And here I have done s/sprintf/sysfs_emit/ with those changes
I've applied this patch to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> +
>  static ssize_t pwm2_enable_show(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
> @@ -2127,9 +2152,12 @@ static ssize_t pwm2_enable_store(struct device *dev,
>  /* Fan1 */
>  static DEVICE_ATTR_RW(pwm1);
>  static DEVICE_ATTR_RW(pwm1_enable);
> -static DEVICE_ATTR_RW(pwm2_enable);
>  static DEVICE_ATTR_RO(fan1_input);
>  static DEVICE_ATTR_RO(fan1_label);
> +/* Fan2 - GPU fan */
> +static DEVICE_ATTR_RW(pwm2_enable);
> +static DEVICE_ATTR_RO(fan2_input);
> +static DEVICE_ATTR_RO(fan2_label);
>  
>  /* Temperature */
>  static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
> @@ -2140,6 +2168,8 @@ static struct attribute *hwmon_attributes[] = {
>  	&dev_attr_pwm2_enable.attr,
>  	&dev_attr_fan1_input.attr,
>  	&dev_attr_fan1_label.attr,
> +	&dev_attr_fan2_input.attr,
> +	&dev_attr_fan2_label.attr,
>  
>  	&dev_attr_temp1_input.attr,
>  	NULL
> @@ -2160,7 +2190,9 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>  	    || attr == &dev_attr_pwm1_enable.attr) {
>  		if (asus->fan_type == FAN_TYPE_NONE)
>  			return 0;
> -	} else if (attr == &dev_attr_pwm2_enable.attr) {
> +	} else if (attr == &dev_attr_fan2_input.attr
> +	    || attr == &dev_attr_fan2_label.attr
> +	    || attr == &dev_attr_pwm2_enable.attr) {
>  		if (asus->gpu_fan_type == FAN_TYPE_NONE)
>  			return 0;
>  	} else if (attr == &dev_attr_temp1_input.attr) {
Luke D. Jones Sept. 20, 2022, 12:21 a.m. UTC | #3 
On Mon, 2022-09-19 at 13:18 +0100, Hans de Goede wrote:
> Hi Luke,
> 
> On 9/16/22 01:46, Luke D. Jones wrote:
> > The previously added patch to add support for pwm change for TUF
> > laptops
> > also is usuable for more than TUF. The same method `0x00110014` is
> > used to read the fan RPM.
> > 
> > Add two extra attributes for reading fan2 plus fan2 label.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> >  drivers/platform/x86/asus-wmi.c | 36
> > +++++++++++++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-wmi.c
> > b/drivers/platform/x86/asus-wmi.c
> > index ae46af731de9..7fe6ce25da0a 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -72,6 +72,7 @@ module_param(fnlock_default, bool, 0444);
> >  
> >  #define ASUS_WMI_FNLOCK_BIOS_DISABLED  BIT(0)
> >  
> > +#define ASUS_GPU_FAN_DESC              "gpu_fan"
> >  #define ASUS_FAN_DESC                  "cpu_fan"
> >  #define ASUS_FAN_MFUN                  0x13
> >  #define ASUS_FAN_SFUN_READ             0x06
> > @@ -2078,6 +2079,30 @@ static ssize_t asus_hwmon_temp1(struct
> > device *dev,
> >  }
> >  
> >  /* GPU fan on modern ROG laptops */
> > +static ssize_t fan2_input_show(struct device *dev,
> > +                                       struct device_attribute
> > *attr,
> > +                                       char *buf)
> > +{
> > +       struct asus_wmi *asus = dev_get_drvdata(dev);
> > +       int value;
> > +       int ret;
> > +
> > +       ret = asus_wmi_get_devstate(asus,
> > ASUS_WMI_DEVID_GPU_FAN_CTRL, &value);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       value &= 0xffff;
> > +
> > +       return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value *
> > 100);
> 
> As already mentioned since you & with 0xffff above the sign bit can
> never be
> set, so the value is never less then < 0, so I have simplified this
> to:
> 
>         return sysfs_emit(buf, "%d\n", value * 100);
> 
> while merging.

Thanks Hans really appreciate it, sorry i missed that. My workload is a
bit too large at the moment :)

Cheers,
Luke.

> 
> > +}
> > +
> > +static ssize_t fan2_label_show(struct device *dev,
> > +                                         struct device_attribute
> > *attr,
> > +                                         char *buf)
> > +{
> > +       return sprintf(buf, "%s\n", ASUS_GPU_FAN_DESC);
> > +}
> 
> And here I have done s/sprintf/sysfs_emit/ with those changes
> I've applied this patch to my review-hans branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
> 
> Regards,
> 
> Hans
> 
> 
> 
> > +
> >  static ssize_t pwm2_enable_show(struct device *dev,
> >                                 struct device_attribute *attr,
> >                                 char *buf)
> > @@ -2127,9 +2152,12 @@ static ssize_t pwm2_enable_store(struct
> > device *dev,
> >  /* Fan1 */
> >  static DEVICE_ATTR_RW(pwm1);
> >  static DEVICE_ATTR_RW(pwm1_enable);
> > -static DEVICE_ATTR_RW(pwm2_enable);
> >  static DEVICE_ATTR_RO(fan1_input);
> >  static DEVICE_ATTR_RO(fan1_label);
> > +/* Fan2 - GPU fan */
> > +static DEVICE_ATTR_RW(pwm2_enable);
> > +static DEVICE_ATTR_RO(fan2_input);
> > +static DEVICE_ATTR_RO(fan2_label);
> >  
> >  /* Temperature */
> >  static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
> > @@ -2140,6 +2168,8 @@ static struct attribute *hwmon_attributes[] =
> > {
> >         &dev_attr_pwm2_enable.attr,
> >         &dev_attr_fan1_input.attr,
> >         &dev_attr_fan1_label.attr,
> > +       &dev_attr_fan2_input.attr,
> > +       &dev_attr_fan2_label.attr,
> >  
> >         &dev_attr_temp1_input.attr,
> >         NULL
> > @@ -2160,7 +2190,9 @@ static umode_t
> > asus_hwmon_sysfs_is_visible(struct kobject *kobj,
> >             || attr == &dev_attr_pwm1_enable.attr) {
> >                 if (asus->fan_type == FAN_TYPE_NONE)
> >                         return 0;
> > -       } else if (attr == &dev_attr_pwm2_enable.attr) {
> > +       } else if (attr == &dev_attr_fan2_input.attr
> > +           || attr == &dev_attr_fan2_label.attr
> > +           || attr == &dev_attr_pwm2_enable.attr) {
> >                 if (asus->gpu_fan_type == FAN_TYPE_NONE)
> >                         return 0;
> >         } else if (attr == &dev_attr_temp1_input.attr) {
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ae46af731de9..7fe6ce25da0a 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -72,6 +72,7 @@  module_param(fnlock_default, bool, 0444);
 
 #define ASUS_WMI_FNLOCK_BIOS_DISABLED	BIT(0)
 
+#define ASUS_GPU_FAN_DESC		"gpu_fan"
 #define ASUS_FAN_DESC			"cpu_fan"
 #define ASUS_FAN_MFUN			0x13
 #define ASUS_FAN_SFUN_READ		0x06
@@ -2078,6 +2079,30 @@  static ssize_t asus_hwmon_temp1(struct device *dev,
 }
 
 /* GPU fan on modern ROG laptops */
+static ssize_t fan2_input_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int value;
+	int ret;
+
+	ret = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_GPU_FAN_CTRL, &value);
+	if (ret < 0)
+		return ret;
+
+	value &= 0xffff;
+
+	return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value * 100);
+}
+
+static ssize_t fan2_label_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return sprintf(buf, "%s\n", ASUS_GPU_FAN_DESC);
+}
+
 static ssize_t pwm2_enable_show(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
@@ -2127,9 +2152,12 @@  static ssize_t pwm2_enable_store(struct device *dev,
 /* Fan1 */
 static DEVICE_ATTR_RW(pwm1);
 static DEVICE_ATTR_RW(pwm1_enable);
-static DEVICE_ATTR_RW(pwm2_enable);
 static DEVICE_ATTR_RO(fan1_input);
 static DEVICE_ATTR_RO(fan1_label);
+/* Fan2 - GPU fan */
+static DEVICE_ATTR_RW(pwm2_enable);
+static DEVICE_ATTR_RO(fan2_input);
+static DEVICE_ATTR_RO(fan2_label);
 
 /* Temperature */
 static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
@@ -2140,6 +2168,8 @@  static struct attribute *hwmon_attributes[] = {
 	&dev_attr_pwm2_enable.attr,
 	&dev_attr_fan1_input.attr,
 	&dev_attr_fan1_label.attr,
+	&dev_attr_fan2_input.attr,
+	&dev_attr_fan2_label.attr,
 
 	&dev_attr_temp1_input.attr,
 	NULL
@@ -2160,7 +2190,9 @@  static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 	    || attr == &dev_attr_pwm1_enable.attr) {
 		if (asus->fan_type == FAN_TYPE_NONE)
 			return 0;
-	} else if (attr == &dev_attr_pwm2_enable.attr) {
+	} else if (attr == &dev_attr_fan2_input.attr
+	    || attr == &dev_attr_fan2_label.attr
+	    || attr == &dev_attr_pwm2_enable.attr) {
 		if (asus->gpu_fan_type == FAN_TYPE_NONE)
 			return 0;
 	} else if (attr == &dev_attr_temp1_input.attr) {