| Message ID | 1381236524-19633-1-git-send-email-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Oct 8, 2013 at 7:48 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote: > Simple driver to enable control of the fan in ASUS laptops. So far this > has only been tested in ASUS Zenbook Prime UX31A, but according to some > online reference [1], it should work in other models as well. > > The implementation is very straight-forward, the only caveat is that the > fan speed needs to be saved after it has been manually changed because > it won't be reported properly until it goes back to 'auto' mode. > > [1] http://forum.notebookreview.com/asus/705656-fan-control-asus-prime-ux31-ux31a-ux32a-ux32vd.html > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > + r = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN, virt_to_phys(&args), 0, &value); I don't like using virt_to_phys() here, but it seems that's what the ACPI code expects. Method (AGFN, 1, Serialized) { If (LEqual (Arg0, Zero)) { Return (GNBF) } Store (Zero, Local0) OperationRegion (\PARM, SystemMemory, Arg0, 0x08) Field (PARM, DWordAcc, NoLock, Preserve) { MFUN, 16, SFUN, 16, LEN, 16, STAS, 8, EROR, 8 } Any suggestions?
T24gVGh1LCAyMDEzLTEwLTEwIGF0IDE2OjU5ICswMTAwLCBDb3JlbnRpbiBDaGFyeSB3cm90ZToN Cg0KPiANCj4gVGhpcyBkb2Vzbid0IHJlYWxseSBzZWVtcyB0byBiZSByZWxhdGVkIHRvIHdtaSwg YW5kIGlzIGxpa2VseSB0byBiZQ0KPiBhdmFpbGFibGUgb25seSBvbiBhIHN1YnNldCBvZiBtb2Rl bHMuIE1heWJlIGl0IHNob3VsZCBhIHNlcGFyYXRlDQo+IGRyaXZlciBpbnN0ZWFkID8NCg0KVGhp cyB2ZXJzaW9uIHNlZW1zIHRvIGJlIGltcGxlbWVudGVkIGVudGlyZWx5IGluIFdNSSwgYW5kIGl0 J3MgdXNpbmcgdGhlDQpzYW1lIFdNSSBHVUlEIGFzIGFzdXMtd21pIC0gaW1wbGVtZW50aW5nIGl0 IGhlcmUgc2VlbXMgYXBwcm9wcmlhdGUuIEkgYW0NCmNvbmNlcm5lZCBhYm91dCB0aGUgcGh5cy92 aXJ0IHRoaW5nLCB0aG91Z2guIFRoZSBBQ1BJIGludGVycHJldGVyIGlzDQpydW5uaW5nIGluIHRo ZSBrZXJuZWwsIG5vdCB0aGUgaGFyZHdhcmUgLSBhcmUgd2UgYXJ0aWZpY2lhbGx5IGxpbWl0aW5n DQpTeXN0ZW1NZW1vcnkgb3ByZWdpb25zIHRvIHBoeXNpY2FsIGFkZHJlc3Nlcz8NCg0KLS0gDQpN YXR0aGV3IEdhcnJldHQgPG1hdHRoZXcuZ2FycmV0dEBuZWJ1bGEuY29tPg0K -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 10, 2013 at 10:59 AM, Corentin Chary <corentin.chary@gmail.com> wrote: > > > > On Tue, Oct 8, 2013 at 1:48 PM, Felipe Contreras > <felipe.contreras@gmail.com> wrote: >> >> Simple driver to enable control of the fan in ASUS laptops. So far this >> has only been tested in ASUS Zenbook Prime UX31A, but according to some >> online reference [1], it should work in other models as well. >> >> The implementation is very straight-forward, the only caveat is that the >> fan speed needs to be saved after it has been manually changed because >> it won't be reported properly until it goes back to 'auto' mode. > > This doesn't really seems to be related to wmi, and is likely to be > available only on a subset of models. Maybe it should a separate driver > instead ? The first patch I sent was standalone, and you said it should be wmi, and now I do it wmi, and you said it should be separate. Which is it?
On Thu, Oct 10, 2013 at 11:06 AM, Matthew Garrett <matthew.garrett@nebula.com> wrote: > On Thu, 2013-10-10 at 16:59 +0100, Corentin Chary wrote: > >> >> This doesn't really seems to be related to wmi, and is likely to be >> available only on a subset of models. Maybe it should a separate >> driver instead ? > > This version seems to be implemented entirely in WMI, and it's using the > same WMI GUID as asus-wmi - implementing it here seems appropriate. I am > concerned about the phys/virt thing, though. The ACPI interpreter is > running in the kernel, not the hardware - are we artificially limiting > SystemMemory opregions to physical addresses? I don't see anything in acpi_ex_system_memory_space_handler() that takes into consideration virtual addresses.
On Sun, Oct 13, 2013 at 04:29:34AM -0500, Felipe Contreras wrote: > I don't see anything in acpi_ex_system_memory_space_handler() that > takes into consideration virtual addresses. The spec doesn't seem to constrain it to physical addresses (it just refers to "Control Methods read and write data to locations in address spaces (for example, System memory and System I/O)", so I'd lean towards changing the behaviour of acpica rather than adding virt_to_phys().
On Sun, Oct 13, 2013 at 10:17 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Sun, Oct 13, 2013 at 04:29:34AM -0500, Felipe Contreras wrote: > >> I don't see anything in acpi_ex_system_memory_space_handler() that >> takes into consideration virtual addresses. > > The spec doesn't seem to constrain it to physical addresses (it just > refers to "Control Methods read and write data to locations in address > spaces (for example, System memory and System I/O)", so I'd lean towards > changing the behaviour of acpica rather than adding virt_to_phys(). And I assume you are not going to do that. Isn't acpica code outside of the scope of Linux kernel development? If not, how do I go about adding that capability. Also, I find it weird that this the first driver that needs this. Finally, I'm much more interested on what happens next, because I want to link this driver to the thermal framework, so when temperature gets too high, the fan gets an increased speed, because right now it seems it's always on low speed, temperature gets high, and instead the CPU gets throttled, which is not desirable. Maybe this driver should be added to the staging area.
On Sun, Oct 13, 2013 at 09:31:25PM -0500, Felipe Contreras wrote: > On Sun, Oct 13, 2013 at 10:17 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > The spec doesn't seem to constrain it to physical addresses (it just > > refers to "Control Methods read and write data to locations in address > > spaces (for example, System memory and System I/O)", so I'd lean towards > > changing the behaviour of acpica rather than adding virt_to_phys(). > > And I assume you are not going to do that. Isn't acpica code outside > of the scope of Linux kernel development? If not, how do I go about > adding that capability. I wasn't planning on it, no. Just write the code and submit it to linux-acpi, and Cc: Bob Moore. > Also, I find it weird that this the first driver that needs this. The normal way to do this would be for the ASL to just define a buffer within the argument, rather than assigning it to an operation region. I haven't seen anyone take this approach before. > Finally, I'm much more interested on what happens next, because I want > to link this driver to the thermal framework, so when temperature gets > too high, the fan gets an increased speed, because right now it seems > it's always on low speed, temperature gets high, and instead the CPU > gets throttled, which is not desirable. It wouldn't be appropriate to alter the firmware behaviour by default, but yeah, that's the kind of thing that the thermal framework exists to do. > Maybe this driver should be added to the staging area. I don't think you can easily register multiple drivers for the same WMI device.
On Mon, Oct 14, 2013 at 10:52 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Sun, Oct 13, 2013 at 09:31:25PM -0500, Felipe Contreras wrote: >> On Sun, Oct 13, 2013 at 10:17 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >> > The spec doesn't seem to constrain it to physical addresses (it just >> > refers to "Control Methods read and write data to locations in address >> > spaces (for example, System memory and System I/O)", so I'd lean towards >> > changing the behaviour of acpica rather than adding virt_to_phys(). >> >> And I assume you are not going to do that. Isn't acpica code outside >> of the scope of Linux kernel development? If not, how do I go about >> adding that capability. > > I wasn't planning on it, no. Just write the code and submit it to > linux-acpi, and Cc: Bob Moore. I might give it a try. >> Also, I find it weird that this the first driver that needs this. > > The normal way to do this would be for the ASL to just define a buffer > within the argument, rather than assigning it to an operation region. I > haven't seen anyone take this approach before. > >> Finally, I'm much more interested on what happens next, because I want >> to link this driver to the thermal framework, so when temperature gets >> too high, the fan gets an increased speed, because right now it seems >> it's always on low speed, temperature gets high, and instead the CPU >> gets throttled, which is not desirable. > > It wouldn't be appropriate to alter the firmware behaviour by default, > but yeah, that's the kind of thing that the thermal framework exists to > do. Well, how do I do that? The driver is up and running, and I can manually set different fan speeds, however nothing seems to happen automatically when the temperature increases. >> Maybe this driver should be added to the staging area. > > I don't think you can easily register multiple drivers for the same WMI > device. I don't mean this one, I mean the standalone one. Actually, the first one I sent doesn't require all this system memory stuff.
On Mon, Oct 14, 2013 at 06:18:36PM -0500, Felipe Contreras wrote: > On Mon, Oct 14, 2013 at 10:52 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > It wouldn't be appropriate to alter the firmware behaviour by default, > > but yeah, that's the kind of thing that the thermal framework exists to > > do. > > Well, how do I do that? The driver is up and running, and I can > manually set different fan speeds, however nothing seems to happen > automatically when the temperature increases. The easiest is to just do it from userspace. I think Intel have some code for doing this, but I haven't looked at the thermal code for years. > > I don't think you can easily register multiple drivers for the same WMI > > device. > > I don't mean this one, I mean the standalone one. Actually, the first > one I sent doesn't require all this system memory stuff. Banging EC registers directly is the wrong thing to do. Going via WMI is correct.
On Mon, Oct 14, 2013 at 6:22 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Mon, Oct 14, 2013 at 06:18:36PM -0500, Felipe Contreras wrote: >> On Mon, Oct 14, 2013 at 10:52 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >> > It wouldn't be appropriate to alter the firmware behaviour by default, >> > but yeah, that's the kind of thing that the thermal framework exists to >> > do. >> >> Well, how do I do that? The driver is up and running, and I can >> manually set different fan speeds, however nothing seems to happen >> automatically when the temperature increases. > > The easiest is to just do it from userspace. I think Intel have some > code for doing this, but I haven't looked at the thermal code for years. That defeats the purpose of the whole thermal binding infrastructure. >> > I don't think you can easily register multiple drivers for the same WMI >> > device. >> >> I don't mean this one, I mean the standalone one. Actually, the first >> one I sent doesn't require all this system memory stuff. > > Banging EC registers directly is the wrong thing to do. Going via WMI is > correct. I'm not going to bother arguing against your absolutist rhetoric. The fact is one patch can be applied, the other can't. Besides, nobody said anything about banging EC registers directly.
On Mon, Oct 14, 2013 at 06:27:33PM -0500, Felipe Contreras wrote: > On Mon, Oct 14, 2013 at 6:22 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > The easiest is to just do it from userspace. I think Intel have some > > code for doing this, but I haven't looked at the thermal code for years. > > That defeats the purpose of the whole thermal binding infrastructure. Not really, but I agree it's not ideal. If there's a mechanism to get the system temperature via WMI then you could easily construct your own thermal zone and associated cooling device, but otherwise you'd have to provide a mechanism for exporting either the CPU information from coretemp or the thermal zones from ACPI. > >> > I don't think you can easily register multiple drivers for the same WMI > >> > device. > >> > >> I don't mean this one, I mean the standalone one. Actually, the first > >> one I sent doesn't require all this system memory stuff. > > > > Banging EC registers directly is the wrong thing to do. Going via WMI is > > correct. > > I'm not going to bother arguing against your absolutist rhetoric. The > fact is one patch can be applied, the other can't. Besides, nobody > said anything about banging EC registers directly. I'm sorry, you're right - you're calling ACPI methods directly instead. This is still incorrect. The platform provides an exported interface, and you should use that exported interface. As long as Corentin doesn't object, I'm happy to merge this driver in its current form (including virt_to_phys()) providing that it's wrapped in CONFIG_STAGING, and assuming that you'll do the supporting work in acpica. I'll pull it out again in 6 months or so if that hasn't been fixed up. Fair?
On Mon, Oct 14, 2013 at 6:34 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Mon, Oct 14, 2013 at 06:27:33PM -0500, Felipe Contreras wrote: >> On Mon, Oct 14, 2013 at 6:22 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >> > The easiest is to just do it from userspace. I think Intel have some >> > code for doing this, but I haven't looked at the thermal code for years. >> >> That defeats the purpose of the whole thermal binding infrastructure. > > Not really, but I agree it's not ideal. So what's the point of thermal_zone_bind_cooling_device? >> >> > I don't think you can easily register multiple drivers for the same WMI >> >> > device. >> >> >> >> I don't mean this one, I mean the standalone one. Actually, the first >> >> one I sent doesn't require all this system memory stuff. >> > >> > Banging EC registers directly is the wrong thing to do. Going via WMI is >> > correct. >> >> I'm not going to bother arguing against your absolutist rhetoric. The >> fact is one patch can be applied, the other can't. Besides, nobody >> said anything about banging EC registers directly. > > I'm sorry, you're right - you're calling ACPI methods directly instead. > This is still incorrect. The platform provides an exported interface, > and you should use that exported interface. Maybe, but I can call those same methods outside asus-wmi. > As long as Corentin doesn't object, I'm happy to merge this driver in > its current form (including virt_to_phys()) providing that it's wrapped > in CONFIG_STAGING, and assuming that you'll do the supporting work in > acpica. I'll pull it out again in 6 months or so if that hasn't been > fixed up. Fair? That's fair, I'll give that a try soon.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 19c313b..5fdfed6 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 #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 */ @@ -155,6 +156,16 @@ struct bios_args { u32 arg1; } __packed; +struct fan_args { + u16 mfun; + u16 sfun; + u16 len; + u8 stas; + u8 err; + u8 fan; + u32 speed; +} __packed; + /* * <platform>/ - debugfs root directory * dev_id - current dev_id @@ -205,6 +216,9 @@ struct asus_wmi { struct asus_rfkill gps; struct asus_rfkill uwb; + struct thermal_cooling_device *fan; + int fan_speed; + struct hotplug_slot *hotplug_slot; struct mutex hotplug_lock; struct mutex wmi_lock; @@ -1022,6 +1036,90 @@ exit: } /* + * Fan + */ + +static int fan_speed(struct asus_wmi *asus, int write, int fan, unsigned long *speed) +{ + struct fan_args args = { + .len = sizeof(args), + .mfun = 0x13, + .sfun = write ? 0x07 : 0x06, + .fan = fan, + .speed = write ? *speed : 0, + }; + int r; + u32 value; + + if (!write && asus->fan_speed >= 0) { + *speed = asus->fan_speed; + return 0; + } + + r = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN, virt_to_phys(&args), 0, &value); + if (r || value || args.err) + return -1; + + if (write) + asus->fan_speed = fan > 0 ? *speed : -1; + else + *speed = args.speed; + + return 0; +} + +static int fan_set_auto(struct asus_wmi *asus) +{ + unsigned long speed = 0; + return fan_speed(asus, 1, 0, &speed); +} + +static int fan_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state) +{ + *state = 0xff; + return 0; +} + +static int fan_get_cur_state(struct thermal_cooling_device *cdev, unsigned long *state) +{ + return fan_speed(cdev->devdata, 0, 1, state); +} + +static int fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state) +{ + return fan_speed(cdev->devdata, 1, 1, &state); +} + +static const struct thermal_cooling_device_ops fan_cooling_ops = { + .get_max_state = fan_get_max_state, + .get_cur_state = fan_get_cur_state, + .set_cur_state = fan_set_cur_state, +}; + +static int asus_wmi_fan_init(struct asus_wmi *asus) +{ + struct thermal_cooling_device *cdev; + + if (fan_set_auto(asus)) + return 0; + + cdev = thermal_cooling_device_register("Fan", asus, &fan_cooling_ops); + if (IS_ERR(cdev)) + return PTR_ERR(cdev); + asus->fan = cdev; + return 0; +} + +static void asus_wmi_fan_exit(struct asus_wmi *asus) +{ + if (!asus->fan) + return; + fan_set_auto(asus); + thermal_cooling_device_unregister(asus->fan); + asus->fan = NULL; +} + +/* * Hwmon device */ static ssize_t asus_hwmon_pwm1(struct device *dev, @@ -1796,6 +1894,10 @@ static int asus_wmi_add(struct platform_device *pdev) if (err) goto fail_rfkill; + err = asus_wmi_fan_init(asus); + if (err) + goto fail_fan; + if (asus->driver->quirks->wmi_backlight_power) acpi_video_dmi_promote_vendor(); if (!acpi_video_backlight_support()) { @@ -1830,6 +1932,8 @@ fail_debugfs: fail_wmi_handler: asus_wmi_backlight_exit(asus); fail_backlight: + asus_wmi_fan_exit(asus); +fail_fan: asus_wmi_rfkill_exit(asus); fail_rfkill: asus_wmi_led_exit(asus); @@ -1855,6 +1959,7 @@ static int asus_wmi_remove(struct platform_device *device) asus_wmi_hwmon_exit(asus); asus_wmi_led_exit(asus); asus_wmi_rfkill_exit(asus); + asus_wmi_fan_exit(asus); asus_wmi_debugfs_exit(asus); asus_wmi_platform_exit(asus);
Simple driver to enable control of the fan in ASUS laptops. So far this has only been tested in ASUS Zenbook Prime UX31A, but according to some online reference [1], it should work in other models as well. The implementation is very straight-forward, the only caveat is that the fan speed needs to be saved after it has been manually changed because it won't be reported properly until it goes back to 'auto' mode. [1] http://forum.notebookreview.com/asus/705656-fan-control-asus-prime-ux31-ux31a-ux32a-ux32vd.html Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- drivers/platform/x86/asus-wmi.c | 105 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+)