[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cone.1241638874.190271.13804.1000@onepiie>
Date: Wed, 06 May 2009 21:41:14 +0200
From: Peter Feuerer <peter@...e.net>
To: petkovbb@...il.com
Cc: LKML <linux-kernel@...r.kernel.org>, lenb@...nel.org,
Matthew Garrett <mjg59@...f.ucam.org>,
Maxim Levitsky <maximlevitsky@...il.com>
Subject: Re: [PATCH] Acer Aspire One Fan Control
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists