[RFC 2/2] asus-wmi: add fan control

[RFC 2/2] asus-wmi: add fan control

[Kast Bernd]

This patch is partially based on Felipe Contrera's earlier patch, that
was discussed here: https://lkml.org/lkml/2013/10/8/800
Some problems of that patch are fixed, now:

1) The main downside of the earlier patch seemed to be the use of
virt_to_phys, thus an acpi-version of that function is used now.
(provided by the first patch of this patchset)

2) random memory corruption occurred on my notebook, thus DMA-able memory
is allocated now, which solves this problem

3) hwmon interface is used instead of the thermal interface, as a
hwmon device is already set up by this driver and seemed more
appropriate than the thermal interface

4) Calling the ACPI-functions was modularized thus it's possible to call
some multifunctions easily, now (by using
asus_wmi_evaluate_method_agfn).

Unfortunately the WMI doesn't support controlling both fans on
a dual-fan notebooks because of an restriction in the acpi-method
"SFNS", that is callable through the wmi. If "SFNV" would be called
direcly even dual fan configurations could be controlled, but not by using
wmi.

Speed readings only work on auto-mode, thus "-1" will be reported in
manual mode.
Additionally the speed readings are reported as hundreds of RPM thus
they are not too precize.

This patch is tested only on one notebook (N551JK) but a similar module,
that contained some code to try to control the second fan also, was
reported to work on an UX32VD, at least for the first fan.

As Felipe already mentioned the low-level functions are described here:
http://forum.notebookreview.com/threads/fan-control-on-asus-prime-ux31-ux31a-ux32a-ux32vd.705656/

Signed-off-by: Kast Bernd <kastbernd@gmx.de>
---
 drivers/platform/x86/asus-wmi.c | 297 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 277 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 7543a56..b16658a 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -78,6 +78,7 @@ MODULE_LICENSE("GPL");
 #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_SFUN		0x4E554653 /* FUNCtionalities */
 #define ASUS_WMI_METHODID_SDSP		0x50534453 /* Set DiSPlay output */
 #define ASUS_WMI_METHODID_GDSP		0x50534447 /* Get DiSPlay output */
@@ -150,11 +151,27 @@ MODULE_LICENSE("GPL");
 #define ASUS_WMI_DSTS_BRIGHTNESS_MASK	0x000000FF
 #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
 
+#define ASUS_FAN_DESC "cpu_fan"
+
 struct bios_args {
 	u32 arg0;
 	u32 arg1;
 } __packed;
 
+struct agfn_args {
+	u16 mfun;
+	u16 sfun;
+	u16 len;
+	u8 stas;
+	u8 err;
+} __packed;
+
+struct fan_args {
+	struct agfn_args agfn;
+	u8 fan;
+	u32 speed;
+} __packed;
+
 /*
  * <platform>/    - debugfs root directory
  *   dev_id      - current dev_id
@@ -204,6 +221,10 @@ struct asus_wmi {
 	struct asus_rfkill gps;
 	struct asus_rfkill uwb;
 
+	int asus_hwmon_pwm;
+	int asus_hwmon_num_fans;
+	int asus_hwmon_fan_manual_mode;
+
 	struct hotplug_slot *hotplug_slot;
 	struct mutex hotplug_lock;
 	struct mutex wmi_lock;
@@ -294,6 +315,39 @@ exit:
 	return 0;
 }
 
+static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
+{
+	struct acpi_buffer input;
+	u32 status;
+	u64 phys_addr;
+	u32 retval;
+
+	/* copy to dma capable address */
+	input.pointer = kzalloc(args.length, GFP_DMA | GFP_KERNEL);
+	input.length = args.length;
+	if (!input.pointer)
+		return -ENOMEM;
+
+	if (acpi_os_get_physical_address(input.pointer, &phys_addr) != AE_OK)
+		goto fail;
+
+	memcpy(input.pointer, args.pointer, args.length);
+
+	status = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN, phys_addr, 0,
+					  &retval);
+	if (status < 0)
+		goto fail;
+
+	memcpy(args.pointer, input.pointer, args.length);
+
+	kfree(input.pointer);
+	return retval;
+
+fail:
+	kfree(input.pointer);
+	return -1;
+}
+
 static int asus_wmi_get_devstate(struct asus_wmi *asus, u32 dev_id, u32 *retval)
 {
 	return asus_wmi_evaluate_method(asus->dsts_id, dev_id, 0, retval);
@@ -1022,35 +1076,187 @@ exit:
 /*
  * Hwmon device
  */
-static ssize_t asus_hwmon_pwm1(struct device *dev,
-			       struct device_attribute *attr,
-			       char *buf)
+static int asus_hwmon_agfn_fan_speed(struct asus_wmi *asus, int write, int fan,
+				     int *speed)
+{
+	int status;
+
+	struct fan_args args = {
+		.agfn.len = sizeof(args),
+		.agfn.mfun = 0x13,
+		.agfn.sfun = write ? 0x07 : 0x06,
+		.fan = fan,
+		.speed = speed ? write ? *speed : 0 : 0,
+	};
+
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+
+	status = asus_wmi_evaluate_method_agfn(input);
+
+	if (status || args.agfn.err)
+		return -1;
+
+	if (!speed)
+		return 0;
+
+	if (write) {
+		if (fan == 1 || fan == 2)
+			asus->asus_hwmon_pwm = fan > 0 ? *speed : -1;
+	} else {
+		*speed = args.speed;
+	}
+
+	return 0;
+}
+
+static int asus_hwmon_get_fan_number(struct asus_wmi *asus, int *num_fans)
+{
+	int status;
+	int speed = 0;
+
+	*num_fans = 0;
+
+	status = asus_hwmon_agfn_fan_speed(asus, 0, 1, &speed);
+	if (status)
+		return 0;
+
+	*num_fans = 1;
+	return 0;
+}
+
+static int asus_hwmon_fan_set_auto(struct asus_wmi *asus)
 {
+	int status;
+
+	status = asus_hwmon_agfn_fan_speed(asus, 1, 0, 0);
+	if (!status) {
+		asus->asus_hwmon_fan_manual_mode = 0;
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+static int asus_hwmon_fan_rpm_show(struct device *dev, int fan)
+{
+	int value;
+	int state;
 	struct asus_wmi *asus = dev_get_drvdata(dev);
-	u32 value;
+
+	/* no speed readable on manual mode */
+	if (asus->asus_hwmon_fan_manual_mode) {
+		value = -1;
+	} else {
+		state = asus_hwmon_agfn_fan_speed(asus, 0, fan+1, &value);
+		if (state) {
+			value = -1;
+			pr_warn("reading fan speed failed: %d\n", state);
+		}
+	}
+	return value;
+}
+
+static void asus_hwmon_pwm_show(struct asus_wmi *asus, int fan, int *value)
+{
 	int err;
 
-	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, &value);
+	if (asus->asus_hwmon_pwm >= 0) {
+		*value = asus->asus_hwmon_pwm;
+		return;
+	}
 
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, value);
 	if (err < 0)
-		return err;
-
-	value &= 0xFF;
+		return;
 
-	if (value == 1) /* Low Speed */
-		value = 85;
-	else if (value == 2)
-		value = 170;
-	else if (value == 3)
-		value = 255;
-	else if (value != 0) {
-		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 asus_hwmon_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);
 }
 
+static ssize_t asus_hwmon_pwm1_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count) {
+	int value;
+	int state;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	kstrtouint(buf, 10, &value);
+	if (value > 255)
+		value = 255;
+
+	state = asus_hwmon_agfn_fan_speed(asus, 1, 1, &value);
+	if (state)
+		pr_warn("Setting fan speed failed: %d\n", state);
+	else
+		asus->asus_hwmon_fan_manual_mode = 1;
+
+	return count;
+}
+
+static ssize_t asus_hwmon_fan1_rpm_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int value = asus_hwmon_fan_rpm_show(dev, 0);
+
+	return sprintf(buf, "%d\n", value < 0 ? value : value*100);
+
+}
+
+static ssize_t asus_hwmon_cur_control_state_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", asus->asus_hwmon_fan_manual_mode);
+}
+
+static ssize_t asus_hwmon_cur_control_state_store(struct device *dev,
+						  struct device_attribute *attr,
+						  const char *buf, size_t count)
+{
+	int state;
+	int status;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	kstrtouint(buf, 10, &state);
+	if (state == 0 || state == 2)
+		status = asus_hwmon_fan_set_auto(asus);
+	else if (state == 1)
+		asus->asus_hwmon_fan_manual_mode = state;
+
+	return count;
+}
+
+static ssize_t asus_hwmon_fan1_label_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return sprintf(buf, "%s\n", ASUS_FAN_DESC);
+}
+
 static ssize_t asus_hwmon_temp1(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
@@ -1069,11 +1275,24 @@ static ssize_t asus_hwmon_temp1(struct device *dev,
 	return sprintf(buf, "%d\n", value);
 }
 
-static DEVICE_ATTR(pwm1, S_IRUGO, asus_hwmon_pwm1, NULL);
+/* Fan1 */
+static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, asus_hwmon_pwm1_show,
+		   asus_hwmon_pwm1_store);
+static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
+		   asus_hwmon_cur_control_state_show,
+		   asus_hwmon_cur_control_state_store);
+static DEVICE_ATTR(fan1_input, S_IRUGO, asus_hwmon_fan1_rpm_show, NULL);
+static DEVICE_ATTR(fan1_label, S_IRUGO, asus_hwmon_fan1_label_show, NULL);
+
+/* Temperature */
 static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
 
 static struct attribute *hwmon_attributes[] = {
 	&dev_attr_pwm1.attr,
+	&dev_attr_pwm1_enable.attr,
+	&dev_attr_fan1_input.attr,
+	&dev_attr_fan1_label.attr,
+
 	&dev_attr_temp1_input.attr,
 	NULL
 };
@@ -1086,6 +1305,7 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 	struct asus_wmi *asus = platform_get_drvdata(pdev);
 	bool ok = true;
 	int dev_id = -1;
+	int fan_attr = -1;
 	u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
 
 	if (attr == &dev_attr_pwm1.attr)
@@ -1093,10 +1313,18 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 	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 (err < 0)
+		if (err < 0 && fan_attr == -1)
 			return 0; /* can't return negative here */
 	}
 
@@ -1112,10 +1340,16 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 		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 value is zero, something is clearly wrong */
-		if (value == 0)
+		if (!value)
 			ok = false;
+	} else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) {
+		ok = true;
+	} else {
+		ok = false;
 	}
 
 	return ok ? attr->mode : 0;
@@ -1723,6 +1957,25 @@ error_debugfs:
 	return -ENOMEM;
 }
 
+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 = 0;
+
+	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 -1;
+	}
+
+	pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
+	return 0;
+}
+
 /*
  * WMI Driver
  */
@@ -1756,6 +2009,9 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		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)
 		goto fail_hwmon;
@@ -1832,6 +2088,7 @@ static int asus_wmi_remove(struct platform_device *device)
 	asus_wmi_rfkill_exit(asus);
 	asus_wmi_debugfs_exit(asus);
 	asus_wmi_platform_exit(asus);
+	asus_hwmon_fan_set_auto(asus);
 
 	kfree(asus);
 	return 0;
-- 
2.3.5

[RFC 1/2] ACPI: activate&export acpi_os_get_physical_address

[Kast Bernd]

acpi_os_get_physical_address will be needed by an acpi driver (asus-wmi.c).
Additionally it could  be used by dell-laptop.c instead of directly calling virt_to_phys.

acpi_os_get_physical_address gets exported and ACPI_FUTURE_USAGE is removed

Signed-off-by: Kast Bernd <kastbernd@gmx.de>
---
 drivers/acpi/osl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index f9eeae8..566e6ad 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -524,7 +524,6 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
 }
 EXPORT_SYMBOL(acpi_os_unmap_generic_address);
 
-#ifdef ACPI_FUTURE_USAGE
 acpi_status
 acpi_os_get_physical_address(void *virt, acpi_physical_address * phys)
 {
@@ -535,7 +534,7 @@ acpi_os_get_physical_address(void *virt, acpi_physical_address * phys)
 
 	return AE_OK;
 }
-#endif
+EXPORT_SYMBOL(acpi_os_get_physical_address);
 
 #define ACPI_MAX_OVERRIDE_LEN 100
 
-- 
2.3.5

[RFC 0/2] asus notebook fan control

[Kast Bernd]

Hello,

This patchset implements a fan control for asus notebooks.
It's based on Felipe Contreras' patch
(https://lkml.org/lkml/2013/10/8/800), that was proposed one and a half
year ago, but never made it to the kernel, because of the use of
"virt_to_phys".
This problem is solved by using the acpi internal function that is
activated by the first patch (ACPI_FUTURE_USAGE is removed).
The second patch implements the actual fan control functions, with a
similar logic than Felipe, but using hwmon instead of the thermal
interface and some bugs removed.

This patchset contains the following 2 patches:
0001-ACPI-activate-export-acpi_os_get_physical_address.patch
0002-asus-wmi-add-fan-control.patch

This is my first kernel patch. I tried to respect all style rules and
implement it properly. I couldn't find any try to fix the old patch in
recent time, thus I wrote this new patch.

I would appreciate any kind of feedback to get this patch upstream.


Kast Bernd (2):
  ACPI: activate&export acpi_os_get_physical_address
  asus-wmi: add fan control

 drivers/acpi/osl.c              |   3 +-
 drivers/platform/x86/asus-wmi.c | 297 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 278 insertions(+), 22 deletions(-)

-- 
2.3.5

Re: [RFC 0/2] asus notebook fan control

[Darren Hart]

On Wed, Apr 22, 2015 at 04:12:26PM +0200, Kast Bernd wrote:
> Hello,
> 
> This patchset implements a fan control for asus notebooks.
> It's based on Felipe Contreras' patch
> (https://lkml.org/lkml/2013/10/8/800), that was proposed one and a half
> year ago, but never made it to the kernel, because of the use of
> "virt_to_phys".
> This problem is solved by using the acpi internal function that is
> activated by the first patch (ACPI_FUTURE_USAGE is removed).
> The second patch implements the actual fan control functions, with a
> similar logic than Felipe, but using hwmon instead of the thermal
> interface and some bugs removed.
> 
> This patchset contains the following 2 patches:
> 0001-ACPI-activate-export-acpi_os_get_physical_address.patch
> 0002-asus-wmi-add-fan-control.patch
> 
> This is my first kernel patch. I tried to respect all style rules and
> implement it properly. I couldn't find any try to fix the old patch in
> recent time, thus I wrote this new patch.
> 
> I would appreciate any kind of feedback to get this patch upstream.

Nice cover letter for your first patch submission, providing the context and the
reference is useful. Thanks for that.

I would suggest including Felipe on Cc (done).

Review to follow...

> 
> 
> Kast Bernd (2):
>   ACPI: activate&export acpi_os_get_physical_address
>   asus-wmi: add fan control
> 
>  drivers/acpi/osl.c              |   3 +-
>  drivers/platform/x86/asus-wmi.c | 297 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 278 insertions(+), 22 deletions(-)
> 
> -- 
> 2.3.5
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [RFC 1/2] ACPI: activate&export acpi_os_get_physical_address

[Darren Hart]

On Wed, Apr 22, 2015 at 04:12:24PM +0200, Kast Bernd wrote:
> acpi_os_get_physical_address will be needed by an acpi driver (asus-wmi.c).
> Additionally it could  be used by dell-laptop.c instead of directly calling virt_to_phys.
> 
> acpi_os_get_physical_address gets exported and ACPI_FUTURE_USAGE is removed
> 

Hrm, well... this doesn't get rid of virt_to_phys, it just wraps it really. I'm
not sure that makes this any more acceptable than the original from Felipe - but
that's not my call.

Rafael?
Len?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [RFC 2/2] asus-wmi: add fan control

[Darren Hart]

On Wed, Apr 22, 2015 at 04:12:23PM +0200, Kast Bernd wrote:
> This patch is partially based on Felipe Contrera's earlier patch, that
> was discussed here: https://lkml.org/lkml/2013/10/8/800
> Some problems of that patch are fixed, now:
> 
> 1) The main downside of the earlier patch seemed to be the use of
> virt_to_phys, thus an acpi-version of that function is used now.
> (provided by the first patch of this patchset)
> 
> 2) random memory corruption occurred on my notebook, thus DMA-able memory
> is allocated now, which solves this problem
> 
> 3) hwmon interface is used instead of the thermal interface, as a
> hwmon device is already set up by this driver and seemed more
> appropriate than the thermal interface
> 
> 4) Calling the ACPI-functions was modularized thus it's possible to call
> some multifunctions easily, now (by using
> asus_wmi_evaluate_method_agfn).
> 
> Unfortunately the WMI doesn't support controlling both fans on
> a dual-fan notebooks because of an restriction in the acpi-method
> "SFNS", that is callable through the wmi. If "SFNV" would be called
> direcly even dual fan configurations could be controlled, but not by using
> wmi.
> 
> Speed readings only work on auto-mode, thus "-1" will be reported in
> manual mode.
> Additionally the speed readings are reported as hundreds of RPM thus
> they are not too precize.
> 
> This patch is tested only on one notebook (N551JK) but a similar module,
> that contained some code to try to control the second fan also, was
> reported to work on an UX32VD, at least for the first fan.
> 
> As Felipe already mentioned the low-level functions are described here:
> http://forum.notebookreview.com/threads/fan-control-on-asus-prime-ux31-ux31a-ux32a-ux32vd.705656/
> 

Corentin, what are your thoughts?

> Signed-off-by: Kast Bernd <kastbernd@gmx.de>
> ---
>  drivers/platform/x86/asus-wmi.c | 297 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 277 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 7543a56..b16658a 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -78,6 +78,7 @@ MODULE_LICENSE("GPL");
>  #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?*/

That's a lot of ???s You sure? ;-) Missing a space after the last one. Drop the
first ones please.

>  #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 */
> @@ -150,11 +151,27 @@ MODULE_LICENSE("GPL");
>  #define ASUS_WMI_DSTS_BRIGHTNESS_MASK	0x000000FF
>  #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK	0x0000FF00
>  
> +#define ASUS_FAN_DESC "cpu_fan"
> +
>  struct bios_args {
>  	u32 arg0;
>  	u32 arg1;
>  } __packed;
>  
> +struct agfn_args {
> +	u16 mfun;
> +	u16 sfun;
> +	u16 len;
> +	u8 stas;
> +	u8 err;
> +} __packed;
> +
> +struct fan_args {
> +	struct agfn_args agfn;
> +	u8 fan;
> +	u32 speed;
> +} __packed;
> +
>  /*
>   * <platform>/    - debugfs root directory
>   *   dev_id      - current dev_id
> @@ -204,6 +221,10 @@ struct asus_wmi {
>  	struct asus_rfkill gps;
>  	struct asus_rfkill uwb;
>  
> +	int asus_hwmon_pwm;
> +	int asus_hwmon_num_fans;
> +	int asus_hwmon_fan_manual_mode;
> +
>  	struct hotplug_slot *hotplug_slot;
>  	struct mutex hotplug_lock;
>  	struct mutex wmi_lock;
> @@ -294,6 +315,39 @@ exit:
>  	return 0;
>  }
>  
> +static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
> +{
> +	struct acpi_buffer input;
> +	u32 status;
> +	u64 phys_addr;
> +	u32 retval;

Order longest to shortest.

> +
> +	/* copy to dma capable address */
> +	input.pointer = kzalloc(args.length, GFP_DMA | GFP_KERNEL);
> +	input.length = args.length;
> +	if (!input.pointer)
> +		return -ENOMEM;
> +
> +	if (acpi_os_get_physical_address(input.pointer, &phys_addr) != AE_OK)
> +		goto fail;
> +
> +	memcpy(input.pointer, args.pointer, args.length);
> +
> +	status = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN, phys_addr, 0,
> +					  &retval);
> +	if (status < 0)
> +		goto fail;
> +
> +	memcpy(args.pointer, input.pointer, args.length);
> +
> +	kfree(input.pointer);
> +	return retval;
> +
> +fail:
> +	kfree(input.pointer);
> +	return -1;

The fail: label doesn't really seem necessary. You could either free and return
at the status < 0 block or you could use a common ret variables and only do the
free and return dance once.

> +}
> +
>  static int asus_wmi_get_devstate(struct asus_wmi *asus, u32 dev_id, u32 *retval)
>  {
>  	return asus_wmi_evaluate_method(asus->dsts_id, dev_id, 0, retval);
> @@ -1022,35 +1076,187 @@ exit:
>  /*
>   * Hwmon device
>   */
> -static ssize_t asus_hwmon_pwm1(struct device *dev,
> -			       struct device_attribute *attr,
> -			       char *buf)
> +static int asus_hwmon_agfn_fan_speed(struct asus_wmi *asus, int write, int fan,
> +				     int *speed)
> +{
> +	int status;
> +
> +	struct fan_args args = {
> +		.agfn.len = sizeof(args),
> +		.agfn.mfun = 0x13,
> +		.agfn.sfun = write ? 0x07 : 0x06,
> +		.fan = fan,
> +		.speed = speed ? write ? *speed : 0 : 0,

Oh that's just horrible :-) But OK, sure, I understand why.

> +	};
> +
> +	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +
> +	status = asus_wmi_evaluate_method_agfn(input);
> +
> +	if (status || args.agfn.err)
> +		return -1;
> +
> +	if (!speed)
> +		return 0;
> +
> +	if (write) {
> +		if (fan == 1 || fan == 2)
> +			asus->asus_hwmon_pwm = fan > 0 ? *speed : -1;

fan is guaranteed to be > 0 here. You can drop the ternary operator.

Would it be appropriate to check for the fan value early and return EINVAL if it
is neither of these values? Then we can drop these extra value checks down here
when were already in the thick of it.

> +	} else {
> +		*speed = args.speed;
> +	}
> +
> +	return 0;
> +}
> +
> +static int asus_hwmon_get_fan_number(struct asus_wmi *asus, int *num_fans)
> +{
> +	int status;
> +	int speed = 0;
> +
> +	*num_fans = 0;
> +
> +	status = asus_hwmon_agfn_fan_speed(asus, 0, 1, &speed);
> +	if (status)
> +		return 0;
> +
> +	*num_fans = 1;

This function needs a comment describing its function, because it's
non-intuitive - at least to me. Read fan speed to determine fan count. Fan count
can only be 0 or 1... but we check for 2 elsewhere...

> +	return 0;
> +}
> +
> +static int asus_hwmon_fan_set_auto(struct asus_wmi *asus)
>  {
> +	int status;
> +
> +	status = asus_hwmon_agfn_fan_speed(asus, 1, 0, 0);

The last arg is a pointer right? Please use NULL. It's hard enough to read
already with the numerical inputs.

> +	if (!status) {
> +		asus->asus_hwmon_fan_manual_mode = 0;

Let's get some defines at least for the modes and reduce the magic number count.

> +		return 0;
> +	} else {
> +		return -1;
> +	}

Move return 0 outside the if block and drop the braces.

> +}
> +
> +static int asus_hwmon_fan_rpm_show(struct device *dev, int fan)
> +{
> +	int value;
> +	int state;
>  	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	u32 value;
> +
> +	/* no speed readable on manual mode */
> +	if (asus->asus_hwmon_fan_manual_mode) {
> +		value = -1;
> +	} else {
> +		state = asus_hwmon_agfn_fan_speed(asus, 0, fan+1, &value);
> +		if (state) {
> +			value = -1;
> +			pr_warn("reading fan speed failed: %d\n", state);

Need to use appropriate error codes here. Consider -ENXIO. See the errno
definitions in include/uapi/asm-generic/errno-base.h as well as the sysfs
documentation Documentation/sysfs-rules.txt.

Applies to all show/store functions.

> +		}
> +	}
> +	return value;
> +}
> +
> +static void asus_hwmon_pwm_show(struct asus_wmi *asus, int fan, int *value)
> +{
>  	int err;
>  
> -	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, &value);
> +	if (asus->asus_hwmon_pwm >= 0) {
> +		*value = asus->asus_hwmon_pwm;
> +		return;
> +	}
>  
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, value);
>  	if (err < 0)
> -		return err;
> -
> -	value &= 0xFF;
> +		return;
>  
> -	if (value == 1) /* Low Speed */
> -		value = 85;
> -	else if (value == 2)
> -		value = 170;
> -	else if (value == 3)
> -		value = 255;
> -	else if (value != 0) {
> -		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 asus_hwmon_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);
>  }
>  
> +static ssize_t asus_hwmon_pwm1_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count) {
> +	int value;
> +	int state;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	kstrtouint(buf, 10, &value);
> +	if (value > 255)
> +		value = 255;
> +
> +	state = asus_hwmon_agfn_fan_speed(asus, 1, 1, &value);
> +	if (state)
> +		pr_warn("Setting fan speed failed: %d\n", state);
> +	else
> +		asus->asus_hwmon_fan_manual_mode = 1;
> +
> +	return count;
> +}
> +
> +static ssize_t asus_hwmon_fan1_rpm_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	int value = asus_hwmon_fan_rpm_show(dev, 0);
> +
> +	return sprintf(buf, "%d\n", value < 0 ? value : value*100);
> +
> +}
> +
> +static ssize_t asus_hwmon_cur_control_state_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", asus->asus_hwmon_fan_manual_mode);
> +}
> +
> +static ssize_t asus_hwmon_cur_control_state_store(struct device *dev,
> +						  struct device_attribute *attr,
> +						  const char *buf, size_t count)
> +{
> +	int state;
> +	int status;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	kstrtouint(buf, 10, &state);
> +	if (state == 0 || state == 2)
> +		status = asus_hwmon_fan_set_auto(asus);
> +	else if (state == 1)
> +		asus->asus_hwmon_fan_manual_mode = state;
> +
> +	return count;
> +}
> +
> +static ssize_t asus_hwmon_fan1_label_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return sprintf(buf, "%s\n", ASUS_FAN_DESC);
> +}
> +
>  static ssize_t asus_hwmon_temp1(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
> @@ -1069,11 +1275,24 @@ static ssize_t asus_hwmon_temp1(struct device *dev,
>  	return sprintf(buf, "%d\n", value);
>  }
>  
> -static DEVICE_ATTR(pwm1, S_IRUGO, asus_hwmon_pwm1, NULL);
> +/* Fan1 */
> +static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, asus_hwmon_pwm1_show,
> +		   asus_hwmon_pwm1_store);
> +static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
> +		   asus_hwmon_cur_control_state_show,
> +		   asus_hwmon_cur_control_state_store);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, asus_hwmon_fan1_rpm_show, NULL);
> +static DEVICE_ATTR(fan1_label, S_IRUGO, asus_hwmon_fan1_label_show, NULL);
> +
> +/* Temperature */
>  static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hw)mon_temp1, NULL);

Please use the DEVICE_ATTR_(RO|RW) macros. See toshiba_acpi.c for examples.

>  
>  static struct attribute *hwmon_attributes[] = {
>  	&dev_attr_pwm1.attr,
> +	&dev_attr_pwm1_enable.attr,
> +	&dev_attr_fan1_input.attr,
> +	&dev_attr_fan1_label.attr,
> +
>  	&dev_attr_temp1_input.attr,
>  	NULL
>  };
> @@ -1086,6 +1305,7 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>  	struct asus_wmi *asus = platform_get_drvdata(pdev);
>  	bool ok = true;
>  	int dev_id = -1;
> +	int fan_attr = -1;
>  	u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
>  
>  	if (attr == &dev_attr_pwm1.attr)
> @@ -1093,10 +1313,18 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>  	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 (err < 0)
> +		if (err < 0 && fan_attr == -1)
>  			return 0; /* can't return negative here */
>  	}
>  
> @@ -1112,10 +1340,16 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>  		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 value is zero, something is clearly wrong */
> -		if (value == 0)
> +		if (!value)
>  			ok = false;
> +	} else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) {
> +		ok = true;
> +	} else {
> +		ok = false;
>  	}
>  
>  	return ok ? attr->mode : 0;
> @@ -1723,6 +1957,25 @@ error_debugfs:
>  	return -ENOMEM;
>  }
>  
> +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 = 0;
> +
> +	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 -1;
> +	}
> +
> +	pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
> +	return 0;
> +}
> +
>  /*
>   * WMI Driver
>   */
> @@ -1756,6 +2009,9 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		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)
>  		goto fail_hwmon;
> @@ -1832,6 +2088,7 @@ static int asus_wmi_remove(struct platform_device *device)
>  	asus_wmi_rfkill_exit(asus);
>  	asus_wmi_debugfs_exit(asus);
>  	asus_wmi_platform_exit(asus);
> +	asus_hwmon_fan_set_auto(asus);
>  
>  	kfree(asus);
>  	return 0;
> -- 
> 2.3.5
> 
> 

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [RFC 1/2] ACPI: activate&export acpi_os_get_physical_address

[Rafael J. Wysocki]

On Thursday, April 30, 2015 11:10:25 AM Darren Hart wrote:
> On Wed, Apr 22, 2015 at 04:12:24PM +0200, Kast Bernd wrote:
> > acpi_os_get_physical_address will be needed by an acpi driver (asus-wmi.c).
> > Additionally it could  be used by dell-laptop.c instead of directly calling virt_to_phys.
> > 
> > acpi_os_get_physical_address gets exported and ACPI_FUTURE_USAGE is removed
> > 
> 
> Hrm, well... this doesn't get rid of virt_to_phys, it just wraps it really. I'm
> not sure that makes this any more acceptable than the original from Felipe - but
> that's not my call.

Use virt_to_phys() if you need to.

This one is in case ACPICA needs to get the virtual-to-physical mapping (hence
ACPI_FUTURE_USAGE).

Rafael

Re: [RFC 1/2] ACPI: activate&export acpi_os_get_physical_address

[Rafael J. Wysocki]

On Friday, May 01, 2015 03:32:29 AM Rafael J. Wysocki wrote:
> On Thursday, April 30, 2015 11:10:25 AM Darren Hart wrote:
> > On Wed, Apr 22, 2015 at 04:12:24PM +0200, Kast Bernd wrote:
> > > acpi_os_get_physical_address will be needed by an acpi driver (asus-wmi.c).
> > > Additionally it could  be used by dell-laptop.c instead of directly calling virt_to_phys.
> > > 
> > > acpi_os_get_physical_address gets exported and ACPI_FUTURE_USAGE is removed
> > > 
> > 
> > Hrm, well... this doesn't get rid of virt_to_phys, it just wraps it really. I'm
> > not sure that makes this any more acceptable than the original from Felipe - but
> > that's not my call.
> 
> Use virt_to_phys() if you need to.
> 
> This one is in case ACPICA needs to get the virtual-to-physical mapping (hence
> ACPI_FUTURE_USAGE).

More to the point, the reason why virt_to_phys() needs to be used in patch [2/2]
seems to be a nasty hack in the ASUS AML that pretty much expects us to provide
the physical address as an argument.

And I don't really understand the Matthew's comment regarding limiting
operation regions to system memory.  This is about a specific operation
region (which BTW only seems to be used as a means to access system memory
at the location pointed to by the arg) in that particular method.

Matthew?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC 1/2] ACPI: activate&export acpi_os_get_physical_address

[Matthew Garrett]

On Fri, May 01, 2015 at 03:45:52AM +0200, Rafael J. Wysocki wrote:

> And I don't really understand the Matthew's comment regarding limiting
> operation regions to system memory.  This is about a specific operation
> region (which BTW only seems to be used as a means to access system memory
> at the location pointed to by the arg) in that particular method.

My feeling was that it really ought to have been the ACPI code dealing 
with this in some way, but having looked at it again I accept that this 
is really something that's limited by the vendor implementation. 
virt_to_phys() isn't the worst thing to do here.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC 2/2] asus-wmi: add fan control

[Corentin Chary]

Mostly comments about the code, I don't have anything against the idea.

On Wed, Apr 22, 2015 at 3:12 PM, Kast Bernd <kastbernd@gmx.de> wrote:
> This patch is partially based on Felipe Contrera's earlier patch, that
> was discussed here: https://lkml.org/lkml/2013/10/8/800
> Some problems of that patch are fixed, now:
>
> 1) The main downside of the earlier patch seemed to be the use of
> virt_to_phys, thus an acpi-version of that function is used now.
> (provided by the first patch of this patchset)
>
> 2) random memory corruption occurred on my notebook, thus DMA-able memory
> is allocated now, which solves this problem
>
> 3) hwmon interface is used instead of the thermal interface, as a
> hwmon device is already set up by this driver and seemed more
> appropriate than the thermal interface
>
> 4) Calling the ACPI-functions was modularized thus it's possible to call
> some multifunctions easily, now (by using
> asus_wmi_evaluate_method_agfn).
>
> Unfortunately the WMI doesn't support controlling both fans on
> a dual-fan notebooks because of an restriction in the acpi-method
> "SFNS", that is callable through the wmi. If "SFNV" would be called
> direcly even dual fan configurations could be controlled, but not by using
> wmi.
>
> Speed readings only work on auto-mode, thus "-1" will be reported in
> manual mode.
> Additionally the speed readings are reported as hundreds of RPM thus
> they are not too precize.
>
> This patch is tested only on one notebook (N551JK) but a similar module,
> that contained some code to try to control the second fan also, was
> reported to work on an UX32VD, at least for the first fan.
>
> As Felipe already mentioned the low-level functions are described here:
> http://forum.notebookreview.com/threads/fan-control-on-asus-prime-ux31-ux31a-ux32a-ux32vd.705656/
>
> Signed-off-by: Kast Bernd <kastbernd@gmx.de>
> ---
>  drivers/platform/x86/asus-wmi.c | 297 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 277 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 7543a56..b16658a 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -78,6 +78,7 @@ MODULE_LICENSE("GPL");
>  #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_SFUN         0x4E554653 /* FUNCtionalities */
>  #define ASUS_WMI_METHODID_SDSP         0x50534453 /* Set DiSPlay output */
>  #define ASUS_WMI_METHODID_GDSP         0x50534447 /* Get DiSPlay output */
> @@ -150,11 +151,27 @@ MODULE_LICENSE("GPL");
>  #define ASUS_WMI_DSTS_BRIGHTNESS_MASK  0x000000FF
>  #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK  0x0000FF00
>
> +#define ASUS_FAN_DESC "cpu_fan"
> +
>  struct bios_args {
>         u32 arg0;
>         u32 arg1;
>  } __packed;
>
> +struct agfn_args {
> +       u16 mfun;
> +       u16 sfun;
> +       u16 len;
> +       u8 stas;
> +       u8 err;
> +} __packed;
> +
> +struct fan_args {
> +       struct agfn_args agfn;
> +       u8 fan;
> +       u32 speed;
> +} __packed;
> +
>  /*
>   * <platform>/    - debugfs root directory
>   *   dev_id      - current dev_id
> @@ -204,6 +221,10 @@ struct asus_wmi {
>         struct asus_rfkill gps;
>         struct asus_rfkill uwb;
>
> +       int asus_hwmon_pwm;
> +       int asus_hwmon_num_fans;
> +       int asus_hwmon_fan_manual_mode;
> +
>         struct hotplug_slot *hotplug_slot;
>         struct mutex hotplug_lock;
>         struct mutex wmi_lock;
> @@ -294,6 +315,39 @@ exit:
>         return 0;
>  }
>
> +static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
> +{
> +       struct acpi_buffer input;
> +       u32 status;
> +       u64 phys_addr;
> +       u32 retval;
> +
> +       /* copy to dma capable address */
> +       input.pointer = kzalloc(args.length, GFP_DMA | GFP_KERNEL);

Maybe add a comment here to explain why GFP_DMA.

> +       input.length = args.length;
> +       if (!input.pointer)
> +               return -ENOMEM;
> +
> +       if (acpi_os_get_physical_address(input.pointer, &phys_addr) != AE_OK)
> +               goto fail;
> +
> +       memcpy(input.pointer, args.pointer, args.length);
> +
> +       status = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN, phys_addr, 0,
> +                                         &retval);
> +       if (status < 0)
> +               goto fail;
> +
> +       memcpy(args.pointer, input.pointer, args.length);
> +
> +       kfree(input.pointer);
> +       return retval;
> +
> +fail:
> +       kfree(input.pointer);
> +       return -1;
> +}
> +
>  static int asus_wmi_get_devstate(struct asus_wmi *asus, u32 dev_id, u32 *retval)
>  {
>         return asus_wmi_evaluate_method(asus->dsts_id, dev_id, 0, retval);
> @@ -1022,35 +1076,187 @@ exit:
>  /*
>   * Hwmon device
>   */
> -static ssize_t asus_hwmon_pwm1(struct device *dev,
> -                              struct device_attribute *attr,
> -                              char *buf)
> +static int asus_hwmon_agfn_fan_speed(struct asus_wmi *asus, int write, int fan,
> +                                    int *speed)

write could be a bool here. Even if it's a bool you may have one
function to read and one to write
because it makes it easier to understand what the function is doing.

> +{
> +       int status;
> +
> +       struct fan_args args = {
> +               .agfn.len = sizeof(args),
> +               .agfn.mfun = 0x13,
> +               .agfn.sfun = write ? 0x07 : 0x06,

Could there be a comment, maybe in the structure, explaining what mfun
and sfun are ? And maybe add these values as constants.

> +               .fan = fan,
> +               .speed = speed ? write ? *speed : 0 : 0,
> +       };
> +
> +       struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +
> +       status = asus_wmi_evaluate_method_agfn(input);
> +
> +       if (status || args.agfn.err)
> +               return -1;
> +
> +       if (!speed)
> +               return 0;
> +
> +       if (write) {
> +               if (fan == 1 || fan == 2)
> +                       asus->asus_hwmon_pwm = fan > 0 ? *speed : -1;

Add some kind of logging if fan is not 1 or 2 ?

> +       } else {
> +               *speed = args.speed;
> +       }
> +
> +       return 0;
> +}
> +
> +static int asus_hwmon_get_fan_number(struct asus_wmi *asus, int *num_fans)
> +{
> +       int status;
> +       int speed = 0;
> +
> +       *num_fans = 0;
> +
> +       status = asus_hwmon_agfn_fan_speed(asus, 0, 1, &speed);
> +       if (status)
> +               return 0;

Add a comment that you just check if there is enough to control one
fan, and if yes assume that there is one.

> +       *num_fans = 1;
> +       return 0;
> +}
> +
> +static int asus_hwmon_fan_set_auto(struct asus_wmi *asus)
>  {
> +       int status;
> +
> +       status = asus_hwmon_agfn_fan_speed(asus, 1, 0, 0);
> +       if (!status) {
> +               asus->asus_hwmon_fan_manual_mode = 0;
> +               return 0;
> +       } else {
> +               return -1;

Return proper error codes here (and other places)

> +       }
> +}
> +
> +static int asus_hwmon_fan_rpm_show(struct device *dev, int fan)
> +{
> +       int value;
> +       int state;
>         struct asus_wmi *asus = dev_get_drvdata(dev);
> -       u32 value;
> +
> +       /* no speed readable on manual mode */
> +       if (asus->asus_hwmon_fan_manual_mode) {
> +               value = -1;
> +       } else {
> +               state = asus_hwmon_agfn_fan_speed(asus, 0, fan+1, &value);
> +               if (state) {
> +                       value = -1;
> +                       pr_warn("reading fan speed failed: %d\n", state);
> +               }
> +       }
> +       return value;
> +}
> +
> +static void asus_hwmon_pwm_show(struct asus_wmi *asus, int fan, int *value)
> +{
>         int err;
>
> -       err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, &value);
> +       if (asus->asus_hwmon_pwm >= 0) {
> +               *value = asus->asus_hwmon_pwm;
> +               return;
> +       }
>
> +       err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, value);
>         if (err < 0)
> -               return err;
> -
> -       value &= 0xFF;
> +               return;
>
> -       if (value == 1) /* Low Speed */
> -               value = 85;
> -       else if (value == 2)
> -               value = 170;
> -       else if (value == 3)
> -               value = 255;
> -       else if (value != 0) {
> -               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 asus_hwmon_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);
>  }
>
> +static ssize_t asus_hwmon_pwm1_store(struct device *dev,
> +                                    struct device_attribute *attr,
> +                                    const char *buf, size_t count) {
> +       int value;
> +       int state;
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       kstrtouint(buf, 10, &value);
> +       if (value > 255)
> +               value = 255;

I think there is a function to clamp values somewhere already.

> +
> +       state = asus_hwmon_agfn_fan_speed(asus, 1, 1, &value);
> +       if (state)
> +               pr_warn("Setting fan speed failed: %d\n", state);
> +       else
> +               asus->asus_hwmon_fan_manual_mode = 1;
> +
> +       return count;
> +}
> +
> +static ssize_t asus_hwmon_fan1_rpm_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{
> +       int value = asus_hwmon_fan_rpm_show(dev, 0);
> +
> +       return sprintf(buf, "%d\n", value < 0 ? value : value*100);
> +
> +}
> +
> +static ssize_t asus_hwmon_cur_control_state_show(struct device *dev,
> +                                                struct device_attribute *attr,
> +                                                char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%d\n", asus->asus_hwmon_fan_manual_mode);
> +}
> +
> +static ssize_t asus_hwmon_cur_control_state_store(struct device *dev,
> +                                                 struct device_attribute *attr,
> +                                                 const char *buf, size_t count)
> +{
> +       int state;
> +       int status;
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       kstrtouint(buf, 10, &state);
> +       if (state == 0 || state == 2)

state could be a constant here to make it more obvious what it is.

> +               status = asus_hwmon_fan_set_auto(asus);
> +       else if (state == 1)
> +               asus->asus_hwmon_fan_manual_mode = state;
> +
> +       return count;
> +}
> +
> +static ssize_t asus_hwmon_fan1_label_show(struct device *dev,
> +                                         struct device_attribute *attr,
> +                                         char *buf)
> +{
> +       return sprintf(buf, "%s\n", ASUS_FAN_DESC);
> +}
> +
>  static ssize_t asus_hwmon_temp1(struct device *dev,
>                                 struct device_attribute *attr,
>                                 char *buf)
> @@ -1069,11 +1275,24 @@ static ssize_t asus_hwmon_temp1(struct device *dev,
>         return sprintf(buf, "%d\n", value);
>  }
>
> -static DEVICE_ATTR(pwm1, S_IRUGO, asus_hwmon_pwm1, NULL);
> +/* Fan1 */
> +static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, asus_hwmon_pwm1_show,
> +                  asus_hwmon_pwm1_store);
> +static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
> +                  asus_hwmon_cur_control_state_show,
> +                  asus_hwmon_cur_control_state_store);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, asus_hwmon_fan1_rpm_show, NULL);
> +static DEVICE_ATTR(fan1_label, S_IRUGO, asus_hwmon_fan1_label_show, NULL);
> +
> +/* Temperature */
>  static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
>
>  static struct attribute *hwmon_attributes[] = {
>         &dev_attr_pwm1.attr,
> +       &dev_attr_pwm1_enable.attr,
> +       &dev_attr_fan1_input.attr,
> +       &dev_attr_fan1_label.attr,
> +
>         &dev_attr_temp1_input.attr,
>         NULL
>  };
> @@ -1086,6 +1305,7 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>         struct asus_wmi *asus = platform_get_drvdata(pdev);
>         bool ok = true;
>         int dev_id = -1;
> +       int fan_attr = -1;
>         u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
>
>         if (attr == &dev_attr_pwm1.attr)
> @@ -1093,10 +1313,18 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>         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 (err < 0)
> +               if (err < 0 && fan_attr == -1)
>                         return 0; /* can't return negative here */
>         }
>
> @@ -1112,10 +1340,16 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
>                 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 value is zero, something is clearly wrong */
> -               if (value == 0)
> +               if (!value)
>                         ok = false;
> +       } else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) {
> +               ok = true;
> +       } else {
> +               ok = false;
>         }
>
>         return ok ? attr->mode : 0;
> @@ -1723,6 +1957,25 @@ error_debugfs:
>         return -ENOMEM;
>  }
>
> +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 = 0;
> +
> +       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 -1;
> +       }
> +
> +       pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
> +       return 0;
> +}
> +
>  /*
>   * WMI Driver
>   */
> @@ -1756,6 +2009,9 @@ static int asus_wmi_add(struct platform_device *pdev)
>         if (err)
>                 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)
>                 goto fail_hwmon;
> @@ -1832,6 +2088,7 @@ static int asus_wmi_remove(struct platform_device *device)
>         asus_wmi_rfkill_exit(asus);
>         asus_wmi_debugfs_exit(asus);
>         asus_wmi_platform_exit(asus);
> +       asus_hwmon_fan_set_auto(asus);
>
>         kfree(asus);
>         return 0;
> --
> 2.3.5
>



-- 
Corentin Chary
http://xf.iksaif.net

Re: [RFC 1/2] ACPI: activate&export acpi_os_get_physical_address

[Darren Hart]

On Fri, May 01, 2015 at 05:56:19AM +0100, Matthew Garrett wrote:
> On Fri, May 01, 2015 at 03:45:52AM +0200, Rafael J. Wysocki wrote:
> 
> > And I don't really understand the Matthew's comment regarding limiting
> > operation regions to system memory.  This is about a specific operation
> > region (which BTW only seems to be used as a means to access system memory
> > at the location pointed to by the arg) in that particular method.
> 
> My feeling was that it really ought to have been the ACPI code dealing 
> with this in some way, but having looked at it again I accept that this 
> is really something that's limited by the vendor implementation. 
> virt_to_phys() isn't the worst thing to do here.

Thank you both for the follow-up here.

Kast,

You have some feedback from Corentin and myself on the basic driver, mostly
around cleanups for legibility and future maintainability. You also have
agreement to move forward with virt_to_phys() in the driver due to limitations
imposed by the vendor AML.

Please incorporate these changes in a v2 and resubmit the patch. Please keep all
those who have provided feedback on Cc, and include them in the Cc lines of the
patch itself (below your Signed-off-by).

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center