Re: [ibm-acpi-devel] Fan level 7 after resume wit 2.6.28-rc3

[Tino Keitel <tino.keitel@tikei.de>]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On Thu, Nov 06, 2008 at 01:35:44 +0100, Tino Keitel wrote:

[...]

> I tried to write a correct patch, but I got lost in all that
> fan_control_desired_level, fan_control_initial_status and
> tp_features.fan_ctrl_status_undef stuff.
> 
> My brain says that one would just read the current fan settings from
> the EC at initialization. Then, after resume, this setting is restored
> unconditionally, or at least if it differs from current_level. The
> attached patch works for me. However, I don't have all the knowledge
> about older models and their specific behaviour.
> 
> Correction: I just tested a bit further, and it doesn't work. If I set
> fan level to 3, suspend, resume, set fan level to auto, and
> resume/suspend again, fan level is restored to 3. This is because
> fan_control_desired_level isn't updated by fan_update_desired_level()
> if it is set back to auto, but kept at the old value. So, my impression
> is that all the values and states should be cleaned up a bit and
> simplified. In the current state, there are a lot of strage checks and
> quirks that have side effects when other parts are changed.

The whole fan level stuff looks a bit complicated to me. Especially the
fan_control_desired_level handling is somethat strange because it
considers values as invalid that are written by fan_set_level() before.
It ignores the TP_EC_FAN_FULLSPEED and TP_EC_FAN_AUTO values. Now, in
fan_resume(), this value is checked against TP_EC_FAN_FULLSPEED which
makes no sense to me.

The attached patch tries to simplify this a bit. It sets
fan_control_desired_level to the current EC value in fan_init(), and
also tries to keep fan_control_desired_level and the real EC value in
sync. I also removed the checks against 7 and TP_EC_FAN_FULLSPEED in
fan_resume() as they made no sense to me. This works as intended on my
X61s after rmmod/insmod and suspend/resume.

Regards,
Tino

[-- Attachment #2: thinkpad-acpi_fan-level-restore-fix_v2.diff --]
[-- Type: text/x-diff, Size: 1366 bytes --]

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 4db1cf9..7f095c4 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -5329,13 +5329,8 @@ TPACPI_HANDLE(sfan, ec, "SFAN",	/* 570 */
  */
 static void fan_update_desired_level(u8 status)
 {
-	if ((status &
-	     (TP_EC_FAN_AUTO | TP_EC_FAN_FULLSPEED)) == 0) {
-		if (status > 7)
-			fan_control_desired_level = 7;
-		else
-			fan_control_desired_level = status;
-	}
+	fan_control_desired_level = status &
+		(TP_EC_FAN_AUTO | TP_EC_FAN_FULLSPEED | 7);
 }
 
 static int fan_get_status(u8 *status)
@@ -5886,6 +5881,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 		if (likely(acpi_ec_read(fan_status_offset,
 					&fan_control_initial_status))) {
 			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
+			fan_update_desired_level(fan_control_initial_status);
 
 			/* In some ThinkPads, neither the EC nor the ACPI
 			 * DSDT initialize the fan status, and it ends up
@@ -6025,9 +6021,8 @@ static void fan_resume(void)
 		break;
 	case TPACPI_FAN_WR_ACPI_FANS:
 	case TPACPI_FAN_WR_TPEC:
-		do_set = ((saved_fan_level & TP_EC_FAN_FULLSPEED) ||
-			  (saved_fan_level == 7 &&
-			   !(current_level & TP_EC_FAN_FULLSPEED)));
+		do_set = (current_level != saved_fan_level) && 
+			!(current_level & TP_EC_FAN_FULLSPEED);
 		break;
 	default:
 		return;