| Message ID | 20211116003844.2133683-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] power: supply: core: Use library interpolation | expand |
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
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.
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);
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(-)