[Patch] acerhdf: Return temperature in milidegree

[Patch] acerhdf: Return temperature in milidegree

[Peter Feuerer]

Hi Boris,

what do you think about this patch?

regards,
--peter

--

Return temperature in milidegree instead of degree, as userspace
applications expect the temperature in milidegree.

Signed-off-by: Peter Feuerer <peter@piie.net>
Cc: Borislav Petkov <petkovbb@gmail.com>
Cc: Andreas Mohr <andi@lisas.de>

---
 drivers/platform/x86/acerhdf.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index b713c6e..a588f96 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -52,7 +52,7 @@
  */
 #undef START_IN_KERNEL_MODE
 
-#define DRV_VER "0.5.17"
+#define DRV_VER "0.5.18"
 
 /*
  * According to the Atom N270 datasheet,
@@ -61,7 +61,7 @@
  * measured by the on-die thermal monitor are within 0 <= Tj <= 90. So,
  * assume 89°C is critical temperature.
  */
-#define ACERHDF_TEMP_CRIT 89
+#define ACERHDF_TEMP_CRIT 89000
 #define ACERHDF_FAN_OFF 0
 #define ACERHDF_FAN_AUTO 1
 
@@ -69,7 +69,7 @@
  * No matter what value the user puts into the fanon variable, turn on the fan
  * at 80 degree Celsius to prevent hardware damage
  */
-#define ACERHDF_MAX_FANON 80
+#define ACERHDF_MAX_FANON 80000
 
 /*
  * Maximum interval between two temperature checks is 15 seconds, as the die
@@ -85,8 +85,8 @@ static int kernelmode;
 #endif
 
 static unsigned int interval = 10;
-static unsigned int fanon = 63;
-static unsigned int fanoff = 58;
+static unsigned int fanon = 63000;
+static unsigned int fanoff = 58000;
 static unsigned int verbose;
 static unsigned int fanstate = ACERHDF_FAN_AUTO;
 static char force_bios[16];
@@ -171,7 +171,7 @@ static int acerhdf_get_temp(int *temp)
 	if (ec_read(bios_cfg->tempreg, &read_temp))
 		return -EINVAL;
 
-	*temp = read_temp;
+	*temp = read_temp * 1000;
 
 	return 0;
 }
-- 
1.6.4.4

Re: [Patch] acerhdf: Return temperature in milidegree

[Andreas Mohr]

Hi,

On Mon, Oct 19, 2009 at 10:21:16AM +0200, Peter Feuerer wrote:
> 
> Hi Boris,
> 
> what do you think about this patch?

Personally I'm hurting a bit due to the open-coded "* 1000" transition
in all places.

I'd add a helper macro
#define TEMP_DEGREE_TO_SYS(x) ((x) * 1000)
and use that in all places where it matters.

Advantage:
- either no mistyping (10000 instead of 1000) _or_ bug occurring in _all_
  places where this macro is used
- easily grepped-for
- easily changed once the system granularity gets updated

Andreas Mohr

Re: [Patch] acerhdf: Return temperature in milidegree

[Borislav Petkov]

Hi,

On Mon, Oct 19, 2009 at 10:21 AM, Peter Feuerer <peter@piie.net> wrote:
> what do you think about this patch?

I don't think it is worth the trouble. Just compare the changes your
patch introduces with a simple

 temp * 1000

you can do in userspace.

If this were a standard interface then maybe the changes could be
warranted but it doesn't seem like so...

-- 
Regards/Gruss,
Boris

Re: [Patch] acerhdf: Return temperature in milidegree

[Peter Feuerer]

Hi Boris,

Borislav Petkov writes:

> Hi,
> 
> On Mon, Oct 19, 2009 at 10:21 AM, Peter Feuerer <peter@piie.net> wrote:
>> what do you think about this patch?
> 
> I don't think it is worth the trouble. Just compare the changes your
> patch introduces with a simple
> 
>  temp * 1000
> 
> you can do in userspace.
> 
> If this were a standard interface then maybe the changes could be
> warranted but it doesn't seem like so...
> 

I just had a look at the documentation concerning this topic: 

Documentation/thermal/sysfs-api.txt:162

temp           Current temperature as reported by thermal zone (sensor)
            Unit: millidegree Celsius
            RO
            Required


It is expected to be in millidegree, so I think we should discuss, modify 
and apply the patch.

regards,
--peter

Re: [Patch] acerhdf: Return temperature in milidegree

[Peter Feuerer]

Hi Andreas,

thank you very much for your brainstorming.

Andreas Mohr writes:

> Hi,
> 
> On Mon, Oct 19, 2009 at 10:21:16AM +0200, Peter Feuerer wrote:
>> 
>> Hi Boris,
>> 
>> what do you think about this patch?
> 
> Personally I'm hurting a bit due to the open-coded "* 1000" transition
> in all places.
> 
> I'd add a helper macro
> #define TEMP_DEGREE_TO_SYS(x) ((x) * 1000)
> and use that in all places where it matters.
> 
> Advantage:
> - either no mistyping (10000 instead of 1000) _or_ bug occurring in _all_
>   places where this macro is used
> - easily grepped-for
> - easily changed once the system granularity gets updated

I agree with you in these points, but I have also some disadvantages to discuss:

Disadvantages:
 - Thinking about the implemenation such a macro would require, users may 
   get confused. They would still set the fanon / fanoff trip points in 
   degree, but when they read documentation or the current temperature, 
   millidegree is used.  
 - I think "TEMP_DEGREE_TO_SYS(59)" in code is not 
   as good readable as "59000"

what about writing something like "59 * 1000" insead of "59000"?

Or something like that:

#define FACTOR_MILLIDEGREE 1000
59 * FACTOR_MILLIDEGREE 

this solution has all your listed advantages and eliminates the 
disadvantages I see in the "TEMP_DEGREE_TO_SYS" solution.

--peter

Re: [Patch] acerhdf: Return temperature in milidegree

[Andreas Mohr]

Hi,

On Mon, Oct 19, 2009 at 11:24:47PM +0200, Peter Feuerer wrote:
> I agree with you in these points, but I have also some disadvantages to discuss:
>
> Disadvantages:
> - Thinking about the implemenation such a macro would require, users may  
>   get confused. They would still set the fanon / fanoff trip points in   
> degree, but when they read documentation or the current temperature,   
> millidegree is used.  - I think "TEMP_DEGREE_TO_SYS(59)" in code is not   
> as good readable as "59000"
>
> what about writing something like "59 * 1000" insead of "59000"?
>
> Or something like that:
>
> #define FACTOR_MILLIDEGREE 1000
> 59 * FACTOR_MILLIDEGREE 
>
> this solution has all your listed advantages and eliminates the  
> disadvantages I see in the "TEMP_DEGREE_TO_SYS" solution.

Well, yes, much better, in fact TEMP_DEGREE_TO_SYS is simple overengineering
(using a generic, _cryptic_ name in order to keep using it in the somewhat unlikely
case of having the factor change when you could just as well have mass-renamed
the couple places that use the macro name)

Andreas

Re: [Patch] acerhdf: Return temperature in milidegree

[Borislav Petkov]

On Mon, Oct 19, 2009 at 11:11:07PM +0200, Peter Feuerer wrote:
> I just had a look at the documentation concerning this topic:
> 
> Documentation/thermal/sysfs-api.txt:162
> 
> temp           Current temperature as reported by thermal zone (sensor)
>            Unit: millidegree Celsius
>            RO
>            Required
> 
> 
> It is expected to be in millidegree, so I think we should discuss,
> modify and apply the patch.

Well well, looka here, it is an interface! I guess the temperature has
to be in millidegrees for better granularity or similar. Now userspace
has to do temp / 1000.0 and so on.

In that case, the patch should be applied. I'll give it a run later.

Minor nitpick: Add the sysfs-api requirement to the commit message
instead of "userspace applications."

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [Patch] acerhdf: Return temperature in milidegree

[Peter Feuerer]

Hi Boris,

Borislav Petkov writes:

> On Mon, Oct 19, 2009 at 11:11:07PM +0200, Peter Feuerer wrote:
>> I just had a look at the documentation concerning this topic:
>> 
>> Documentation/thermal/sysfs-api.txt:162
>> 
>> temp           Current temperature as reported by thermal zone (sensor)
>>            Unit: millidegree Celsius
>>            RO
>>            Required
>> 
>> 
>> It is expected to be in millidegree, so I think we should discuss,
>> modify and apply the patch.
> 
> Well well, looka here, it is an interface! I guess the temperature has
> to be in millidegrees for better granularity or similar. Now userspace
> has to do temp / 1000.0 and so on.

This is what the userspace applications like thermal-plugin of xfce-panel 
do.

> 
> In that case, the patch should be applied. I'll give it a run later.

Did you already give it a try?

> 
> Minor nitpick: Add the sysfs-api requirement to the commit message
> instead of "userspace applications."

will change this, when submitting the patch to Andrew.

kind regards,
--peter

Re: [Patch] acerhdf: Return temperature in milidegree

[Borislav Petkov]

>> In that case, the patch should be applied. I'll give it a run later.
>
> Did you already give it a try?

Yep, sorry for the delay.

Tested-by: Borislav Petkov <petkovbb@gmail.com>

-- 
Regards/Gruss,
Boris