Hi Jerome, Jonathan,
On Fri, 26 Mar 2010 17:30:13 -0400, Jerome Oufella wrote:
> I discovered two issues.
> First the previous sht15_calc_temp() loop did not iterate through the
> temppoints array since the (data->supply_uV > temppoints[i - 1].vdd)
> test is always true in this direction.
>
> Also the two-points linear interpolation function was returning biased
> values which I adressed using a different form of interpolation.
>
> Signed-off-by: Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> drivers/hwmon/sht15.c | 14 +++++++-------
> 1 files changed, 7 insertions(+), 7 deletions(-)
>
>
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 864a371..a6ad93b 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -303,15 +303,15 @@ error_ret:
> static inline int sht15_calc_temp(struct sht15_data *data)
> {
> int d1 = 0;
> - int i;
> + int i, t;
>
> - for (i = 1; i < ARRAY_SIZE(temppoints); i++)
> + for (i = ARRAY_SIZE(temppoints) - 1; i > 0; i--)
> /* Find pointer to interpolate */
> - if (data->supply_uV > temppoints[i - 1].vdd) {
> - d1 = (data->supply_uV/1000 - temppoints[i - 1].vdd)
> - * (temppoints[i].d1 - temppoints[i - 1].d1)
> - / (temppoints[i].vdd - temppoints[i - 1].vdd)
> - + temppoints[i - 1].d1;
> + if (data->supply_uV >= temppoints[i - 1].vdd) {
> + t = (data->supply_uV - temppoints[i-1].vdd) /
> + ((temppoints[i].vdd - temppoints[i-1].vdd) / 10000);
> +
> + d1 = (temppoints[i].d1 * t + (10000 - t) * temppoints[i-1].d1) / 10000;
> break;
> }
>
May I suggest the more simple fix below?
---
drivers/hwmon/sht15.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- linux-2.6.34-rc3.orig/drivers/hwmon/sht15.c 2010-04-01 13:41:15.000000000 +0200
+++ linux-2.6.34-rc3/drivers/hwmon/sht15.c 2010-04-01 13:41:55.000000000 +0200
@@ -305,10 +305,10 @@ static inline int sht15_calc_temp(struct
int d1 = 0;
int i;
- for (i = 1; i < ARRAY_SIZE(temppoints); i++)
+ for (i = ARRAY_SIZE(temppoints) - 1; i > 0 ;i--)
/* Find pointer to interpolate */
if (data->supply_uV > temppoints[i - 1].vdd) {
- d1 = (data->supply_uV/1000 - temppoints[i - 1].vdd)
+ d1 = (data->supply_uV - temppoints[i - 1].vdd)
* (temppoints[i].d1 - temppoints[i - 1].d1)
/ (temppoints[i].vdd - temppoints[i - 1].vdd)
+ temppoints[i - 1].d1;
It leads to the same numbers as with Jerome's patch, with the
advantages that 1* it is a much smaller change, more suitable for
applying to stable kernels and 2* it avoids the magic constant number
10000.
The "/1000" seems to be a relict of former times when temppoints[*].vdd
was probably expressed in millivolt instead of microvolt. And the
inverted loop iteration is obviously a bug.
Note that in both cases, something should be done about values which
are outside of the temppoints array. I don't know how likely these are,
but they are seriously mishandled. For supply_uV values below
temppoints[0].vdd, d1 defaults to 0, so no adjustment is done at all.
temppoints[0].d1 would seem to be a much better default, if we don't
want to do any interpolation in that case. For supply_uV values above
temppoints[4].vdd, we do interpolate, which seems reasonable.
Opinions?
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
