[PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Andres Salomon]

The hardware has an extra bit that specifies that the battery is trickle
charging, so when determining if the battery is present/charging the TRICKLE
bit needs to be checked as well.  Because battery diagnostics might want to
know whether trickle charging is happening or not, and also because trickle
charging falls somewhere between charging and not charging (read: Richard got
mad at me when I tried to set CHARGING when in trickle charge.  He gets so
angry sometimes), we add a new TRICKLE status to sysfs.

Signed-off-by: Andres Salomon <dilinger@collabora.co.uk>
---
 drivers/power/olpc_battery.c       |   13 ++++++++++---
 drivers/power/power_supply_sysfs.c |    3 ++-
 include/linux/power_supply.h       |    1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index f8d2d6b..3ecf484 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -35,6 +35,7 @@
 #define BAT_STAT_AC		0x10
 #define BAT_STAT_CHARGING	0x20
 #define BAT_STAT_DISCHARGING	0x40
+#define BAT_STAT_TRICKLE	0x80
 
 #define BAT_ERR_INFOFAIL	0x02
 #define BAT_ERR_OVERVOLTAGE	0x04
@@ -89,7 +90,12 @@ static char bat_serial[17]; /* Ick */
 static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
 {
 	if (olpc_platform_info.ecver > 0x44) {
-		if (ec_byte & BAT_STAT_CHARGING)
+		if (ec_byte & BAT_STAT_TRICKLE)
+			/* Note that the order here is important; the EC sets
+			 * both the CHARGING and TRICKLE bits at the same time
+			 * to support older kernels. */
+			val->intval = POWER_SUPPLY_STATUS_TRICKLE_CHARGING;
+		else if (ec_byte & BAT_STAT_CHARGING)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
 		else if (ec_byte & BAT_STAT_DISCHARGING)
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
@@ -219,7 +225,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
 	   It doesn't matter though -- the EC will return the last-known
 	   information, and it's as if we just ran that _little_ bit faster
 	   and managed to read it out before the battery went away. */
-	if (!(ec_byte & BAT_STAT_PRESENT) && psp != POWER_SUPPLY_PROP_PRESENT)
+	if (!(ec_byte & (BAT_STAT_PRESENT|BAT_STAT_TRICKLE)) &&
+			psp != POWER_SUPPLY_PROP_PRESENT)
 		return -ENODEV;
 
 	switch (psp) {
@@ -229,7 +236,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 			return ret;
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
-		val->intval = !!(ec_byte & BAT_STAT_PRESENT);
+		val->intval = !!(ec_byte & (BAT_STAT_PRESENT|BAT_STAT_TRICKLE));
 		break;
 
 	case POWER_SUPPLY_PROP_HEALTH:
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index add5f39..eb1affc 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -41,7 +41,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf) {
 	static char *status_text[] = {
-		"Unknown", "Charging", "Discharging", "Not charging", "Full"
+		"Unknown", "Charging", "Charging (trickle)", "Discharging", "Not charging",
+		"Full",
 	};
 	static char *health_text[] = {
 		"Unknown", "Good", "Overheat", "Dead", "Over voltage",
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 1e45cbc..50ee079 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -33,6 +33,7 @@
 enum {
 	POWER_SUPPLY_STATUS_UNKNOWN = 0,
 	POWER_SUPPLY_STATUS_CHARGING,
+	POWER_SUPPLY_STATUS_TRICKLE_CHARGING,
 	POWER_SUPPLY_STATUS_DISCHARGING,
 	POWER_SUPPLY_STATUS_NOT_CHARGING,
 	POWER_SUPPLY_STATUS_FULL,
-- 
1.5.6.5

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Mark Brown]

On Mon, Jun 22, 2009 at 11:46:07PM -0400, Andres Salomon wrote:

> The hardware has an extra bit that specifies that the battery is trickle
> charging, so when determining if the battery is present/charging the TRICKLE
> bit needs to be checked as well.  Because battery diagnostics might want to
> know whether trickle charging is happening or not, and also because trickle
> charging falls somewhere between charging and not charging (read: Richard got
> mad at me when I tried to set CHARGING when in trickle charge.  He gets so
> angry sometimes), we add a new TRICKLE status to sysfs.

To avoid confusing usere applications might it be better to add another
piece of information with detail on the charger status rather than a new
state?  The fact that the new state name shares a prefix with the
regular charging state does help here but you might still confuse things
and it makes the name of the new state in sysfs a bit awkward.

I'd also be tempted to do this the other way around and add a fast
charge status; certainly in embedded cases trickle charging is more of a
default state since it requires less incoming power (important if you're
using USB) and there's less risk of damage to the battery.

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Richard A. Smith]

Mark Brown wrote:

> On Mon, Jun 22, 2009 at 11:46:07PM -0400, Andres Salomon wrote:
> 
>> The hardware has an extra bit that specifies that the battery is trickle
>> charging, so when determining if the battery is present/charging the TRICKLE
>> bit needs to be checked as well.  Because battery diagnostics might want to
>> know whether trickle charging is happening or not, and also because trickle
>> charging falls somewhere between charging and not charging (read: Richard got
>> mad at me when I tried to set CHARGING when in trickle charge.  He gets so
>> angry sometimes), we add a new TRICKLE status to sysfs.
> 
> To avoid confusing usere applications might it be better to add another
> piece of information with detail on the charger status rather than a new
> state?  The fact that the new state name shares a prefix with the
> regular charging state does help here but you might still confuse things
> and it makes the name of the new state in sysfs a bit awkward.
> 
> I'd also be tempted to do this the other way around and add a fast
> charge status; certainly in embedded cases trickle charging is more of a
> default state since it requires less incoming power (important if you're
> using USB) and there's less risk of damage to the battery.

On the OLPC the trickle charging really is a trickle rather than just a
slower charge rate.  To fully charge a XO battery at trickle would take
about 150 hours.

Trickle is enabled when the battery voltage has dropped outside of the
normal charging envelope.  In that zone the battery voltage curve is
very non-linear so even small currents will make the voltage rise
quickly.  Once the voltage hits the safe zone regular the normal
charging rate is enabled.

I don't have any strong feelings about what nomenclature we use to
describe it.  I just want it to be a separate identifiable state (or
status).  In (OLPCs) normal charging/discharging cycle the trickle zone 
should not be reached so its an indication that you are outside the 
envelope.  If it shows up repeatedly then its an indication that 
something is amiss.

My front line for diagnosing battery problem out in the field is a small 
script that logs various bits of info from the battery system every 10 
seconds.  If its run on a charge/recharge cycle then it gives a pretty 
good picture of whats going on with the battery.

I would prefer that the stuff returned from sysfs is unique, short yet 
descriptive.  It helps if I don't have to grovel around in multiple 
locations and do boolean logic to come up with what state of the 
charging profile is active.

That makes it easy to describe how to use the info in the diagnostic 
documentation we give to deployments and keep my simple script simple.

It also aids in my processing programs that loop over my (growing) 
collection of log files looking for trends and other statistical items.

-- 
Richard Smith  <richard@laptop.org>
One Laptop Per Child

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Mark Brown]

On Tue, Jun 23, 2009 at 03:28:00PM -0400, Richard A. Smith wrote:
> Mark Brown wrote:

>> I'd also be tempted to do this the other way around and add a fast
>> charge status; certainly in embedded cases trickle charging is more of a
>> default state since it requires less incoming power (important if you're
>> using USB) and there's less risk of damage to the battery.

> On the OLPC the trickle charging really is a trickle rather than just a
> slower charge rate.  To fully charge a XO battery at trickle would take
> about 150 hours.

That's exactly the same in the more embedded case, the only difference
is that the batteries being charged are bigger in your case.  It still
has a major impact on overall charge times that makes people unhappy,
it's just easier to get a situation where you need to stay in trickle
charge due to other demands on the system.

> Trickle is enabled when the battery voltage has dropped outside of the
> normal charging envelope.  In that zone the battery voltage curve is
> very non-linear so even small currents will make the voltage rise
> quickly.  Once the voltage hits the safe zone regular the normal
> charging rate is enabled.

Indeed; the other thing is that when the battery is very drained the
trickle charge does less damage than a fast charge would.

> I don't have any strong feelings about what nomenclature we use to
> describe it.  I just want it to be a separate identifiable state (or
> status).  In (OLPCs) normal charging/discharging cycle the trickle zone  
> should not be reached so its an indication that you are outside the  
> envelope.  If it shows up repeatedly then its an indication that  
> something is amiss.

As far as the naming goes I really would rather see "Trickle charging"
and "Fast charging" as extra states.  Charging could still be used for
when the driver doesn't know or for whatever would be classed as normal.

I looked at this when I submitted the WM8350 PMU driver but decided that
the ABI issues were more trouble than I wanted to deal with at the time
and I'd try to revist it for the next chip.

> I would prefer that the stuff returned from sysfs is unique, short yet  
> descriptive.  It helps if I don't have to grovel around in multiple  
> locations and do boolean logic to come up with what state of the  
> charging profile is active.

The problem here is that anything which is already looking at these
statuses is going to have to be able to cope with the information too.
In the OLPC case it probably doesn't matter so much if they get
confused since this is an unusual case but in the embedded case it's
much more normal (and more likely to happen while a user interacts with
the device since that tends to burn power which is a problem if you're
charging off USB).

A separate file with the detailed information would sidestep this since
it's a new interface.  I'm not strongly opposed to adding the new states
but I do think it's worth considering other ways of doing this.

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Richard A. Smith]

Mark Brown wrote:

> The problem here is that anything which is already looking at these
> statuses is going to have to be able to cope with the information too.
> In the OLPC case it probably doesn't matter so much if they get
> confused since this is an unusual case but in the embedded case it's
> much more normal (and more likely to happen while a user interacts with
> the device since that tends to burn power which is a problem if you're
> charging off USB).

> A separate file with the detailed information would sidestep this since
> it's a new interface.  I'm not strongly opposed to adding the new states
> but I do think it's worth considering other ways of doing this.

I've got no problems with a new interface.  But, i'm also not the person 
writing the driver. :)

I can change my script to test for the presence of this new interface 
and use that if it exists rather easily.  I already have to handle a few 
cases like that where Andres changed things when I wasn't watching.

Suggestions on what it would look like?

-- 
Richard Smith  <richard@laptop.org>
One Laptop Per Child

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Mark Brown]

On Tue, Jun 23, 2009 at 07:13:17PM -0400, Richard A. Smith wrote:
> Mark Brown wrote:

>> A separate file with the detailed information would sidestep this since
>> it's a new interface.  I'm not strongly opposed to adding the new states
>> but I do think it's worth considering other ways of doing this.

> I've got no problems with a new interface.  But, i'm also not the person  
> writing the driver. :)

It's really no bother from the driver point of view either way, it's the
user space applications that are the concern here - are they going to
cope well with a new state appearing there?

> Suggestions on what it would look like?

Probably another sysfs file in the same directory, I'd imagine.

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Richard A. Smith]

Mark Brown wrote:
> On Tue, Jun 23, 2009 at 07:13:17PM -0400, Richard A. Smith wrote:
>> Mark Brown wrote:
> 
>>> A separate file with the detailed information would sidestep this since
>>> it's a new interface.  I'm not strongly opposed to adding the new states
>>> but I do think it's worth considering other ways of doing this.
> 
>> I've got no problems with a new interface.  But, i'm also not the person  
>> writing the driver. :)
> 
> It's really no bother from the driver point of view either way, it's the
> user space applications that are the concern here - are they going to
> cope well with a new state appearing there?

I can only speak for OLPC's usage.  But my script, the documentation on 
our wiki, and HAL are the only things that I know of that use that info 
and none of them should have any problem with it changing.

-- 
Richard Smith  <richard@laptop.org>
One Laptop Per Child

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Mark Brown]

On Tue, Jun 23, 2009 at 07:46:26PM -0400, Richard A. Smith wrote:
> Mark Brown wrote:

>> It's really no bother from the driver point of view either way, it's the
>> user space applications that are the concern here - are they going to
>> cope well with a new state appearing there?

> I can only speak for OLPC's usage.  But my script, the documentation on  
> our wiki, and HAL are the only things that I know of that use that info  
> and none of them should have any problem with it changing.

The other thing that has a reasonable number of users is the in-kernel
APM emulation - when I last looked quite a few users were using that
still.

I'd expect that either HAL or things that use it would need to know
about the new state to do something sensible with it.  All the widely
deployed users should be using those, I don't recall any of the other
things I found being things that I'd particularly heard of.

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Andres Salomon]

On Tue, 23 Jun 2009 19:13:17 -0400
"Richard A. Smith" <richard@laptop.org> wrote:

> Mark Brown wrote:
> 
> > The problem here is that anything which is already looking at these
> > statuses is going to have to be able to cope with the information
> > too. In the OLPC case it probably doesn't matter so much if they get
> > confused since this is an unusual case but in the embedded case it's
> > much more normal (and more likely to happen while a user interacts
> > with the device since that tends to burn power which is a problem
> > if you're charging off USB).
> 
> > A separate file with the detailed information would sidestep this
> > since it's a new interface.  I'm not strongly opposed to adding the
> > new states but I do think it's worth considering other ways of
> > doing this.
> 
> I've got no problems with a new interface.  But, i'm also not the
> person writing the driver. :)
> 
> I can change my script to test for the presence of this new interface 
> and use that if it exists rather easily.  I already have to handle a
> few cases like that where Andres changed things when I wasn't
> watching.
> 
> Suggestions on what it would look like?
> 

Richard, can you please verify that you're happy w/ the proposed
API in the latest bunch of patches?

Re: [PATCH 3/5] power_supply: add a TRICKLE_CHARGING status, and add it to the olpc driver

[Anton Vorontsov]

On Mon, Jun 22, 2009 at 11:46:07PM -0400, Andres Salomon wrote:
> 
> The hardware has an extra bit that specifies that the battery is trickle
> charging, so when determining if the battery is present/charging the TRICKLE
> bit needs to be checked as well.  Because battery diagnostics might want to
> know whether trickle charging is happening or not, and also because trickle
> charging falls somewhere between charging and not charging (read: Richard got
> mad at me when I tried to set CHARGING when in trickle charge.  He gets so
> angry sometimes), we add a new TRICKLE status to sysfs.

:-)

Technically the patch looks OK. But are we sure that userspace
won't break when it'll see a new charging state?

To be on a safe side, and maybe it'll be a better idea anyway,
we could implement charging_mode property? With at least following
values: "N/A", "Trickle", "Slow", "Fast"?


Thanks!

> Signed-off-by: Andres Salomon <dilinger@collabora.co.uk>
> ---
>  drivers/power/olpc_battery.c       |   13 ++++++++++---
>  drivers/power/power_supply_sysfs.c |    3 ++-
>  include/linux/power_supply.h       |    1 +
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> index f8d2d6b..3ecf484 100644
> --- a/drivers/power/olpc_battery.c
> +++ b/drivers/power/olpc_battery.c
> @@ -35,6 +35,7 @@
>  #define BAT_STAT_AC		0x10
>  #define BAT_STAT_CHARGING	0x20
>  #define BAT_STAT_DISCHARGING	0x40
> +#define BAT_STAT_TRICKLE	0x80
>  
>  #define BAT_ERR_INFOFAIL	0x02
>  #define BAT_ERR_OVERVOLTAGE	0x04
> @@ -89,7 +90,12 @@ static char bat_serial[17]; /* Ick */
>  static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
>  {
>  	if (olpc_platform_info.ecver > 0x44) {
> -		if (ec_byte & BAT_STAT_CHARGING)
> +		if (ec_byte & BAT_STAT_TRICKLE)
> +			/* Note that the order here is important; the EC sets
> +			 * both the CHARGING and TRICKLE bits at the same time
> +			 * to support older kernels. */
> +			val->intval = POWER_SUPPLY_STATUS_TRICKLE_CHARGING;
> +		else if (ec_byte & BAT_STAT_CHARGING)
>  			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>  		else if (ec_byte & BAT_STAT_DISCHARGING)
>  			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> @@ -219,7 +225,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  	   It doesn't matter though -- the EC will return the last-known
>  	   information, and it's as if we just ran that _little_ bit faster
>  	   and managed to read it out before the battery went away. */
> -	if (!(ec_byte & BAT_STAT_PRESENT) && psp != POWER_SUPPLY_PROP_PRESENT)
> +	if (!(ec_byte & (BAT_STAT_PRESENT|BAT_STAT_TRICKLE)) &&
> +			psp != POWER_SUPPLY_PROP_PRESENT)
>  		return -ENODEV;
>  
>  	switch (psp) {
> @@ -229,7 +236,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  			return ret;
>  		break;
>  	case POWER_SUPPLY_PROP_PRESENT:
> -		val->intval = !!(ec_byte & BAT_STAT_PRESENT);
> +		val->intval = !!(ec_byte & (BAT_STAT_PRESENT|BAT_STAT_TRICKLE));
>  		break;
>  
>  	case POWER_SUPPLY_PROP_HEALTH:
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index add5f39..eb1affc 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -41,7 +41,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
>  	static char *status_text[] = {
> -		"Unknown", "Charging", "Discharging", "Not charging", "Full"
> +		"Unknown", "Charging", "Charging (trickle)", "Discharging", "Not charging",
> +		"Full",
>  	};
>  	static char *health_text[] = {
>  		"Unknown", "Good", "Overheat", "Dead", "Over voltage",
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 1e45cbc..50ee079 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -33,6 +33,7 @@
>  enum {
>  	POWER_SUPPLY_STATUS_UNKNOWN = 0,
>  	POWER_SUPPLY_STATUS_CHARGING,
> +	POWER_SUPPLY_STATUS_TRICKLE_CHARGING,
>  	POWER_SUPPLY_STATUS_DISCHARGING,
>  	POWER_SUPPLY_STATUS_NOT_CHARGING,
>  	POWER_SUPPLY_STATUS_FULL,
> -- 
> 1.5.6.5
> 

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2