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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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