On 28 Oct 2004 at 18h10, Georg C. Kaindl wrote:
Hi,
> But when watching the behaviour of the fan speed stepping code I noticed that
> it happens quite often that when reducing the fan speed it will get set to
> values < fan_speed because of (var > -2). I don't know wether this is
> intended but I think it is a indeed a good idea to slightly expand the range
> between starting the fan and overriding the value, starting out really slow.
> However it would be good to add another check for a reasonably high new_speed
> because by changing the fan_speed parameter one could reach fan speeds as low
> as 1. Maybe 20 is a good lower bound value...
> additionally, it doesn't look like the special case of overriding the
> specified fan speed is needed anymore because at var == 8 and latest at var
> ==9 (because of hysteresis) we reach 255 anyways, but var == 9 also is the
> first case where we would override...
Thanks for your feedback. You're right, I missed these two points... Here's
an updated patch that should behave more logically (ie, starting when temp
== lim and stopping when temp < lim - 2), and removing the override case that
isn't needed anymore.
I may not be able to follow up on this until monday evening, but will do asap.
--
Colin
http://dudusdl.sf.net/ : a free Puzzle Bubble clone
--- a/drivers/macintosh/therm_adt746x.c 2004-10-28 16:02:13.363979632 +0200
+++ b/drivers/macintosh/therm_adt746x.c 2004-10-28 22:10:19.433920744 +0200
@@ -61,8 +61,8 @@
"by N degrees.");
MODULE_PARM(fan_speed,"i");
-MODULE_PARM_DESC(fan_speed,"Specify fan speed (0-255) when lim < temp < lim+8 "
- "(default 128)");
+MODULE_PARM_DESC(fan_speed,"Specify starting fan speed (0-255) "
+ "(default 64)");
struct thermostat {
struct i2c_client clt;
@@ -71,7 +71,7 @@
u8 initial_limits[3];
u8 limits[3];
int last_speed[2];
- int overriding[2];
+ int last_var[2];
};
static enum {ADT7460, ADT7467} therm_type;
@@ -80,7 +80,7 @@
static struct thermostat* thermostat;
static struct task_struct *thread_therm = NULL;
-static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
+static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
int busno);
static void write_both_fan_speed(struct thermostat *th, int speed);
@@ -151,7 +151,7 @@
printk(KERN_INFO "adt746x: Putting max temperatures back from "
"%d, %d, %d to %d, %d, %d\n",
th->limits[0], th->limits[1], th->limits[2],
- th->initial_limits[0], th->initial_limits[1],
+ th->initial_limits[0], th->initial_limits[1],
th->initial_limits[2]);
for (i = 0; i < 3; i++)
@@ -211,10 +211,10 @@
if (th->last_speed[fan] != speed) {
if (speed == -1)
- printk(KERN_INFO "adt746x: Setting speed to automatic "
+ printk(KERN_DEBUG "adt746x: Setting speed to automatic "
"for %s fan.\n", fan?"GPU":"CPU");
else
- printk(KERN_INFO "adt746x: Setting speed to %d "
+ printk(KERN_DEBUG "adt746x: Setting speed to %d "
"for %s fan.\n", speed, fan?"GPU":"CPU");
} else
return;
@@ -226,10 +226,10 @@
} else {
/* back to automatic */
if(therm_type == ADT7460) {
- manual = read_reg(th,
+ manual = read_reg(th,
MANUAL_MODE[fan]) & (~MANUAL_MASK);
- write_reg(th,
+ write_reg(th,
MANUAL_MODE[fan], manual|REM_CONTROL[fan]);
} else {
manual = read_reg(th, MANUAL_MODE[fan]);
@@ -243,7 +243,7 @@
static void read_sensors(struct thermostat *th)
{
int i = 0;
-
+
for (i = 0; i < 3; i++)
th->temps[i] = read_reg(th, TEMP_REG[i]);
}
@@ -272,49 +272,49 @@
{
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 = (therm_type == ADT7460 && i == 2);
int var = th->temps[i] - th->limits[i];
- if (var > 8) {
- if (th->overriding[fan_number] == 0)
- printk(KERN_INFO "adt746x: Limit exceeded by "
- "%d, overriding specified fan speed "
- "for %s.\n", var,
- fan_number?"GPU":"CPU");
-
- th->overriding[fan_number] = 1;
- write_fan_speed(th, 255, fan_number);
- started = 1;
- } else if ((!th->overriding[fan_number] || var < 6) && var > 0)
{
- if (th->overriding[fan_number] == 1)
- printk(KERN_INFO "adt746x: Limit exceeded by "
- "%d, setting speed to specified "
- "for %s.\n", var,
- fan_number?"GPU":"CPU");
-
- th->overriding[fan_number] = 0;
- write_fan_speed(th, fan_speed, fan_number);
+
+ 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 (var > 6 || abs(var - th->last_var[fan_number]) < 2)
+ continue;
+
started = 1;
- } else if (var < -1) {
- /* don't stop iBook fan if GPU is cold and CPU is not
+ new_speed = fan_speed + ((var-1)*step);
+ if (new_speed < fan_speed)
+ new_speed = fan_speed;
+
+ printk(KERN_DEBUG "adt746x: setting fans speed to %d "
+ "(limit exceeded by %d on %s) \n",
+ new_speed, var,
+ fan_number?"GPU/pwr":"CPU");
+ write_both_fan_speed(th, new_speed);
+ th->last_var[fan_number] = var;
+ } else if (var < -2) {
+ /* don't stop fan if GPU/power is cold and CPU is not
* so cold (lastvar >= -1) */
- if (therm_type == ADT7460 || lastvar < -1 || i == 1) {
+ if (i == 2 && lastvar < -1) {
if (th->last_speed[fan_number] != 0)
- printk(KERN_INFO "adt746x: Stopping %s "
- "fan.\n",
- fan_number?"GPU":"CPU");
- write_fan_speed(th, 0, fan_number);
+ printk(KERN_DEBUG "adt746x: Stopping "
+ "fans.\n");
+ write_both_fan_speed(th, 0);
}
}
lastvar = var;
- if (started && therm_type == ADT7467)
+ if (started)
return; /* we don't want to re-stop the fan
- * if CPU is heating and GPU is not */
+ * if CPU is heating and GPU/power is not */
}
}
@@ -327,7 +327,7 @@
refrigerator(PF_FREEZE);
msleep_interruptible(2000);
-
+
#ifndef DEBUG
if (fan_speed != -1)
read_sensors(th);
@@ -357,7 +357,7 @@
th->limits[i] = default_limits_local[i] + limit_adjust;
}
-static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
+static int attach_one_thermostat(struct i2c_adapter *adapter, int addr,
int busno)
{
struct thermostat* th;
@@ -391,7 +391,7 @@
/* force manual control to start the fan quieter */
if (fan_speed == -1)
- fan_speed=128;
+ fan_speed=64;
if(therm_type == ADT7460) {
printk(KERN_INFO "adt746x: ADT7460 initializing\n");
@@ -407,8 +407,8 @@
printk(KERN_INFO "adt746x: Lowering max temperatures from %d, %d, %d"
" to %d, %d, %d\n",
- th->initial_limits[0], th->initial_limits[1],
- th->initial_limits[2], th->limits[0], th->limits[1],
+ th->initial_limits[0], th->initial_limits[1],
+ th->initial_limits[2], th->limits[0], th->limits[1],
th->limits[2]);
thermostat = th;
@@ -424,7 +424,9 @@
/* be sure to really write fan speed the first time */
th->last_speed[0] = -2;
th->last_speed[1] = -2;
-
+ th->last_var[0] = -80;
+ th->last_var[1] = -80;
+
if (fan_speed != -1) {
/* manual mode, stop fans */
write_both_fan_speed(th, 0);
@@ -585,9 +587,9 @@
device_remove_file(&of_dev->dev, &dev_attr_limit_adjust);
device_remove_file(&of_dev->dev, &dev_attr_specified_fan_speed);
device_remove_file(&of_dev->dev, &dev_attr_cpu_fan_speed);
-
+
if(therm_type == ADT7460)
- device_remove_file(&of_dev->dev,
+ device_remove_file(&of_dev->dev,
&dev_attr_gpu_fan_speed);
of_device_unregister(of_dev);
