[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: powerbook fan behaviour, therm_adt7467



hi,

> I'd like to keep it simple stupid. If you want a stepping, you'll end up
> with a lot of code to handle hysteresis - which is, don't slow down the
> fan as soon as the temp is going under one limit (or the other way) or
> your fan will change speed every two seconds. Which is very
> annoying to hear...

hmm, see, personally I find it more annoying to have no fan at all because I'm 
just reading mail or writing some absolutely crazy ocaml code while heat 
gradually builds up. then suddenly, out of nowhere, the fan kicks in really 
loud.

the only choice right now is to set the fan_speed to a lower value, however in 
that case it becomes more likely to reach lim+8 rather quickly and then the 
fan reeaally kicks in...

I'd just like some more configuration options so that I can decide on my own 
what's annoying and what's not...

but it's right, the module basically does its job pretty well, it's just the 
stepping that I miss...

therefore I've slightly modified it to make it more configurable (patch 
against 2.6.9 attached)

1) you get 3 new parameters:
limit_override: like limit_adjust, but sets the temperature at which we force 
full speed
fan_step: the step in fan speed that we want to have
step_degrees: the temperature step after which we increase the fan_speed by 
fan_step.

thus, we get stepping starting at limit_adjust, but once limit_override is 
reached, we force full speed again.

the hysteresis problem is not really addressed, instead we will simply "lag" 
one step when reducing the fan speed in order to prevent cases where temp 
falls one step, we reduce the fan speed, then immediately the temp jumps up a 
step again -> we'll rather wait until the temp drops 2 steps and then we 
reduce the fan speed by one step. At lim-2, the fan stops completely.

If you do not set the new parameters upon module insertion or via sysfs, the 
module behaves just like the original one does...

I've just tried a step_degrees = 1, fan_step = 20, fan_speed = 60, 
limit_adjust = -3 and limit_override = 6. for my taste, it works the way I 
want it to right now (on a 12" powerbook), although the parameters surely can 
be tweaked again... 
when you start up cpu-intensive tasks, the fan speed gradually increases (but 
that's what I wanted), when you are doing non-intensive tasks, the fan only 
runs at low speeds like 60 or 80, being really quiet. "Jumping" between two 
values hasn't happened yet... 

still, I outcommented the printk when setting a new fan speed in order to keep 
the syslog clean...

I'd recommend to try it out, if you like it I may submit it...

regards,
georg kaindl
--- linux-2.6.9/drivers/macintosh/therm_adt746x.c.orig	2004-10-26 19:06:04.699402232 +0200
+++ linux-2.6.9/drivers/macintosh/therm_adt746x.c	2004-10-26 19:05:05.506400936 +0200
@@ -48,6 +48,9 @@ static u8 default_limits_chip[3] = {80, 
 
 static int limit_adjust = 0;
 static int fan_speed = -1;
+static int limit_override = 8;
+static int fan_step = 0;
+static int step_degrees = 2;
 
 MODULE_AUTHOR("Colin Leroy <colin@colino.net>");
 MODULE_DESCRIPTION("Driver for ADT746x thermostat in iBook G4 and Powerbook G4 Alu");
@@ -57,12 +60,20 @@ MODULE_PARM(limit_adjust,"i");
 MODULE_PARM_DESC(limit_adjust,"Adjust maximum temperatures (50 cpu, 70 gpu) 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(limit_override,"i");
+MODULE_PARM_DESC(limit_override,"Temperature offset at which the fans should be set to full speed");
+MODULE_PARM(fan_step,"i");
+MODULE_PARM_DESC(fan_step,"Increment to fan_speed that is applied at every <step_degrees> degree increment in temperature");
+MODULE_PARM(step_degrees,"i");
+MODULE_PARM_DESC(step_degrees,"The temperature step after which the fan_speed gets increased by <fan_step>, or decreased respectively");
+
 
 struct thermostat {
 	struct i2c_client	clt;
 	u8			cached_temp[3];
 	u8			initial_limits[3];
 	u8			limits[3];
+	u8			limits_override[3];
 	int			last_speed[2];
 	int			overriding[2];
 };
@@ -203,9 +214,12 @@ static void write_fan_speed(struct therm
 		if (speed == -1)
 			printk(KERN_INFO "adt746x: Setting speed to: automatic for %s fan.\n",
 				fan?"GPU":"CPU");
+		/* too noisy on the syslog, therefore commented out */
+		/*
 		else
 			printk(KERN_INFO "adt746x: Setting speed to: %d for %s fan.\n",
 				speed, fan?"GPU":"CPU");
+		*/
 	} else
 		return;
 	
@@ -232,6 +246,7 @@ static int monitor_task(void *arg)
 	struct thermostat* th = arg;
 	u8 temps[3];
 	u8 lims[3];
+	u8 lims_override[3];
 	int i;
 #ifdef DEBUG
 	int mfan_speed;
@@ -248,8 +263,9 @@ static int monitor_task(void *arg)
 		if (fan_speed != -1) {
 #endif
 			for (i = 0; i < 3; i++) {
-				temps[i]  = read_reg(th, TEMP_REG[i]);
-				lims[i]   = th->limits[i];
+				temps[i]         = read_reg(th, TEMP_REG[i]);
+				lims[i]          = th->limits[i];
+				lims_override[i] = th->limits_override[i];
 			}
 #ifndef DEBUG
 		}
@@ -259,20 +275,28 @@ static int monitor_task(void *arg)
 			for (i = 1; i < 3; i++) {	/* we don't care about local sensor */
 				int started = 0;
 				int fan_number = (therm_type == ADT7460 && i == 2);
-				int var = temps[i] - lims[i];
-				if (var > 8) {
+				int var   = temps[i] - lims[i];
+				int var_o = temps[i] - lims_override[i];
+				if (var_o > 0) {
 					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");
+						printk(KERN_INFO "adt746x: Override-Limit exceeded by %d, overriding specified fan speed for %s.\n",
+							var_o, 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) {
+				} else if ((!th->overriding[fan_number] || var_o < -2) && var > 0) {
+					int new_fan_speed = fan_speed + (var - 1)/step_degrees * fan_step;
+
+					/* don't decrease the fan speed immediately, let the temp drop another step_degrees first */
+					/* note that the case in which temp < lim-2 is not handled here but in the next if statement */
+					if (new_fan_speed == th->last_speed[fan_number]-fan_step) continue;
+					else if (new_fan_speed < th->last_speed[fan_number]) new_fan_speed += fan_step;
+					
 					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");					
+						printk(KERN_INFO "adt746x: Limit exceeded by %d, setting speed to %d for %s.\n",
+							var, new_fan_speed, fan_number?"GPU":"CPU");					
 					th->overriding[fan_number] = 0;
-					write_fan_speed(th, fan_speed, fan_number);
+					write_fan_speed(th, new_fan_speed, fan_number);
 					started = 1;
 				} else if (var < -1) {
 					/* don't stop iBook fan if GPU is cold and CPU is not
@@ -325,6 +349,7 @@ set_limit(struct thermostat *th, int i)
 		
 		/* set our limits to normal */
 		th->limits[i] = default_limits_local[i] + limit_adjust;
+		th->limits_override[i] = default_limits_local[i] + limit_override;
 }
 	
 static int
@@ -375,6 +400,10 @@ attach_one_thermostat(struct i2c_adapter
 		th->initial_limits[0], th->initial_limits[1], th->initial_limits[2], 
 		th->limits[0], th->limits[1], th->limits[2]);
 
+	printk(KERN_INFO "adt746x: limit_adjust = %d, limit_override = %d, "
+		"fan_speed = %d, fan_step = %d, step_degrees = %d\n",
+		limit_adjust, limit_override, fan_speed, fan_step, step_degrees);
+
 	thermostat = th;
 
 	if (i2c_attach_client(&th->clt)) {
@@ -426,28 +455,31 @@ static ssize_t show_##name(struct device
 		);						\
 }
 
-#define BUILD_STORE_FUNC_DEG(name, data)			\
+#define BUILD_STORE_FUNC_DEG(name, data, logstr)		\
 static ssize_t store_##name(struct device *dev, const char *buf, size_t n) \
 {								\
 	int val;						\
 	int i;							\
 	val = simple_strtol(buf, NULL, 10);			\
-	printk(KERN_INFO "Adjusting limits by %d°C\n", val);	\
-	limit_adjust = val;					\
+	printk(KERN_INFO "adt746x: " logstr,  val);		\
+	name = val;						\
 	for (i=0; i < 3; i++)					\
 		set_limit(thermostat, i);			\
 	return n;						\
 }
 
-#define BUILD_STORE_FUNC_INT(name, data)			\
+#define BUILD_STORE_FUNC_INT(name, data, logstr)		\
 static ssize_t store_##name(struct device *dev, const char *buf, size_t n) \
 {								\
 	u32 val;						\
 	val = simple_strtoul(buf, NULL, 10);			\
 	if (val < 0 || val > 255)				\
 		return -EINVAL;					\
-	printk(KERN_INFO "Setting specified fan speed to %d\n", val);	\
+	printk(KERN_INFO "adt746x: " logstr, val);		\
 	data = val;						\
+	if (step_degrees == 0) {				\
+		step_degrees = 1;				\
+	}							\
 	return n;						\
 }
 
@@ -455,14 +487,22 @@ BUILD_SHOW_FUNC_INT(cpu_temperature,	 (r
 BUILD_SHOW_FUNC_INT(gpu_temperature,	 (read_reg(thermostat, TEMP_REG[2])))
 BUILD_SHOW_FUNC_INT(cpu_limit,		 thermostat->limits[1])
 BUILD_SHOW_FUNC_INT(gpu_limit,		 thermostat->limits[2])
+BUILD_SHOW_FUNC_INT(cpu_limit_override,	 thermostat->limits_override[1])
+BUILD_SHOW_FUNC_INT(gpu_limit_override,	 thermostat->limits_override[2])
 
 BUILD_SHOW_FUNC_INT(specified_fan_speed, fan_speed)
+BUILD_SHOW_FUNC_INT(specified_fan_step, fan_step)
+BUILD_SHOW_FUNC_INT(specified_step_degrees, step_degrees)
 BUILD_SHOW_FUNC_FAN(cpu_fan_speed,	 0)
 BUILD_SHOW_FUNC_FAN(gpu_fan_speed,	 1)
 
-BUILD_STORE_FUNC_INT(specified_fan_speed,fan_speed)
+BUILD_STORE_FUNC_INT(specified_fan_speed,fan_speed,"Setting specified fan speed to %d\n")
+BUILD_STORE_FUNC_INT(specified_fan_step,fan_step,"Setting specified fan step to %d\n")
+BUILD_STORE_FUNC_INT(specified_step_degrees,step_degrees,"Setting fan speed increment temperature threshold to %d\n")
 BUILD_SHOW_FUNC_INT(limit_adjust,	 limit_adjust)
-BUILD_STORE_FUNC_DEG(limit_adjust,	 thermostat)
+BUILD_SHOW_FUNC_INT(limit_override,	 limit_override)
+BUILD_STORE_FUNC_DEG(limit_adjust,	 thermostat,"Adjusting limits by %d°C\n")
+BUILD_STORE_FUNC_DEG(limit_override,	 thermostat,"Adjusting override limits by %d°C\n")
 		
 static DEVICE_ATTR(cpu_temperature,	S_IRUGO,
 		   show_cpu_temperature,NULL);
@@ -472,10 +512,17 @@ static DEVICE_ATTR(cpu_limit,		S_IRUGO,
 		   show_cpu_limit,	NULL);
 static DEVICE_ATTR(gpu_limit,		S_IRUGO,
 		   show_gpu_limit,	NULL);
+static DEVICE_ATTR(cpu_limit_override,	S_IRUGO,
+		   show_cpu_limit_override,NULL);
+static DEVICE_ATTR(gpu_limit_override,	S_IRUGO,
+		   show_gpu_limit_override,NULL);
 
 static DEVICE_ATTR(specified_fan_speed,	S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
 		   show_specified_fan_speed,store_specified_fan_speed);
 
+static DEVICE_ATTR(specified_fan_step,	S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
+		   show_specified_fan_step,store_specified_fan_step);
+
 static DEVICE_ATTR(cpu_fan_speed,	S_IRUGO,
 		   show_cpu_fan_speed,	NULL);
 static DEVICE_ATTR(gpu_fan_speed,	S_IRUGO,
@@ -484,6 +531,11 @@ static DEVICE_ATTR(gpu_fan_speed,	S_IRUG
 static DEVICE_ATTR(limit_adjust,	S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
 		   show_limit_adjust,	store_limit_adjust);
 
+static DEVICE_ATTR(limit_override,	S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
+		   show_limit_override,	store_limit_override);
+
+static DEVICE_ATTR(specified_step_degrees,S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
+		   show_specified_step_degrees,	store_specified_step_degrees);
 
 static int __init
 thermostat_init(void)
@@ -521,9 +573,14 @@ thermostat_init(void)
 	device_create_file(&of_dev->dev, &dev_attr_gpu_temperature);
 	device_create_file(&of_dev->dev, &dev_attr_cpu_limit);
 	device_create_file(&of_dev->dev, &dev_attr_gpu_limit);
+	device_create_file(&of_dev->dev, &dev_attr_cpu_limit_override);
+	device_create_file(&of_dev->dev, &dev_attr_gpu_limit_override);
 	device_create_file(&of_dev->dev, &dev_attr_limit_adjust);
 	device_create_file(&of_dev->dev, &dev_attr_specified_fan_speed);
 	device_create_file(&of_dev->dev, &dev_attr_cpu_fan_speed);
+	device_create_file(&of_dev->dev, &dev_attr_specified_fan_step);
+	device_create_file(&of_dev->dev, &dev_attr_limit_override);
+	device_create_file(&of_dev->dev, &dev_attr_specified_step_degrees);
 	if(therm_type == ADT7460)
 		device_create_file(&of_dev->dev, &dev_attr_gpu_fan_speed);
 

Reply to: