Re: [PATCH] Acer Aspire One Fan Control

[Joe Perches <joe@perches.com>]

On Sat, 2009-04-25 at 10:42 +0200, Peter Feuerer wrote:
> The patch is compiled and tested against current git/torvalds/linux-2.6.git checkout.
> 
> What do you think? Do you have any questions?

Trivial comments:

> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> new file mode 100644
> index 0000000..63dc485
> --- /dev/null
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -0,0 +1,594 @@
> +/*
> + * acerhdf - A kernelmodule which monitors the temperature

kernel module

> + *           of the aspire one netbook, turns on/off the fan
> + *           as soon as the upper/lower threshold is reached.
> + *
> + * (C) 2009 - Peter Feuerer     peter (a) piie.net
> + *                              http://piie.net
> + *
> + *
> + *
> + * Inspired by and many thanks to:
> + *  o acerfand   - Rachel Greenham
> + *  o acer_ec.pl - Michael Kurz     michi.kurz (at) googlemail.com
> + *               - Petr Tomasek     tomasek (#) etf,cuni,cz
> + *               - Carlos Corbacho  cathectic (at) gmail.com
> + *
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + *
> + *  06-February-2009: Version 0.1:
> + *  - first relase, containing absolutely no bugs! ;)
[]
> + *  25-April-2009: Version 0.5:
> + *  - ported to 2.6.30
> + *  - removed kthread and used polling of thermal api

Version information in comments isn't generally useful

> +/* print currend operation mode - kernel / user */

current

> +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");
> +		thermal->polling_delay = 0;
> +		thermal_zone_device_update(thermal);
> +		change_fanstate(1);
> +		/* silly hack - let the polling thread disable
> +		 * kernelmode. This ensures, that the polling thread
> +		 * doesn't switch off the fan again */
> +		recently_changed = 1;
> +	} else if (mode == THERMAL_DEVICE_ENABLED) {
> +		if (verbose)
> +			printk(KERN_NOTICE "acerhdf: kernelmode ON\n");
> +		thermal->polling_delay = interval*1000;
> +		thermal_zone_device_update(thermal);
> +		kernelmode = 1;
> +	}
> +	return 0;
> +}

There's no other thermal device_mode state than ENABLED/DISABLED.
Maybe just if/else?

> +/* print the name of the trip point */
> +static int get_trip_type(struct thermal_zone_device *thermal,
> +		int trip, enum thermal_trip_type *type)
> +{
> +	if (trip == 0)
> +		*type = THERMAL_TRIP_ACTIVE;
> +	return 0;
> +}

Comment doesn't match code.  I see no printing here.

> +/* print the temperature at which the trip point gets active */
> +static int get_trip_temp(struct thermal_zone_device *thermal,
> +		int trip, unsigned long *temp)
> +{
> +	if (trip == 0)
> +		*temp = fanon;
> +	return 0;
> +}

No printing here either.

You could #define pr_fmt(fmt) "acerhd: " fmt
instead of prefixing all the printks with "acerhd:"