| Message ID | 20220415203638.361074-1-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 again Linus, For some reason I am always slightly terrified when charging is controlled by the software ;) On 4/15/22 23:36, Linus Walleij wrote: > The battery info contains a voltage indicating when the voltage > is so low that it is time to restart the CC/CV charging. > Make the AB8500 respect and prioritize this setting over the > hardcoded 95% threshold. > > Break out the check into its own function and add some safeguards > so we do not run into unpredictable side effects. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/power/supply/ab8500_chargalg.c | 30 +++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/ab8500_chargalg.c b/drivers/power/supply/ab8500_chargalg.c > index 94c22fdfe963..b9622eb9fc72 100644 > --- a/drivers/power/supply/ab8500_chargalg.c > +++ b/drivers/power/supply/ab8500_chargalg.c > @@ -1216,6 +1216,34 @@ static void ab8500_chargalg_external_power_changed(struct power_supply *psy) > queue_work(di->chargalg_wq, &di->chargalg_work); > } > > +/** > + * ab8500_chargalg_time_to_restart() - time to restart CC/CV charging? > + * @di: charging algorithm state > + * > + * This checks if the voltage or capacity of the battery has fallen so > + * low that we need to restart the CC/CV charge cycle. > + */ > +static bool ab8500_chargalg_time_to_restart(struct ab8500_chargalg *di) > +{ > + struct power_supply_battery_info *bi = di->bm->bi; > + > + /* Sanity check - these need to have some reasonable values */ > + if (!di->batt_data.volt_uv || !di->batt_data.percent) > + return false; > + > + /* Some batteries tell us at which voltage we should restart charging */ Is utilizing this limit something that has already existed for these batteries? I am just slightly worrying if this can cause problems at low temperatures? I am by no means an expert on this area (as I've told earlier :]) so I may be completely off here. Anyways, I think I've seen voltage curves for batteries at different temparetures - and AFAIR, the voltage of a battery with near 100% SOC at -20 Ccan be close to the voltage of a nearly depleted battery at +40 C. Hence I am just asking if this is not causing my phone to keep charging even when the battery is full. I mean, when I am at next autumn spending the night in a tent at Enontekiö Finland - and forget to disconnect my phone from charger before the campfire fades away? :] (Okay, I usually take my phone in a sleeping bag for night - but anyways, can this cause problems in cold conditions? Should we have some temperature check - or are the batteries with the charge_restart_voltage_uv set just smart enough?). Please, just ignore my questions if this was existing functionality. > + if (bi->charge_restart_voltage_uv > 0) { > + if (di->batt_data.volt_uv <= bi->charge_restart_voltage_uv) > + return true; > + /* Else we restart as we reach a certain capacity */ > + } else { > + if (di->batt_data.percent <= AB8500_RECHARGE_CAP) > + return true; > + } > + > + return false; > +} > + > /** > * ab8500_chargalg_algorithm() - Main function for the algorithm > * @di: pointer to the ab8500_chargalg structure > @@ -1459,7 +1487,7 @@ static void ab8500_chargalg_algorithm(struct ab8500_chargalg *di) > fallthrough; > > case STATE_WAIT_FOR_RECHARGE: > - if (di->batt_data.percent <= AB8500_RECHARGE_CAP) > + if (ab8500_chargalg_time_to_restart(di)) > ab8500_chargalg_state_to(di, STATE_NORMAL_INIT); > break; > Best Regards -- Matti
Hi Matti, sorry for slow replies! On Tue, Apr 19, 2022 at 10:51 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > Hi again Linus, > > For some reason I am always slightly terrified when charging is > controlled by the software ;) That is a normal reaction... The AB8500 has a few hardware safeguards to make sure it is somewhat safe. Such as multiple thermal sensors and over-voltage protection. But it also has a pretty elaborate state machine. > On 4/15/22 23:36, Linus Walleij wrote: > > + /* Some batteries tell us at which voltage we should restart charging */ > > Is utilizing this limit something that has already existed for these > batteries? Yes, look for example at the Kyle battery, Samsung SDI EB425161LA: https://github.com/linusw/u8500/blob/Samsung-SGH-I407-Kyle/arch/arm/mach-ux500/board-kyle-bm.c line 312 called "recharge_vol" is set to 4.3 V Then in the charging algorithm: https://github.com/linusw/u8500/blob/Samsung-SGH-I407-Kyle/drivers/power/ab8500_chargalg.c line 2229 you can see how it is used the same way. > I am just slightly worrying if this can cause problems at low > temperatures? I am by no means an expert on this area (as I've told > earlier :]) so I may be completely off here. Anyways, I think I've seen > voltage curves for batteries at different temparetures - and AFAIR, the > voltage of a battery with near 100% SOC at -20 Ccan be close to the > voltage of a nearly depleted battery at +40 C. Probably true, because the batteries have operational conditions for low/high temperatures which change the behaviour of the charging, so that is how they choose to deal with this. > Hence I am just asking if this is not causing my phone to keep charging > even when the battery is full. I mean, when I am at next autumn spending > the night in a tent at Enontekiö Finland - and forget to disconnect my > phone from charger before the campfire fades away? :] The battery in my example EB425161LA is defined in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/samsung-sdi-battery.c lines 677-719, there you find .temp_alert_min = 0, and if you look in the charging algorithm: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/ab8500_chargalg.c below that temperature the charging algorithm will switch to a lower recharging current by going into the state TEMP_UNDEROVER where this recharging voltage does not apply, instead a much lower voltage alert_low_temp_charge_voltage_uv = 4000000 will apply, 4.0V instead of 4.3V. Further you see that temp_min = -30, so if the temperature goes below -30 degrees, the algorithm will shut down charging altogether. Yours, Linus Walleij
On 5/5/22 23:17, Linus Walleij wrote: > Hi Matti, > > sorry for slow replies! No sweat. I didn't hold my breath :) > But it also has a pretty elaborate state > machine. Indeed... This is also what I overlooked. >> Hence I am just asking if this is not causing my phone to keep charging >> even when the battery is full. I mean, when I am at next autumn spending >> the night in a tent at Enontekiö Finland - and forget to disconnect my >> phone from charger before the campfire fades away? :] > > The battery in my example EB425161LA is defined in > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/samsung-sdi-battery.c > lines 677-719, there you find .temp_alert_min = 0, and if you look in > the charging algorithm: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/ab8500_chargalg.c > below that temperature the charging algorithm will switch to a lower recharging > current by going into the state TEMP_UNDEROVER where this > recharging voltage does not apply, instead a much lower voltage > alert_low_temp_charge_voltage_uv = 4000000 will apply, 4.0V > instead of 4.3V. > > Further you see that temp_min = -30, so if the temperature goes > below -30 degrees, the algorithm will shut down charging altogether. Thanks for the thorough walkthrough. And sorry - I should have noticed the ab8500_chargalg_time_to_restart() is only entered from the state 'STATE_WAIT_FOR_RECHARGE' - which is not the state we are sitting in when temp goes down. So my mistake - I am perfectly happy with the patch then. Also, really happy for receiving the explanation. Thanks!:] Best Regards -- Matti
On Fri, May 6, 2022 at 7:38 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > I am perfectly happy with the patch > then. Also, really happy for receiving the explanation. Thanks!:] I record that as an Acked-by, OK? Yours, Linus Walleij
On 5/9/22 13:33, Linus Walleij wrote: > On Fri, May 6, 2022 at 7:38 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > >> I am perfectly happy with the patch >> then. Also, really happy for receiving the explanation. Thanks!:] > > I record that as an Acked-by, OK? > Sure! And sorry! I would have added it explicitly but I was under impression I already said that I am Ok with the patch when my doubt is pointed wrong. It appears I said that for the other patch only. Best Regards -- Matti
On Fri, Apr 15, 2022 at 10:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > The battery info contains a voltage indicating when the voltage > is so low that it is time to restart the CC/CV charging. > Make the AB8500 respect and prioritize this setting over the > hardcoded 95% threshold. > > Break out the check into its own function and add some safeguards > so we do not run into unpredictable side effects. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Sebastian could you merge this patch (and patch 2/2) now that I convinced Matti it's safe? Thanks, Linus Walleij
Hi, On Fri, May 20, 2022 at 11:36:29PM +0200, Linus Walleij wrote: > On Fri, Apr 15, 2022 at 10:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > The battery info contains a voltage indicating when the voltage > > is so low that it is time to restart the CC/CV charging. > > Make the AB8500 respect and prioritize this setting over the > > hardcoded 95% threshold. > > > > Break out the check into its own function and add some safeguards > > so we do not run into unpredictable side effects. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > Sebastian could you merge this patch (and patch 2/2) now that I > convinced Matti it's safe? Yes, I queued both patches to power-supply's for-next branch now. -- Sebastian
diff --git a/drivers/power/supply/ab8500_chargalg.c b/drivers/power/supply/ab8500_chargalg.c index 94c22fdfe963..b9622eb9fc72 100644 --- a/drivers/power/supply/ab8500_chargalg.c +++ b/drivers/power/supply/ab8500_chargalg.c @@ -1216,6 +1216,34 @@ static void ab8500_chargalg_external_power_changed(struct power_supply *psy) queue_work(di->chargalg_wq, &di->chargalg_work); } +/** + * ab8500_chargalg_time_to_restart() - time to restart CC/CV charging? + * @di: charging algorithm state + * + * This checks if the voltage or capacity of the battery has fallen so + * low that we need to restart the CC/CV charge cycle. + */ +static bool ab8500_chargalg_time_to_restart(struct ab8500_chargalg *di) +{ + struct power_supply_battery_info *bi = di->bm->bi; + + /* Sanity check - these need to have some reasonable values */ + if (!di->batt_data.volt_uv || !di->batt_data.percent) + return false; + + /* Some batteries tell us at which voltage we should restart charging */ + if (bi->charge_restart_voltage_uv > 0) { + if (di->batt_data.volt_uv <= bi->charge_restart_voltage_uv) + return true; + /* Else we restart as we reach a certain capacity */ + } else { + if (di->batt_data.percent <= AB8500_RECHARGE_CAP) + return true; + } + + return false; +} + /** * ab8500_chargalg_algorithm() - Main function for the algorithm * @di: pointer to the ab8500_chargalg structure @@ -1459,7 +1487,7 @@ static void ab8500_chargalg_algorithm(struct ab8500_chargalg *di) fallthrough; case STATE_WAIT_FOR_RECHARGE: - if (di->batt_data.percent <= AB8500_RECHARGE_CAP) + if (ab8500_chargalg_time_to_restart(di)) ab8500_chargalg_state_to(di, STATE_NORMAL_INIT); break;
The battery info contains a voltage indicating when the voltage is so low that it is time to restart the CC/CV charging. Make the AB8500 respect and prioritize this setting over the hardcoded 95% threshold. Break out the check into its own function and add some safeguards so we do not run into unpredictable side effects. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/power/supply/ab8500_chargalg.c | 30 +++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)