Re: [PATCH][drivers/misc/thinkpad_acpi.c] duplicate test if (level& TP_EC_FAN_FULLSPEED)

[Henrique de Moraes Holschuh]

On Tue, 05 Feb 2008, Roel Kluin wrote:
> Roland Dreier wrote:
> >  >                 /* safety net should the EC not support AUTO
> >  >                  * or FULLSPEED mode bits and just ignore them */
> >  >                 if (level & TP_EC_FAN_FULLSPEED)
> >  >                         level |= 7;     /* safety min speed 7 */
> >  >                 else if (level & TP_EC_FAN_FULLSPEED)
> >  >                         level |= 4;     /* safety min speed 4 */
> >  > 
> >  > Note the duplicate test 'if (level & TP_EC_FAN_FULLSPEED)'. should
> >  > this be replaced by
> > 
> > Actually I suspect one of the two tests should be against TP_EC_FAN_AUTO
> > (based on the comment).
> 
> Thanks Roland, for your info
> 
> based on the comments in commit eaa7571b2d1a08873e4bdd8e6db3431df61cd9ad,
> I think this should be modified like below:
> 
> ACPI: thinkpad-acpi: add a safety net for TPEC fan control mode
> The Linux ThinkPad community is not positive that all ThinkPads that do
> HFSP EC fan control do implement full-speed and auto modes, some of the
> earlier ones supporting HFSP might not.
> 
> If the EC ignores the AUTO or FULL-SPEED bits, it will pay attention to the
> lower three bits that set the fan level. And as thinkpad-acpi was leaving
> these set to zero, it would stop(!) the fan, which is Not A Good Thing.
> So, as a safety net, we now make sure to also set the fan level part of the
> HFSP register to speed 7 for full-speed, and a minimum of speed 4 for auto
> mode.
> --
> second TP_EC_FAN_FULLSPEED should be P_EC_FAN_AUTO
> 
> 
> Signed-off-by: Roel Kluin <12o3l@xxxxxxxxxx>
> ---
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index cf56647..3c323fe 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -4138,7 +4138,7 @@ static int fan_set_level(int level)
>  		 * or FULLSPEED mode bits and just ignore them */
>  		if (level & TP_EC_FAN_FULLSPEED)
>  			level |= 7;	/* safety min speed 7 */
> -		else if (level & TP_EC_FAN_FULLSPEED)
> +		else if (level & TP_EC_FAN_AUTO)
>  			level |= 4;	/* safety min speed 4 */
>  
>  		if (!acpi_ec_write(fan_status_offset, level))

ACK.  This needs to be sent to stable as well.  I think both 2.6.22 and
2.6.23 need this patch.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/