[2/2] power: supply: ab8500: Exit maintenance if too low voltage

The maintenance charging is supposedly designed such that the
maintenance current compensates for the battery discharge curve,
and as the charging progress from CC/CV -> maintenance A ->
maintenance B states, we end up on a reasonable voltage to
restart ordinary CC/CV charging after the safety timer at the
maintenance B state exits.

However: old batteries discharge quicker, and in an old
battery we might not get to the expiration of the maintenance B
timer before the battery is completely depleted and the system
powers off with an empty battery.

This is hardly the desire of anyone leaving their phone in the
charger for a few days!

Introduce a second clause in both maintenance states such that
we exit the state and return to ordinary CC/CV charging if
the voltage drops below charge_restart_voltage_uv or 95%
if this is not defined for the battery.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/power/supply/ab8500_chargalg.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Hi deee Ho Linus,

On 4/15/22 23:36, Linus Walleij wrote:
> The maintenance charging is supposedly designed such that the
> maintenance current compensates for the battery discharge curve,
> and as the charging progress from CC/CV -> maintenance A ->
> maintenance B states, we end up on a reasonable voltage to
> restart ordinary CC/CV charging after the safety timer at the
> maintenance B state exits.
> 
> However: old batteries discharge quicker, and in an old
> battery we might not get to the expiration of the maintenance B
> timer before the battery is completely depleted and the system
> powers off with an empty battery.
> 
> This is hardly the desire of anyone leaving their phone in the
> charger for a few days!
> 
> Introduce a second clause in both maintenance states such that
> we exit the state and return to ordinary CC/CV charging if
> the voltage drops below charge_restart_voltage_uv or 95%
> if this is not defined for the battery.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   drivers/power/supply/ab8500_chargalg.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/power/supply/ab8500_chargalg.c b/drivers/power/supply/ab8500_chargalg.c
> index b9622eb9fc72..1b23b5261881 100644
> --- a/drivers/power/supply/ab8500_chargalg.c
> +++ b/drivers/power/supply/ab8500_chargalg.c
> @@ -1514,6 +1514,14 @@ static void ab8500_chargalg_algorithm(struct ab8500_chargalg *di)
>   			ab8500_chargalg_stop_maintenance_timer(di);
>   			ab8500_chargalg_state_to(di, STATE_MAINTENANCE_B_INIT);
>   		}
> +		/*
> +		 * This happens if the voltage drops too quickly during
> +		 * maintenance charging, especially in older batteries.
> +		 */
> +		if (ab8500_chargalg_time_to_restart(di)) {
> +			ab8500_chargalg_state_to(di, STATE_NORMAL_INIT);
> +			dev_info(di->dev, "restarted charging from maintenance state A - battery getting old?\n");
> +		}
>   		break;
>   
>   	case STATE_MAINTENANCE_B_INIT:
> @@ -1538,6 +1546,14 @@ static void ab8500_chargalg_algorithm(struct ab8500_chargalg *di)
>   			ab8500_chargalg_stop_maintenance_timer(di);
>   			ab8500_chargalg_state_to(di, STATE_NORMAL_INIT);
>   		}
> +		/*
> +		 * This happens if the voltage drops too quickly during
> +		 * maintenance charging, especially in older batteries.
> +		 */
> +		if (ab8500_chargalg_time_to_restart(di)) {
> +			ab8500_chargalg_state_to(di, STATE_NORMAL_INIT);
> +			dev_info(di->dev, "restarted charging from maintenance state B - battery getting old?\n");
> +		}
>   		break;
>   
>   	case STATE_TEMP_LOWHIGH_INIT:


Probably just a matter of taste (like underscores in private function 
names ;] ) - I would prefer combining the cases for INITs to something like:

	case STATE_MAINTENANCE_A_INIT:
	case STATE_MAINTENANCE_B_INIT:

		mt = power_supply_get_maintenance_charging_setting(bi,
			(di->charge_state == STATE_MAINTENANCE_B_INIT));

...
		ab8500_chargalg_state_to(di, di->charge_state + 1);

	break;

That would slightly reduce the code although at the cost of additional 
arithmetics. I'm leaving this to you though.

FWIW: After someone telling me that I should not worry about the cold 
weather (ref. my comment for the patch 1/2)

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>


Best Regards
	-- Matti

On Tue, Apr 19, 2022 at 11:26 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> Probably just a matter of taste (like underscores in private function
> names ;] ) - I would prefer combining the cases for INITs to something like:
>
>         case STATE_MAINTENANCE_A_INIT:
>         case STATE_MAINTENANCE_B_INIT:
>
>                 mt = power_supply_get_maintenance_charging_setting(bi,
>                         (di->charge_state == STATE_MAINTENANCE_B_INIT));
>
> ...
>                 ab8500_chargalg_state_to(di, di->charge_state + 1);
>
>         break;
>
> That would slightly reduce the code although at the cost of additional
> arithmetics. I'm leaving this to you though.

Yeah there is something like a firehose of stuff here when it comes
to coding style in this charging driver, and on top of that it
"would be nice" if the kernel had some state machine primitives one
could use in order to centralize such code and make it more robust...

> FWIW: After someone telling me that I should not worry about the cold
> weather (ref. my comment for the patch 1/2)
>
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Thanks, are you pleased with my answer to 1/2?

Yours,
Linus Walleij