[lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jerome Oufella]

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@savoirfairelinux.com>
---

 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;
 		}
 
-- 
1.6.3.3


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jerome Oufella]

Hi,

My conclusion is that the measurements are biased everytime.

I wrote a small test suite testing measurements at the manufacturer-supplied reference points.
As you can see the bias is substantial.

Jonathan, any objection ?



==> Test #1
  Supply: 2500000uV, raw reading: 7000, 
  original calc_temp: 31599mC;
  patched calc_temp: 30600mC
  hand-made calc gives: 30600

==> Test #2
  Supply: 3000000uV, raw reading: 6800, 
  original calc_temp: 29598mC;
  patched calc_temp: 28400mC
  hand-made calc gives: 28400

==> Test #3
  Supply: 3500000uV, raw reading: 6400, 
  original calc_temp: 25598mC;
  patched calc_temp: 24300mC
  hand-made calc gives: 24300

==> Test #4
  Supply: 4000000uV, raw reading: 6000, 
  original calc_temp: 21598mC;
  patched calc_temp: 20200mC
  hand-made calc gives: 20200

==> Test #5
  Supply: 5000000uV, raw reading: 5400, 
  original calc_temp: 15598mC;
  patched calc_temp: 13900mC
  hand-made calc gives: 13900

Jerome

> 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 at
> savoirfairelinux.com>
> ---
> 
>  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;
>  		}
>  
> -- 
> 1.6.3.3

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jonathan Cameron]

On 03/29/10 17:23, Jerome Oufella wrote:
> Hi,
> 
> My conclusion is that the measurements are biased everytime.
> 
> I wrote a small test suite testing measurements at the manufacturer-supplied reference points.
> As you can see the bias is substantial.
> 
> Jonathan, any objection ?

If it works it is fine with me, but please add some documentation so that any
one coming to the code in the future can understand why you aren't using
what the datasheet lists.

Given it would be useful to provide everything you have listed here, I'd
be inclined to add a file in Documentation/hwmon supported by a comment inline
with the code.  If you have any details on date of manufacture of the part you
tested etc or any comments from the manufacturer it would be useful to put
those in as well.

Jonathan

> 
> 
> 
> ==> Test #1
>   Supply: 2500000uV, raw reading: 7000, 
>   original calc_temp: 31599mC;
>   patched calc_temp: 30600mC
>   hand-made calc gives: 30600
> 
> ==> Test #2
>   Supply: 3000000uV, raw reading: 6800, 
>   original calc_temp: 29598mC;
>   patched calc_temp: 28400mC
>   hand-made calc gives: 28400
> 
> ==> Test #3
>   Supply: 3500000uV, raw reading: 6400, 
>   original calc_temp: 25598mC;
>   patched calc_temp: 24300mC
>   hand-made calc gives: 24300
> 
> ==> Test #4
>   Supply: 4000000uV, raw reading: 6000, 
>   original calc_temp: 21598mC;
>   patched calc_temp: 20200mC
>   hand-made calc gives: 20200
> 
> ==> Test #5
>   Supply: 5000000uV, raw reading: 5400, 
>   original calc_temp: 15598mC;
>   patched calc_temp: 13900mC
>   hand-made calc gives: 13900
> 
> Jerome
> 
>> 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 at
>> savoirfairelinux.com>
>> ---
>>
>>  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;
>>  		}
>>  
>> -- 
>> 1.6.3.3
> 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jerome Oufella]

----- "Jonathan Cameron" <jic23@cam.ac.uk> wrote :
> If it works it is fine with me, but please add some documentation so
> that any
> one coming to the code in the future can understand why you aren't
> using
> what the datasheet lists.
> 
> Given it would be useful to provide everything you have listed here,
> I'd
> be inclined to add a file in Documentation/hwmon supported by a
> comment inline
> with the code.  If you have any details on date of manufacture of the
> part you
> tested etc or any comments from the manufacturer it would be useful to
> put
> those in as well.
> 
> Jonathan

Hi Jonathan,

In fact, the new function is doing what the datasheet says,
the function used to interpolate the corresponding value between two
of those reference points being left to the programmer's discretion.

In the current version of sht15_calc_temp(), the interpolation that was
done was being broken because of fixed point divisions that messed
the things up. In fact, the temperatures returned so far were wrong.

So after different tries on trying to improve the current function's 
precision, I ended up getting better results with a different approach
with a proportional factor based linear interpolation.

The manufacturer data remains valid, unchanged, and respected.

Jerome




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jonathan Cameron]

On 03/29/10 20:58, Jerome Oufella wrote:
> ----- "Jonathan Cameron" <jic23@cam.ac.uk> wrote :
>> If it works it is fine with me, but please add some documentation so
>> that any
>> one coming to the code in the future can understand why you aren't
>> using
>> what the datasheet lists.
>>
>> Given it would be useful to provide everything you have listed here,
>> I'd
>> be inclined to add a file in Documentation/hwmon supported by a
>> comment inline
>> with the code.  If you have any details on date of manufacture of the
>> part you
>> tested etc or any comments from the manufacturer it would be useful to
>> put
>> those in as well.
>>
>> Jonathan
> 
> Hi Jonathan,
> 
> In fact, the new function is doing what the datasheet says,
> the function used to interpolate the corresponding value between two
> of those reference points being left to the programmer's discretion.
> 
> In the current version of sht15_calc_temp(), the interpolation that was
> done was being broken because of fixed point divisions that messed
> the things up. In fact, the temperatures returned so far were wrong.
> 
> So after different tries on trying to improve the current function's 
> precision, I ended up getting better results with a different approach
> with a proportional factor based linear interpolation.
> 
> The manufacturer data remains valid, unchanged, and respected.
> 
> Jerome
Fair enough.  Then it is good as is, feel free to add

Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

to the patch.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jerome Oufella]

----- "Jonathan Cameron" <jic23@cam.ac.uk> a écrit :
> Fair enough.  Then it is good as is, feel free to add
> 
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> to the patch.

All right, thanks Jonathan.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jerome Oufella]

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, making the temperature reading inaccurate by a non neglectable
factor. This is adressed using a a proportional factor based linear
interpolation.

Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
Cc: Jonathan Cameron <jic23@cam.ac.uk>
Cc: Jean Delvare <khali@linux-fr.org>
---

 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;
 		}
 
-- 
1.6.3.3


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp interpolation function

[Jean Delvare]

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@savoirfairelinux.com>
> ---
> 
>  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

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jean Delvare]

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@savoirfairelinux.com>
> ---
> 
>  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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp interpolation function

[Jerome Oufella]

----- "Jean Delvare" <khali@linux-fr.org> wrote :
> 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

That's fine, it does a good job for me, in the expected voltage range.

Jerome Oufella

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jerome Oufella]

----- "Jean Delvare" <khali@linux-fr.org> wrote :
> 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

That's fine, it does a good job for me, in the expected voltage range.

Jerome Oufella

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp interpolation function

[Jonathan Cameron]

On 04/01/10 13:54, Jerome Oufella wrote:
> ----- "Jean Delvare" <khali@linux-fr.org> wrote :
>> 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
> 
> That's fine, it does a good job for me, in the expected voltage range.

Seems sensible.  I'm not quite sure but I think the code in question predates
my involvement with the driver, so I'm guessing I never actually looked
closely enough at it.

Jonathan

Re: [lm-sensors] [PATCH] hwmon: sht15: Fix sht15_calc_temp

[Jonathan Cameron]

On 04/01/10 13:54, Jerome Oufella wrote:
> ----- "Jean Delvare" <khali@linux-fr.org> wrote :
>> 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
> 
> That's fine, it does a good job for me, in the expected voltage range.

Seems sensible.  I'm not quite sure but I think the code in question predates
my involvement with the driver, so I'm guessing I never actually looked
closely enough at it.

Jonathan

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] [PATCH] hwmon: (sht15) Fix sht15_calc_temp

[Jean Delvare]

From: Jerome Oufella <jerome.oufella@savoirfairelinux.com>

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 due to a stray division by 1000 which shouldn't be there.

[JD: Also change the default value for d1 from 0 to something saner.]

Signed-off-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: stable@kernel.org
---
 drivers/hwmon/sht15.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-2.6.34-rc3.orig/drivers/hwmon/sht15.c	2010-04-01 16:24:33.000000000 +0200
+++ linux-2.6.34-rc3/drivers/hwmon/sht15.c	2010-04-01 16:49:45.000000000 +0200
@@ -302,13 +302,13 @@ error_ret:
  **/
 static inline int sht15_calc_temp(struct sht15_data *data)
 {
-	int d1 = 0;
+	int d1 = temppoints[0].d1;
 	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;


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors