[PATCH v2] power: supply: core: Use library interpolation

[PATCH v2] power: supply: core: Use library interpolation

[Linus Walleij]

The power supply core appears to contain two open coded
linear interpolations. Use the kernel fixpoint arithmetic
interpolation library function instead.

Cc: Chunyan Zhang <chunyan.zhang@unisoc.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Break the table loop at table_len - 1 so we don't index
  past the end of the table. (Thanks Baolin!)

Chunyan: The sc27xx fuel gauge seems to be the only driver
using this, so it'd be great if you could test this to make
sure it works as intended.
---
 drivers/power/supply/power_supply_core.c | 59 ++++++++++++------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index fc12a4f407f4..2983466a4914 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -21,6 +21,7 @@
 #include <linux/power_supply.h>
 #include <linux/property.h>
 #include <linux/thermal.h>
+#include <linux/fixp-arith.h>
 #include "power_supply.h"
 
 /* exported for the APM Power driver, APM emulation */
@@ -783,26 +784,25 @@ EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
 int power_supply_temp2resist_simple(struct power_supply_resistance_temp_table *table,
 				    int table_len, int temp)
 {
-	int i, resist;
+	int i, high, low;
 
-	for (i = 0; i < table_len; i++)
+	/* Break loop at table_len - 1 because that is the highest index */
+	for (i = 0; i < table_len - 1; i++)
 		if (temp > table[i].temp)
 			break;
 
-	if (i > 0 && i < table_len) {
-		int tmp;
-
-		tmp = (table[i - 1].resistance - table[i].resistance) *
-			(temp - table[i].temp);
-		tmp /= table[i - 1].temp - table[i].temp;
-		resist = tmp + table[i].resistance;
-	} else if (i == 0) {
-		resist = table[0].resistance;
-	} else {
-		resist = table[table_len - 1].resistance;
-	}
-
-	return resist;
+	/* The library function will deal with high == low */
+	if (i > 0)
+		high = i - 1;
+	else
+		high = i; /* i.e. i == 0 */
+	low = i;
+
+	return fixp_linear_interpolate(table[low].temp,
+				       table[low].resistance,
+				       table[high].temp,
+				       table[high].resistance,
+				       temp);
 }
 EXPORT_SYMBOL_GPL(power_supply_temp2resist_simple);
 
@@ -821,24 +821,25 @@ EXPORT_SYMBOL_GPL(power_supply_temp2resist_simple);
 int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
 				int table_len, int ocv)
 {
-	int i, cap, tmp;
+	int i, high, low;
 
+	/* Break loop at table_len - 1 because that is the highest index */
 	for (i = 0; i < table_len; i++)
 		if (ocv > table[i].ocv)
 			break;
 
-	if (i > 0 && i < table_len) {
-		tmp = (table[i - 1].capacity - table[i].capacity) *
-			(ocv - table[i].ocv);
-		tmp /= table[i - 1].ocv - table[i].ocv;
-		cap = tmp + table[i].capacity;
-	} else if (i == 0) {
-		cap = table[0].capacity;
-	} else {
-		cap = table[table_len - 1].capacity;
-	}
-
-	return cap;
+	/* The library function will deal with high == low */
+	if (i > 0)
+		high = i - 1;
+	else
+		high = i; /* i.e. i == 0 */
+	low = i;
+
+	return fixp_linear_interpolate(table[low].ocv,
+				       table[low].capacity,
+				       table[high].ocv,
+				       table[high].capacity,
+				       ocv);
 }
 EXPORT_SYMBOL_GPL(power_supply_ocv2cap_simple);
 
-- 
2.31.1

Re: [PATCH v2] power: supply: core: Use library interpolation

[Baolin Wang]

Hi Linus,

On 2021/11/16 8:38, Linus Walleij wrote:
> The power supply core appears to contain two open coded
> linear interpolations. Use the kernel fixpoint arithmetic
> interpolation library function instead.
> 
> Cc: Chunyan Zhang <chunyan.zhang@unisoc.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Break the table loop at table_len - 1 so we don't index
>    past the end of the table. (Thanks Baolin!)
> 
> Chunyan: The sc27xx fuel gauge seems to be the only driver
> using this, so it'd be great if you could test this to make
> sure it works as intended.
> ---
>   drivers/power/supply/power_supply_core.c | 59 ++++++++++++------------
>   1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index fc12a4f407f4..2983466a4914 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -21,6 +21,7 @@
>   #include <linux/power_supply.h>
>   #include <linux/property.h>
>   #include <linux/thermal.h>
> +#include <linux/fixp-arith.h>
>   #include "power_supply.h"
>   
>   /* exported for the APM Power driver, APM emulation */
> @@ -783,26 +784,25 @@ EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
>   int power_supply_temp2resist_simple(struct power_supply_resistance_temp_table *table,
>   				    int table_len, int temp)
>   {
> -	int i, resist;
> +	int i, high, low;
>   
> -	for (i = 0; i < table_len; i++)
> +	/* Break loop at table_len - 1 because that is the highest index */
> +	for (i = 0; i < table_len - 1; i++)
>   		if (temp > table[i].temp)
>   			break;
>   
> -	if (i > 0 && i < table_len) {
> -		int tmp;
> -
> -		tmp = (table[i - 1].resistance - table[i].resistance) *
> -			(temp - table[i].temp);
> -		tmp /= table[i - 1].temp - table[i].temp;
> -		resist = tmp + table[i].resistance;
> -	} else if (i == 0) {
> -		resist = table[0].resistance;
> -	} else {
> -		resist = table[table_len - 1].resistance;
> -	}
> -
> -	return resist;
> +	/* The library function will deal with high == low */
> +	if (i > 0)
> +		high = i - 1;
> +	else
> +		high = i; /* i.e. i == 0 */
> +	low = i;
> +
> +	return fixp_linear_interpolate(table[low].temp,
> +				       table[low].resistance,
> +				       table[high].temp,
> +				       table[high].resistance,
> +				       temp);
Thanks for your patch, and overall looks good to me. But I still think 
we should not do interpolation if the temperature is larger than the 
maximum value of the table, we can just return the maximum value of the 
table instead. Something like below untested code, how do you think?

--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -783,26 +783,27 @@ void power_supply_put_battery_info(struct 
power_supply *psy,
  int power_supply_temp2resist_simple(struct 
power_supply_resistance_temp_table *table,
                                     int table_len, int temp)
  {
-       int i, resist;
+       int i, high, low;

         for (i = 0; i < table_len; i++)
                 if (temp > table[i].temp)
                         break;

-       if (i > 0 && i < table_len) {
-               int tmp;
-
-               tmp = (table[i - 1].resistance - table[i].resistance) *
-                       (temp - table[i].temp);
-               tmp /= table[i - 1].temp - table[i].temp;
-               resist = tmp + table[i].resistance;
-       } else if (i == 0) {
-               resist = table[0].resistance;
-       } else {
-               resist = table[table_len - 1].resistance;
-       }
-
-       return resist;
+       if (i == table_len)
+               return table[table_len - 1].resistance;
+
+       /* The library function will deal with high == low */
+       if (i > 0)
+               high = i - 1;
+       else
+               high = i; /* i.e. i == 0 */
+       low = i;
+
+       return fixp_linear_interpolate(table[low].temp,
+                                      table[low].resistance,
+                                      table[high].temp,
+                                      table[high].resistance,
+                                      temp);
  }

>   }
>   EXPORT_SYMBOL_GPL(power_supply_temp2resist_simple);
>   
> @@ -821,24 +821,25 @@ EXPORT_SYMBOL_GPL(power_supply_temp2resist_simple);
>   int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
>   				int table_len, int ocv)
>   {
> -	int i, cap, tmp;
> +	int i, high, low;
>   
> +	/* Break loop at table_len - 1 because that is the highest index */
>   	for (i = 0; i < table_len; i++)
>   		if (ocv > table[i].ocv)
>   			break;
>   
> -	if (i > 0 && i < table_len) {
> -		tmp = (table[i - 1].capacity - table[i].capacity) *
> -			(ocv - table[i].ocv);
> -		tmp /= table[i - 1].ocv - table[i].ocv;
> -		cap = tmp + table[i].capacity;
> -	} else if (i == 0) {
> -		cap = table[0].capacity;
> -	} else {
> -		cap = table[table_len - 1].capacity;
> -	}
> -
> -	return cap;
> +	/* The library function will deal with high == low */
> +	if (i > 0)
> +		high = i - 1;
> +	else
> +		high = i; /* i.e. i == 0 */
> +	low = i;
> +
> +	return fixp_linear_interpolate(table[low].ocv,
> +				       table[low].capacity,
> +				       table[high].ocv,
> +				       table[high].capacity,
> +				       ocv);
>   }
>   EXPORT_SYMBOL_GPL(power_supply_ocv2cap_simple);
>   
> 

Re: [PATCH v2] power: supply: core: Use library interpolation

[Linus Walleij]

On Tue, Nov 16, 2021 at 3:28 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:

> Thanks for your patch, and overall looks good to me. But I still think
> we should not do interpolation if the temperature is larger than the
> maximum value of the table, we can just return the maximum value of the
> table instead. Something like below untested code, how do you think?

You are right, but if I understand correctly
fixp_linear_interpolate() already does what you want,
perhaps a bit unintuitively. See include/linux/fixp-arith.h:

static inline int fixp_linear_interpolate(int x0, int y0, int x1, int y1, int x)
{
        if (y0 == y1 || x == x0)
                return y0;
        if (x1 == x0 || x == x1)
                return y1;

        return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
}

Yours,
Linus Walleij

Re: [PATCH v2] power: supply: core: Use library interpolation

[Baolin Wang]

On 2021/11/16 21:34, Linus Walleij wrote:
> On Tue, Nov 16, 2021 at 3:28 AM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
> 
>> Thanks for your patch, and overall looks good to me. But I still think
>> we should not do interpolation if the temperature is larger than the
>> maximum value of the table, we can just return the maximum value of the
>> table instead. Something like below untested code, how do you think?
> 
> You are right, but if I understand correctly
> fixp_linear_interpolate() already does what you want,
> perhaps a bit unintuitively. See include/linux/fixp-arith.h:
> 
> static inline int fixp_linear_interpolate(int x0, int y0, int x1, int y1, int x)
> {
>          if (y0 == y1 || x == x0)
>                  return y0;
>          if (x1 == x0 || x == x1)
>                  return y1;
> 
>          return y0 + ((y1 - y0) * (x - x0) / (x1 - x0));
> }
> 

Sorry for confusing, let me try to make it clear. Suppose we have a 
temperature table as below, and try to get the resistance percent in the 
temp=-20 Celsius.

resistance-temp-table = <20 100>, <10 90>, <0 80>, <(-10) 60>;

With your patch, we will get i=table_len-1, which is 3. Then high=2 and 
low=3.

+	for (i = 0; i < table_len - 1; i++)
  		if (temp > table[i].temp)
  			break;

So in fixp_linear_interpolate(): x0=-10, y0=60, x1=0, y1=80, x=-20, then 
will return 60 + (80-60)*(-20-(-10))/(0-(-10)) = 40.

But actually the -20 Celsius is less than (-10), which is the last 
member in the array, we do not need the interpolation, return 60 
directly if I understand correctly. Which means for any other lower 
temperature points, the resistance of the baterry is always 60% of the 
battery internal resistence in normal temperature.