[2/3] power: supply: cpcap-battery: Check voltage before orderly_poweroff
diff mbox series

Message ID 20190917213501.16907-3-tony@atomide.com
State New
Headers show
Series
  • cpcap charger and battery fixes
Related show

Commit Message

Tony Lindgren Sept. 17, 2019, 9:35 p.m. UTC
We can get the low voltage interrupt trigger sometimes way too early,
maybe because of CPU load spikes. This causes orderly_poweroff() be
called too easily.

Let's check the voltage before orderly_poweroff in case it was not
yet a permanent condition. We will be getting more interrupts anyways
if the condition persists.

Let's also show the measured voltages for low battery and battery
empty warnings since we have them.

Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Pavel Machek Sept. 19, 2019, 9:14 a.m. UTC | #1
On Tue 2019-09-17 14:35:00, Tony Lindgren wrote:
> We can get the low voltage interrupt trigger sometimes way too early,
> maybe because of CPU load spikes. This causes orderly_poweroff() be
> called too easily.
> 
> Let's check the voltage before orderly_poweroff in case it was not
> yet a permanent condition. We will be getting more interrupts anyways
> if the condition persists.
> 
> Let's also show the measured voltages for low battery and battery
> empty warnings since we have them.

Well, this is decision that will shorten battery lifetime. There's
very little capacity left when battery is down to 3.3V...

What kind of "way too early" do you see?

> @@ -562,12 +562,15 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
>  	switch (d->action) {
>  	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
>  		if (latest->current_ua >= 0)
> -			dev_warn(ddata->dev, "Battery low at 3.3V!\n");
> +			dev_warn(ddata->dev, "Battery low at %i!\n",
> +				latest->voltage);
>  		break;

I'd still leave unit ("uV"?) there. Or do /1000, as and display mV, as
our 
> -				  "Battery empty at 3.1V, powering off\n");
> +				  "Battery empty at %i, powering off\n",
> +				  latest->voltage);
>  			orderly_poweroff(true);

Same here.

Plus I see bigger problem: shutdown from mainline seems to leave
something powered in the phone (I believe I seen USB charge pump, for
example), so the battery will be completely empty next time I attempt
to use the phone. (I learned to reboot into stock android and shutdown
there).

Phone should last days when powered off, but it seems to only last
hours.

Unfortunately I don't know how to debug that :-(.

Best regards,
									Pavel
Tony Lindgren Sept. 20, 2019, 2:12 p.m. UTC | #2
* Pavel Machek <pavel@ucw.cz> [190919 09:15]:
> On Tue 2019-09-17 14:35:00, Tony Lindgren wrote:
> > We can get the low voltage interrupt trigger sometimes way too early,
> > maybe because of CPU load spikes. This causes orderly_poweroff() be
> > called too easily.
> > 
> > Let's check the voltage before orderly_poweroff in case it was not
> > yet a permanent condition. We will be getting more interrupts anyways
> > if the condition persists.
> > 
> > Let's also show the measured voltages for low battery and battery
> > empty warnings since we have them.
> 
> Well, this is decision that will shorten battery lifetime. There's
> very little capacity left when battery is down to 3.3V...
> 
> What kind of "way too early" do you see?

I've seen it trigger spontaneously already around battery low
interrupt time.

> > @@ -562,12 +562,15 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
> >  	switch (d->action) {
> >  	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
> >  		if (latest->current_ua >= 0)
> > -			dev_warn(ddata->dev, "Battery low at 3.3V!\n");
> > +			dev_warn(ddata->dev, "Battery low at %i!\n",
> > +				latest->voltage);
> >  		break;
> 
> I'd still leave unit ("uV"?) there. Or do /1000, as and display mV, as
> our 
> > -				  "Battery empty at 3.1V, powering off\n");
> > +				  "Battery empty at %i, powering off\n",
> > +				  latest->voltage);
> >  			orderly_poweroff(true);
> 
> Same here.

Sure yeah I'll update it.

> Plus I see bigger problem: shutdown from mainline seems to leave
> something powered in the phone (I believe I seen USB charge pump, for
> example), so the battery will be completely empty next time I attempt
> to use the phone. (I learned to reboot into stock android and shutdown
> there).
> 
> Phone should last days when powered off, but it seems to only last
> hours.
> 
> Unfortunately I don't know how to debug that :-(.

Yes there's some issue with shutdown. I think it's somehow related
to mdm6600 being powered where the poweroff gpio does not allow
device to shut down with modem powered. We could try adding a
.power_off function to the modem code to see if it helps.

Additionally I've noticed that we leave some PMIC features powered
when device is powered off without a modem consuming about 2.5mW
while powering off from Android shows power consumption in uW
range probably with only RTC being powered.

Regards,

Tony
Tony Lindgren Sept. 22, 2019, 6 p.m. UTC | #3
* Tony Lindgren <tony@atomide.com> [190920 14:13]:
> * Pavel Machek <pavel@ucw.cz> [190919 09:15]:
> > Plus I see bigger problem: shutdown from mainline seems to leave
> > something powered in the phone (I believe I seen USB charge pump, for
> > example), so the battery will be completely empty next time I attempt
> > to use the phone. (I learned to reboot into stock android and shutdown
> > there).
> > 
> > Phone should last days when powered off, but it seems to only last
> > hours.
> > 
> > Unfortunately I don't know how to debug that :-(.
> 
> Yes there's some issue with shutdown. I think it's somehow related
> to mdm6600 being powered where the poweroff gpio does not allow
> device to shut down with modem powered. We could try adding a
> .power_off function to the modem code to see if it helps.

Sorry I mean .shutdown function. But I doubt this is a modem
issue you're seeing, I already fixed that issue most likely with
commit 8ead7e817224 ("usb: core: Add PM runtime calls to
usb_hcd_platform_shutdown"). Well at least things are shutting
down for me now after checking few times. I recall the symptoms
of the shut down failing issue is that the also the LCD backlight
stays on.

There are some issues left with musb configured as a usb
gadget, but I have not been able to quite track those down
so far. It seems that there are some gadget framework kobject
warnings with the musb controlling device (omap2430) unloaded
and then doing a shutdown. The device shuts down though.

> Additionally I've noticed that we leave some PMIC features powered
> when device is powered off without a modem consuming about 2.5mW
> while powering off from Android shows power consumption in uW
> range probably with only RTC being powered.

AFAIK this issue still remains. I'll take a look at adding a
.shutdown somewhere for cpcap driver(s) so we get it cleared
for a low-power state.

Regards,

Tony

Patch
diff mbox series

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -562,12 +562,15 @@  static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
 	switch (d->action) {
 	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
 		if (latest->current_ua >= 0)
-			dev_warn(ddata->dev, "Battery low at 3.3V!\n");
+			dev_warn(ddata->dev, "Battery low at %i!\n",
+				latest->voltage);
 		break;
 	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
-		if (latest->current_ua >= 0) {
+		if (latest->current_ua >= 0 && latest->voltage >= 0 &&
+		    latest->voltage <= 3100000) {
 			dev_emerg(ddata->dev,
-				  "Battery empty at 3.1V, powering off\n");
+				  "Battery empty at %i, powering off\n",
+				  latest->voltage);
 			orderly_poweroff(true);
 		}
 		break;