diff mbox

ACPI battery driver emits POWER_SUPPLY_STATUS_FULL when power lead plugged in (resend)

Message ID 1233148811.4231.0.camel@hughsie-work.lan (mailing list archive)
State Accepted
Headers show

Commit Message

Richard Hughes Jan. 28, 2009, 1:20 p.m. UTC 
On Sun, 2009-01-25 at 11:42 -0200, Henrique de Moraes Holschuh wrote:
>  So, the above test will still break on any proper battery subsystem with the
> high watermark set below 100%, as those systems update full_charge_capacity
> *only* when the cells really are full (and not because the EC decided to
> stop the charging before they were full).

This is what I observed when testing my patch.

The attached patch checks the last full and design charge, as this seems
to work in all cases I have here. In the case of broken batteries or
broken hardware, we just return UNKNOWN in this "settling" state, which
is much better for userspace than falling back to full.

With the attached patch userspace gets the right states. In the future,
maybe we can do some sort of metric over time (watching to see if the
present charge changes) but for the most part the patch fixes things up
for userspace.

Please review,

Richard.

Comments

Henrique de Moraes Holschuh Jan. 30, 2009, 2:07 p.m. UTC | #1 
On Wed, 28 Jan 2009, Richard Hughes wrote:
> The attached patch checks the last full and design charge, as this seems
> to work in all cases I have here. In the case of broken batteries or
> broken hardware, we just return UNKNOWN in this "settling" state, which
> is much better for userspace than falling back to full.
> 
> With the attached patch userspace gets the right states. In the future,
> maybe we can do some sort of metric over time (watching to see if the
> present charge changes) but for the most part the patch fixes things up
> for userspace.

I am ok with this patch, but it does have *one* corner case.

Brand new good batteries will be reported as full while still charging, when
they're near 100% (they will be above the design capacity).

That isn't enough for me to complain about the patch, and the patch makes it
MUCH better than the current broken behaviour.

So you can have my Acked-by if you want it, for whatever little it is worth.
Len Brown Feb. 8, 2009, 3:59 a.m. UTC | #2 
On Fri, 30 Jan 2009, Henrique de Moraes Holschuh wrote:

> On Wed, 28 Jan 2009, Richard Hughes wrote:
> > The attached patch checks the last full and design charge, as this seems
> > to work in all cases I have here. In the case of broken batteries or
> > broken hardware, we just return UNKNOWN in this "settling" state, which
> > is much better for userspace than falling back to full.
> > 
> > With the attached patch userspace gets the right states. In the future,
> > maybe we can do some sort of metric over time (watching to see if the
> > present charge changes) but for the most part the patch fixes things up
> > for userspace.
> 
> I am ok with this patch, but it does have *one* corner case.
> 
> Brand new good batteries will be reported as full while still charging, when
> they're near 100% (they will be above the design capacity).
> 
> That isn't enough for me to complain about the patch, and the patch makes it
> MUCH better than the current broken behaviour.
> 
> So you can have my Acked-by if you want it, for whatever little it is worth.

Applied to acpi-test.

thanks,
Len Brown, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Hughes Feb. 8, 2009, 10:08 a.m. UTC | #3 
On Sat, 2009-02-07 at 22:59 -0500, Len Brown wrote:
> Applied to acpi-test.

Thanks dude.

Richard.


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

>From 3b0fb1239e5bc064766ffa3d7a45265e722fb9eb Mon Sep 17 00:00:00 2001
From: Richard Hughes <hughsient@gmail.com>
Date: Sun, 25 Jan 2009 15:05:50 +0000
Subject: [PATCH] battery: don't assume we are fully charged when not charging or discharging

On hardware like the T61 it can take a couple of seconds for the battery
to start charging after the power is connected, and we incorrectly tell
userspace that we are fully charged, and then go back to charging.

Only mark a battery as fully charged when the preset charge matches either
the last full charge, or the design charge.

Signed-off-by: Richard Hughes <hughsient@gmail.com>
---
 drivers/acpi/battery.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 65132f9..69cbc57 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -138,6 +138,29 @@  static int acpi_battery_technology(struct acpi_battery *battery)
 
 static int acpi_battery_get_state(struct acpi_battery *battery);
 
+static int acpi_battery_is_charged(struct acpi_battery *battery)
+{
+	/* either charging or discharging */
+	if (battery->state != 0)
+		return 0;
+
+	/* battery not reporting charge */
+	if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN ||
+	    battery->capacity_now == 0)
+		return 0;
+
+	/* good batteries update full_charge as the batteries degrade */
+	if (battery->full_charge_capacity == battery->capacity_now)
+		return 1;
+
+	/* fallback to using design values for broken batteries */
+	if (battery->design_capacity == battery->capacity_now)
+		return 1;
+
+	/* we don't do any sort of metric based on percentages */
+	return 0;
+}
+
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,
 				     union power_supply_propval *val)
@@ -155,7 +178,7 @@  static int acpi_battery_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		else if (battery->state & 0x02)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else if (battery->state == 0)
+		else if (acpi_battery_is_charged(battery))
 			val->intval = POWER_SUPPLY_STATUS_FULL;
 		else
 			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
-- 
1.6.0.6