Message-ID: <cone.1240858632.795110.5316.1000@onepiie>
Date:	Mon, 27 Apr 2009 20:57:12 +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>
Subject: Re: [PATCH] Acer Aspire One Fan Control

Hi Boris,

thank you very much for your email.

Borislav Petkov writes:

> I did some testing on my Aspire One machine here and it looks pretty
> nice: while compiling a kernel watched the fan going on when the
> temperature reaches 67-68 (I don't think this is Celsius though, no?)
> and then turning itself off when temp goes below 60.

It is Celsius, the specification of the chipset and the atom core say that 
the chips are allowed to get 99 degree celsuis hot. So I think 70 degree 
Celsius are fine.

>>
>> +config ACERHDF
>> +	tristate "Acer Aspire One temperature and fan driver"
>> +	depends on THERMAL
>> +	depends on THERMAL_HWMON
> 
> depends on THERMAL && THERMAL_HWMON

What do you mean? Sorry, don't get it.

>> +/* module parameters */
>> +module_param(interval, int, 0600);
>> +MODULE_PARM_DESC(interval, "Polling interval of temperature check");
>> +module_param(fanon, int, 0600);
> 
> This allows for the user to potentially melt his CPU by entering a too high
> value. You should check that in the acerhdf_init() against the max allowed
> according to spec, I gather it is 67?

I will add a maximum temperature, I guess something about 80 degree 
Celsuis. But anyways, the user can still melt his/her cpu by switching to 
user mode and turning off the fan.

>> +struct bios_settings_t {
>> +	const char *vendor;
>> +	const char *version;
>> +	unsigned char fanreg;
>> +	unsigned char tempreg;
>> +	unsigned char cmd_off;
>> +	unsigned char cmd_auto;
>> +	unsigned char state_off;
> 
> obviously cmd_off and state_off are the same values so remove one of them.

I think it makes sense to leave it this way, because we don't know, what 
acer does for the next BIOS release :)

> let's put a prefix to all those function names, otherwise their names
> are all too generic:

Yes, will do that.

> 
> get_temp -> acerhdf_get_temp
> change_fanstate -> acerhdf_change_fanstate
> 

> you can wrap all those "if (verbose)"-checks in a macro making the code more
> readable:

Hm... I like those "if (verbose)" lines, as you can see directly, when the 
line is printed and don't have to search for the acerhdf_printk definition.

>> +	ec_write(bios_settings[bios_version].fanreg,
>> +			(state) ? bios_settings[bios_version].cmd_auto :
>> +			bios_settings[bios_version].cmd_off);
> 
> too unreadable.
> 
> how about:
> 
> 	u8 cmd = (state) ? bios_settings[bios_version].cmd_auto
> 			 : bios_settings[bios_version].cmd_off;
> 
> 	ec_write(bios_settings[bios_version].fanreg, cmd);

yes will do it your way.

>> +/* return temperature */
> 
> no need for stating the obvious.

jip :)

>> +/* set operation mode;
>> + * kernel: a kernel thread takes care about managing the
>> + *	 fan (see acerhdf_thread)
> 
> where is that acerhdf_thread? maybe stale comment from the kthread bits?

Old comment, will correct it.

> 
>> + * user: kernel thread is stopped and a userspace tool
>> + *	 should take care about managing the fan
>> + */
>> +static int set_mode(struct thermal_zone_device *thermal,
>> +		enum thermal_device_mode mode)
>> +{
>> +	if (mode == THERMAL_DEVICE_DISABLED) {
>> +		if (verbose)
>> +			printk(KERN_NOTICE "acerhdf: kernelmode OFF\n");
> 
> those printk's shouldn't depend on verbose since this is important info
> IMHO and I want to know that I've changed modes successfully and that my
> CPU doesn't get fried.

I think they should only be printed out in verbose mode, as it would flood 
the log file otherwise. Besides that the people who use this module can 
trust it as it is in the mainline kernel :)

>> +	*temperature = 89;
> 
> #define ACERHDF_TEMP_CRIT 89

jip.

>> +/* print current fan state */
>> +static int get_cur_state(struct thermal_cooling_device *cdev,
>> +		unsigned long *state)
>> +{
>> +	*state = get_fanstate();
> 
> you need error handling here:
> 
> 	if (*state < 0) {
> 		*state = 0xffff;
> 		return 1;
> 	}
> 	return 0;
> 
> or some other invalid value similar to how it's done in get_temp()
> above.

Will do it.

>> +	if (state && !old_state)
>> +		change_fanstate(1);
> 
> let's have defines for those fan states
> #define ACERHDF_FAN_OFF		0
> #define ACERHDF_FAN_AUTO	1
> 
> and then do
> 
> change_fanstate(ACERHDF_FAN_AUTO);

jip.

>> +	/* if started in user mode, prevent the kernel from switching
>> +	 * off the fan */
>> +	if (!kernelmode) {
>> +		recently_changed = 1;
>> +		printk(KERN_NOTICE
>> +			"acerhdf: kernelmode disabled\n");
>> +		printk(KERN_NOTICE
>> +			"acerhdf: to enable kernelmode:\n");
>> +		printk(KERN_NOTICE
>> +			"acerhdf: echo -n \"enabled\" > "
>> +			"/sys/class/thermal/thermal_zone0/mode\n");
> 
> maybe I'm missing something but shouldn't this be enabled by default and
> only when the user wants to have acerfand or some other uspace tool do
> the controlling, only then turn it off. I'd rather trust this is done
> in the kernel instead of some flaky uspace thread which could maybe
> segfault and we fry our nice little netbook :).

Matthew suggested to start the module in usermode, where the BIOS takes 
care about the fan as long as there is no userspace tool or the user want 
the kernel to care about the fan.

>> +	/* register platform device */
> 
> don't need those comments, function name is enough

jip

>> +	if (platform_driver_register(&acerhdf_driver)) {


best 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