| Message ID | 20220415203638.361074-2-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] power: supply: ab8500: Respect charge_restart_voltage_uv | expand |
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 4/19/22 12:26, Matti Vaittinen wrote: >> --- 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. Oh. I was misreading the code. There is fallthrough and not break as the actual 'maintenance states' are handled directly after the 'maintenance init states'. Well, it seems to me also the actual maintenance states could be combined into one case though. But as I wrote - your decision :) 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
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:
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(+)