* [PATCH 1/1]: thermal driver therm_adt746.c @ 2015-02-23 11:58 Thomas Haschka 2015-02-23 15:23 ` Cedric Le Goater 2015-02-23 21:18 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 9+ messages in thread From: Thomas Haschka @ 2015-02-23 11:58 UTC (permalink / raw) To: stable, benh, linuxppc-dev [-- Attachment #1.1: Type: text/plain, Size: 2038 bytes --] Hi everyone, The current driver linux/drivers/macintosh/therm_adt746x.c does not take the HDD BUTTOMSIDE sensor into account. It actually should as the 12" Powerbooks and IBooks are build in a way that the airflow cools the harddrive and components around it. Actually there are air intake openings just beneath the Harddrive. If you experiance hot enviromental temperatures as I did in summer on certain occations, you will find out that in MacOSX the fan spins up while it doesn't in linux. As this probably causes harddrives, and maybe other components to fail early I think this should be regarded as a fix to a severe bug. Hence, I created this patch for single fan 12" Albooks, Ibooks etc. Further I changed the output /sys/devices/temperatures so that all 3 sensors are readable from this locations. The output is furhter multiplied by 1000 in order correspond to the ranges printed by other sensor readings. ( This also makes the readings monitorable with tools such as xosview ) I sign this contribution as demanded, and hope that it will help keeping several Powerbooks and Ibooks out there running a bit longer: (a) The contribution was created in whole by me and I have the right to submit it under the GPLv2 (Gnu Public License version 2) (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. Signed-off-by: Thomas Haschka (haschka@gmail.com) [-- Attachment #1.2: Type: text/html, Size: 2288 bytes --] [-- Attachment #2: therm_adt746x.patch --] [-- Type: text/x-patch, Size: 9346 bytes --] --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-23 12:19:03.984000000 +0100 +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-23 12:22:34.980000000 +0100 @@ -1,7 +1,8 @@ /* * Device driver for the i2c thermostat found on the iBook G4, Albook G4 * - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt + * Copyright (C) 2003, 2004, 2015 + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka * * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 @@ -45,7 +46,7 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; static u8 FAN_SPEED[2] = {0x28, 0x2a}; static u8 FAN_SPD_SET[2] = {0x30, 0x31}; -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ static const char *sensor_location[3] = { "?", "?", "?" }; @@ -225,59 +226,123 @@ static void display_stats(struct thermos static void update_fans_speed (struct thermostat *th) { - int lastvar = 0; /* last variation, for iBook */ - int i = 0; - - /* we don't care about local sensor, so we start at sensor 1 */ - for (i = 1; i < 3; i++) { - int started = 0; - int fan_number = (th->type == ADT7460 && i == 2); - int var = th->temps[i] - th->limits[i]; - - if (var > -1) { - int step = (255 - fan_speed) / 7; - int new_speed = 0; + + /* Multfan Laptops */ + if ( th->type == ADT7460 ) { + int lastvar = 0; /* last variation, for iBook */ + int i = 0; + /* we don't care about local sensor, so we start at sensor 1 */ + for (i = 1; i < 3; i++) { + int started = 0; + int fan_number = (th->type == ADT7460 && i == 2); + int var = th->temps[i] - th->limits[i]; + + if (var > -1) { + int step = (255 - fan_speed) / 7; + int new_speed = 0; /* hysteresis : change fan speed only if variation is * more than two degrees */ - if (abs(var - th->last_var[fan_number]) < 2) - continue; - - started = 1; - new_speed = fan_speed + ((var-1)*step); + if (abs(var - th->last_var[fan_number]) < 2) + continue; - if (new_speed < fan_speed) - new_speed = fan_speed; - if (new_speed > 255) - new_speed = 255; + started = 1; + new_speed = fan_speed + ((var-1)*step); - if (verbose) - printk(KERN_DEBUG "adt746x: Setting fans speed to %d " - "(limit exceeded by %d on %s)\n", - new_speed, var, - sensor_location[fan_number+1]); - write_both_fan_speed(th, new_speed); - th->last_var[fan_number] = var; - } else if (var < -2) { + if (new_speed < fan_speed) + new_speed = fan_speed; + if (new_speed > 255) + new_speed = 255; + + if (verbose) + printk(KERN_DEBUG "adt746x: Setting fans speed to %d " + "(limit exceeded by %d on %s)\n", + new_speed, var, + sensor_location[fan_number+1]); + write_both_fan_speed(th, new_speed); + th->last_var[fan_number] = var; + } else if (var < -2) { /* don't stop fan if sensor2 is cold and sensor1 is not * so cold (lastvar >= -1) */ - if (i == 2 && lastvar < -1) { - if (th->last_speed[fan_number] != 0) - if (verbose) + if (i == 2 && lastvar < -1) { + if (th->last_speed[fan_number] != 0) + if (verbose) + printk(KERN_DEBUG "adt746x: Stopping " + "fans.\n"); + write_both_fan_speed(th, 0); + } + } + + lastvar = var; + + if (started) + return; /* we don't want to re-stop the fan + * if sensor1 is heating and sensor2 is not */ + } + + } else { + /*single fan laptops i.e. 12 inch powerbook/ibook*/ + int lastvar = 0; /* last variation, for iBook */ + int i = 0; + int var = 0; + int var_sensor[3]; + int started = 0; + int fan_number = 0; + for (i = 0; i < 3; i++) { + var_sensor[i] = th->temps[i] - th->limits[i]; + } + var = var_sensor[0]; + for (i = 1; i < 3; i++) { + if (var_sensor[i] > var) { + var = var_sensor[i]; + } + } + if (var > -1) { + int step = (255 - fan_speed) / 7; + int new_speed = 0; + + /* hysteresis : change fan speed only if variation is + * more than two degrees */ + if (abs(var - th->last_var[fan_number]) > 2) { + + started = 1; + new_speed = fan_speed + ((var-1)*step); + + if (new_speed < fan_speed) + new_speed = fan_speed; + if (new_speed > 255) + new_speed = 255; + + if (verbose) + printk( + KERN_DEBUG "adt746x: Setting fans speed to %d " + "(limit exceeded by %d on %s)\n", + new_speed, var, + sensor_location[fan_number+1]); + write_both_fan_speed(th, new_speed); + th->last_var[fan_number] = var; + } + } else if (var < -2) { + /* don't stop fan if sensor2 is cold and sensor1 is not + * so cold (lastvar >= -1) */ + if (i == 2 && lastvar < -1) { + if (th->last_speed[fan_number] != 0) + if (verbose) printk(KERN_DEBUG "adt746x: Stopping " "fans.\n"); - write_both_fan_speed(th, 0); + write_both_fan_speed(th, 0); } } - lastvar = var; + lastvar = var; if (started) - return; /* we don't want to re-stop the fan - * if sensor1 is heating and sensor2 is not */ + return; /* we don't want to re-stop the fan + * if sensor1 is heating and sensor2 is not */ } } + static int monitor_task(void *arg) { struct thermostat* th = arg; @@ -371,10 +436,13 @@ static ssize_t store_##name(struct devic return n; \ } -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) @@ -387,14 +455,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) BUILD_STORE_FUNC_DEG(limit_adjust, th) +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, + show_sensor0_temperature,NULL); static DEVICE_ATTR(sensor1_temperature, S_IRUGO, show_sensor1_temperature,NULL); static DEVICE_ATTR(sensor2_temperature, S_IRUGO, show_sensor2_temperature,NULL); +static DEVICE_ATTR(sensor0_limit, S_IRUGO, + show_sensor0_limit, NULL); static DEVICE_ATTR(sensor1_limit, S_IRUGO, show_sensor1_limit, NULL); static DEVICE_ATTR(sensor2_limit, S_IRUGO, show_sensor2_limit, NULL); +static DEVICE_ATTR(sensor0_location, S_IRUGO, + show_sensor0_location, NULL); static DEVICE_ATTR(sensor1_location, S_IRUGO, show_sensor1_location, NULL); static DEVICE_ATTR(sensor2_location, S_IRUGO, @@ -426,10 +500,13 @@ static void thermostat_create_files(stru return; dev = &th->pdev->dev; dev_set_drvdata(dev, th); - err = device_create_file(dev, &dev_attr_sensor1_temperature); + err = device_create_file(dev, &dev_attr_sensor0_temperature); + err |= device_create_file(dev, &dev_attr_sensor1_temperature); err |= device_create_file(dev, &dev_attr_sensor2_temperature); + err |= device_create_file(dev, &dev_attr_sensor0_limit); err |= device_create_file(dev, &dev_attr_sensor1_limit); err |= device_create_file(dev, &dev_attr_sensor2_limit); + err |= device_create_file(dev, &dev_attr_sensor0_location); err |= device_create_file(dev, &dev_attr_sensor1_location); err |= device_create_file(dev, &dev_attr_sensor2_location); err |= device_create_file(dev, &dev_attr_limit_adjust); @@ -449,10 +526,13 @@ static void thermostat_remove_files(stru if (!th->pdev) return; dev = &th->pdev->dev; + device_remove_file(dev, &dev_attr_sensor0_temperature); device_remove_file(dev, &dev_attr_sensor1_temperature); device_remove_file(dev, &dev_attr_sensor2_temperature); + device_remove_file(dev, &dev_attr_sensor0_limit); device_remove_file(dev, &dev_attr_sensor1_limit); device_remove_file(dev, &dev_attr_sensor2_limit); + device_remove_file(dev, &dev_attr_sensor0_location); device_remove_file(dev, &dev_attr_sensor1_location); device_remove_file(dev, &dev_attr_sensor2_location); device_remove_file(dev, &dev_attr_limit_adjust); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-02-23 11:58 [PATCH 1/1]: thermal driver therm_adt746.c Thomas Haschka @ 2015-02-23 15:23 ` Cedric Le Goater 2015-02-23 21:18 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 9+ messages in thread From: Cedric Le Goater @ 2015-02-23 15:23 UTC (permalink / raw) To: Thomas Haschka, stable, benh, linuxppc-dev On 02/23/2015 12:58 PM, Thomas Haschka wrote: > Hi everyone, > > The current driver linux/drivers/macintosh/therm_adt746x.c does not take the HDD BUTTOMSIDE sensor into account. It actually should as the 12" Powerbooks and IBooks are build in a way that the airflow cools the harddrive and components around it. Actually there are air intake openings just beneath the Harddrive. If you experiance hot enviromental temperatures as I did in summer on certain occations, you will find out that in MacOSX the fan spins up while it doesn't in linux. As this probably causes harddrives, and maybe other components to fail early I think this should be regarded as a fix to a severe bug. > > Hence, I created this patch for single fan 12" Albooks, Ibooks etc. I would happy to give it a try on the "brand new" iBook G4 I just got this week-end but I would need you to reformat your patch. Could you please send it again to the mailing using : git send-email --to linuxppc-dev@lists.ozlabs.org my.patch You might want to run the script ./scripts/checkpatch.pl on the patch file before sending. Cheers, C. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-02-23 11:58 [PATCH 1/1]: thermal driver therm_adt746.c Thomas Haschka 2015-02-23 15:23 ` Cedric Le Goater @ 2015-02-23 21:18 ` Benjamin Herrenschmidt 2015-02-25 11:24 ` Thomas Haschka 1 sibling, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2015-02-23 21:18 UTC (permalink / raw) To: Thomas Haschka; +Cc: linuxppc-dev Hi ! Please try sending patches inline rather than as attachments, it makes replying a bit easier... Also don't CC stable, we can shoot to stable later on if we think it's justified but first we need to get the patch upstream A few comments: On Mon, 2015-02-23 at 12:58 +0100, Thomas Haschka wrote: > --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-23 12:19:03.984000000 +0100 > +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-23 12:22:34.980000000 +0100 > @@ -1,7 +1,8 @@ > /* > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > * > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > + * Copyright (C) 2003, 2004, 2015 > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > * > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > @@ -45,7 +46,7 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ Here you change the limit for the local sensor for existing machines, care to explain ? I *think* that got adjusted a while back due to a bunch of bogus error on some machines. > static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > static const char *sensor_location[3] = { "?", "?", "?" }; > > @@ -225,59 +226,123 @@ static void display_stats(struct thermos > > static void update_fans_speed (struct thermostat *th) > { > - int lastvar = 0; /* last variation, for iBook */ > - int i = 0; > - > - /* we don't care about local sensor, so we start at sensor 1 */ > - for (i = 1; i < 3; i++) { > - int started = 0; > - int fan_number = (th->type == ADT7460 && i == 2); > - int var = th->temps[i] - th->limits[i]; > - > - if (var > -1) { > - int step = (255 - fan_speed) / 7; > - int new_speed = 0; > + > + /* Multfan Laptops */ > + if ( th->type == ADT7460 ) { > + int lastvar = 0; /* last variation, for iBook */ > + int i = 0; > + /* we don't care about local sensor, so we start at sensor 1 */ > + for (i = 1; i < 3; i++) { > + int started = 0; > + int fan_number = (th->type == ADT7460 && i == 2); > + int var = th->temps[i] - th->limits[i]; > + > + if (var > -1) { > + int step = (255 - fan_speed) / 7; > + int new_speed = 0; The function is too big, please break it down into two sub functions, one for multifan and one for single fan. It is also unclear due to the indentation changes whether you changed the behaviour on "other" laptops. makes the review a bit harder. > /* hysteresis : change fan speed only if variation is > * more than two degrees */ > - if (abs(var - th->last_var[fan_number]) < 2) > - continue; > - > - started = 1; > - new_speed = fan_speed + ((var-1)*step); > + if (abs(var - th->last_var[fan_number]) < 2) > + continue; > > - if (new_speed < fan_speed) > - new_speed = fan_speed; > - if (new_speed > 255) > - new_speed = 255; > + started = 1; > + new_speed = fan_speed + ((var-1)*step); > > - if (verbose) > - printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > - "(limit exceeded by %d on %s)\n", > - new_speed, var, > - sensor_location[fan_number+1]); > - write_both_fan_speed(th, new_speed); > - th->last_var[fan_number] = var; > - } else if (var < -2) { > + if (new_speed < fan_speed) > + new_speed = fan_speed; > + if (new_speed > 255) > + new_speed = 255; > + > + if (verbose) > + printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > + "(limit exceeded by %d on %s)\n", > + new_speed, var, > + sensor_location[fan_number+1]); > + write_both_fan_speed(th, new_speed); > + th->last_var[fan_number] = var; > + } else if (var < -2) { > /* don't stop fan if sensor2 is cold and sensor1 is not > * so cold (lastvar >= -1) */ > - if (i == 2 && lastvar < -1) { > - if (th->last_speed[fan_number] != 0) > - if (verbose) > + if (i == 2 && lastvar < -1) { > + if (th->last_speed[fan_number] != 0) > + if (verbose) > + printk(KERN_DEBUG "adt746x: Stopping " > + "fans.\n"); > + write_both_fan_speed(th, 0); > + } > + } > + > + lastvar = var; > + > + if (started) > + return; /* we don't want to re-stop the fan > + * if sensor1 is heating and sensor2 is not */ > + } > + > + } else { > + /*single fan laptops i.e. 12 inch powerbook/ibook*/ > + int lastvar = 0; /* last variation, for iBook */ > + int i = 0; > + int var = 0; > + int var_sensor[3]; > + int started = 0; > + int fan_number = 0; > + for (i = 0; i < 3; i++) { > + var_sensor[i] = th->temps[i] - th->limits[i]; > + } > + var = var_sensor[0]; > + for (i = 1; i < 3; i++) { > + if (var_sensor[i] > var) { > + var = var_sensor[i]; > + } > + } > + if (var > -1) { > + int step = (255 - fan_speed) / 7; > + int new_speed = 0; > + > + /* hysteresis : change fan speed only if variation is > + * more than two degrees */ > + if (abs(var - th->last_var[fan_number]) > 2) { > + > + started = 1; > + new_speed = fan_speed + ((var-1)*step); > + > + if (new_speed < fan_speed) > + new_speed = fan_speed; > + if (new_speed > 255) > + new_speed = 255; > + > + if (verbose) > + printk( > + KERN_DEBUG "adt746x: Setting fans speed to %d " > + "(limit exceeded by %d on %s)\n", > + new_speed, var, > + sensor_location[fan_number+1]); > + write_both_fan_speed(th, new_speed); > + th->last_var[fan_number] = var; > + } > + } else if (var < -2) { > + /* don't stop fan if sensor2 is cold and sensor1 is not > + * so cold (lastvar >= -1) */ > + if (i == 2 && lastvar < -1) { > + if (th->last_speed[fan_number] != 0) > + if (verbose) > printk(KERN_DEBUG "adt746x: Stopping " > "fans.\n"); > - write_both_fan_speed(th, 0); > + write_both_fan_speed(th, 0); > } > } > > - lastvar = var; > + lastvar = var; > > if (started) > - return; /* we don't want to re-stop the fan > - * if sensor1 is heating and sensor2 is not */ > + return; /* we don't want to re-stop the fan > + * if sensor1 is heating and sensor2 is not */ > } > } > > + > static int monitor_task(void *arg) > { > struct thermostat* th = arg; > @@ -371,10 +436,13 @@ static ssize_t store_##name(struct devic > return n; \ > } > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) You changed the units here, that should be documented and why. > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > @@ -387,14 +455,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > + show_sensor0_temperature,NULL); > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > show_sensor1_temperature,NULL); > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > show_sensor2_temperature,NULL); > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > + show_sensor0_limit, NULL); > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > show_sensor1_limit, NULL); > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > show_sensor2_limit, NULL); > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > + show_sensor0_location, NULL); > static DEVICE_ATTR(sensor1_location, S_IRUGO, > show_sensor1_location, NULL); > static DEVICE_ATTR(sensor2_location, S_IRUGO, > @@ -426,10 +500,13 @@ static void thermostat_create_files(stru > return; > dev = &th->pdev->dev; > dev_set_drvdata(dev, th); > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > err |= device_create_file(dev, &dev_attr_sensor1_limit); > err |= device_create_file(dev, &dev_attr_sensor2_limit); > + err |= device_create_file(dev, &dev_attr_sensor0_location); > err |= device_create_file(dev, &dev_attr_sensor1_location); > err |= device_create_file(dev, &dev_attr_sensor2_location); > err |= device_create_file(dev, &dev_attr_limit_adjust); > @@ -449,10 +526,13 @@ static void thermostat_remove_files(stru > if (!th->pdev) > return; > dev = &th->pdev->dev; > + device_remove_file(dev, &dev_attr_sensor0_temperature); > device_remove_file(dev, &dev_attr_sensor1_temperature); > device_remove_file(dev, &dev_attr_sensor2_temperature); > + device_remove_file(dev, &dev_attr_sensor0_limit); > device_remove_file(dev, &dev_attr_sensor1_limit); > device_remove_file(dev, &dev_attr_sensor2_limit); > + device_remove_file(dev, &dev_attr_sensor0_location); > device_remove_file(dev, &dev_attr_sensor1_location); > device_remove_file(dev, &dev_attr_sensor2_location); > device_remove_file(dev, &dev_attr_limit_adjust); Cheers, Ben. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-02-23 21:18 ` Benjamin Herrenschmidt @ 2015-02-25 11:24 ` Thomas Haschka 2015-02-25 21:12 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Thomas Haschka @ 2015-02-25 11:24 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linuxppc-dev Hi, Thanks for your comments. Here's a new patch taking them into account. Cheers, (a) The contribution was created in whole or in part by me and I have the right to submit it under the GPL v2 (Gnu Public License Version 2) (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. Signed-off-by: Thomas Haschka <haschka@gmail.com> --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-25 11:26:43.164000000 +0100 +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-25 11:40:54.240000000 +0100 @@ -1,7 +1,8 @@ /* * Device driver for the i2c thermostat found on the iBook G4, Albook G4 * - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt + * Copyright (C) 2003, 2004, 2015 + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka * * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 @@ -45,8 +46,10 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; static u8 FAN_SPEED[2] = {0x28, 0x2a}; static u8 FAN_SPD_SET[2] = {0x30, 0x31}; -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ -static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ +/* Local sensor is down to 45 in order to assure stable harddrive and + * components temperature on single fan machines */ +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ +static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ static const char *sensor_location[3] = { "?", "?", "?" }; static int limit_adjust; @@ -223,7 +226,7 @@ static void display_stats(struct thermos } #endif -static void update_fans_speed (struct thermostat *th) +static void update_fans_speed_multifan (struct thermostat *th) { int lastvar = 0; /* last variation, for iBook */ int i = 0; @@ -278,6 +281,83 @@ static void update_fans_speed (struct th } } +static void update_fans_speed_singlefan (struct thermostat *th) { + + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. + * Necessites the readout of all three thermal sensors + * in order to avoid the harddisk and other components to overheat + * in the case of cold CPU. */ + + int lastvar = 0; /* last variation, for iBook */ + int i = 0; + int var = 0; + int var_sensor[3]; + int started = 0; + int fan_number = 0; + for (i = 0; i < 3; i++) { + var_sensor[i] = th->temps[i] - th->limits[i]; + } + var = var_sensor[0]; + for (i = 1; i < 3; i++) { + if (var_sensor[i] > var) { + var = var_sensor[i]; + } + } + if (var > -1) { + int step = (255 - fan_speed) / 7; + int new_speed = 0; + /* hysteresis : change fan speed only if variation is + * more than two degrees */ + if (abs(var - th->last_var[fan_number]) > 2) { + + started = 1; + new_speed = fan_speed + ((var-1)*step); + + if (new_speed < fan_speed) + new_speed = fan_speed; + if (new_speed > 255) + new_speed = 255; + + if (verbose) + printk(KERN_DEBUG "adt746x:" + " Setting fans speed to %d " + "(limit exceeded by %d on %s)\n", + new_speed, var, + sensor_location[fan_number+1]); + write_both_fan_speed(th, new_speed); + th->last_var[fan_number] = var; + } + } else if (var < -2) { + /* don't stop fan if sensor2 is cold and sensor1 is not + * so cold (lastvar >= -1) */ + if (i == 2 && lastvar < -1) { + if (th->last_speed[fan_number] != 0) + if (verbose) + printk(KERN_DEBUG "adt746x:" + " Stopping " + "fans.\n"); + write_both_fan_speed(th, 0); + } + } + + lastvar = var; + + if (started) + return; /* we don't want to re-stop the fan + * if sensor1 is heating and sensor2 is not */ +} + +static void update_fans_speed(struct thermostat *th) { + + if (th->type == ADT7460) { + /* Multifan Laptops */ + update_fans_speed_multifan(th); + } else { + /* Singlefan Laptops i.e. 12 inch Powerbook, Ibook etc. */ + update_fans_speed_singlefan(th); + } +} + static int monitor_task(void *arg) { struct thermostat* th = arg; @@ -371,10 +451,18 @@ static ssize_t store_##name(struct devic return n; \ } -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) +/* Several nits are multiplied by 1000 in order to have + * coherent readings with other + * temperature sensors such as intel coretemp. This further + * allows programs such as xosview to read these sensors correctly */ + +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) @@ -387,14 +475,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) BUILD_STORE_FUNC_DEG(limit_adjust, th) +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, + show_sensor0_temperature,NULL); static DEVICE_ATTR(sensor1_temperature, S_IRUGO, show_sensor1_temperature,NULL); static DEVICE_ATTR(sensor2_temperature, S_IRUGO, show_sensor2_temperature,NULL); +static DEVICE_ATTR(sensor0_limit, S_IRUGO, + show_sensor0_limit, NULL); static DEVICE_ATTR(sensor1_limit, S_IRUGO, show_sensor1_limit, NULL); static DEVICE_ATTR(sensor2_limit, S_IRUGO, show_sensor2_limit, NULL); +static DEVICE_ATTR(sensor0_location, S_IRUGO, + show_sensor0_location, NULL); static DEVICE_ATTR(sensor1_location, S_IRUGO, show_sensor1_location, NULL); static DEVICE_ATTR(sensor2_location, S_IRUGO, @@ -426,10 +520,13 @@ static void thermostat_create_files(stru return; dev = &th->pdev->dev; dev_set_drvdata(dev, th); - err = device_create_file(dev, &dev_attr_sensor1_temperature); + err = device_create_file(dev, &dev_attr_sensor0_temperature); + err |= device_create_file(dev, &dev_attr_sensor1_temperature); err |= device_create_file(dev, &dev_attr_sensor2_temperature); + err |= device_create_file(dev, &dev_attr_sensor0_limit); err |= device_create_file(dev, &dev_attr_sensor1_limit); err |= device_create_file(dev, &dev_attr_sensor2_limit); + err |= device_create_file(dev, &dev_attr_sensor0_location); err |= device_create_file(dev, &dev_attr_sensor1_location); err |= device_create_file(dev, &dev_attr_sensor2_location); err |= device_create_file(dev, &dev_attr_limit_adjust); @@ -449,10 +546,13 @@ static void thermostat_remove_files(stru if (!th->pdev) return; dev = &th->pdev->dev; + device_remove_file(dev, &dev_attr_sensor0_temperature); device_remove_file(dev, &dev_attr_sensor1_temperature); device_remove_file(dev, &dev_attr_sensor2_temperature); + device_remove_file(dev, &dev_attr_sensor0_limit); device_remove_file(dev, &dev_attr_sensor1_limit); device_remove_file(dev, &dev_attr_sensor2_limit); + device_remove_file(dev, &dev_attr_sensor0_location); device_remove_file(dev, &dev_attr_sensor1_location); device_remove_file(dev, &dev_attr_sensor2_location); device_remove_file(dev, &dev_attr_limit_adjust); On Tue, Feb 24, 2015 at 08:18:38AM +1100, Benjamin Herrenschmidt wrote: > Hi ! Please try sending patches inline rather than as attachments, it > makes replying a bit easier... Also don't CC stable, we can shoot to > stable later on if we think it's justified but first we need to get the > patch upstream > > A few comments: > > On Mon, 2015-02-23 at 12:58 +0100, Thomas Haschka wrote: > > --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-23 12:19:03.984000000 +0100 > > +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-23 12:22:34.980000000 +0100 > > @@ -1,7 +1,8 @@ > > /* > > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > > * > > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > > + * Copyright (C) 2003, 2004, 2015 > > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > > * > > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > > @@ -45,7 +46,7 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ > > Here you change the limit for the local sensor for existing machines, > care to explain ? I *think* that got adjusted a while back due to > a bunch of bogus error on some machines. > > > static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > > static const char *sensor_location[3] = { "?", "?", "?" }; > > > > @@ -225,59 +226,123 @@ static void display_stats(struct thermos > > > > static void update_fans_speed (struct thermostat *th) > > { > > - int lastvar = 0; /* last variation, for iBook */ > > - int i = 0; > > - > > - /* we don't care about local sensor, so we start at sensor 1 */ > > - for (i = 1; i < 3; i++) { > > - int started = 0; > > - int fan_number = (th->type == ADT7460 && i == 2); > > - int var = th->temps[i] - th->limits[i]; > > - > > - if (var > -1) { > > - int step = (255 - fan_speed) / 7; > > - int new_speed = 0; > > + > > + /* Multfan Laptops */ > > + if ( th->type == ADT7460 ) { > > + int lastvar = 0; /* last variation, for iBook */ > > + int i = 0; > > + /* we don't care about local sensor, so we start at sensor 1 */ > > + for (i = 1; i < 3; i++) { > > + int started = 0; > > + int fan_number = (th->type == ADT7460 && i == 2); > > + int var = th->temps[i] - th->limits[i]; > > + > > + if (var > -1) { > > + int step = (255 - fan_speed) / 7; > > + int new_speed = 0; > > The function is too big, please break it down into two sub functions, > one for multifan and one for single fan. > > It is also unclear due to the indentation changes whether you changed > the behaviour on "other" laptops. makes the review a bit harder. > > > /* hysteresis : change fan speed only if variation is > > * more than two degrees */ > > - if (abs(var - th->last_var[fan_number]) < 2) > > - continue; > > - > > - started = 1; > > - new_speed = fan_speed + ((var-1)*step); > > + if (abs(var - th->last_var[fan_number]) < 2) > > + continue; > > > > - if (new_speed < fan_speed) > > - new_speed = fan_speed; > > - if (new_speed > 255) > > - new_speed = 255; > > + started = 1; > > + new_speed = fan_speed + ((var-1)*step); > > > > - if (verbose) > > - printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > > - "(limit exceeded by %d on %s)\n", > > - new_speed, var, > > - sensor_location[fan_number+1]); > > - write_both_fan_speed(th, new_speed); > > - th->last_var[fan_number] = var; > > - } else if (var < -2) { > > + if (new_speed < fan_speed) > > + new_speed = fan_speed; > > + if (new_speed > 255) > > + new_speed = 255; > > + > > + if (verbose) > > + printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > > + "(limit exceeded by %d on %s)\n", > > + new_speed, var, > > + sensor_location[fan_number+1]); > > + write_both_fan_speed(th, new_speed); > > + th->last_var[fan_number] = var; > > + } else if (var < -2) { > > /* don't stop fan if sensor2 is cold and sensor1 is not > > * so cold (lastvar >= -1) */ > > - if (i == 2 && lastvar < -1) { > > - if (th->last_speed[fan_number] != 0) > > - if (verbose) > > + if (i == 2 && lastvar < -1) { > > + if (th->last_speed[fan_number] != 0) > > + if (verbose) > > + printk(KERN_DEBUG "adt746x: Stopping " > > + "fans.\n"); > > + write_both_fan_speed(th, 0); > > + } > > + } > > + > > + lastvar = var; > > + > > + if (started) > > + return; /* we don't want to re-stop the fan > > + * if sensor1 is heating and sensor2 is not */ > > + } > > + > > + } else { > > + /*single fan laptops i.e. 12 inch powerbook/ibook*/ > > + int lastvar = 0; /* last variation, for iBook */ > > + int i = 0; > > + int var = 0; > > + int var_sensor[3]; > > + int started = 0; > > + int fan_number = 0; > > + for (i = 0; i < 3; i++) { > > + var_sensor[i] = th->temps[i] - th->limits[i]; > > + } > > + var = var_sensor[0]; > > + for (i = 1; i < 3; i++) { > > + if (var_sensor[i] > var) { > > + var = var_sensor[i]; > > + } > > + } > > + if (var > -1) { > > + int step = (255 - fan_speed) / 7; > > + int new_speed = 0; > > + > > + /* hysteresis : change fan speed only if variation is > > + * more than two degrees */ > > + if (abs(var - th->last_var[fan_number]) > 2) { > > + > > + started = 1; > > + new_speed = fan_speed + ((var-1)*step); > > + > > + if (new_speed < fan_speed) > > + new_speed = fan_speed; > > + if (new_speed > 255) > > + new_speed = 255; > > + > > + if (verbose) > > + printk( > > + KERN_DEBUG "adt746x: Setting fans speed to %d " > > + "(limit exceeded by %d on %s)\n", > > + new_speed, var, > > + sensor_location[fan_number+1]); > > + write_both_fan_speed(th, new_speed); > > + th->last_var[fan_number] = var; > > + } > > + } else if (var < -2) { > > + /* don't stop fan if sensor2 is cold and sensor1 is not > > + * so cold (lastvar >= -1) */ > > + if (i == 2 && lastvar < -1) { > > + if (th->last_speed[fan_number] != 0) > > + if (verbose) > > printk(KERN_DEBUG "adt746x: Stopping " > > "fans.\n"); > > - write_both_fan_speed(th, 0); > > + write_both_fan_speed(th, 0); > > } > > } > > > > - lastvar = var; > > + lastvar = var; > > > > if (started) > > - return; /* we don't want to re-stop the fan > > - * if sensor1 is heating and sensor2 is not */ > > + return; /* we don't want to re-stop the fan > > + * if sensor1 is heating and sensor2 is not */ > > } > > } > > > > + > > static int monitor_task(void *arg) > > { > > struct thermostat* th = arg; > > @@ -371,10 +436,13 @@ static ssize_t store_##name(struct devic > > return n; \ > > } > > > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) > > You changed the units here, that should be documented and why. > > > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > > > @@ -387,14 +455,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, > > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > > + show_sensor0_temperature,NULL); > > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > > show_sensor1_temperature,NULL); > > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > > show_sensor2_temperature,NULL); > > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > > + show_sensor0_limit, NULL); > > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > > show_sensor1_limit, NULL); > > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > > show_sensor2_limit, NULL); > > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > > + show_sensor0_location, NULL); > > static DEVICE_ATTR(sensor1_location, S_IRUGO, > > show_sensor1_location, NULL); > > static DEVICE_ATTR(sensor2_location, S_IRUGO, > > @@ -426,10 +500,13 @@ static void thermostat_create_files(stru > > return; > > dev = &th->pdev->dev; > > dev_set_drvdata(dev, th); > > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > > err |= device_create_file(dev, &dev_attr_sensor1_limit); > > err |= device_create_file(dev, &dev_attr_sensor2_limit); > > + err |= device_create_file(dev, &dev_attr_sensor0_location); > > err |= device_create_file(dev, &dev_attr_sensor1_location); > > err |= device_create_file(dev, &dev_attr_sensor2_location); > > err |= device_create_file(dev, &dev_attr_limit_adjust); > > @@ -449,10 +526,13 @@ static void thermostat_remove_files(stru > > if (!th->pdev) > > return; > > dev = &th->pdev->dev; > > + device_remove_file(dev, &dev_attr_sensor0_temperature); > > device_remove_file(dev, &dev_attr_sensor1_temperature); > > device_remove_file(dev, &dev_attr_sensor2_temperature); > > + device_remove_file(dev, &dev_attr_sensor0_limit); > > device_remove_file(dev, &dev_attr_sensor1_limit); > > device_remove_file(dev, &dev_attr_sensor2_limit); > > + device_remove_file(dev, &dev_attr_sensor0_location); > > device_remove_file(dev, &dev_attr_sensor1_location); > > device_remove_file(dev, &dev_attr_sensor2_location); > > device_remove_file(dev, &dev_attr_limit_adjust); > > Cheers, > Ben. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-02-25 11:24 ` Thomas Haschka @ 2015-02-25 21:12 ` Benjamin Herrenschmidt 2015-02-26 9:48 ` Thomas Haschka 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2015-02-25 21:12 UTC (permalink / raw) To: Thomas Haschka; +Cc: linuxppc-dev On Wed, 2015-02-25 at 12:24 +0100, Thomas Haschka wrote: > Hi, > > Thanks for your comments. Here's a new patch taking them into account. Thanks. Note that this legalese boilerplate is not useful, the Signed-off-by: is enough to indicate your acceptance of the Linux licencing terms. However I do need a properly formatted changeset comment. Cheers, Ben. > Cheers, > > > (a) The contribution was created in whole or in part by me and I > have the right to submit it under the GPL v2 > (Gnu Public License Version 2) > > (b) The contribution is based upon previous work that, to the best > of my knowledge, is covered under an appropriate open source > license and I have the right under that license to submit that > work with modifications, whether created in whole or in part > by me, under the same open source license (unless I am > permitted to submit under a different license), as indicated > in the file; or > > (c) I understand and agree that this project and the contribution > are public and that a record of the contribution (including all > personal information I submit with it, including my sign-off) is > maintained indefinitely and may be redistributed consistent with > this project or the open source license(s) involved. > > Signed-off-by: Thomas Haschka <haschka@gmail.com> > > --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-25 11:26:43.164000000 +0100 > +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-25 11:40:54.240000000 +0100 > @@ -1,7 +1,8 @@ > /* > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > * > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > + * Copyright (C) 2003, 2004, 2015 > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > * > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > @@ -45,8 +46,10 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > -static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > +/* Local sensor is down to 45 in order to assure stable harddrive and > + * components temperature on single fan machines */ > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ > +static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > static const char *sensor_location[3] = { "?", "?", "?" }; > > static int limit_adjust; > @@ -223,7 +226,7 @@ static void display_stats(struct thermos > } > #endif > > -static void update_fans_speed (struct thermostat *th) > +static void update_fans_speed_multifan (struct thermostat *th) > { > int lastvar = 0; /* last variation, for iBook */ > int i = 0; > @@ -278,6 +281,83 @@ static void update_fans_speed (struct th > } > } > > +static void update_fans_speed_singlefan (struct thermostat *th) { > + > + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. > + * Necessites the readout of all three thermal sensors > + * in order to avoid the harddisk and other components to overheat > + * in the case of cold CPU. */ > + > + int lastvar = 0; /* last variation, for iBook */ > + int i = 0; > + int var = 0; > + int var_sensor[3]; > + int started = 0; > + int fan_number = 0; > + for (i = 0; i < 3; i++) { > + var_sensor[i] = th->temps[i] - th->limits[i]; > + } > + var = var_sensor[0]; > + for (i = 1; i < 3; i++) { > + if (var_sensor[i] > var) { > + var = var_sensor[i]; > + } > + } > + if (var > -1) { > + int step = (255 - fan_speed) / 7; > + int new_speed = 0; > + /* hysteresis : change fan speed only if variation is > + * more than two degrees */ > + if (abs(var - th->last_var[fan_number]) > 2) { > + > + started = 1; > + new_speed = fan_speed + ((var-1)*step); > + > + if (new_speed < fan_speed) > + new_speed = fan_speed; > + if (new_speed > 255) > + new_speed = 255; > + > + if (verbose) > + printk(KERN_DEBUG "adt746x:" > + " Setting fans speed to %d " > + "(limit exceeded by %d on %s)\n", > + new_speed, var, > + sensor_location[fan_number+1]); > + write_both_fan_speed(th, new_speed); > + th->last_var[fan_number] = var; > + } > + } else if (var < -2) { > + /* don't stop fan if sensor2 is cold and sensor1 is not > + * so cold (lastvar >= -1) */ > + if (i == 2 && lastvar < -1) { > + if (th->last_speed[fan_number] != 0) > + if (verbose) > + printk(KERN_DEBUG "adt746x:" > + " Stopping " > + "fans.\n"); > + write_both_fan_speed(th, 0); > + } > + } > + > + lastvar = var; > + > + if (started) > + return; /* we don't want to re-stop the fan > + * if sensor1 is heating and sensor2 is not */ > +} > + > +static void update_fans_speed(struct thermostat *th) { > + > + if (th->type == ADT7460) { > + /* Multifan Laptops */ > + update_fans_speed_multifan(th); > + } else { > + /* Singlefan Laptops i.e. 12 inch Powerbook, Ibook etc. */ > + update_fans_speed_singlefan(th); > + } > +} > + > static int monitor_task(void *arg) > { > struct thermostat* th = arg; > @@ -371,10 +451,18 @@ static ssize_t store_##name(struct devic > return n; \ > } > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > +/* Several nits are multiplied by 1000 in order to have > + * coherent readings with other > + * temperature sensors such as intel coretemp. This further > + * allows programs such as xosview to read these sensors correctly */ > + > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > @@ -387,14 +475,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > + show_sensor0_temperature,NULL); > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > show_sensor1_temperature,NULL); > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > show_sensor2_temperature,NULL); > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > + show_sensor0_limit, NULL); > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > show_sensor1_limit, NULL); > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > show_sensor2_limit, NULL); > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > + show_sensor0_location, NULL); > static DEVICE_ATTR(sensor1_location, S_IRUGO, > show_sensor1_location, NULL); > static DEVICE_ATTR(sensor2_location, S_IRUGO, > @@ -426,10 +520,13 @@ static void thermostat_create_files(stru > return; > dev = &th->pdev->dev; > dev_set_drvdata(dev, th); > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > err |= device_create_file(dev, &dev_attr_sensor1_limit); > err |= device_create_file(dev, &dev_attr_sensor2_limit); > + err |= device_create_file(dev, &dev_attr_sensor0_location); > err |= device_create_file(dev, &dev_attr_sensor1_location); > err |= device_create_file(dev, &dev_attr_sensor2_location); > err |= device_create_file(dev, &dev_attr_limit_adjust); > @@ -449,10 +546,13 @@ static void thermostat_remove_files(stru > if (!th->pdev) > return; > dev = &th->pdev->dev; > + device_remove_file(dev, &dev_attr_sensor0_temperature); > device_remove_file(dev, &dev_attr_sensor1_temperature); > device_remove_file(dev, &dev_attr_sensor2_temperature); > + device_remove_file(dev, &dev_attr_sensor0_limit); > device_remove_file(dev, &dev_attr_sensor1_limit); > device_remove_file(dev, &dev_attr_sensor2_limit); > + device_remove_file(dev, &dev_attr_sensor0_location); > device_remove_file(dev, &dev_attr_sensor1_location); > device_remove_file(dev, &dev_attr_sensor2_location); > device_remove_file(dev, &dev_attr_limit_adjust); > > On Tue, Feb 24, 2015 at 08:18:38AM +1100, Benjamin Herrenschmidt wrote: > > Hi ! Please try sending patches inline rather than as attachments, it > > makes replying a bit easier... Also don't CC stable, we can shoot to > > stable later on if we think it's justified but first we need to get the > > patch upstream > > > > A few comments: > > > > On Mon, 2015-02-23 at 12:58 +0100, Thomas Haschka wrote: > > > --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-23 12:19:03.984000000 +0100 > > > +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-23 12:22:34.980000000 +0100 > > > @@ -1,7 +1,8 @@ > > > /* > > > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > > > * > > > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > > > + * Copyright (C) 2003, 2004, 2015 > > > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > > > * > > > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > > > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > > > @@ -45,7 +46,7 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > > > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > > > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > > > > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > > > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ > > > > Here you change the limit for the local sensor for existing machines, > > care to explain ? I *think* that got adjusted a while back due to > > a bunch of bogus error on some machines. > > > > > static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > > > static const char *sensor_location[3] = { "?", "?", "?" }; > > > > > > @@ -225,59 +226,123 @@ static void display_stats(struct thermos > > > > > > static void update_fans_speed (struct thermostat *th) > > > { > > > - int lastvar = 0; /* last variation, for iBook */ > > > - int i = 0; > > > - > > > - /* we don't care about local sensor, so we start at sensor 1 */ > > > - for (i = 1; i < 3; i++) { > > > - int started = 0; > > > - int fan_number = (th->type == ADT7460 && i == 2); > > > - int var = th->temps[i] - th->limits[i]; > > > - > > > - if (var > -1) { > > > - int step = (255 - fan_speed) / 7; > > > - int new_speed = 0; > > > + > > > + /* Multfan Laptops */ > > > + if ( th->type == ADT7460 ) { > > > + int lastvar = 0; /* last variation, for iBook */ > > > + int i = 0; > > > + /* we don't care about local sensor, so we start at sensor 1 */ > > > + for (i = 1; i < 3; i++) { > > > + int started = 0; > > > + int fan_number = (th->type == ADT7460 && i == 2); > > > + int var = th->temps[i] - th->limits[i]; > > > + > > > + if (var > -1) { > > > + int step = (255 - fan_speed) / 7; > > > + int new_speed = 0; > > > > The function is too big, please break it down into two sub functions, > > one for multifan and one for single fan. > > > > It is also unclear due to the indentation changes whether you changed > > the behaviour on "other" laptops. makes the review a bit harder. > > > > > /* hysteresis : change fan speed only if variation is > > > * more than two degrees */ > > > - if (abs(var - th->last_var[fan_number]) < 2) > > > - continue; > > > - > > > - started = 1; > > > - new_speed = fan_speed + ((var-1)*step); > > > + if (abs(var - th->last_var[fan_number]) < 2) > > > + continue; > > > > > > - if (new_speed < fan_speed) > > > - new_speed = fan_speed; > > > - if (new_speed > 255) > > > - new_speed = 255; > > > + started = 1; > > > + new_speed = fan_speed + ((var-1)*step); > > > > > > - if (verbose) > > > - printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > > > - "(limit exceeded by %d on %s)\n", > > > - new_speed, var, > > > - sensor_location[fan_number+1]); > > > - write_both_fan_speed(th, new_speed); > > > - th->last_var[fan_number] = var; > > > - } else if (var < -2) { > > > + if (new_speed < fan_speed) > > > + new_speed = fan_speed; > > > + if (new_speed > 255) > > > + new_speed = 255; > > > + > > > + if (verbose) > > > + printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > > > + "(limit exceeded by %d on %s)\n", > > > + new_speed, var, > > > + sensor_location[fan_number+1]); > > > + write_both_fan_speed(th, new_speed); > > > + th->last_var[fan_number] = var; > > > + } else if (var < -2) { > > > /* don't stop fan if sensor2 is cold and sensor1 is not > > > * so cold (lastvar >= -1) */ > > > - if (i == 2 && lastvar < -1) { > > > - if (th->last_speed[fan_number] != 0) > > > - if (verbose) > > > + if (i == 2 && lastvar < -1) { > > > + if (th->last_speed[fan_number] != 0) > > > + if (verbose) > > > + printk(KERN_DEBUG "adt746x: Stopping " > > > + "fans.\n"); > > > + write_both_fan_speed(th, 0); > > > + } > > > + } > > > + > > > + lastvar = var; > > > + > > > + if (started) > > > + return; /* we don't want to re-stop the fan > > > + * if sensor1 is heating and sensor2 is not */ > > > + } > > > + > > > + } else { > > > + /*single fan laptops i.e. 12 inch powerbook/ibook*/ > > > + int lastvar = 0; /* last variation, for iBook */ > > > + int i = 0; > > > + int var = 0; > > > + int var_sensor[3]; > > > + int started = 0; > > > + int fan_number = 0; > > > + for (i = 0; i < 3; i++) { > > > + var_sensor[i] = th->temps[i] - th->limits[i]; > > > + } > > > + var = var_sensor[0]; > > > + for (i = 1; i < 3; i++) { > > > + if (var_sensor[i] > var) { > > > + var = var_sensor[i]; > > > + } > > > + } > > > + if (var > -1) { > > > + int step = (255 - fan_speed) / 7; > > > + int new_speed = 0; > > > + > > > + /* hysteresis : change fan speed only if variation is > > > + * more than two degrees */ > > > + if (abs(var - th->last_var[fan_number]) > 2) { > > > + > > > + started = 1; > > > + new_speed = fan_speed + ((var-1)*step); > > > + > > > + if (new_speed < fan_speed) > > > + new_speed = fan_speed; > > > + if (new_speed > 255) > > > + new_speed = 255; > > > + > > > + if (verbose) > > > + printk( > > > + KERN_DEBUG "adt746x: Setting fans speed to %d " > > > + "(limit exceeded by %d on %s)\n", > > > + new_speed, var, > > > + sensor_location[fan_number+1]); > > > + write_both_fan_speed(th, new_speed); > > > + th->last_var[fan_number] = var; > > > + } > > > + } else if (var < -2) { > > > + /* don't stop fan if sensor2 is cold and sensor1 is not > > > + * so cold (lastvar >= -1) */ > > > + if (i == 2 && lastvar < -1) { > > > + if (th->last_speed[fan_number] != 0) > > > + if (verbose) > > > printk(KERN_DEBUG "adt746x: Stopping " > > > "fans.\n"); > > > - write_both_fan_speed(th, 0); > > > + write_both_fan_speed(th, 0); > > > } > > > } > > > > > > - lastvar = var; > > > + lastvar = var; > > > > > > if (started) > > > - return; /* we don't want to re-stop the fan > > > - * if sensor1 is heating and sensor2 is not */ > > > + return; /* we don't want to re-stop the fan > > > + * if sensor1 is heating and sensor2 is not */ > > > } > > > } > > > > > > + > > > static int monitor_task(void *arg) > > > { > > > struct thermostat* th = arg; > > > @@ -371,10 +436,13 @@ static ssize_t store_##name(struct devic > > > return n; \ > > > } > > > > > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > > > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > > > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > > > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > > > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > > > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > > > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > > > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > > > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > > > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) > > > > You changed the units here, that should be documented and why. > > > > > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > > > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > > > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > > > > > @@ -387,14 +455,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, > > > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > > > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > > > > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > > > + show_sensor0_temperature,NULL); > > > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > > > show_sensor1_temperature,NULL); > > > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > > > show_sensor2_temperature,NULL); > > > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > > > + show_sensor0_limit, NULL); > > > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > > > show_sensor1_limit, NULL); > > > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > > > show_sensor2_limit, NULL); > > > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > > > + show_sensor0_location, NULL); > > > static DEVICE_ATTR(sensor1_location, S_IRUGO, > > > show_sensor1_location, NULL); > > > static DEVICE_ATTR(sensor2_location, S_IRUGO, > > > @@ -426,10 +500,13 @@ static void thermostat_create_files(stru > > > return; > > > dev = &th->pdev->dev; > > > dev_set_drvdata(dev, th); > > > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > > > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > > > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > > > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > > > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > > > err |= device_create_file(dev, &dev_attr_sensor1_limit); > > > err |= device_create_file(dev, &dev_attr_sensor2_limit); > > > + err |= device_create_file(dev, &dev_attr_sensor0_location); > > > err |= device_create_file(dev, &dev_attr_sensor1_location); > > > err |= device_create_file(dev, &dev_attr_sensor2_location); > > > err |= device_create_file(dev, &dev_attr_limit_adjust); > > > @@ -449,10 +526,13 @@ static void thermostat_remove_files(stru > > > if (!th->pdev) > > > return; > > > dev = &th->pdev->dev; > > > + device_remove_file(dev, &dev_attr_sensor0_temperature); > > > device_remove_file(dev, &dev_attr_sensor1_temperature); > > > device_remove_file(dev, &dev_attr_sensor2_temperature); > > > + device_remove_file(dev, &dev_attr_sensor0_limit); > > > device_remove_file(dev, &dev_attr_sensor1_limit); > > > device_remove_file(dev, &dev_attr_sensor2_limit); > > > + device_remove_file(dev, &dev_attr_sensor0_location); > > > device_remove_file(dev, &dev_attr_sensor1_location); > > > device_remove_file(dev, &dev_attr_sensor2_location); > > > device_remove_file(dev, &dev_attr_limit_adjust); > > > > Cheers, > > Ben. > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-02-25 21:12 ` Benjamin Herrenschmidt @ 2015-02-26 9:48 ` Thomas Haschka 2015-03-25 5:36 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Thomas Haschka @ 2015-02-26 9:48 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linuxppc-dev Hello, I hope I get it correct this time, thanks again. Just to recapitulate, here's a quick list of the changes made: 1. changes in sensors limits, we are down from 70, 50, 70 to 45, 50, 70, where the first value is the environement sensor that is located on the bottumside of the Harddrive. Empirical testing shows that this speeds up the Fan almoast in the same moment as in OS X 2. A function has been added in order to take the environement sensor into account on SingleFan Apple Notebooks (i.e. those that have air inlets next to the harddrive and that are cooling, besides other components using the single fan in these machines. 3. Output values in /sys/devices/temperatures/ etc.. have been changed. First: All three sensors can be monitored right now instead of only the GPU and CPU sensors. Second: The units have been changed in order to correspond to the units given by other temperature sensors, such as the coretemp (intel) sensor. This allows to monitor them using several applications, notably xosview. cheers, Signed-off-by: Thomas Haschka <haschka@gmail.com> --- drivers/macintosh/therm_adt746x.c | 118 +++++++++++++++++++++++++++++++++++--- 1 file changed, 109 insertions(+), 9 deletions(-) diff --git a/drivers/macintosh/therm_adt746x.c b/drivers/macintosh/therm_adt746x.c index f433521..3ade51a 100644 --- a/drivers/macintosh/therm_adt746x.c +++ b/drivers/macintosh/therm_adt746x.c @@ -1,7 +1,8 @@ /* * Device driver for the i2c thermostat found on the iBook G4, Albook G4 * - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt + * Copyright (C) 2003, 2004, 2015 + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka * * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 @@ -45,8 +46,10 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; static u8 FAN_SPEED[2] = {0x28, 0x2a}; static u8 FAN_SPD_SET[2] = {0x30, 0x31}; -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ -static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ +/* Local sensor is down to 45 in order to assure stable harddrive and + * components temperature on single fan machines */ +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ +static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ static const char *sensor_location[3] = { "?", "?", "?" }; static int limit_adjust; @@ -223,7 +226,7 @@ static void display_stats(struct thermostat *th) } #endif -static void update_fans_speed (struct thermostat *th) +static void update_fans_speed_multifan (struct thermostat *th) { int lastvar = 0; /* last variation, for iBook */ int i = 0; @@ -278,6 +281,83 @@ static void update_fans_speed (struct thermostat *th) } } +static void update_fans_speed_singlefan (struct thermostat *th) { + + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. + * Necessites the readout of all three thermal sensors + * in order to avoid the harddisk and other components to overheat + * in the case of cold CPU. */ + + int lastvar = 0; /* last variation, for iBook */ + int i = 0; + int var = 0; + int var_sensor[3]; + int started = 0; + int fan_number = 0; + for (i = 0; i < 3; i++) { + var_sensor[i] = th->temps[i] - th->limits[i]; + } + var = var_sensor[0]; + for (i = 1; i < 3; i++) { + if (var_sensor[i] > var) { + var = var_sensor[i]; + } + } + if (var > -1) { + int step = (255 - fan_speed) / 7; + int new_speed = 0; + /* hysteresis : change fan speed only if variation is + * more than two degrees */ + if (abs(var - th->last_var[fan_number]) > 2) { + + started = 1; + new_speed = fan_speed + ((var-1)*step); + + if (new_speed < fan_speed) + new_speed = fan_speed; + if (new_speed > 255) + new_speed = 255; + + if (verbose) + printk(KERN_DEBUG "adt746x:" + " Setting fans speed to %d " + "(limit exceeded by %d on %s)\n", + new_speed, var, + sensor_location[fan_number+1]); + write_both_fan_speed(th, new_speed); + th->last_var[fan_number] = var; + } + } else if (var < -2) { + /* don't stop fan if sensor2 is cold and sensor1 is not + * so cold (lastvar >= -1) */ + if (i == 2 && lastvar < -1) { + if (th->last_speed[fan_number] != 0) + if (verbose) + printk(KERN_DEBUG "adt746x:" + " Stopping " + "fans.\n"); + write_both_fan_speed(th, 0); + } + } + + lastvar = var; + + if (started) + return; /* we don't want to re-stop the fan + * if sensor1 is heating and sensor2 is not */ +} + +static void update_fans_speed(struct thermostat *th) { + + if (th->type == ADT7460) { + /* Multifan Laptops */ + update_fans_speed_multifan(th); + } else { + /* Singlefan Laptops i.e. 12 inch Powerbook, Ibook etc. */ + update_fans_speed_singlefan(th); + } +} + static int monitor_task(void *arg) { struct thermostat* th = arg; @@ -371,10 +451,18 @@ static ssize_t store_##name(struct device *dev, struct device_attribute *attr, c return n; \ } -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) +/* Several nits are multiplied by 1000 in order to have + * coherent readings with other + * temperature sensors such as intel coretemp. This further + * allows programs such as xosview to read these sensors correctly */ + +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) @@ -387,14 +475,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, 1) BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) BUILD_STORE_FUNC_DEG(limit_adjust, th) +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, + show_sensor0_temperature,NULL); static DEVICE_ATTR(sensor1_temperature, S_IRUGO, show_sensor1_temperature,NULL); static DEVICE_ATTR(sensor2_temperature, S_IRUGO, show_sensor2_temperature,NULL); +static DEVICE_ATTR(sensor0_limit, S_IRUGO, + show_sensor0_limit, NULL); static DEVICE_ATTR(sensor1_limit, S_IRUGO, show_sensor1_limit, NULL); static DEVICE_ATTR(sensor2_limit, S_IRUGO, show_sensor2_limit, NULL); +static DEVICE_ATTR(sensor0_location, S_IRUGO, + show_sensor0_location, NULL); static DEVICE_ATTR(sensor1_location, S_IRUGO, show_sensor1_location, NULL); static DEVICE_ATTR(sensor2_location, S_IRUGO, @@ -426,10 +520,13 @@ static void thermostat_create_files(struct thermostat *th) return; dev = &th->pdev->dev; dev_set_drvdata(dev, th); - err = device_create_file(dev, &dev_attr_sensor1_temperature); + err = device_create_file(dev, &dev_attr_sensor0_temperature); + err |= device_create_file(dev, &dev_attr_sensor1_temperature); err |= device_create_file(dev, &dev_attr_sensor2_temperature); + err |= device_create_file(dev, &dev_attr_sensor0_limit); err |= device_create_file(dev, &dev_attr_sensor1_limit); err |= device_create_file(dev, &dev_attr_sensor2_limit); + err |= device_create_file(dev, &dev_attr_sensor0_location); err |= device_create_file(dev, &dev_attr_sensor1_location); err |= device_create_file(dev, &dev_attr_sensor2_location); err |= device_create_file(dev, &dev_attr_limit_adjust); @@ -449,10 +546,13 @@ static void thermostat_remove_files(struct thermostat *th) if (!th->pdev) return; dev = &th->pdev->dev; + device_remove_file(dev, &dev_attr_sensor0_temperature); device_remove_file(dev, &dev_attr_sensor1_temperature); device_remove_file(dev, &dev_attr_sensor2_temperature); + device_remove_file(dev, &dev_attr_sensor0_limit); device_remove_file(dev, &dev_attr_sensor1_limit); device_remove_file(dev, &dev_attr_sensor2_limit); + device_remove_file(dev, &dev_attr_sensor0_location); device_remove_file(dev, &dev_attr_sensor1_location); device_remove_file(dev, &dev_attr_sensor2_location); device_remove_file(dev, &dev_attr_limit_adjust); -- 2.0.5 On Thu, Feb 26, 2015 at 08:12:43AM +1100, Benjamin Herrenschmidt wrote: > On Wed, 2015-02-25 at 12:24 +0100, Thomas Haschka wrote: > > Hi, > > > > Thanks for your comments. Here's a new patch taking them into account. > > Thanks. Note that this legalese boilerplate is not useful, the > Signed-off-by: is enough to indicate your acceptance of the Linux > licencing terms. However I do need a properly formatted changeset > comment. > > Cheers, > Ben. > > > Cheers, > > > > > > (a) The contribution was created in whole or in part by me and I > > have the right to submit it under the GPL v2 > > (Gnu Public License Version 2) > > > > (b) The contribution is based upon previous work that, to the best > > of my knowledge, is covered under an appropriate open source > > license and I have the right under that license to submit that > > work with modifications, whether created in whole or in part > > by me, under the same open source license (unless I am > > permitted to submit under a different license), as indicated > > in the file; or > > > > (c) I understand and agree that this project and the contribution > > are public and that a record of the contribution (including all > > personal information I submit with it, including my sign-off) is > > maintained indefinitely and may be redistributed consistent with > > this project or the open source license(s) involved. > > > > Signed-off-by: Thomas Haschka <haschka@gmail.com> > > > > --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-25 11:26:43.164000000 +0100 > > +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-25 11:40:54.240000000 +0100 > > @@ -1,7 +1,8 @@ > > /* > > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > > * > > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > > + * Copyright (C) 2003, 2004, 2015 > > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > > * > > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > > @@ -45,8 +46,10 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > > -static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > > +/* Local sensor is down to 45 in order to assure stable harddrive and > > + * components temperature on single fan machines */ > > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ > > +static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > > static const char *sensor_location[3] = { "?", "?", "?" }; > > > > static int limit_adjust; > > @@ -223,7 +226,7 @@ static void display_stats(struct thermos > > } > > #endif > > > > -static void update_fans_speed (struct thermostat *th) > > +static void update_fans_speed_multifan (struct thermostat *th) > > { > > int lastvar = 0; /* last variation, for iBook */ > > int i = 0; > > @@ -278,6 +281,83 @@ static void update_fans_speed (struct th > > } > > } > > > > +static void update_fans_speed_singlefan (struct thermostat *th) { > > + > > + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. > > + * Necessites the readout of all three thermal sensors > > + * in order to avoid the harddisk and other components to overheat > > + * in the case of cold CPU. */ > > + > > + int lastvar = 0; /* last variation, for iBook */ > > + int i = 0; > > + int var = 0; > > + int var_sensor[3]; > > + int started = 0; > > + int fan_number = 0; > > + for (i = 0; i < 3; i++) { > > + var_sensor[i] = th->temps[i] - th->limits[i]; > > + } > > + var = var_sensor[0]; > > + for (i = 1; i < 3; i++) { > > + if (var_sensor[i] > var) { > > + var = var_sensor[i]; > > + } > > + } > > + if (var > -1) { > > + int step = (255 - fan_speed) / 7; > > + int new_speed = 0; > > + /* hysteresis : change fan speed only if variation is > > + * more than two degrees */ > > + if (abs(var - th->last_var[fan_number]) > 2) { > > + > > + started = 1; > > + new_speed = fan_speed + ((var-1)*step); > > + > > + if (new_speed < fan_speed) > > + new_speed = fan_speed; > > + if (new_speed > 255) > > + new_speed = 255; > > + > > + if (verbose) > > + printk(KERN_DEBUG "adt746x:" > > + " Setting fans speed to %d " > > + "(limit exceeded by %d on %s)\n", > > + new_speed, var, > > + sensor_location[fan_number+1]); > > + write_both_fan_speed(th, new_speed); > > + th->last_var[fan_number] = var; > > + } > > + } else if (var < -2) { > > + /* don't stop fan if sensor2 is cold and sensor1 is not > > + * so cold (lastvar >= -1) */ > > + if (i == 2 && lastvar < -1) { > > + if (th->last_speed[fan_number] != 0) > > + if (verbose) > > + printk(KERN_DEBUG "adt746x:" > > + " Stopping " > > + "fans.\n"); > > + write_both_fan_speed(th, 0); > > + } > > + } > > + > > + lastvar = var; > > + > > + if (started) > > + return; /* we don't want to re-stop the fan > > + * if sensor1 is heating and sensor2 is not */ > > +} > > + > > +static void update_fans_speed(struct thermostat *th) { > > + > > + if (th->type == ADT7460) { > > + /* Multifan Laptops */ > > + update_fans_speed_multifan(th); > > + } else { > > + /* Singlefan Laptops i.e. 12 inch Powerbook, Ibook etc. */ > > + update_fans_speed_singlefan(th); > > + } > > +} > > + > > static int monitor_task(void *arg) > > { > > struct thermostat* th = arg; > > @@ -371,10 +451,18 @@ static ssize_t store_##name(struct devic > > return n; \ > > } > > > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > > +/* Several nits are multiplied by 1000 in order to have > > + * coherent readings with other > > + * temperature sensors such as intel coretemp. This further > > + * allows programs such as xosview to read these sensors correctly */ > > + > > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) > > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > > > @@ -387,14 +475,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, > > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > > + show_sensor0_temperature,NULL); > > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > > show_sensor1_temperature,NULL); > > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > > show_sensor2_temperature,NULL); > > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > > + show_sensor0_limit, NULL); > > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > > show_sensor1_limit, NULL); > > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > > show_sensor2_limit, NULL); > > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > > + show_sensor0_location, NULL); > > static DEVICE_ATTR(sensor1_location, S_IRUGO, > > show_sensor1_location, NULL); > > static DEVICE_ATTR(sensor2_location, S_IRUGO, > > @@ -426,10 +520,13 @@ static void thermostat_create_files(stru > > return; > > dev = &th->pdev->dev; > > dev_set_drvdata(dev, th); > > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > > err |= device_create_file(dev, &dev_attr_sensor1_limit); > > err |= device_create_file(dev, &dev_attr_sensor2_limit); > > + err |= device_create_file(dev, &dev_attr_sensor0_location); > > err |= device_create_file(dev, &dev_attr_sensor1_location); > > err |= device_create_file(dev, &dev_attr_sensor2_location); > > err |= device_create_file(dev, &dev_attr_limit_adjust); > > @@ -449,10 +546,13 @@ static void thermostat_remove_files(stru > > if (!th->pdev) > > return; > > dev = &th->pdev->dev; > > + device_remove_file(dev, &dev_attr_sensor0_temperature); > > device_remove_file(dev, &dev_attr_sensor1_temperature); > > device_remove_file(dev, &dev_attr_sensor2_temperature); > > + device_remove_file(dev, &dev_attr_sensor0_limit); > > device_remove_file(dev, &dev_attr_sensor1_limit); > > device_remove_file(dev, &dev_attr_sensor2_limit); > > + device_remove_file(dev, &dev_attr_sensor0_location); > > device_remove_file(dev, &dev_attr_sensor1_location); > > device_remove_file(dev, &dev_attr_sensor2_location); > > device_remove_file(dev, &dev_attr_limit_adjust); > > > > On Tue, Feb 24, 2015 at 08:18:38AM +1100, Benjamin Herrenschmidt wrote: > > > Hi ! Please try sending patches inline rather than as attachments, it > > > makes replying a bit easier... Also don't CC stable, we can shoot to > > > stable later on if we think it's justified but first we need to get the > > > patch upstream > > > > > > A few comments: > > > > > > On Mon, 2015-02-23 at 12:58 +0100, Thomas Haschka wrote: > > > > --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-23 12:19:03.984000000 +0100 > > > > +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-23 12:22:34.980000000 +0100 > > > > @@ -1,7 +1,8 @@ > > > > /* > > > > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > > > > * > > > > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > > > > + * Copyright (C) 2003, 2004, 2015 > > > > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > > > > * > > > > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > > > > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > > > > @@ -45,7 +46,7 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > > > > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > > > > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > > > > > > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > > > > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ > > > > > > Here you change the limit for the local sensor for existing machines, > > > care to explain ? I *think* that got adjusted a while back due to > > > a bunch of bogus error on some machines. > > > > > > > static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > > > > static const char *sensor_location[3] = { "?", "?", "?" }; > > > > > > > > @@ -225,59 +226,123 @@ static void display_stats(struct thermos > > > > > > > > static void update_fans_speed (struct thermostat *th) > > > > { > > > > - int lastvar = 0; /* last variation, for iBook */ > > > > - int i = 0; > > > > - > > > > - /* we don't care about local sensor, so we start at sensor 1 */ > > > > - for (i = 1; i < 3; i++) { > > > > - int started = 0; > > > > - int fan_number = (th->type == ADT7460 && i == 2); > > > > - int var = th->temps[i] - th->limits[i]; > > > > - > > > > - if (var > -1) { > > > > - int step = (255 - fan_speed) / 7; > > > > - int new_speed = 0; > > > > + > > > > + /* Multfan Laptops */ > > > > + if ( th->type == ADT7460 ) { > > > > + int lastvar = 0; /* last variation, for iBook */ > > > > + int i = 0; > > > > + /* we don't care about local sensor, so we start at sensor 1 */ > > > > + for (i = 1; i < 3; i++) { > > > > + int started = 0; > > > > + int fan_number = (th->type == ADT7460 && i == 2); > > > > + int var = th->temps[i] - th->limits[i]; > > > > + > > > > + if (var > -1) { > > > > + int step = (255 - fan_speed) / 7; > > > > + int new_speed = 0; > > > > > > The function is too big, please break it down into two sub functions, > > > one for multifan and one for single fan. > > > > > > It is also unclear due to the indentation changes whether you changed > > > the behaviour on "other" laptops. makes the review a bit harder. > > > > > > > /* hysteresis : change fan speed only if variation is > > > > * more than two degrees */ > > > > - if (abs(var - th->last_var[fan_number]) < 2) > > > > - continue; > > > > - > > > > - started = 1; > > > > - new_speed = fan_speed + ((var-1)*step); > > > > + if (abs(var - th->last_var[fan_number]) < 2) > > > > + continue; > > > > > > > > - if (new_speed < fan_speed) > > > > - new_speed = fan_speed; > > > > - if (new_speed > 255) > > > > - new_speed = 255; > > > > + started = 1; > > > > + new_speed = fan_speed + ((var-1)*step); > > > > > > > > - if (verbose) > > > > - printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > > > > - "(limit exceeded by %d on %s)\n", > > > > - new_speed, var, > > > > - sensor_location[fan_number+1]); > > > > - write_both_fan_speed(th, new_speed); > > > > - th->last_var[fan_number] = var; > > > > - } else if (var < -2) { > > > > + if (new_speed < fan_speed) > > > > + new_speed = fan_speed; > > > > + if (new_speed > 255) > > > > + new_speed = 255; > > > > + > > > > + if (verbose) > > > > + printk(KERN_DEBUG "adt746x: Setting fans speed to %d " > > > > + "(limit exceeded by %d on %s)\n", > > > > + new_speed, var, > > > > + sensor_location[fan_number+1]); > > > > + write_both_fan_speed(th, new_speed); > > > > + th->last_var[fan_number] = var; > > > > + } else if (var < -2) { > > > > /* don't stop fan if sensor2 is cold and sensor1 is not > > > > * so cold (lastvar >= -1) */ > > > > - if (i == 2 && lastvar < -1) { > > > > - if (th->last_speed[fan_number] != 0) > > > > - if (verbose) > > > > + if (i == 2 && lastvar < -1) { > > > > + if (th->last_speed[fan_number] != 0) > > > > + if (verbose) > > > > + printk(KERN_DEBUG "adt746x: Stopping " > > > > + "fans.\n"); > > > > + write_both_fan_speed(th, 0); > > > > + } > > > > + } > > > > + > > > > + lastvar = var; > > > > + > > > > + if (started) > > > > + return; /* we don't want to re-stop the fan > > > > + * if sensor1 is heating and sensor2 is not */ > > > > + } > > > > + > > > > + } else { > > > > + /*single fan laptops i.e. 12 inch powerbook/ibook*/ > > > > + int lastvar = 0; /* last variation, for iBook */ > > > > + int i = 0; > > > > + int var = 0; > > > > + int var_sensor[3]; > > > > + int started = 0; > > > > + int fan_number = 0; > > > > + for (i = 0; i < 3; i++) { > > > > + var_sensor[i] = th->temps[i] - th->limits[i]; > > > > + } > > > > + var = var_sensor[0]; > > > > + for (i = 1; i < 3; i++) { > > > > + if (var_sensor[i] > var) { > > > > + var = var_sensor[i]; > > > > + } > > > > + } > > > > + if (var > -1) { > > > > + int step = (255 - fan_speed) / 7; > > > > + int new_speed = 0; > > > > + > > > > + /* hysteresis : change fan speed only if variation is > > > > + * more than two degrees */ > > > > + if (abs(var - th->last_var[fan_number]) > 2) { > > > > + > > > > + started = 1; > > > > + new_speed = fan_speed + ((var-1)*step); > > > > + > > > > + if (new_speed < fan_speed) > > > > + new_speed = fan_speed; > > > > + if (new_speed > 255) > > > > + new_speed = 255; > > > > + > > > > + if (verbose) > > > > + printk( > > > > + KERN_DEBUG "adt746x: Setting fans speed to %d " > > > > + "(limit exceeded by %d on %s)\n", > > > > + new_speed, var, > > > > + sensor_location[fan_number+1]); > > > > + write_both_fan_speed(th, new_speed); > > > > + th->last_var[fan_number] = var; > > > > + } > > > > + } else if (var < -2) { > > > > + /* don't stop fan if sensor2 is cold and sensor1 is not > > > > + * so cold (lastvar >= -1) */ > > > > + if (i == 2 && lastvar < -1) { > > > > + if (th->last_speed[fan_number] != 0) > > > > + if (verbose) > > > > printk(KERN_DEBUG "adt746x: Stopping " > > > > "fans.\n"); > > > > - write_both_fan_speed(th, 0); > > > > + write_both_fan_speed(th, 0); > > > > } > > > > } > > > > > > > > - lastvar = var; > > > > + lastvar = var; > > > > > > > > if (started) > > > > - return; /* we don't want to re-stop the fan > > > > - * if sensor1 is heating and sensor2 is not */ > > > > + return; /* we don't want to re-stop the fan > > > > + * if sensor1 is heating and sensor2 is not */ > > > > } > > > > } > > > > > > > > + > > > > static int monitor_task(void *arg) > > > > { > > > > struct thermostat* th = arg; > > > > @@ -371,10 +436,13 @@ static ssize_t store_##name(struct devic > > > > return n; \ > > > > } > > > > > > > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > > > > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > > > > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > > > > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > > > > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > > > > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > > > > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > > > > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > > > > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > > > > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) > > > > > > You changed the units here, that should be documented and why. > > > > > > > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > > > > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > > > > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > > > > > > > @@ -387,14 +455,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, > > > > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > > > > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > > > > > > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > > > > + show_sensor0_temperature,NULL); > > > > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > > > > show_sensor1_temperature,NULL); > > > > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > > > > show_sensor2_temperature,NULL); > > > > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > > > > + show_sensor0_limit, NULL); > > > > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > > > > show_sensor1_limit, NULL); > > > > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > > > > show_sensor2_limit, NULL); > > > > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > > > > + show_sensor0_location, NULL); > > > > static DEVICE_ATTR(sensor1_location, S_IRUGO, > > > > show_sensor1_location, NULL); > > > > static DEVICE_ATTR(sensor2_location, S_IRUGO, > > > > @@ -426,10 +500,13 @@ static void thermostat_create_files(stru > > > > return; > > > > dev = &th->pdev->dev; > > > > dev_set_drvdata(dev, th); > > > > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > > > > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > > > > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > > > > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > > > > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > > > > err |= device_create_file(dev, &dev_attr_sensor1_limit); > > > > err |= device_create_file(dev, &dev_attr_sensor2_limit); > > > > + err |= device_create_file(dev, &dev_attr_sensor0_location); > > > > err |= device_create_file(dev, &dev_attr_sensor1_location); > > > > err |= device_create_file(dev, &dev_attr_sensor2_location); > > > > err |= device_create_file(dev, &dev_attr_limit_adjust); > > > > @@ -449,10 +526,13 @@ static void thermostat_remove_files(stru > > > > if (!th->pdev) > > > > return; > > > > dev = &th->pdev->dev; > > > > + device_remove_file(dev, &dev_attr_sensor0_temperature); > > > > device_remove_file(dev, &dev_attr_sensor1_temperature); > > > > device_remove_file(dev, &dev_attr_sensor2_temperature); > > > > + device_remove_file(dev, &dev_attr_sensor0_limit); > > > > device_remove_file(dev, &dev_attr_sensor1_limit); > > > > device_remove_file(dev, &dev_attr_sensor2_limit); > > > > + device_remove_file(dev, &dev_attr_sensor0_location); > > > > device_remove_file(dev, &dev_attr_sensor1_location); > > > > device_remove_file(dev, &dev_attr_sensor2_location); > > > > device_remove_file(dev, &dev_attr_limit_adjust); > > > > > > Cheers, > > > Ben. > > > > > > > > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-02-26 9:48 ` Thomas Haschka @ 2015-03-25 5:36 ` Benjamin Herrenschmidt 2015-03-26 16:23 ` Thomas Haschka 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2015-03-25 5:36 UTC (permalink / raw) To: Thomas Haschka; +Cc: linuxppc-dev On Thu, 2015-02-26 at 10:48 +0100, Thomas Haschka wrote: > Hello, > > I hope I get it correct this time, thanks again. Thanks. Sorry for the delay, I've been a bit swamped. I looked at your code a bit more in depth and I would appreciate a couple more changes if you don't mind: .../... > +static void update_fans_speed_singlefan (struct thermostat *th) { > + > + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. > + * Necessites the readout of all three thermal sensors > + * in order to avoid the harddisk and other components to overheat > + * in the case of cold CPU. */ > + > + int lastvar = 0; /* last variation, for iBook */ > + int i = 0; > + int var = 0; > + int var_sensor[3]; > + int started = 0; > + int fan_number = 0; So here you hard code fan_number instead of looping accross all fans, you could just use a constant... > + for (i = 0; i < 3; i++) { > + var_sensor[i] = th->temps[i] - th->limits[i]; > + } > + var = var_sensor[0]; > + for (i = 1; i < 3; i++) { > + if (var_sensor[i] > var) { > + var = var_sensor[i]; > + } > + } Why two loops ? > + if (var > -1) { > + int step = (255 - fan_speed) / 7; > + int new_speed = 0; > + /* hysteresis : change fan speed only if variation is > + * more than two degrees */ > + if (abs(var - th->last_var[fan_number]) > 2) { > + > + started = 1; > + new_speed = fan_speed + ((var-1)*step); > + > + if (new_speed < fan_speed) > + new_speed = fan_speed; > + if (new_speed > 255) > + new_speed = 255; > + > + if (verbose) > + printk(KERN_DEBUG "adt746x:" > + " Setting fans speed to %d " > + "(limit exceeded by %d on %s)\n", > + new_speed, var, > + sensor_location[fan_number+1]); > + write_both_fan_speed(th, new_speed); > + th->last_var[fan_number] = var; > + } > + } else if (var < -2) { > + /* don't stop fan if sensor2 is cold and sensor1 is not > + * so cold (lastvar >= -1) */ > + if (i == 2 && lastvar < -1) { > + if (th->last_speed[fan_number] != 0) > + if (verbose) > + printk(KERN_DEBUG "adt746x:" > + " Stopping " > + "fans.\n"); > + write_both_fan_speed(th, 0); > + } > + } This looks identical between the two functions (single/multi fan), can you factor it out into a single static helper that they both call ? Cheers, Ben. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-03-25 5:36 ` Benjamin Herrenschmidt @ 2015-03-26 16:23 ` Thomas Haschka 2015-03-26 21:31 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Thomas Haschka @ 2015-03-26 16:23 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linuxppc-dev Hello, Here's an updated version taking your suggested changements into account (merged the two loops into one, changed a hardcoded value into a constant). Just to keep everything together, here are the changements: 1. changes in sensors limits, we are down from 70, 50, 70 to 45, 50, 70, where the first value is the environement sensor that is located on the bottumside of the Harddrive. Empirical testing shows that this speeds up the Fan almoast in the same moment as in OS X 2. A function has been added in order to take the environement sensor into account on SingleFan Apple Notebooks (i.e. those that have air inlets next to the harddrive and that are cooling, besides other components using the single fan in these machines. 3. Output values in /sys/devices/temperatures/ etc.. have been changed. First: All three sensors can be monitored right now instead of only the GPU and CPU sensors. Second: The units have been changed in order to correspond to the units given by other temperature sensors, such as the coretemp (intel) sensor. This allows to monitor them using several applications, notably xosview. Signed-off-by: Thomas Haschka <haschka@gmail.com> --- drivers/macintosh/therm_adt746x.c | 123 +++++++++++++++++++++++++++++++++++--- 1 file changed, 114 insertions(+), 9 deletions(-) diff --git a/drivers/macintosh/therm_adt746x.c b/drivers/macintosh/therm_adt746x.c index f433521..1a36ea44 100644 --- a/drivers/macintosh/therm_adt746x.c +++ b/drivers/macintosh/therm_adt746x.c @@ -1,7 +1,8 @@ /* * Device driver for the i2c thermostat found on the iBook G4, Albook G4 * - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt + * Copyright (C) 2003, 2004, 2015 + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka * * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 @@ -45,8 +46,10 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; static u8 FAN_SPEED[2] = {0x28, 0x2a}; static u8 FAN_SPD_SET[2] = {0x30, 0x31}; -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ -static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ +/* Local sensor is down to 45 in order to assure stable harddrive and + * components temperature on single fan machines */ +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ +static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ static const char *sensor_location[3] = { "?", "?", "?" }; static int limit_adjust; @@ -223,7 +226,7 @@ static void display_stats(struct thermostat *th) } #endif -static void update_fans_speed (struct thermostat *th) +static void update_fans_speed_multifan (struct thermostat *th) { int lastvar = 0; /* last variation, for iBook */ int i = 0; @@ -278,6 +281,88 @@ static void update_fans_speed (struct thermostat *th) } } +static void update_fans_speed_singlefan (struct thermostat *th) { + + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. + * Necessites the readout of all three thermal sensors + * in order to avoid the harddisk and other components to overheat + * in the case of cold CPU. */ + + int lastvar = 0; /* last variation, for iBook */ + int i = 0; + int var = 0; + int var_sensor[3]; + int started = 0; + const int fan_number = 0; + + /* select the highest delta from all three sensors and store it to var + */ + + var_sensor[0] = th->temps[0] - th->limits[0]; + var = var_sensor[0]; + + for (i = 1; i < 3; i++) { + var_sensor[i] = th->temps[i] - th->limits[i]; + if (var_sensor[i] > var) { + var = var_sensor[i]; + } + } + + if (var > -1) { + int step = (255 - fan_speed) / 7; + int new_speed = 0; + /* hysteresis : change fan speed only if variation is + * more than two degrees */ + if (abs(var - th->last_var[fan_number]) > 2) { + + started = 1; + new_speed = fan_speed + ((var-1)*step); + + if (new_speed < fan_speed) + new_speed = fan_speed; + if (new_speed > 255) + new_speed = 255; + + if (verbose) + printk(KERN_DEBUG "adt746x:" + " Setting fans speed to %d " + "(limit exceeded by %d on %s)\n", + new_speed, var, + sensor_location[fan_number+1]); + write_both_fan_speed(th, new_speed); + th->last_var[fan_number] = var; + } + } else if (var < -2) { + /* don't stop fan if sensor2 is cold and sensor1 is not + * so cold (lastvar >= -1) */ + if (i == 2 && lastvar < -1) { + if (th->last_speed[fan_number] != 0) + if (verbose) + printk(KERN_DEBUG "adt746x:" + " Stopping " + "fans.\n"); + write_both_fan_speed(th, 0); + } + } + + lastvar = var; + + if (started) + return; /* we don't want to re-stop the fan + * if sensor1 is heating and sensor2 is not */ +} + +static void update_fans_speed(struct thermostat *th) { + + if (th->type == ADT7460) { + /* Multifan Laptops */ + update_fans_speed_multifan(th); + } else { + /* Singlefan Laptops i.e. 12 inch Powerbook, Ibook etc. */ + update_fans_speed_singlefan(th); + } +} + static int monitor_task(void *arg) { struct thermostat* th = arg; @@ -371,10 +456,18 @@ static ssize_t store_##name(struct device *dev, struct device_attribute *attr, c return n; \ } -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) +/* Several units are multiplied by 1000 in order to have + * coherent readings with other + * temperature sensors such as intel coretemp. This further + * allows programs such as xosview to read these sensors correctly */ + +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) @@ -387,14 +480,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, 1) BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) BUILD_STORE_FUNC_DEG(limit_adjust, th) +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, + show_sensor0_temperature,NULL); static DEVICE_ATTR(sensor1_temperature, S_IRUGO, show_sensor1_temperature,NULL); static DEVICE_ATTR(sensor2_temperature, S_IRUGO, show_sensor2_temperature,NULL); +static DEVICE_ATTR(sensor0_limit, S_IRUGO, + show_sensor0_limit, NULL); static DEVICE_ATTR(sensor1_limit, S_IRUGO, show_sensor1_limit, NULL); static DEVICE_ATTR(sensor2_limit, S_IRUGO, show_sensor2_limit, NULL); +static DEVICE_ATTR(sensor0_location, S_IRUGO, + show_sensor0_location, NULL); static DEVICE_ATTR(sensor1_location, S_IRUGO, show_sensor1_location, NULL); static DEVICE_ATTR(sensor2_location, S_IRUGO, @@ -426,10 +525,13 @@ static void thermostat_create_files(struct thermostat *th) return; dev = &th->pdev->dev; dev_set_drvdata(dev, th); - err = device_create_file(dev, &dev_attr_sensor1_temperature); + err = device_create_file(dev, &dev_attr_sensor0_temperature); + err |= device_create_file(dev, &dev_attr_sensor1_temperature); err |= device_create_file(dev, &dev_attr_sensor2_temperature); + err |= device_create_file(dev, &dev_attr_sensor0_limit); err |= device_create_file(dev, &dev_attr_sensor1_limit); err |= device_create_file(dev, &dev_attr_sensor2_limit); + err |= device_create_file(dev, &dev_attr_sensor0_location); err |= device_create_file(dev, &dev_attr_sensor1_location); err |= device_create_file(dev, &dev_attr_sensor2_location); err |= device_create_file(dev, &dev_attr_limit_adjust); @@ -449,10 +551,13 @@ static void thermostat_remove_files(struct thermostat *th) if (!th->pdev) return; dev = &th->pdev->dev; + device_remove_file(dev, &dev_attr_sensor0_temperature); device_remove_file(dev, &dev_attr_sensor1_temperature); device_remove_file(dev, &dev_attr_sensor2_temperature); + device_remove_file(dev, &dev_attr_sensor0_limit); device_remove_file(dev, &dev_attr_sensor1_limit); device_remove_file(dev, &dev_attr_sensor2_limit); + device_remove_file(dev, &dev_attr_sensor0_location); device_remove_file(dev, &dev_attr_sensor1_location); device_remove_file(dev, &dev_attr_sensor2_location); device_remove_file(dev, &dev_attr_limit_adjust); -- 2.0.5 On Wed, Mar 25, 2015 at 04:36:40PM +1100, Benjamin Herrenschmidt wrote: > On Thu, 2015-02-26 at 10:48 +0100, Thomas Haschka wrote: > > Hello, > > > > I hope I get it correct this time, thanks again. > > Thanks. Sorry for the delay, I've been a bit swamped. I looked at your > code a bit more in depth and I would appreciate a couple more changes > if you don't mind: > > > .../... > > > +static void update_fans_speed_singlefan (struct thermostat *th) { > > + > > + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. > > + * Necessites the readout of all three thermal sensors > > + * in order to avoid the harddisk and other components to overheat > > + * in the case of cold CPU. */ > > + > > + int lastvar = 0; /* last variation, for iBook */ > > + int i = 0; > > + int var = 0; > > + int var_sensor[3]; > > + int started = 0; > > + int fan_number = 0; > > So here you hard code fan_number instead of looping accross all fans, > you could just use a constant... > > > + for (i = 0; i < 3; i++) { > > + var_sensor[i] = th->temps[i] - th->limits[i]; > > + } > > + var = var_sensor[0]; > > + for (i = 1; i < 3; i++) { > > + if (var_sensor[i] > var) { > > + var = var_sensor[i]; > > + } > > + } > > Why two loops ? > > > + if (var > -1) { > > + int step = (255 - fan_speed) / 7; > > + int new_speed = 0; > > + /* hysteresis : change fan speed only if variation is > > + * more than two degrees */ > > + if (abs(var - th->last_var[fan_number]) > 2) { > > + > > + started = 1; > > + new_speed = fan_speed + ((var-1)*step); > > + > > + if (new_speed < fan_speed) > > + new_speed = fan_speed; > > + if (new_speed > 255) > > + new_speed = 255; > > + > > + if (verbose) > > + printk(KERN_DEBUG "adt746x:" > > + " Setting fans speed to %d " > > + "(limit exceeded by %d on %s)\n", > > + new_speed, var, > > + sensor_location[fan_number+1]); > > + write_both_fan_speed(th, new_speed); > > + th->last_var[fan_number] = var; > > + } > > + } else if (var < -2) { > > + /* don't stop fan if sensor2 is cold and sensor1 is not > > + * so cold (lastvar >= -1) */ > > + if (i == 2 && lastvar < -1) { > > + if (th->last_speed[fan_number] != 0) > > + if (verbose) > > + printk(KERN_DEBUG "adt746x:" > > + " Stopping " > > + "fans.\n"); > > + write_both_fan_speed(th, 0); > > + } > > + } > > This looks identical between the two functions (single/multi fan), can > you factor it out into a single static helper that they both call ? > > Cheers, > Ben. > > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1]: thermal driver therm_adt746.c 2015-03-26 16:23 ` Thomas Haschka @ 2015-03-26 21:31 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 9+ messages in thread From: Benjamin Herrenschmidt @ 2015-03-26 21:31 UTC (permalink / raw) To: Thomas Haschka; +Cc: linuxppc-dev On Thu, 2015-03-26 at 17:23 +0100, Thomas Haschka wrote: > Hello, > > Here's an updated version taking your suggested changements into account > (merged the two loops into one, changed a hardcoded value into a constant). Can you still move the core of the loop into a helper function ? There is still too much code duplication.... > Just to keep everything together, hereare the changements: > > 1. changes in sensors limits, we are down from 70, 50, 70 > to 45, 50, 70, where the first value is the environement sensor that is > located on the bottumside of the Harddrive. Empirical testing > shows that this speeds up the Fan almoast in the same moment as in OS X > > 2. A function has been added in order to take the environement sensor into > account on SingleFan Apple Notebooks (i.e. those that have air inlets > next to the harddrive and that are cooling, besides other components > using the single fan in these machines. > > 3. Output values in /sys/devices/temperatures/ etc.. have been changed. > First: All three sensors can be monitored right now instead of only the > GPU and CPU sensors. > Second: The units have been changed in order to correspond to the units > given by other temperature sensors, such as the coretemp (intel) sensor. > This allows to monitor them using several applications, notably xosview. > > Signed-off-by: Thomas Haschka <haschka@gmail.com> > --- > drivers/macintosh/therm_adt746x.c | 123 +++++++++++++++++++++++++++++++++++--- > 1 file changed, 114 insertions(+), 9 deletions(-) > > diff --git a/drivers/macintosh/therm_adt746x.c b/drivers/macintosh/therm_adt746x.c > index f433521..1a36ea44 100644 > --- a/drivers/macintosh/therm_adt746x.c > +++ b/drivers/macintosh/therm_adt746x.c > @@ -1,7 +1,8 @@ > /* > * Device driver for the i2c thermostat found on the iBook G4, Albook G4 > * > - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt > + * Copyright (C) 2003, 2004, 2015 > + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka > * > * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf > * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467 > @@ -45,8 +46,10 @@ static u8 REM_CONTROL[2] = {0x00, 0x40}; > static u8 FAN_SPEED[2] = {0x28, 0x2a}; > static u8 FAN_SPD_SET[2] = {0x30, 0x31}; > > -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */ > -static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > +/* Local sensor is down to 45 in order to assure stable harddrive and > + * components temperature on single fan machines */ > +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */ > +static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */ > static const char *sensor_location[3] = { "?", "?", "?" }; > > static int limit_adjust; > @@ -223,7 +226,7 @@ static void display_stats(struct thermostat *th) > } > #endif > > -static void update_fans_speed (struct thermostat *th) > +static void update_fans_speed_multifan (struct thermostat *th) > { > int lastvar = 0; /* last variation, for iBook */ > int i = 0; > @@ -278,6 +281,88 @@ static void update_fans_speed (struct thermostat *th) > } > } > > +static void update_fans_speed_singlefan (struct thermostat *th) { > + > + /* Single Fan Laptops i.e. 12 inch Powerbook, Ibook etc. > + * Necessites the readout of all three thermal sensors > + * in order to avoid the harddisk and other components to overheat > + * in the case of cold CPU. */ > + > + int lastvar = 0; /* last variation, for iBook */ > + int i = 0; > + int var = 0; > + int var_sensor[3]; > + int started = 0; > + const int fan_number = 0; > + > + /* select the highest delta from all three sensors and store it to var > + */ > + > + var_sensor[0] = th->temps[0] - th->limits[0]; > + var = var_sensor[0]; > + > + for (i = 1; i < 3; i++) { > + var_sensor[i] = th->temps[i] - th->limits[i]; > + if (var_sensor[i] > var) { > + var = var_sensor[i]; > + } > + } > + > + if (var > -1) { > + int step = (255 - fan_speed) / 7; > + int new_speed = 0; > + /* hysteresis : change fan speed only if variation is > + * more than two degrees */ > + if (abs(var - th->last_var[fan_number]) > 2) { > + > + started = 1; > + new_speed = fan_speed + ((var-1)*step); > + > + if (new_speed < fan_speed) > + new_speed = fan_speed; > + if (new_speed > 255) > + new_speed = 255; > + > + if (verbose) > + printk(KERN_DEBUG "adt746x:" > + " Setting fans speed to %d " > + "(limit exceeded by %d on %s)\n", > + new_speed, var, > + sensor_location[fan_number+1]); > + write_both_fan_speed(th, new_speed); > + th->last_var[fan_number] = var; > + } > + } else if (var < -2) { > + /* don't stop fan if sensor2 is cold and sensor1 is not > + * so cold (lastvar >= -1) */ > + if (i == 2 && lastvar < -1) { > + if (th->last_speed[fan_number] != 0) > + if (verbose) > + printk(KERN_DEBUG "adt746x:" > + " Stopping " > + "fans.\n"); > + write_both_fan_speed(th, 0); > + } > + } > + > + lastvar = var; > + > + if (started) > + return; /* we don't want to re-stop the fan > + * if sensor1 is heating and sensor2 is not */ > +} > + > +static void update_fans_speed(struct thermostat *th) { > + > + if (th->type == ADT7460) { > + /* Multifan Laptops */ > + update_fans_speed_multifan(th); > + } else { > + /* Singlefan Laptops i.e. 12 inch Powerbook, Ibook etc. */ > + update_fans_speed_singlefan(th); > + } > +} > + > static int monitor_task(void *arg) > { > struct thermostat* th = arg; > @@ -371,10 +456,18 @@ static ssize_t store_##name(struct device *dev, struct device_attribute *attr, c > return n; \ > } > > -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))) > -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))) > -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1]) > -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2]) > +/* Several units are multiplied by 1000 in order to have > + * coherent readings with other > + * temperature sensors such as intel coretemp. This further > + * allows programs such as xosview to read these sensors correctly */ > + > +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000) > +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000) > +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000) > +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000) > +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000) > +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000) > +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0]) > BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1]) > BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2]) > > @@ -387,14 +480,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed, 1) > BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust) > BUILD_STORE_FUNC_DEG(limit_adjust, th) > > +static DEVICE_ATTR(sensor0_temperature, S_IRUGO, > + show_sensor0_temperature,NULL); > static DEVICE_ATTR(sensor1_temperature, S_IRUGO, > show_sensor1_temperature,NULL); > static DEVICE_ATTR(sensor2_temperature, S_IRUGO, > show_sensor2_temperature,NULL); > +static DEVICE_ATTR(sensor0_limit, S_IRUGO, > + show_sensor0_limit, NULL); > static DEVICE_ATTR(sensor1_limit, S_IRUGO, > show_sensor1_limit, NULL); > static DEVICE_ATTR(sensor2_limit, S_IRUGO, > show_sensor2_limit, NULL); > +static DEVICE_ATTR(sensor0_location, S_IRUGO, > + show_sensor0_location, NULL); > static DEVICE_ATTR(sensor1_location, S_IRUGO, > show_sensor1_location, NULL); > static DEVICE_ATTR(sensor2_location, S_IRUGO, > @@ -426,10 +525,13 @@ static void thermostat_create_files(struct thermostat *th) > return; > dev = &th->pdev->dev; > dev_set_drvdata(dev, th); > - err = device_create_file(dev, &dev_attr_sensor1_temperature); > + err = device_create_file(dev, &dev_attr_sensor0_temperature); > + err |= device_create_file(dev, &dev_attr_sensor1_temperature); > err |= device_create_file(dev, &dev_attr_sensor2_temperature); > + err |= device_create_file(dev, &dev_attr_sensor0_limit); > err |= device_create_file(dev, &dev_attr_sensor1_limit); > err |= device_create_file(dev, &dev_attr_sensor2_limit); > + err |= device_create_file(dev, &dev_attr_sensor0_location); > err |= device_create_file(dev, &dev_attr_sensor1_location); > err |= device_create_file(dev, &dev_attr_sensor2_location); > err |= device_create_file(dev, &dev_attr_limit_adjust); > @@ -449,10 +551,13 @@ static void thermostat_remove_files(struct thermostat *th) > if (!th->pdev) > return; > dev = &th->pdev->dev; > + device_remove_file(dev, &dev_attr_sensor0_temperature); > device_remove_file(dev, &dev_attr_sensor1_temperature); > device_remove_file(dev, &dev_attr_sensor2_temperature); > + device_remove_file(dev, &dev_attr_sensor0_limit); > device_remove_file(dev, &dev_attr_sensor1_limit); > device_remove_file(dev, &dev_attr_sensor2_limit); > + device_remove_file(dev, &dev_attr_sensor0_location); > device_remove_file(dev, &dev_attr_sensor1_location); > device_remove_file(dev, &dev_attr_sensor2_location); > device_remove_file(dev, &dev_attr_limit_adjust); ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-03-26 21:31 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-23 11:58 [PATCH 1/1]: thermal driver therm_adt746.c Thomas Haschka 2015-02-23 15:23 ` Cedric Le Goater 2015-02-23 21:18 ` Benjamin Herrenschmidt 2015-02-25 11:24 ` Thomas Haschka 2015-02-25 21:12 ` Benjamin Herrenschmidt 2015-02-26 9:48 ` Thomas Haschka 2015-03-25 5:36 ` Benjamin Herrenschmidt 2015-03-26 16:23 ` Thomas Haschka 2015-03-26 21:31 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).