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 <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andreas Mohr <[email protected]>
---
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
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
Hi,
On Mon, Oct 19, 2009 at 10:21 AM, Peter Feuerer <[email protected]> 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
Hi Boris,
Borislav Petkov writes:
> Hi,
>
> On Mon, Oct 19, 2009 at 10:21 AM, Peter Feuerer <[email protected]> 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
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
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
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.
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
>> 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 <[email protected]>
--
Regards/Gruss,
Boris