Messages in this thread | | | From | Peter Feuerer <> | Subject | Re: [PATCH] Acer Aspire One Fan Control | Date | Mon, 01 Jun 2009 16:12:21 +0200 |
| |
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
| |