Re: [PATCH] Acer Aspire One Fan Control

[Peter Feuerer <peter@piie.net>]

Hi Boris,

thank you very much! A modified patch with your suggestions will follow 
as soon as I tested it. Just some things I would like to discuss:

Borislav Petkov writes:
>> +/* change current fan state - is overwritten when running in kernel mode */
>> +static int set_cur_state(struct thermal_cooling_device *cdev,
>> +		unsigned long state)
>> +{
>> +	int old_state;
>> +
>> +	/* let the thermal layer disable kernelmode. This ensures that
>> +	 * the thermal layer doesn't switch off the fan again */
>> +	if (disable_kernelmode) {
>> +		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
>> +		disable_kernelmode = 0;
>> +		kernelmode = 0;
>> +		return 0;
>> +	}
>> +
>> +	if (!kernelmode) {
>> +		acerhdf_change_fanstate(state);
>> +		return 0;
>> +	}
> 
> why are we changing the fan state if kernelmode is off? If I'm not
> mistaken, the BIOS should be controlling the fan here.

The user can control the fan in userspace (by e.g. echoing 0 to the 
/sys/class/thermal.../cur_state file) then this function is called. This is 
useful if somebody wants to program an userspace tool to handle the 
temperature and the fan.

>> +	old_state = acerhdf_get_fanstate();
>> +
>> +	/* if reading the fan's state returns unexpected value, there's a
>> +	 * problem with the ec register. -> let the BIOS take control of
>> +	 * the fan to prevent hardware damage */
>> +	if (old_state != fanstate) {
> 
> you should be checking 
> 
> 	old_state == ACERHDF_ERROR
> 
> here instead.

Comparing to fanstate is better. It implys the ACERHDF_ERROR check, as 
fanstate can only be ACERHDF_FAN_AUTO or ACERHDF_FAN_OFF. And on the other 
Hand there will be thrown an error too, if the ec register contains an 
unexpected value. So it is more failsafe this way.

What do you think will be the next steps to get the kernel into mainline? 
Matthew said I should CC Len Brown, as he is responsible to include 
the module. But Len didn't write anything yet :-(

kind regards,
--peter