[PATCH] power_supply: support CHARGE_NOW in OLPC battery

[PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Andres Salomon]

This is originally based on a patch by
David Woodhouse <dwmw2@infradead.org>.  Add support for PROP_CHARGE_NOW
by querying the ACR (accumulated current register) from the EC.

Signed-off-by: Andres Salomon <dilinger@debian.org>
---
 drivers/power/olpc_battery.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index e3f6ec8..517dacd 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -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 */
-- 
1.5.5.1

Re: [PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Anton Vorontsov]

On Thu, May 08, 2008 at 12:34:54AM -0400, Andres Salomon wrote:
> 
> This is originally based on a patch by
> David Woodhouse <dwmw2@infradead.org>.  Add support for PROP_CHARGE_NOW
> by querying the ACR (accumulated current register) from the EC.
> 
> Signed-off-by: Andres Salomon <dilinger@debian.org>
> ---

Hi Andres,

>  drivers/power/olpc_battery.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> index e3f6ec8..517dacd 100644
> --- a/drivers/power/olpc_battery.c
> +++ b/drivers/power/olpc_battery.c
> @@ -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..?

>  #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.

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

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

Re: [PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Andres Salomon]

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



On Thu, 8 May 2008 14:51:45 +0400
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Thu, May 08, 2008 at 12:34:54AM -0400, Andres Salomon wrote:
> > 
> > This is originally based on a patch by
> > David Woodhouse <dwmw2@infradead.org>.  Add support for
> > PROP_CHARGE_NOW by querying the ACR (accumulated current register)
> > from the EC.
> > 
> > Signed-off-by: Andres Salomon <dilinger@debian.org>
> > ---
> 
> Hi Andres,
> 
> >  drivers/power/olpc_battery.c |   10 +++++++++-
> >  1 files changed, 9 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/power/olpc_battery.c
> > b/drivers/power/olpc_battery.c index e3f6ec8..517dacd 100644
> > --- a/drivers/power/olpc_battery.c
> > +++ b/drivers/power/olpc_battery.c
> > @@ -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..?

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  */



> 
> >  #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.


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.


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

Re: [PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Anton Vorontsov]

On Thu, May 08, 2008 at 01:01:02PM -0400, Andres Salomon wrote:
> Ok, CC'ing Richard as I should've done in the first place...
[...]
> > > +	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.
> 
> 
> 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.

> 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.

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

Re: [PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Richard A. Smith]

Anton Vorontsov wrote:
> On Thu, May 08, 2008 at 01:01:02PM -0400, Andres Salomon wrote:
>> Ok, CC'ing Richard as I should've done in the first place...
> [...]
>>>> +	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.
>>
>> 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

>> 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%.

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

Re: [PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Anton Vorontsov]

On Thu, May 08, 2008 at 02:53:25PM -0400, Richard A. Smith wrote:
> Anton Vorontsov wrote:
>> On Thu, May 08, 2008 at 01:01:02PM -0400, Andres Salomon wrote:
>>> Ok, CC'ing Richard as I should've done in the first place...
>> [...]
>>>>> +	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.
>>>
>>> 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.

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?

> 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...

> 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...

>>> 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.

> 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?

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

Re: [PATCH] power_supply: support CHARGE_NOW in OLPC battery

[Richard A. Smith]

Anton Vorontsov wrote:

> 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.

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

>> 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...

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.

>> 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.

> 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.

>> 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.

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

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

[Andres Salomon]

On Thu, 08 May 2008 16:44:43 -0400
"Richard A. Smith" <richard@laptop.org> wrote:

> Anton Vorontsov wrote:
> 
> > 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.
> 
> >> 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
> 

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


[...]
> 
> >> 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 <dilinger@debian.org>

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 <dilinger@debian.org>
---
 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(-)

diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
index a8686e5..c6cd495 100644
--- a/Documentation/power/power_supply_class.txt
+++ b/Documentation/power/power_supply_class.txt
@@ -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.
diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index e3f6ec8..a928165 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -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 */
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index c444d6b..82e1246 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -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),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 68ed19c..ea96ead 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -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,
-- 
1.5.5.1

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

[Anton Vorontsov]

On Mon, May 12, 2008 at 09:46:29PM -0400, Andres Salomon wrote:
[...]
> From: Andres Salomon <dilinger@debian.org>
> 
> 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. :-)

> Signed-off-by: Andres Salomon <dilinger@debian.org>
> ---
>  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(-)
> 
> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> index a8686e5..c6cd495 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -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.
> diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> index e3f6ec8..a928165 100644
> --- a/drivers/power/olpc_battery.c
> +++ b/drivers/power/olpc_battery.c
> @@ -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 */
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index c444d6b..82e1246 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -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),
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 68ed19c..ea96ead 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -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,
> -- 
> 1.5.5.1
> 

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

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

[Richard A. Smith]

Andres Salomon wrote:

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

Whats wrong with:

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

Which does not have overflow problems and keeps more precision.

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

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

[Andres Salomon]

On Tue, 13 May 2008 10:20:15 -0400
"Richard A. Smith" <richard@laptop.org> wrote:

> Andres Salomon wrote:
> 
> > +		ec_word = be16_to_cpu(ec_word);
> > +		val->intval = ec_word * 4167 / 10;
> 
> Whats wrong with:
> 
>  > +		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 <dilinger@debian.org>
---
 drivers/power/olpc_battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index a928165..f5d712a 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -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);
-- 
1.5.5.1

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

[Anton Vorontsov]

On Tue, May 13, 2008 at 12:23:30PM -0400, Andres Salomon wrote:
> On Tue, 13 May 2008 10:20:15 -0400
> "Richard A. Smith" <richard@laptop.org> wrote:
> 
> > Andres Salomon wrote:
> > 
> > > +		ec_word = be16_to_cpu(ec_word);
> > > +		val->intval = ec_word * 4167 / 10;
> > 
> > Whats wrong with:
> > 
> >  > +		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!

> 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 <dilinger@debian.org>
> ---
>  drivers/power/olpc_battery.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> index a928165..f5d712a 100644
> --- a/drivers/power/olpc_battery.c
> +++ b/drivers/power/olpc_battery.c
> @@ -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);
> -- 
> 1.5.5.1

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