Re: [PATCH 2/5] power: supply: rk817-charger: Resolve boot state of charge from the coulomb counter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] power: supply: rk817-charger: flawed keep_saved check on unplugged boot
--

commit ca12b28c400d82b7a66e329e1e7db82f71128a48
Author: Chris Morgan <macromorgan@hotmail.com>

power: supply: rk817-charger: Resolve boot state of charge from the coulomb counter

This commit resolves the boot state of charge from the coulomb counter on
non-first boots instead of relying on PWRON_VOL when OFF_CNT is three or
more, and adds periodic recalibration against the relaxed open-circuit
voltage pair. It also exposes POWER_SUPPLY_PROP_VOLTAGE_BOOT and
POWER_SUPPLY_PROP_VOLTAGE_OCV.

> diff --git a/drivers/power/supply/rk817_charger.c b/drivers/power/supply/rk817_charger.c
> index 86da0462fb594..50fbea298127f 100644
> --- a/drivers/power/supply/rk817_charger.c
> +++ b/drivers/power/supply/rk817_charger.c

[ ... ]

> @@ -803,39 +889,54 @@ rk817_read_or_set_full_charge_on_boot(struct rk817_charger *charger,
>  		if (ret < 0)
>  			return ret;
>  
> +		/*
> +		 * The coulomb counter keeps counting while the system is off,
> +		 * so take it, capped at full, unless it reads invalid; the
> +		 * saved soc is then at most one update interval old. A counter
> +		 * that is merely wrong is corrected by the next rested relax
> +		 * pair.
> +		 */
>  		regmap_bulk_read(rk808->regmap, RK817_GAS_GAUGE_Q_PRES_H3,
>  				 bulk_reg, 4);
>  		tmp = get_unaligned_be32(bulk_reg);

[ ... ]

> +		regmap_read(rk808->regmap, RK817_GAS_GAUGE_OFF_CNT, &off_cnt);
> +		bat_uv = rk817_bat_voltage_uv(charger, RK817_GAS_GAUGE_BAT_VOL_H);
> +		dev_dbg(charger->dev,
> +			"boot: counter %d uAh, saved soc %d, fcc %d mAh, battery %d uV, off_cnt %d\n",
> +			tmp > 0 ? ADC_TO_CHARGE_UAH(tmp, charger->res_div) : 0,
> +			charger->soc, charger->fcc_mah, bat_uv, off_cnt);
> +		if (tmp > 0) {
> +			boot_charge_mah = ADC_TO_CHARGE_UAH(tmp,
> +							    charger->res_div) / 1000;
> +			if (boot_charge_mah > charger->fcc_mah)
> +				boot_charge_mah = charger->fcc_mah;
> +			boot_soc = boot_charge_mah * 100000 / charger->fcc_mah;
> +
> +			/*
> +			 * The gauge re-derives this count from its own ocv
> +			 * model while the pack rests, so bound it with the ocv
> +			 * table: under load the terminal voltage sits below the
> +			 * open circuit voltage, making the table a floor, and
> +			 * under charge above it, making it a ceiling on any
> +			 * saved soc that would displace the counter.
> +			 */
> +			table_soc = power_supply_batinfo_ocv2cap(bat_info, bat_uv,
> +								 20) * 1000;
> +			if (charger->plugged_in)
> +				keep_saved = boot_soc < charger->soc - 10000 &&
> +					     charger->soc <= table_soc + 10000;
> +			else
> +				keep_saved = boot_soc < table_soc - 10000;
> +
> +			if (keep_saved)
> +				dev_dbg(charger->dev,
> +					"counter %d, voltage allows %d, keeping saved soc %d\n",
> +					boot_soc, table_soc, charger->soc);
> +			else
> +				charger->soc = boot_soc;

[Severity: Medium]
In rk817_read_or_set_full_charge_on_boot() in
drivers/power/supply/rk817_charger.c, when !charger->plugged_in, keep_saved
is set to:

	keep_saved = boot_soc < table_soc - 10000;

without comparing charger->soc (from NVRAM) against either boot_soc or
table_soc.

Immediately after this branch, rk817_read_or_set_full_charge_on_boot() writes
charger->soc back into the hardware coulomb counter:

	boot_charge_mah = charger->soc * charger->fcc_mah / 100 / 1000;
	if (boot_charge_mah > charger->fcc_mah)
		boot_charge_mah = charger->fcc_mah;
	tmp = CHARGE_TO_ADC(boot_charge_mah, charger->res_div);
	put_unaligned_be32(tmp, bulk_reg);
	ret = regmap_bulk_write(rk808->regmap, RK817_GAS_GAUGE_Q_INIT_H3,
			  bulk_reg, 4);

If a device is powered off at a low state of charge (for example,
charger->soc = 15000), charged while powered off, and then unplugged before
booting (!charger->plugged_in), and the gauge's internal OCV model re-derives
boot_soc to more than 10% below table_soc (say, table_soc = 80000 and
boot_soc = 65000), keep_saved evaluates to true. That discards boot_soc and
reinitializes RK817_GAS_GAUGE_Q_INIT_H3 with the stale pre-charge NVRAM
charger->soc (15000), which is even further below the table_soc floor.

Conversely, if the device is powered off at a high state of charge (for
example, charger->soc = 95000) and discharges while off to table_soc = 50000
with boot_soc = 38000, keep_saved also evaluates to true and overwrites
RK817_GAS_GAUGE_Q_INIT_H3 with the stale 95000 value.

In addition, both the commit message ("Keep the PWRON_VOL lookup for the
first boot. Otherwise take the counter, capped at full, unless it reads zero
or negative, in which case keep the saved soc...") and the comment above this
block say that a positive coulomb counter is adopted on boot and corrected
later by the next rested relax voltage pair.

Should the !charger->plugged_in case also verify that charger->soc is a
better estimate than boot_soc before keeping the saved value, or should
rk817_read_or_set_full_charge_on_boot() unconditionally use boot_soc when
tmp > 0 as described in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026174834.1652309-1-macroalpha82@gmail.com?part=2