power_supply: support CHARGE_NOW in OLPC battery

PATCH In review Andres Salomon 4 participants 12 messages May 8, 2008 – May 18, 2008

Subsystems (2)

Suspend to RAM Power supply

lore

Share & tools

lore

Andres Salomon

author

2008-05-08 04:31 UTC #0 lore raw

PATCH 1/1 power_supply: support CHARGE_NOW in OLPC battery

This is originally based on a patch by
David Woodhouse <[email protected]>. Add support for PROP_CHARGE_NOW by querying the ACR (accumulated current register) from the EC.

Signed-off-by Andres Salomon <[email protected]>

---
 drivers/power/olpc_battery.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

drivers/power/olpc_battery.c +9 -1

index e3f6ec8..517dacd 100644
@@ -19,7 +19,7 @@
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
-#define EC_BAT_ACR	0x12
+#define EC_BAT_ACR	0x12	/* int16_t,	*416.667,    µAh  */
 #define EC_BAT_TEMP	0x13	/* uint16_t,	*100/256,   °C  */
 #define EC_AMB_TEMP	0x14	/* uint16_t,	*100/256,   °C  */
 #define EC_BAT_STATUS	0x15	/* uint8_t,	bitmask */

@@ -289,6 +289,13 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		ec_word = be16_to_cpu(ec_word);
 		val->intval = ec_word * 100 / 256;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
+		if (ret)
+			return ret;
+
+		val->intval = be16_to_cpu(ec_word);
+		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
 		if (ret)

@@ -317,6 +324,7 @@ static enum power_supply_property olpc_bat_props[] = {
 	POWER_SUPPLY_PROP_TEMP_AMBIENT,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
 };
 
 /* EEPROM reading goes completely around the power_supply API, sadly */

Signature · 1 line

1.5.5.1

Anton Vorontsov

2008-05-08 10:58 UTC #1 lore raw

Quoted 7 lines · Andres Salomon

This is originally based on a patch by
David Woodhouse <[email protected]>. Add support for PROP_CHARGE_NOW by querying the ACR (accumulated current register) from the EC.
Signed-off-by Andres Salomon <[email protected]>
---

Hi Andres,

drivers/power/olpc_battery.c line 22

Quoted 13 lines

 drivers/power/olpc_battery.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

drivers/power/olpc_battery.c +1 -1

index e3f6ec8..517dacd 100644
@@ -19,7 +19,7 @@
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
-#define EC_BAT_ACR	0x12
+#define EC_BAT_ACR	0x12	/* int16_t,	*416.667,    µAh  */

The hardware reports uAh * 416.667..?

drivers/power/olpc_battery.c static int olpc_bat_get_property(struct power_supply *psy, · line 297

Quoted 13 lines

drivers/power/olpc_battery.c +6 -0

 #define EC_BAT_TEMP	0x13	/* uint16_t,	*100/256,   °C  */
 #define EC_AMB_TEMP	0x14	/* uint16_t,	*100/256,   °C  */
 #define EC_BAT_STATUS	0x15	/* uint8_t,	bitmask */

@@ -289,6 +289,13 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		ec_word = be16_to_cpu(ec_word);
 		val->intval = ec_word * 100 / 256;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
+		if (ret)
+			return ret;
+
+		val->intval = be16_to_cpu(ec_word);

But you didn't convert it to the uAh, I think you should.

Quoted 12 lines

drivers/power/olpc_battery.c +2 -0

+		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
 		if (ret)

@@ -317,6 +324,7 @@ static enum power_supply_property olpc_bat_props[] = {
 	POWER_SUPPLY_PROP_TEMP_AMBIENT,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
 };
 
 /* EEPROM reading goes completely around the power_supply API, sadly */

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Andres Salomon

author

2008-05-08 16:57 UTC #2 lore raw

Ok, CC'ing Richard as I should've done in the first place...

Quoted 21 lines · On Thu, 8 May 2008 14:51:45 +0400 Anton Vorontsov <[email protected]>…

Quoted 8 lines · Andres Salomon
This is originally based on a patch by
David Woodhouse <[email protected]>. Add support for PROP_CHARGE_NOW by querying the ACR (accumulated current register) from the EC.
Signed-off-by Andres Salomon <[email protected]>
---
Hi Andres,
Quoted 9 lines
 drivers/power/olpc_battery.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
(quoted diff) +0 -0
b/drivers/power/olpc_battery.c index e3f6ec8..517dacd 100644
drivers/power/olpc_battery.c +0 -0
@@ -19,7 +19,7 @@
 

Quoted 7 lines

Quoted 5 lines
drivers/power/olpc_battery.c +0 -0
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,
 *9.76/32,    mV   */ #define EC_BAT_CURRENT	0x11	/*
int16_t, *15.625/120, mA */ -#define EC_BAT_ACR 0x12
+#define EC_BAT_ACR 0x12 /* int16_t,
*416.667, µAh */
The hardware reports uAh * 416.667..?

Perhaps. Richard informs me that this might be incorrect. Richard, would you care to clarify exactly what the ACR register value contains? It sounds like we can't get uAh without taking two reads over time and computing them.

Bonus points if one of us remembers to update
http://wiki.laptop.org/go/Ec_specification with the information. :)

The code snippet above is mangled due to wrapping. It was originally:

-#define EC_BAT_ACR 0x12
+#define EC_BAT_ACR 0x12 /* int16_t, *416.667, µAh */

Quoted 17 lines

Quoted 11 lines
#define EC_BAT_TEMP 0x13 /* uint16_t,
*100/256, °C */ #define EC_AMB_TEMP 0x14 /*
uint16_t, *100/256, °C */ #define EC_BAT_STATUS
0x15 /* uint8_t, bitmask */ @@ -289,6 +289,13 @@
static int olpc_bat_get_property(struct power_supply *psy, ec_word = be16_to_cpu(ec_word); val->intval = ec_word * 100 / 256;
drivers/power/olpc_battery.c +2 -0
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void
*)&ec_word, 2);
drivers/power/olpc_battery.c static int olpc_bat_get_property(struct power_supply *psy, · line 297
Quoted 4 lines
drivers/power/olpc_battery.c +4 -0
+		if (ret)
+			return ret;
+
+		val->intval = be16_to_cpu(ec_word);
But you didn't convert it to the uAh, I think you should.

We have logic in userspace that converts ACR to a useful number; for our purposes, we'd just like to read the raw ACR values.

Perhaps CHARGE_NOW is the wrong thing to be using, if we can't get uAh and CHARGE_* requires that unit.

Quoted 15 lines

Quoted 14 lines

drivers/power/olpc_battery.c +1 -0

+		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void

*)&ser_buf, 8); if (ret)

drivers/power/olpc_battery.c +1 -0

@@ -317,6 +324,7 @@ static enum power_supply_property
 olpc_bat_props[] = { POWER_SUPPLY_PROP_TEMP_AMBIENT,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
 };
 
 /* EEPROM reading goes completely around the power_supply API,

sadly */

Anton Vorontsov

2008-05-08 17:13 UTC #3 lore raw

Quoted 1 line · Andres Salomon

Ok, CC'ing Richard as I should've done in the first place...

[...]

Quoted 11 lines

Quoted 9 lines
drivers/power/olpc_battery.c static int olpc_bat_get_property(struct power_supply *psy, · line 297
Quoted 7 lines
(quoted diff) +2 -0
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void
*)&ec_word, 2);
drivers/power/olpc_battery.c +4 -0
+		if (ret)
+			return ret;
+
+		val->intval = be16_to_cpu(ec_word);
But you didn't convert it to the uAh, I think you should.

Quoted 2 lines

We have logic in userspace that converts ACR to a useful number; for our purposes, we'd just like to read the raw ACR values.

No. The purpose of power supply class is to report values in common units. Not some random value that userspace then should guess how to compute.

Quoted 2 lines

Perhaps CHARGE_NOW is the wrong thing to be using, if we can't get uAh and CHARGE_* requires that unit.

I don't think that converting X * uAh or uAh / Y to uAh is impossible. I'd suggest you to convert the ACR value to uAh using fractions, see ds2760_battery.c. Even if the final value losing precision a bit, it is still useful to do CHARGE_NOW.

Of course, additionally you can create your own private attribute which reports raw value, if this is anyhow useful.

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Richard A. Smith

2008-05-08 18:54 UTC #4 lore raw

Quoted 12 lines · Anton Vorontsov

Quoted 1 line · Andres Salomon
Ok, CC'ing Richard as I should've done in the first place...
[...]
Quoted 9 lines
Quoted 8 lines
drivers/power/olpc_battery.c static int olpc_bat_get_property(struct power_supply *psy, · line 297
Quoted 7 lines
(quoted diff) +2 -0
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void
*)&ec_word, 2);
drivers/power/olpc_battery.c +4 -0
+		if (ret)
+			return ret;
+
+		val->intval = be16_to_cpu(ec_word);
But you didn't convert it to the uAh, I think you should.

Quoted 6 lines

Quoted 2 lines
We have logic in userspace that converts ACR to a useful number; for our purposes, we'd just like to read the raw ACR values.
No. The purpose of power supply class is to report values in common units. Not some random value that userspace then should guess how to compute.

I'm fine with that. I'll have to make my scripts do some detection on what kernel is running and convert or not convert but thats not too hard.

I remember Dave asking me if I wanted it converted and I told him not to bother and just to export the raw value. I would deal with the units. At the time I was just trying to get some simple debugging info. Turns out that ACR is really useful for debugging battery problems and it stuck.

A single ACR reading itself does not tell you anything usefull. Its a signed 16 bit free running coulomb counter that counts up and down based on how much juice goes into or out of the battery. So to extract any meaningful info you need 2 readings separated by some instance in time.
The difference between the 2 tells you the relative energy you have
consumed or added.

The units for the ACR register in the DS2765 are:

6.25 uVh / Rsense
OLPC Rsense = .015 Ohms

The integer math conversion for ACR to mAh that I use in my scripts is:

(ACR2 - ACR1) * 625 / 1500

Quoted 7 lines

Quoted 2 lines
Perhaps CHARGE_NOW is the wrong thing to be using, if we can't get uAh and CHARGE_* requires that unit.
I don't think that converting X * uAh or uAh / Y to uAh is impossible. I'd suggest you to convert the ACR value to uAh using fractions, see ds2760_battery.c. Even if the final value losing precision a bit, it is still useful to do CHARGE_NOW.

Is CHARGE_NOW supposed to be an absolute indication of the level of charge in the battery? If so then thats not ACR. That would be SOC% * battery capacity. The EC uses the relative motion of the ACR to update the SOC%.

Signature · 2 lines

Richard Smith  <[email protected]>
One Laptop Per Child

Anton Vorontsov

2008-05-08 19:36 UTC #5 lore raw

Quoted 29 lines · Richard A. Smith

Quoted 12 lines · Anton Vorontsov
Quoted 1 line · Andres Salomon
Ok, CC'ing Richard as I should've done in the first place...
[...]
Quoted 9 lines
Quoted 8 lines
drivers/power/olpc_battery.c static int olpc_bat_get_property(struct power_supply *psy, · line 297
Quoted 7 lines
(quoted diff) +2 -0
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void
*)&ec_word, 2);
drivers/power/olpc_battery.c +4 -0
+		if (ret)
+			return ret;
+
+		val->intval = be16_to_cpu(ec_word);
But you didn't convert it to the uAh, I think you should.
Quoted 6 lines
Quoted 2 lines
We have logic in userspace that converts ACR to a useful number; for our purposes, we'd just like to read the raw ACR values.
No. The purpose of power supply class is to report values in common units. Not some random value that userspace then should guess how to compute.
I'm fine with that. I'll have to make my scripts do some detection on what kernel is running and convert or not convert but thats not too hard.
I remember Dave asking me if I wanted it converted and I told him not to bother and just to export the raw value. I would deal with the units. At the time I was just trying to get some simple debugging info. Turns out that ACR is really useful for debugging battery problems and it stuck.

Quoted 6 lines

A single ACR reading itself does not tell you anything usefull. Its a signed 16 bit free running coulomb counter that counts up and down based on how much juice goes into or out of the battery. So to extract any meaningful info you need 2 readings separated by some instance in time. The difference between the 2 tells you the relative energy you have consumed or added.

Cool, I believe this is exactly same thing as we have on DS2760. ACR counts up and down. For example, if there was a current of 10 uA for 30 minutes, ACR will show 5 uAh. Additional 30 minutes with current -10 uA will result in 0 uAh in ACR... Right?

Quoted 1 line

The units for the ACR register in the DS2765 are:

Do you have any specs for the ds2765? I'd like to see one, if it isn't restricted matter. google:ds2765 pdf wasn't helpful...

Quoted 6 lines

6.25 uVh / Rsense
OLPC Rsense = .015 Ohms
The integer math conversion for ACR to mAh that I use in my scripts is:
(ACR2 - ACR1) * 625 / 1500

Just curious.. What does the final value mean and how do you use it? ACR is the Ah counter. You're subtracting one from another, and that value will show you.. difference of Ah between T2 and T1..?

You'll get something useful (current) if you divide the value by T2-T1 though... ;-) But I believe OLPC can report current by itself...

Quoted 10 lines

Quoted 7 lines
Quoted 2 lines
Perhaps CHARGE_NOW is the wrong thing to be using, if we can't get uAh and CHARGE_* requires that unit.
I don't think that converting X * uAh or uAh / Y to uAh is impossible. I'd suggest you to convert the ACR value to uAh using fractions, see ds2760_battery.c. Even if the final value losing precision a bit, it is still useful to do CHARGE_NOW.
Is CHARGE_NOW supposed to be an absolute indication of the level of charge in the battery? If so then thats not ACR.

This depends. On DS2760 ACR is the only way to determine battery's "charge" at the given time (thus the capacity in percents, since ds2760 can't report it).

It needs calibration though... ds2760_battery.c doing this: resetting the ACR to CHARGE_FULL value when battery is in charging state with current flow < 10 mA.

Works quite good here.

Quoted 3 lines

That would be SOC% *
battery capacity. The EC uses the relative motion of the ACR to update the SOC%.

Since you don't seem to use ACR as CHARGE_NOW, I think maybe we indeed should implement PROP_ACR or better PROP_CHARGE_COUNTER? With units of uAh... would that work?

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Richard A. Smith

2008-05-08 20:44 UTC #6 lore raw

Quoted 4 lines · Anton Vorontsov

Cool, I believe this is exactly same thing as we have on DS2760. ACR counts up and down. For example, if there was a current of 10 uA for 30 minutes, ACR will show 5 uAh. Additional 30 minutes with current -10 uA will result in 0 uAh in ACR... Right?

Yes. Exactly correct.

Quoted 5 lines

Quoted 1 line
The units for the ACR register in the DS2765 are:
Do you have any specs for the ds2765? I'd like to see one, if it isn't restricted matter. google:ds2765 pdf wasn't helpful...

Ooop. Sorry my typo. DS2756.

http://wiki.laptop.org/images/e/e9/DS2756.pdf
or
http://www.maxim-ic.com/quick_view2.cfm/qv_pk/5104

Quoted 5 lines

Quoted 5 lines
6.25 uVh / Rsense
OLPC Rsense = .015 Ohms
The integer math conversion for ACR to mAh that I use in my scripts is:

Quoted 8 lines

Quoted 1 line
(ACR2 - ACR1) * 625 / 1500
Just curious.. What does the final value mean and how do you use it? ACR is the Ah counter. You're subtracting one from another, and that value will show you.. difference of Ah between T2 and T1..?
You'll get something useful (current) if you divide the value by T2-T1 though... ;-) But I believe OLPC can report current by itself...

There is no final value. Only relative values. The EC can detect the ends of the spectrum. Battery empty and battery full. At those endpoints it sets the SOC% to x% (exact x varies depending on the battery type) But lets use full and 90% as the example.

So you charge the battery until the criteria for a full battery are met and then you set the SOC% to 90%. This value is stored in the EEPROM of the DS2756. So then when the EC senses that the battery is discharging it monitors the relative ACR change. It uses that to compute how much % loss and updates the SOC%. If you are partially discharged and Ext power is provided so that the battery begins to charge then the reverse happens. The absolute value of the ACR is never used. Only the difference. The corrections of any errors happen when the full or empty conditions are met.

Quoted 5 lines

Quoted 1 line
Is CHARGE_NOW supposed to be an absolute indication of the level of charge in the battery? If so then thats not ACR.
This depends. On DS2760 ACR is the only way to determine battery's "charge" at the given time (thus the capacity in percents, since ds2760 can't report it).

Ours is the same way. The EC just does the extra work to keep an SOC% value.

Quoted 5 lines

It needs calibration though... ds2760_battery.c doing this: resetting the ACR to CHARGE_FULL value when battery is in charging state with current flow < 10 mA.
Works quite good here.

Ah.. I see now. You are using the ACR directly as the SOC value. Whereas OLPC does not.

If your battery capacity stays reasonably constant across the operating temp range then I see how that would work fine. With NiMh chemistry the capacity changes quite a bit wrt temperature. Li chemistries capacity are much less temp sensitive.

I can't say for certain because I didn't do the original code design but from what I see in the EC code it tries to compensate for that capacity change when using NiMh. So they record the net ACR change and use that to try and compute the real SOC% change. I suppose you could still make it work with just the ACR register but thats not how Quanta chose to do the implementation.

Quoted 5 lines

Quoted 1 line
That would be SOC% * battery capacity. The EC uses the relative motion of the ACR to update the SOC%.
Since you don't seem to use ACR as CHARGE_NOW, I think maybe we indeed should implement PROP_ACR or better PROP_CHARGE_COUNTER? With units of uAh... would that work?

That would work fine.

Signature · 2 lines

Richard Smith  <[email protected]>
One Laptop Per Child

Andres Salomon

author

2008-05-13 01:42 UTC #7 lore raw

PATCH 1/1 power_supply: add CHARGE_COUNTER property and olpc_battery support for it

Quoted 15 lines · On Thu, 08 May 2008 16:44:43 -0400 "Richard A. Smith" <[email protected]>…

Quoted 4 lines · Anton Vorontsov
Cool, I believe this is exactly same thing as we have on DS2760. ACR counts up and down. For example, if there was a current of 10 uA for 30 minutes, ACR will show 5 uAh. Additional 30 minutes with current -10 uA will result in 0 uAh in ACR... Right?
Yes. Exactly correct.
Quoted 5 lines
Quoted 1 line
The units for the ACR register in the DS2765 are:
Do you have any specs for the ds2765? I'd like to see one, if it isn't restricted matter. google:ds2765 pdf wasn't helpful...

Quoted 6 lines

Ooop. Sorry my typo. DS2756.
http://wiki.laptop.org/images/e/e9/DS2756.pdf
or
http://www.maxim-ic.com/quick_view2.cfm/qv_pk/5104

Wow, having the data sheet makes things so much clearer. :)

[...]

Quoted 11 lines

Quoted 8 lines
Quoted 4 lines
That would be SOC% *
battery capacity. The EC uses the relative motion of the ACR to update the SOC%.
Since you don't seem to use ACR as CHARGE_NOW, I think maybe we indeed should implement PROP_ACR or better PROP_CHARGE_COUNTER? With units of uAh... would that work?
That would work fine.

Alright folks, how does this look?

From: Andres Salomon <[email protected]>

This adds PROP_CHARGE_COUNTER to the power supply class (documenting it as well). The OLPC battery driver uses this for spitting out its ACR values (in uAh). We have some rounding errors (the data sheet claims 416.7, the math actually works out to 416.666667, so we're forced to choose between overflows or precision loss. I chose precision loss, and stuck w/ data sheet values), but I don't think anyone will care that much.

Signed-off-by Andres Salomon <[email protected]>

---
 Documentation/power/power_supply_class.txt |    4 ++++
 drivers/power/olpc_battery.c               |   11 ++++++++++-
 drivers/power/power_supply_sysfs.c         |    1 +
 include/linux/power_supply.h               |    1 +
 4 files changed, 16 insertions(+), 1 deletions(-)

Documentation/power/power_supply_class.txt +4 -0

index a8686e5..c6cd495 100644
@@ -101,6 +101,10 @@ of charge when battery became full/empty". It also could mean "value of
 charge when battery considered full/empty at given conditions (temperature,
 age)". I.e. these attributes represents real thresholds, not design values.
 
+CHARGE_COUNTER - the current charge counter (in µAh).  This could easily
+be negative; there is no empty or full value.  It is only useful for
+relative, time-based measurements.
+
 ENERGY_FULL, ENERGY_EMPTY - same as above but for energy.
 
 CAPACITY - capacity in percents.

drivers/power/olpc_battery.c +10 -1

index e3f6ec8..a928165 100644
@@ -19,7 +19,7 @@
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
-#define EC_BAT_ACR	0x12
+#define EC_BAT_ACR	0x12	/* int16_t,	*416.7,      µAh  */
 #define EC_BAT_TEMP	0x13	/* uint16_t,	*100/256,   °C  */
 #define EC_AMB_TEMP	0x14	/* uint16_t,	*100/256,   °C  */
 #define EC_BAT_STATUS	0x15	/* uint8_t,	bitmask */

@@ -289,6 +289,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		ec_word = be16_to_cpu(ec_word);
 		val->intval = ec_word * 100 / 256;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
+		if (ret)
+			return ret;
+
+		ec_word = be16_to_cpu(ec_word);
+		val->intval = ec_word * 4167 / 10;
+		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
 		if (ret)

@@ -317,6 +325,7 @@ static enum power_supply_property olpc_bat_props[] = {
 	POWER_SUPPLY_PROP_TEMP_AMBIENT,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 };
 
 /* EEPROM reading goes completely around the power_supply API, sadly */

drivers/power/power_supply_sysfs.c +1 -0

index c444d6b..82e1246 100644
@@ -99,6 +99,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(charge_empty),
 	POWER_SUPPLY_ATTR(charge_now),
 	POWER_SUPPLY_ATTR(charge_avg),
+	POWER_SUPPLY_ATTR(charge_counter),
 	POWER_SUPPLY_ATTR(energy_full_design),
 	POWER_SUPPLY_ATTR(energy_empty_design),
 	POWER_SUPPLY_ATTR(energy_full),

include/linux/power_supply.h +1 -0

index 68ed19c..ea96ead 100644
@@ -78,6 +78,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_CHARGE_EMPTY,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CHARGE_AVG,
+	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,

Signature · 1 line

1.5.5.1

Anton Vorontsov

Applied

2008-05-13 08:42 UTC #8 lore raw

Re: [PATCH] power_supply: add CHARGE_COUNTER property and olpc_battery support for it

On Mon, May 12, 2008 at 09:46:29PM -0400, Andres Salomon wrote:
[...]

Quoted 9 lines

From: Andres Salomon <[email protected]>
This adds PROP_CHARGE_COUNTER to the power supply class (documenting it as well). The OLPC battery driver uses this for spitting out its ACR values (in uAh). We have some rounding errors (the data sheet claims 416.7, the math actually works out to 416.666667, so we're forced to choose between overflows or precision loss. I chose precision loss, and stuck w/ data sheet values), but I don't think anyone will care that much.

Applied to battery-2.6.git, thanks.

And another thanks for keeping documentation in sync. :-)

Quoted 86 lines

Signed-off-by Andres Salomon <[email protected]>

---
 Documentation/power/power_supply_class.txt |    4 ++++
 drivers/power/olpc_battery.c               |   11 ++++++++++-
 drivers/power/power_supply_sysfs.c         |    1 +
 include/linux/power_supply.h               |    1 +
 4 files changed, 16 insertions(+), 1 deletions(-)

Documentation/power/power_supply_class.txt +4 -0

index a8686e5..c6cd495 100644
@@ -101,6 +101,10 @@ of charge when battery became full/empty". It also could mean "value of
 charge when battery considered full/empty at given conditions (temperature,
 age)". I.e. these attributes represents real thresholds, not design values.
 
+CHARGE_COUNTER - the current charge counter (in µAh).  This could easily
+be negative; there is no empty or full value.  It is only useful for
+relative, time-based measurements.
+
 ENERGY_FULL, ENERGY_EMPTY - same as above but for energy.
 
 CAPACITY - capacity in percents.

drivers/power/olpc_battery.c +10 -1

index e3f6ec8..a928165 100644
@@ -19,7 +19,7 @@
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
-#define EC_BAT_ACR	0x12
+#define EC_BAT_ACR	0x12	/* int16_t,	*416.7,      µAh  */
 #define EC_BAT_TEMP	0x13	/* uint16_t,	*100/256,   °C  */
 #define EC_AMB_TEMP	0x14	/* uint16_t,	*100/256,   °C  */
 #define EC_BAT_STATUS	0x15	/* uint8_t,	bitmask */

@@ -289,6 +289,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		ec_word = be16_to_cpu(ec_word);
 		val->intval = ec_word * 100 / 256;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
+		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
+		if (ret)
+			return ret;
+
+		ec_word = be16_to_cpu(ec_word);
+		val->intval = ec_word * 4167 / 10;
+		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
 		if (ret)

@@ -317,6 +325,7 @@ static enum power_supply_property olpc_bat_props[] = {
 	POWER_SUPPLY_PROP_TEMP_AMBIENT,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
+	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 };
 
 /* EEPROM reading goes completely around the power_supply API, sadly */

drivers/power/power_supply_sysfs.c +1 -0

index c444d6b..82e1246 100644
@@ -99,6 +99,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(charge_empty),
 	POWER_SUPPLY_ATTR(charge_now),
 	POWER_SUPPLY_ATTR(charge_avg),
+	POWER_SUPPLY_ATTR(charge_counter),
 	POWER_SUPPLY_ATTR(energy_full_design),
 	POWER_SUPPLY_ATTR(energy_empty_design),
 	POWER_SUPPLY_ATTR(energy_full),

include/linux/power_supply.h +1 -0

index 68ed19c..ea96ead 100644
@@ -78,6 +78,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_CHARGE_EMPTY,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CHARGE_AVG,
+	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,

Signature · 1 line

1.5.5.1

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Richard A. Smith

2008-05-13 14:20 UTC #9 lore raw

Re: [PATCH] power_supply: add CHARGE_COUNTER property and olpc_battery support for it

In reply to Andres Salomon

drivers/power/olpc_battery.c static int olpc_bat_get_property(struct power_supply *psy, · line 298

Quoted 2 lines · Andres Salomon

drivers/power/olpc_battery.c +2 -0

+		ec_word = be16_to_cpu(ec_word);
+		val->intval = ec_word * 4167 / 10;

Whats wrong with:

Review

Quoted 1 line

(quoted diff) +1 -0

+		val->intval = ((int)ec_word) * 6250 / 15;

Which does not have overflow problems and keeps more precision.

Signature · 2 lines

Richard Smith  <[email protected]>
One Laptop Per Child

Andres Salomon

author

2008-05-13 16:19 UTC #10 lore raw

Re: [PATCH] power_supply: add CHARGE_COUNTER property and olpc_battery support for it

Quoted 11 lines · On Tue, 13 May 2008 10:20:15 -0400 "Richard A. Smith" <[email protected]>…

Review
Quoted 2 lines · Andres Salomon
(quoted diff) +2 -0
+		ec_word = be16_to_cpu(ec_word);
+		val->intval = ec_word * 4167 / 10;
Whats wrong with:
Review
Quoted 1 line
(quoted diff) +1 -0
+		val->intval = ((int)ec_word) * 6250 / 15;
Which does not have overflow problems and keeps more precision.

You're right, of course. Anton, can you please apply the following?

Subject: [PATCH] power_supply: fix up CHARGE_COUNTER output to be more precise

As Richard Smith pointed out, ACR * 6250 / 15 provides for less precision loss than ACR * 4167 / 10, _and_ it doesn't overflow. Switch to using that equation for CHARGE_COUNTER.

Signed-off-by Andres Salomon <[email protected]>

---
 drivers/power/olpc_battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/power/olpc_battery.c +2 -2

index a928165..f5d712a 100644
@@ -19,7 +19,7 @@
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
-#define EC_BAT_ACR	0x12	/* int16_t,	*416.7,      µAh  */
+#define EC_BAT_ACR	0x12	/* int16_t,	*6250/15,    µAh  */
 #define EC_BAT_TEMP	0x13	/* uint16_t,	*100/256,   °C  */
 #define EC_AMB_TEMP	0x14	/* uint16_t,	*100/256,   °C  */
 #define EC_BAT_STATUS	0x15	/* uint8_t,	bitmask */

@@ -295,7 +295,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 			return ret;
 
 		ec_word = be16_to_cpu(ec_word);
-		val->intval = ec_word * 4167 / 10;
+		val->intval = ec_word * 6250 / 15;
 		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);

Signature · 1 line

1.5.5.1

Anton Vorontsov

2008-05-18 21:47 UTC #11 lore raw

Re: [PATCH] power_supply: add CHARGE_COUNTER property and olpc_battery support for it

Quoted 10 lines · Andres Salomon

Quoted 7 lines · On Tue, 13 May 2008 10:20:15 -0400 "Richard A. Smith" <[email protected]>…
Review
Quoted 2 lines · Andres Salomon
(quoted diff) +2 -0
+		ec_word = be16_to_cpu(ec_word);
+		val->intval = ec_word * 4167 / 10;
Whats wrong with:

Quoted 6 lines

Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+		val->intval = ((int)ec_word) * 6250 / 15;
Which does not have overflow problems and keeps more precision.
You're right, of course. Anton, can you please apply the following?

Sorry for the delay... Applied, thanks!

Quoted 35 lines

Subject: [PATCH] power_supply: fix up CHARGE_COUNTER output to be more precise

As Richard Smith pointed out, ACR * 6250 / 15 provides for less precision loss than ACR * 4167 / 10, _and_ it doesn't overflow. Switch to using that equation for CHARGE_COUNTER.

Signed-off-by Andres Salomon <[email protected]>

---
 drivers/power/olpc_battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/power/olpc_battery.c +2 -2

index a928165..f5d712a 100644
@@ -19,7 +19,7 @@
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
-#define EC_BAT_ACR	0x12	/* int16_t,	*416.7,      µAh  */
+#define EC_BAT_ACR	0x12	/* int16_t,	*6250/15,    µAh  */
 #define EC_BAT_TEMP	0x13	/* uint16_t,	*100/256,   °C  */
 #define EC_AMB_TEMP	0x14	/* uint16_t,	*100/256,   °C  */
 #define EC_BAT_STATUS	0x15	/* uint8_t,	bitmask */

@@ -295,7 +295,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 			return ret;
 
 		ec_word = be16_to_cpu(ec_word);
-		val->intval = ec_word * 4167 / 10;
+		val->intval = ec_word * 6250 / 15;
 		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);

Signature · 1 line

1.5.5.1

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2