Re: [PATCH 3/5] power: supply: rk817-charger: Correct the gauge across system sleep

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] power: supply: rk817-charger: rk817_suspend() treats discharge current as charge
- [Medium] power: supply: rk817-charger: uncalibrated hardware counter in rk817_bat_sleep_adjust()
- [Medium] power: supply: rk817-charger: stale charge_now_uah stamped into slp_charge_uah
--

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

power: supply: rk817-charger: Correct the gauge across system sleep

This commit adjusts the RK817 gas gauge across system sleep by stamping
boottime, charge, current, and charging status at suspend, and then
recalibrating on resume from either a latched relax voltage or an
estimated constant-current charge delta.

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

[ ... ]

> @@ -1323,12 +1336,96 @@ static int rk817_charger_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +/*
> + * The coulomb counter is erratic while the system sleeps, and the signals
> + * that could correct it (a live relax status, a finished charge) are gone
> + * before the monitor runs again. Correct from what persists instead, as
> + * the BSP driver does.
> + */
> +static void rk817_bat_sleep_adjust(struct rk817_charger *charger)
> +{
> +	struct rk808 *rk808 = charger->rk808;
> +	time64_t elapsed = ktime_get_boottime_seconds() - charger->slp_boottime;
> +	int expected_uah, moved_uah, charge_uah, credit_uah, tmp;
> +	unsigned int sts;
> +	u8 bulk_reg[4];
> +
> +	if (!charger->battery_present || elapsed <= 0)
> +		return;
> +
> +	regmap_read(rk808->regmap, RK817_SYS_STS, &sts);
> +	charger->plugged_in = (sts & RK817_PLUG_IN_STS);
> +
> +	/* Start from what the counter did during sleep, not the pre-sleep soc. */
> +	regmap_bulk_read(rk808->regmap, RK817_GAS_GAUGE_Q_PRES_H3,
> +			 bulk_reg, 4);
> +	tmp = get_unaligned_be32(bulk_reg);
> +	charge_uah = tmp > 0 ? ADC_TO_CHARGE_UAH(tmp, charger->res_div) : 0;
> +	if (charge_uah > charger->fcc_mah * 1000)
> +		charge_uah = charger->fcc_mah * 1000;
> +	charger->soc = div_s64((s64)charge_uah * 100, charger->fcc_mah);
> +
> +	/* Flags were cleared at suspend: any pair now is from this sleep. */
> +	charger->relax_voltage_uv = rk817_bat_relax_voltage_uv(charger, false);
> +	dev_dbg(charger->dev,
> +		"resume after %llds: counter %d uAh (%d at suspend), battery %d uV, relax %d uV\n",
> +		elapsed, charge_uah, charger->slp_charge_uah,
> +		rk817_bat_voltage_uv(charger, RK817_GAS_GAUGE_BAT_VOL_H),
> +		charger->relax_voltage_uv);
> +	if (charger->relax_voltage_uv) {
> +		rk817_bat_ocv_recalibrate(charger);
> +		return;
> +	}
> +
> +	if (!charger->slp_charging || !charger->plugged_in)
> +		return;
> +
> +	moved_uah = charge_uah - charger->slp_charge_uah;
> +	expected_uah = div_s64((s64)charger->slp_cur_ua * elapsed, 3600);
> +	credit_uah = expected_uah - moved_uah;
> +
> +	dev_info(charger->dev,
> +		 "slept charging for %llds: counter moved %d of expected %d uAh\n",
> +		 (long long)elapsed, moved_uah, expected_uah);
> +
> +	if (credit_uah <= 0)
> +		return;

[Severity: Medium]
What happens to the hardware coulomb counter (RK817_GAS_GAUGE_Q_PRES_H3) when
it overshoots charger->fcc_mah * 1000 or underflows below 0 during sleep?

In rk817_bat_sleep_adjust(), charge_uah and charger->soc are clamped in
software to [0, charger->fcc_mah * 1000] without writing the clamped value
back to RK817_GAS_GAUGE_Q_INIT_H3.

If a relax voltage was latched during sleep (for example, after finishing
charge and resting at 95%-100% OCV, or resting at 0%-5% OCV after counter
underflow), rk817_bat_sleep_adjust() calls rk817_bat_ocv_recalibrate():

drivers/power/supply/rk817_charger.c:rk817_bat_ocv_recalibrate() {
    ...
	ocv_soc = power_supply_batinfo_ocv2cap(charger->bat_info,
					       charger->relax_voltage_uv,
					       20) * 1000;
	if (ocv_soc < 0 || abs(ocv_soc - charger->soc) <= 5000)
		return;
    ...
}

Because charger->soc was already clamped to 100000 (or 0),
abs(ocv_soc - charger->soc) <= 5000 holds and rk817_bat_ocv_recalibrate()
returns without updating RK817_GAS_GAUGE_Q_INIT_H3.

Similarly, if no relax voltage was latched, rk817_bat_sleep_adjust() returns
without writing RK817_GAS_GAUGE_Q_INIT_H3 whenever !charger->slp_charging,
!charger->plugged_in, or credit_uah <= 0 (which happens whenever the counter
over-counted during sleep), and moved_uah = charge_uah - charger->slp_charge_uah
is computed from the clamped charge_uah rather than the actual counter delta.

Since rk817_bat_calib_cap() only caps RK817_GAS_GAUGE_Q_PRES_H3 when
charge_status == CHARGE_FINISH && charger->soc_cal, won't the hardware
accumulator retain the phantom excess above full capacity (or deficit below 0)
and keep the reported SoC pinned after resume?

> +
> +	/*
> +	 * Credit the shortfall, saturating at full - covers a charge that
> +	 * terminated during sleep. A constant-current estimate reads high
> +	 * across a taper; the cap bounds it, the next rested pair corrects
> +	 * the rest.
> +	 */
> +	charge_uah += credit_uah;
> +	if (charge_uah > charger->fcc_mah * 1000)
> +		charge_uah = charger->fcc_mah * 1000;
> +
> +	charger->soc = div_s64((s64)charge_uah * 100, charger->fcc_mah);
> +	tmp = CHARGE_TO_ADC(charge_uah / 1000, charger->res_div);
> +	put_unaligned_be32(tmp, bulk_reg);
> +	regmap_bulk_write(rk808->regmap, RK817_GAS_GAUGE_Q_INIT_H3,
> +			  bulk_reg, 4);
> +	rk817_record_battery_nvram_values(charger);
> +}
> +
>  static int __maybe_unused rk817_suspend(struct device *dev)
>  {
>  	struct rk817_charger *charger = dev_get_drvdata(dev);
>  
>  	cancel_delayed_work_sync(&charger->work);
>  
> +	charger->slp_boottime = ktime_get_boottime_seconds();
> +	charger->slp_charge_uah = charger->charge_now_uah;

[Severity: Medium]
Can charger->slp_charge_uah be stamped with a stale charger->charge_now_uah
value here?

In rk817_charging_monitor(), rk817_read_props() populates
charger->charge_now_uah from RK817_GAS_GAUGE_Q_PRES_H3 before
rk817_bat_calib_cap() and rk817_bat_ocv_recalibrate() run. When either of
those functions rewrites RK817_GAS_GAUGE_Q_INIT_H3, charger->charge_now_uah is
not updated until the next 8-second monitor cycle.

Likewise, in rk817_resume(), rk817_bat_sleep_adjust() reads
RK817_GAS_GAUGE_Q_PRES_H3 into a local charge_uah variable and either calls
rk817_bat_ocv_recalibrate() or writes the credited charge to
RK817_GAS_GAUGE_Q_INIT_H3 without updating charger->charge_now_uah.

If the system suspends during the 8-second window after a recalibration, or
after rk817_resume() before the queued charger->work executes (which
cancel_delayed_work_sync(&charger->work) then cancels), wouldn't
rk817_suspend() stamp the stale charger->charge_now_uah into
charger->slp_charge_uah and skew moved_uah and credit_uah on the next resume?

> +	charger->slp_cur_ua = abs(charger->cur_avg_ua);
> +	charger->slp_charging = charger->plugged_in &&
> +				(charger->charge_status == CC_OR_CV_CHRG ||
> +				 charger->charge_status == TRICKLE_CHRG);

[Severity: Medium]
Could taking abs(charger->cur_avg_ua) in rk817_suspend() cause
rk817_bat_sleep_adjust() to credit discharge current as charge?

In rk817_read_props(), charger->cur_avg_ua is a signed value where negative
readings indicate net battery discharge, which rk817_bat_get_prop() checks
before inspecting charger->charge_status:

drivers/power/supply/rk817_charger.c:rk817_bat_get_prop() {
    ...
	case POWER_SUPPLY_PROP_STATUS:
		if (charger->cur_avg_ua < 0) {
			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
			break;
		}
    ...
}

If the system suspends while plugged in with charge_status == CC_OR_CV_CHRG or
TRICKLE_CHRG while charger->cur_avg_ua < 0 (for instance, when pre-suspend
system load exceeds the input current limit, or right after plug-in before the
average current register slews positive), rk817_suspend() sets
charger->slp_charging = true and turns the negative current into a positive
charger->slp_cur_ua. On resume, wouldn't rk817_bat_sleep_adjust() then compute
a positive expected_uah from that discharge rate and credit phantom charge to
charger->soc, RK817_GAS_GAUGE_Q_INIT_H3, and NVRAM?

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