Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753549AbZFAOTo (ORCPT ); Mon, 1 Jun 2009 10:19:44 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751279AbZFAOTi (ORCPT ); Mon, 1 Jun 2009 10:19:38 -0400 Received: from smtprelay04.ispgateway.de ([80.67.31.38]:59215 "EHLO smtprelay04.ispgateway.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750965AbZFAOTh (ORCPT ); Mon, 1 Jun 2009 10:19:37 -0400 References: <1240913055.29860.14.camel@maxim-laptop> <1240950704.3781.12.camel@maxim-laptop> <20090503184617.GA3555@liondog.tnic> <20090509171432.GA31126@liondog.tnic> <20090512060231.GB26286@liondog.tnic> <20090524192219.GA30285@liondog.tnic> Message-ID: X-Mailer: http://www.courier-mta.org/cone/ From: Peter Feuerer To: Borislav Petkov Cc: LKML , lenb@kernel.org, Matthew Garrett , Maxim Levitsky Subject: Re: [PATCH] Acer Aspire One Fan Control Date: Mon, 01 Jun 2009 16:12:21 +0200 Mime-Version: 1.0 Content-Type: text/plain; format=flowed; charset="US-ASCII" Content-Disposition: inline Content-Transfer-Encoding: 7bit X-Df-Sender: 404094 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2151 Lines: 79 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/