All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
@ 2022-04-15 20:36 Linus Walleij
  2022-04-15 20:36 ` [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage Linus Walleij
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Linus Walleij @ 2022-04-15 20:36 UTC (permalink / raw)
  To: Sebastian Reichel, Marcus Cooper; +Cc: linux-pm, Matti Vaittinen, Linus Walleij

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 */
+	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;
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage
  2022-04-15 20:36 [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Linus Walleij
@ 2022-04-15 20:36 ` Linus Walleij
  2022-04-19  9:26   ` Matti Vaittinen
  2022-04-19  8:50 ` [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Vaittinen, Matti
  2022-05-20 21:36 ` Linus Walleij
  2 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2022-04-15 20:36 UTC (permalink / raw)
  To: Sebastian Reichel, Marcus Cooper; +Cc: linux-pm, Matti Vaittinen, Linus Walleij

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:
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-04-15 20:36 [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Linus Walleij
  2022-04-15 20:36 ` [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage Linus Walleij
@ 2022-04-19  8:50 ` Vaittinen, Matti
  2022-05-05 20:17   ` Linus Walleij
  2022-05-20 21:36 ` Linus Walleij
  2 siblings, 1 reply; 12+ messages in thread
From: Vaittinen, Matti @ 2022-04-19  8:50 UTC (permalink / raw)
  To: Linus Walleij, Sebastian Reichel, Marcus Cooper; +Cc: linux-pm

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

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage
  2022-04-15 20:36 ` [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage Linus Walleij
@ 2022-04-19  9:26   ` Matti Vaittinen
  2022-04-19  9:44     ` Matti Vaittinen
  2022-05-05 20:22     ` Linus Walleij
  0 siblings, 2 replies; 12+ messages in thread
From: Matti Vaittinen @ 2022-04-19  9:26 UTC (permalink / raw)
  To: Linus Walleij, Sebastian Reichel, Marcus Cooper; +Cc: linux-pm

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage
  2022-04-19  9:26   ` Matti Vaittinen
@ 2022-04-19  9:44     ` Matti Vaittinen
  2022-05-05 20:22     ` Linus Walleij
  1 sibling, 0 replies; 12+ messages in thread
From: Matti Vaittinen @ 2022-04-19  9:44 UTC (permalink / raw)
  To: Linus Walleij, Sebastian Reichel, Marcus Cooper; +Cc: linux-pm

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-04-19  8:50 ` [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Vaittinen, Matti
@ 2022-05-05 20:17   ` Linus Walleij
  2022-05-06  5:38     ` Vaittinen, Matti
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2022-05-05 20:17 UTC (permalink / raw)
  To: Vaittinen, Matti; +Cc: Sebastian Reichel, Marcus Cooper, linux-pm

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage
  2022-04-19  9:26   ` Matti Vaittinen
  2022-04-19  9:44     ` Matti Vaittinen
@ 2022-05-05 20:22     ` Linus Walleij
  1 sibling, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2022-05-05 20:22 UTC (permalink / raw)
  To: Matti Vaittinen; +Cc: Sebastian Reichel, Marcus Cooper, linux-pm

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-05-05 20:17   ` Linus Walleij
@ 2022-05-06  5:38     ` Vaittinen, Matti
  2022-05-09 10:33       ` Linus Walleij
  0 siblings, 1 reply; 12+ messages in thread
From: Vaittinen, Matti @ 2022-05-06  5:38 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Marcus Cooper, linux-pm

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

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-05-06  5:38     ` Vaittinen, Matti
@ 2022-05-09 10:33       ` Linus Walleij
  2022-05-09 13:27         ` Vaittinen, Matti
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2022-05-09 10:33 UTC (permalink / raw)
  To: Vaittinen, Matti; +Cc: Sebastian Reichel, Marcus Cooper, linux-pm

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-05-09 10:33       ` Linus Walleij
@ 2022-05-09 13:27         ` Vaittinen, Matti
  0 siblings, 0 replies; 12+ messages in thread
From: Vaittinen, Matti @ 2022-05-09 13:27 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Sebastian Reichel, Marcus Cooper, linux-pm

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

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-04-15 20:36 [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Linus Walleij
  2022-04-15 20:36 ` [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage Linus Walleij
  2022-04-19  8:50 ` [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Vaittinen, Matti
@ 2022-05-20 21:36 ` Linus Walleij
  2022-06-09 20:12   ` Sebastian Reichel
  2 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2022-05-20 21:36 UTC (permalink / raw)
  To: Sebastian Reichel, Marcus Cooper; +Cc: linux-pm, Matti Vaittinen

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv
  2022-05-20 21:36 ` Linus Walleij
@ 2022-06-09 20:12   ` Sebastian Reichel
  0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Reichel @ 2022-06-09 20:12 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Marcus Cooper, linux-pm, Matti Vaittinen

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-06-09 20:12 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 20:36 [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Linus Walleij
2022-04-15 20:36 ` [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage Linus Walleij
2022-04-19  9:26   ` Matti Vaittinen
2022-04-19  9:44     ` Matti Vaittinen
2022-05-05 20:22     ` Linus Walleij
2022-04-19  8:50 ` [PATCH 1/2] power: supply: ab8500: Respect charge_restart_voltage_uv Vaittinen, Matti
2022-05-05 20:17   ` Linus Walleij
2022-05-06  5:38     ` Vaittinen, Matti
2022-05-09 10:33       ` Linus Walleij
2022-05-09 13:27         ` Vaittinen, Matti
2022-05-20 21:36 ` Linus Walleij
2022-06-09 20:12   ` Sebastian Reichel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.