Re: [PATCH] Acer Aspire One Fan Control

[Peter Feuerer]

Hi Boris,

thanks again for all you helpful comments. A new patch will follow shortly.

Borislav Petkov writes:

> Ok, minor nitpicks below but it starting to shape up quite ok. You could
> send it for inclusion upstream.

How exactly do I send the patch for inclusion?

> > +/* acer ec functions */
> > +static int acerhdf_get_temp(void)
> > +{
> > +	u8 temp;
> > +	/* read temperature */
> > +	if (!ec_read(bios_settings[bios_version].tempreg, &temp)) {
> > +		if (verbose)
>
> you need to check the error status here before printing the temperature
> since it might be invalid if the ec_read has failed:
>
> 	u8 temp;
> 	int err;
>
> 	err = ec_read(bios_settings[bios_version].tempreg, &temp);
>
> 	if (err)
> 		return ACERHDF_ERROR;
>
> 	if (verbose)
> 		acerhdf_notice("temp %d\n", temp);
>
> 	return temp;
> }

The printf was already omitted when ec_read fails the way I wrote it, wasn't 
it? - Only if ec_read returns 0, the printf is launched and the temperature 
is returned.

> > +			acerhdf_notice("temp %d\n", temp);
> > +		return temp;
> > +	}
> > +	return ACERHDF_ERROR;
> > +}
> > +
> > +
> > +		if (verbose)
> > +			acerhdf_error("read state: %d expected state: %d\n",
> > +					old_state, fanstate);
> > +
> > +		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> > +		disable_kernelmode = 1;
> > +	}
> > +
> > +	if (state == 0) {
> > +		/* turn fan off only if below fanoff temperature */
> > +		if ((old_state == ACERHDF_FAN_AUTO) &&
> > +				(acerhdf_get_temp() < fanoff))
>
> it might be cool to tell the user why you're not turning off the fan.
>
> 		if (verbose)
> 			acerhdf_notice("Not turning off fan due to current temp "
> 				       "exceeding fanoff value\n");

Hm.. I think it should be clear that the fan is turned off, as soon as the 
temperature is below the fanoff temperature. In my opinion printing this 
message would be a case for a "verbose==2" verbose mode :)

kind regards,
--peter
--
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/