[PATCH 0/2] Fixes for rk817_charger driver

[PATCH 0/2] Fixes for rk817_charger driver

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

After using the driver for a few months I noticed a few issues that
this patch series seeks to address. Namely, there appears to be some
left over code that was used for debugging during development that is
no longer needed. Additionally, when the state of charge drops to 0
there is an issue with reading the value on boot because the columb
counter appears to hold a signed value.

With these fixes in place the battery driver appears to operate better
and without the odd bug of an unsigned integer overflow on the columb
counter.

Chris Morgan (2):
  power: supply: Remove unneeded code in rk817_charger
  power: supply: Fix low SOC bugs in rk817 driver

 drivers/power/supply/rk817_charger.c | 48 ++++++++++++++++------------
 1 file changed, 28 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/2] power: supply: Remove unneeded code in rk817_charger

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Some code was left over from debugging the driver while it was in
development. Please remove this code as it's not needed.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/power/supply/rk817_charger.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
index 36f807b5ec44..eba9a17d991b 100644
--- a/drivers/power/supply/rk817_charger.c
+++ b/drivers/power/supply/rk817_charger.c
@@ -816,19 +816,6 @@ rk817_read_or_set_full_charge_on_boot(struct rk817_charger *charger,
 		}
 	}
 
-	regmap_bulk_read(rk808->regmap, RK817_GAS_GAUGE_PWRON_VOL_H,
-			 bulk_reg, 2);
-	tmp = get_unaligned_be16(bulk_reg);
-	boot_voltage = (charger->voltage_k * tmp) + 1000 * charger->voltage_b;
-	regmap_bulk_read(rk808->regmap, RK817_GAS_GAUGE_Q_PRES_H3,
-			 bulk_reg, 4);
-	tmp = get_unaligned_be32(bulk_reg);
-	boot_charge_mah = ADC_TO_CHARGE_UAH(tmp, charger->res_div) / 1000;
-	regmap_bulk_read(rk808->regmap, RK817_GAS_GAUGE_OCV_VOL_H,
-			 bulk_reg, 2);
-	tmp = get_unaligned_be16(bulk_reg);
-	boot_voltage = (charger->voltage_k * tmp) + 1000 * charger->voltage_b;
-
 	/*
 	 * Now we have our full charge capacity and soc, init the columb
 	 * counter.
-- 
2.34.1

[PATCH 2/2] power: supply: Fix low SOC bugs in rk817 driver

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

When the SOC approaches zero, an integer overflows in the columb
counter causing the driver to react poorly. This makes the driver
think it's at (above) the fully charged capacity when in fact it's
zero. It would then write this full capacity to NVRAM which would be
used on boot if the device remained off for less than 5 hours and
not plugged in.

This can be fixed and guarded against by doing the following:
 - Changing the type of tmp in rk817_read_or_set_full_charge_on_boot()
   to be an int instead of a u32. That way we can account for negative
   numbers.
 - Guard against negative values for the full charge on boot by setting
   the charge to 0 if the system charge reports less than 0.
 - Catch scenarios where the battery voltage is below the design
   minimum voltage and set the system SOC to 0 at that time and update
   the columb counter with a charge level of 0.
 - Change the off time value from 5 hours to 30 minutes before we
   recalculate the current capacity based on the OCV tables.

These changes allow the driver to operate better at low voltage/low
capacity conditions.

Fixes: 3268a4d9b0b8 ("power: supply: rk817: Fix unsigned comparison with less than zero")
Fixes: 11cb8da0189b ("power: supply: Add charger driver for Rockchip RK817")
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/power/supply/rk817_charger.c | 35 ++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
index eba9a17d991b..8c977d5987f3 100644
--- a/drivers/power/supply/rk817_charger.c
+++ b/drivers/power/supply/rk817_charger.c
@@ -335,6 +335,22 @@ static int rk817_bat_calib_cap(struct rk817_charger *charger)
 			charger->fcc_mah * 1000);
 	}
 
+	/*
+	 * Set the SOC to 0 if we are below the minimum system voltage.
+	 */
+	if (volt_avg <= charger->bat_voltage_min_design_uv) {
+		charger->soc = 0;
+		charge_now_adc = CHARGE_TO_ADC(0,
+					       charger->res_div);
+		put_unaligned_be32(charge_now_adc, bulk_reg);
+		regmap_bulk_write(rk808->regmap,
+				  RK817_GAS_GAUGE_Q_INIT_H3,
+				  bulk_reg, 4);
+		dev_warn(charger->dev,
+			 "Battery voltage %d below minimum voltage %d\n",
+			 volt_avg, charger->bat_voltage_min_design_uv);
+		}
+
 	rk817_record_battery_nvram_values(charger);
 
 	return 0;
@@ -710,9 +726,10 @@ static int rk817_read_battery_nvram_values(struct rk817_charger *charger)
 
 	/*
 	 * Read the nvram for state of charge. Sanity check for values greater
-	 * than 100 (10000). If the value is off it should get corrected
-	 * automatically when the voltage drops to the min (soc is 0) or when
-	 * the battery is full (soc is 100).
+	 * than 100 (10000) or less than 0, because other things (BSP kernels,
+	 * U-Boot, or even i2cset) can write to this register. If the value is
+	 * off it should get corrected automatically when the voltage drops to
+	 * the min (soc is 0) or when the battery is full (soc is 100).
 	 */
 	ret = regmap_bulk_read(charger->rk808->regmap,
 			       RK817_GAS_GAUGE_BAT_R1, bulk_reg, 3);
@@ -721,6 +738,8 @@ static int rk817_read_battery_nvram_values(struct rk817_charger *charger)
 	charger->soc = get_unaligned_le24(bulk_reg);
 	if (charger->soc > 10000)
 		charger->soc = 10000;
+	if (charger->soc < 0)
+		charger->soc = 0;
 
 	return 0;
 }
@@ -731,8 +750,8 @@ rk817_read_or_set_full_charge_on_boot(struct rk817_charger *charger,
 {
 	struct rk808 *rk808 = charger->rk808;
 	u8 bulk_reg[4];
-	u32 boot_voltage, boot_charge_mah, tmp;
-	int ret, reg, off_time;
+	u32 boot_voltage, boot_charge_mah;
+	int ret, reg, off_time, tmp;
 	bool first_boot;
 
 	/*
@@ -785,10 +804,12 @@ rk817_read_or_set_full_charge_on_boot(struct rk817_charger *charger,
 		regmap_bulk_read(rk808->regmap, RK817_GAS_GAUGE_Q_PRES_H3,
 				 bulk_reg, 4);
 		tmp = get_unaligned_be32(bulk_reg);
+		if (tmp < 0)
+			tmp = 0;
 		boot_charge_mah = ADC_TO_CHARGE_UAH(tmp,
 						    charger->res_div) / 1000;
 		/*
-		 * Check if the columb counter has been off for more than 300
+		 * Check if the columb counter has been off for more than 30
 		 * minutes as it tends to drift downward. If so, re-init soc
 		 * with the boot voltage instead. Note the unit values for the
 		 * OFF_CNT register appear to be in decaminutes and stops
@@ -799,7 +820,7 @@ rk817_read_or_set_full_charge_on_boot(struct rk817_charger *charger,
 		 * than 0 on a reboot anyway.
 		 */
 		regmap_read(rk808->regmap, RK817_GAS_GAUGE_OFF_CNT, &off_time);
-		if (off_time >= 30) {
+		if (off_time >= 3) {
 			regmap_bulk_read(rk808->regmap,
 					 RK817_GAS_GAUGE_PWRON_VOL_H,
 					 bulk_reg, 2);
-- 
2.34.1

Re: [PATCH 0/2] Fixes for rk817_charger driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

Hi,

On Fri, Apr 07, 2023 at 11:18:25AM -0500, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> After using the driver for a few months I noticed a few issues that
> this patch series seeks to address. Namely, there appears to be some
> left over code that was used for debugging during development that is
> no longer needed. Additionally, when the state of charge drops to 0
> there is an issue with reading the value on boot because the columb
> counter appears to hold a signed value.
> 
> With these fixes in place the battery driver appears to operate better
> and without the odd bug of an unsigned integer overflow on the columb
> counter.
> 
> Chris Morgan (2):
>   power: supply: Remove unneeded code in rk817_charger
>   power: supply: Fix low SOC bugs in rk817 driver
> 
>  drivers/power/supply/rk817_charger.c | 48 ++++++++++++++++------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Thanks, queued.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]