2009-07-15 16:20:58

by Daniel Mack

[permalink] [raw]
Subject: [PATCH 1/4] ds2760: delay power supply registration

This fixes a race condition I recently introduced with the PMOD feature
addition (cef437e3: "w1: ds2760_battery: add support for sleep mode
feature").

Postpone the call to power_supply_register() to fix it.

Signed-off-by: Daniel Mack <[email protected]>
Cc: Szabolcs Gyurko <[email protected]>
Cc: Matt Reimer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
---
drivers/power/ds2760_battery.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 520b5c4..cf07c43 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -381,12 +381,6 @@ static int ds2760_battery_probe(struct platform_device *pdev)

di->charge_status = POWER_SUPPLY_STATUS_UNKNOWN;

- retval = power_supply_register(&pdev->dev, &di->bat);
- if (retval) {
- dev_err(di->dev, "failed to register battery\n");
- goto batt_failed;
- }
-
/* enable sleep mode feature */
ds2760_battery_read_status(di);
status = di->raw[DS2760_STATUS_REG];
@@ -397,6 +391,12 @@ static int ds2760_battery_probe(struct platform_device *pdev)

ds2760_battery_write_status(di, status);

+ retval = power_supply_register(&pdev->dev, &di->bat);
+ if (retval) {
+ dev_err(di->dev, "failed to register battery\n");
+ goto batt_failed;
+ }
+
INIT_DELAYED_WORK(&di->monitor_work, ds2760_battery_work);
di->monitor_wqueue = create_singlethread_workqueue(dev_name(&pdev->dev));
if (!di->monitor_wqueue) {
--
1.6.3.1


2009-07-15 16:21:03

by Daniel Mack

[permalink] [raw]
Subject: [PATCH 2/4] ds2760: export more features

Export POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW and POWER_SUPPLY_PROP_CAPACITY
features to the power supply core.

Signed-off-by: Daniel Mack <[email protected]>
Cc: Szabolcs Gyurko <[email protected]>
Cc: Matt Reimer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
---
drivers/power/ds2760_battery.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index cf07c43..f439071 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -337,6 +337,12 @@ static int ds2760_battery_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_TEMP:
val->intval = di->temp_C;
break;
+ case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
+ val->intval = di->life_sec;
+ break;
+ case POWER_SUPPLY_PROP_CAPACITY:
+ val->intval = di->rem_capacity;
+ break;
default:
return -EINVAL;
}
@@ -353,6 +359,8 @@ static enum power_supply_property ds2760_battery_props[] = {
POWER_SUPPLY_PROP_CHARGE_EMPTY,
POWER_SUPPLY_PROP_CHARGE_NOW,
POWER_SUPPLY_PROP_TEMP,
+ POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
+ POWER_SUPPLY_PROP_CAPACITY,
};

static int ds2760_battery_probe(struct platform_device *pdev)
--
1.6.3.1

2009-07-15 16:21:17

by Daniel Mack

[permalink] [raw]
Subject: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

In systems where the battery monitor is not part of the battery pack and
is hence not bootstrapped with sane values, the full_active_uAh is
likely to be zero.

Handle that case by defaulting to the rated_capacity information which
can be passed to the driver using the new module parameter.

Signed-off-by: Daniel Mack <[email protected]>
Cc: Szabolcs Gyurko <[email protected]>
Cc: Matt Reimer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
---
drivers/power/ds2760_battery.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index ed0ea5e..2d0e5ed 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -172,8 +172,13 @@ static int ds2760_battery_read_status(struct ds2760_device_info *di)
di->full_active_uAh = di->raw[DS2760_ACTIVE_FULL] << 8 |
di->raw[DS2760_ACTIVE_FULL + 1];

- scale[0] = di->raw[DS2760_ACTIVE_FULL] << 8 |
- di->raw[DS2760_ACTIVE_FULL + 1];
+ /* If the full_active_uAh value is not given, fall back to the rated
+ * capacity. This is likely to happen when chips are not part of the
+ * battery pack and is therefore not bootstrapped. */
+ if (di->full_active_uAh == 0)
+ di->full_active_uAh = di->rated_capacity / 1000L;
+
+ scale[0] = di->full_active_uAh;
for (i = 1; i < 5; i++)
scale[i] = scale[i - 1] + di->raw[DS2760_ACTIVE_FULL + 2 + i];

--
1.6.3.1

2009-07-15 16:21:22

by Daniel Mack

[permalink] [raw]
Subject: [PATCH 3/4] ds2760: add rated_capacity module parameter

For systems where the ds2760 is soldered directly on the PCB, the 'rated
capacity' register is not set to anything useful.

In order to allow users to bootstrap this value, introduce a new module
parameter 'rated_capacity' and use it to write the internal EEPROM in
case the value differes from what's been given.

Signed-off-by: Daniel Mack <[email protected]>
Cc: Szabolcs Gyurko <[email protected]>
Cc: Matt Reimer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
---
drivers/power/ds2760_battery.c | 19 +++++++++++++++++++
1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index f439071..ed0ea5e 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -66,6 +66,10 @@ static unsigned int pmod_enabled;
module_param(pmod_enabled, bool, 0644);
MODULE_PARM_DESC(pmod_enabled, "PMOD enable bit");

+static unsigned int rated_capacity;
+module_param(rated_capacity, uint, 0644);
+MODULE_PARM_DESC(rated_capacity, "rated battery capacity, 10*mAh or index");
+
/* Some batteries have their rated capacity stored a N * 10 mAh, while
* others use an index into this table. */
static int rated_capacities[] = {
@@ -274,6 +278,17 @@ static void ds2760_battery_write_status(struct ds2760_device_info *di,
w1_ds2760_recall_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
}

+static void ds2760_battery_write_rated_capacity(struct ds2760_device_info *di,
+ unsigned char rated_capacity)
+{
+ if (rated_capacity == di->raw[DS2760_RATED_CAPACITY])
+ return;
+
+ w1_ds2760_write(di->w1_dev, &rated_capacity, DS2760_RATED_CAPACITY, 1);
+ w1_ds2760_store_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
+ w1_ds2760_recall_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
+}
+
static void ds2760_battery_work(struct work_struct *work)
{
struct ds2760_device_info *di = container_of(work,
@@ -399,6 +414,10 @@ static int ds2760_battery_probe(struct platform_device *pdev)

ds2760_battery_write_status(di, status);

+ /* set rated capacity from module param */
+ if (rated_capacity)
+ ds2760_battery_write_rated_capacity(di, rated_capacity);
+
retval = power_supply_register(&pdev->dev, &di->bat);
if (retval) {
dev_err(di->dev, "failed to register battery\n");
--
1.6.3.1

2009-07-15 18:13:43

by Matthew Reimer

[permalink] [raw]
Subject: Re: [PATCH 1/4] ds2760: delay power supply registration

On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:

> This fixes a race condition I recently introduced with the PMOD
> feature
> addition (cef437e3: "w1: ds2760_battery: add support for sleep mode
> feature").
>
> Postpone the call to power_supply_register() to fix it.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Szabolcs Gyurko <[email protected]>
> Cc: Matt Reimer <[email protected]>
> Cc: Anton Vorontsov <[email protected]>

Acked-by: Matt Reimer <[email protected]>

Matt

2009-07-15 18:18:44

by Matthew Reimer

[permalink] [raw]
Subject: Re: [PATCH 3/4] ds2760: add rated_capacity module parameter

On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:

> For systems where the ds2760 is soldered directly on the PCB, the
> 'rated
> capacity' register is not set to anything useful.
>
> In order to allow users to bootstrap this value, introduce a new
> module
> parameter 'rated_capacity' and use it to write the internal EEPROM in
> case the value differes from what's been given.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Szabolcs Gyurko <[email protected]>
> Cc: Matt Reimer <[email protected]>
> Cc: Anton Vorontsov <[email protected]>

Acked-by: Matt Reimer <[email protected]>

Matt

2009-07-15 18:18:45

by Matthew Reimer

[permalink] [raw]
Subject: Re: [PATCH 2/4] ds2760: export more features

On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:

> Export POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW and
> POWER_SUPPLY_PROP_CAPACITY
> features to the power supply core.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Szabolcs Gyurko <[email protected]>
> Cc: Matt Reimer <[email protected]>
> Cc: Anton Vorontsov <[email protected]>

Acked-by: Matt Reimer <[email protected]>

Matt

2009-07-15 18:18:56

by Matthew Reimer

[permalink] [raw]
Subject: Re: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:

> In systems where the battery monitor is not part of the battery pack
> and
> is hence not bootstrapped with sane values, the full_active_uAh is
> likely to be zero.
>
> Handle that case by defaulting to the rated_capacity information which
> can be passed to the driver using the new module parameter.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Szabolcs Gyurko <[email protected]>
> Cc: Matt Reimer <[email protected]>
> Cc: Anton Vorontsov <[email protected]>

Acked-by: Matt Reimer <[email protected]>

Matt

2009-07-15 18:52:07

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

On Wed, Jul 15, 2009 at 11:06:29AM -0700, Matt Reimer wrote:
> On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:
>
>> In systems where the battery monitor is not part of the battery pack
>> and
>> is hence not bootstrapped with sane values, the full_active_uAh is
>> likely to be zero.
>>
>> Handle that case by defaulting to the rated_capacity information which
>> can be passed to the driver using the new module parameter.
>>
>> Signed-off-by: Daniel Mack <[email protected]>
>> Cc: Szabolcs Gyurko <[email protected]>
>> Cc: Matt Reimer <[email protected]>
>> Cc: Anton Vorontsov <[email protected]>
>
> Acked-by: Matt Reimer <[email protected]>

Thanks Matt :) Andrew, will they go thru your channel?

Daniel

2009-07-15 19:07:12

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

On Wed, 15 Jul 2009 20:51:59 +0200
Daniel Mack <[email protected]> wrote:

> On Wed, Jul 15, 2009 at 11:06:29AM -0700, Matt Reimer wrote:
> > On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:
> >
> >> In systems where the battery monitor is not part of the battery pack
> >> and
> >> is hence not bootstrapped with sane values, the full_active_uAh is
> >> likely to be zero.
> >>
> >> Handle that case by defaulting to the rated_capacity information which
> >> can be passed to the driver using the new module parameter.
> >>
> >> Signed-off-by: Daniel Mack <[email protected]>
> >> Cc: Szabolcs Gyurko <[email protected]>
> >> Cc: Matt Reimer <[email protected]>
> >> Cc: Anton Vorontsov <[email protected]>
> >
> > Acked-by: Matt Reimer <[email protected]>
>
> Thanks Matt :) Andrew, will they go thru your channel?
>

Well. I'll look at them, as I try to do when someone cc'ed me. But
drivers/power is usually handled by Anton?

2009-07-15 19:28:35

by Anton Vorontsov

[permalink] [raw]
Subject: Re: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

On Wed, Jul 15, 2009 at 12:06:25PM -0700, Andrew Morton wrote:
> On Wed, 15 Jul 2009 20:51:59 +0200
> Daniel Mack <[email protected]> wrote:
>
> > On Wed, Jul 15, 2009 at 11:06:29AM -0700, Matt Reimer wrote:
> > > On Jul 15, 2009, at 9:20 AM, Daniel Mack wrote:
> > >
> > >> In systems where the battery monitor is not part of the battery pack
> > >> and
> > >> is hence not bootstrapped with sane values, the full_active_uAh is
> > >> likely to be zero.
> > >>
> > >> Handle that case by defaulting to the rated_capacity information which
> > >> can be passed to the driver using the new module parameter.
> > >>
> > >> Signed-off-by: Daniel Mack <[email protected]>
> > >> Cc: Szabolcs Gyurko <[email protected]>
> > >> Cc: Matt Reimer <[email protected]>
> > >> Cc: Anton Vorontsov <[email protected]>
> > >
> > > Acked-by: Matt Reimer <[email protected]>
> >
> > Thanks Matt :) Andrew, will they go thru your channel?
> >
>
> Well. I'll look at them, as I try to do when someone cc'ed me. But
> drivers/power is usually handled by Anton?

Yep. All patches look good, I'll apply them to battery-2.6.git.

Thanks,

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

2009-07-15 20:02:52

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

On Wed, Jul 15, 2009 at 11:28:33PM +0400, Anton Vorontsov wrote:
> On Wed, Jul 15, 2009 at 12:06:25PM -0700, Andrew Morton wrote:
> > Well. I'll look at them, as I try to do when someone cc'ed me. But
> > drivers/power is usually handled by Anton?
>
> Yep. All patches look good, I'll apply them to battery-2.6.git.

Thanks. Please consider applying the one below as well if you're fine
with it. We need that parameter for bootstrapping in the factory when
devices are produced.

Thanks,
Daniel

>From 75b680d069596351da674023146f34b1737ea8ab Mon Sep 17 00:00:00 2001
From: Daniel Mack <[email protected]>
Date: Wed, 15 Jul 2009 21:57:29 +0200
Subject: [PATCH] ds2760: add current_accum module parameter

When connecting a ds2760 to a partly loaded battery the first time,
there must be a way to bootstrap the current_accum value. Without that,
the current capactity value is bogus until the battery is fully charged
for the first time.

Signed-off-by: Daniel Mack <[email protected]>
Cc: Szabolcs Gyurko <[email protected]>
Cc: Matt Reimer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
---
drivers/power/ds2760_battery.c | 36 +++++++++++++++++++++++-------------
1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 2d0e5ed..606f074 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -70,6 +70,10 @@ static unsigned int rated_capacity;
module_param(rated_capacity, uint, 0644);
MODULE_PARM_DESC(rated_capacity, "rated battery capacity, 10*mAh or index");

+static unsigned int current_accum;
+module_param(current_accum, uint, 0644);
+MODULE_PARM_DESC(current_accum, "current accumulator value");
+
/* Some batteries have their rated capacity stored a N * 10 mAh, while
* others use an index into this table. */
static int rated_capacities[] = {
@@ -215,6 +219,18 @@ static int ds2760_battery_read_status(struct ds2760_device_info *di)
return 0;
}

+static void ds2760_battery_set_current_accum(struct ds2760_device_info *di,
+ unsigned int acr_val)
+{
+ unsigned char acr[2];
+
+ acr[0] = acr_val >> 8;
+ acr[1] = acr_val & 0xff;
+
+ if (w1_ds2760_write(di->w1_dev, acr, DS2760_CURRENT_ACCUM_MSB, 2) < 2)
+ dev_warn(di->dev, "ACR write failed\n");
+}
+
static void ds2760_battery_update_status(struct ds2760_device_info *di)
{
int old_charge_status = di->charge_status;
@@ -246,20 +262,9 @@ static void ds2760_battery_update_status(struct ds2760_device_info *di)
if (di->full_counter < 2) {
di->charge_status = POWER_SUPPLY_STATUS_CHARGING;
} else {
- unsigned char acr[2];
- int acr_val;
-
/* acr is in units of 0.25 mAh */
- acr_val = di->full_active_uAh * 4L / 1000;
-
- acr[0] = acr_val >> 8;
- acr[1] = acr_val & 0xff;
-
- if (w1_ds2760_write(di->w1_dev, acr,
- DS2760_CURRENT_ACCUM_MSB, 2) < 2)
- dev_warn(di->dev,
- "ACR reset failed\n");
-
+ int acr_val = di->full_active_uAh * 4L / 1000;
+ ds2760_battery_set_current_accum(di, acr_val);
di->charge_status = POWER_SUPPLY_STATUS_FULL;
}
}
@@ -423,6 +428,11 @@ static int ds2760_battery_probe(struct platform_device *pdev)
if (rated_capacity)
ds2760_battery_write_rated_capacity(di, rated_capacity);

+ /* set current accumulator if given as parameter.
+ * this should only be done for bootstrapping the value */
+ if (current_accum)
+ ds2760_battery_set_current_accum(di, current_accum);
+
retval = power_supply_register(&pdev->dev, &di->bat);
if (retval) {
dev_err(di->dev, "failed to register battery\n");
--
1.6.3.1

2009-07-15 20:57:22

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH 4/4] ds2760: handle full_active_uAh == 0 case correctly

On Wed, Jul 15, 2009 at 10:02:46PM +0200, Daniel Mack wrote:
> On Wed, Jul 15, 2009 at 11:28:33PM +0400, Anton Vorontsov wrote:
> > Yep. All patches look good, I'll apply them to battery-2.6.git.
>
> Thanks. Please consider applying the one below as well if you're fine
> with it. We need that parameter for bootstrapping in the factory when
> devices are produced.

> When connecting a ds2760 to a partly loaded battery the first time,
> there must be a way to bootstrap the current_accum value. Without that,
> the current capactity value is bogus until the battery is fully charged
> for the first time.

Erm, sorry, here's another version that does the 0.25mAh -> Ah
calculation in ds2760_battery_set_current_accum().

Daniel

>From e89cd102afe30dcb26fa3c788b439aebc52a93c3 Mon Sep 17 00:00:00 2001
From: Daniel Mack <[email protected]>
Date: Wed, 15 Jul 2009 21:57:29 +0200
Subject: [PATCH] ds2760: add current_accum module parameter

When connecting a ds2760 to a partly loaded battery the first time,
there must be a way to bootstrap the current_accum value. Without that,
the current capactity value is bogus until the battery is fully charged
for the first time.

Signed-off-by: Daniel Mack <[email protected]>
Cc: Szabolcs Gyurko <[email protected]>
Cc: Matt Reimer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
---
drivers/power/ds2760_battery.c | 41 ++++++++++++++++++++++++++-------------
1 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 2d0e5ed..f4a9258 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -70,6 +70,10 @@ static unsigned int rated_capacity;
module_param(rated_capacity, uint, 0644);
MODULE_PARM_DESC(rated_capacity, "rated battery capacity, 10*mAh or index");

+static unsigned int current_accum;
+module_param(current_accum, uint, 0644);
+MODULE_PARM_DESC(current_accum, "current accumulator value");
+
/* Some batteries have their rated capacity stored a N * 10 mAh, while
* others use an index into this table. */
static int rated_capacities[] = {
@@ -215,6 +219,22 @@ static int ds2760_battery_read_status(struct ds2760_device_info *di)
return 0;
}

+static void ds2760_battery_set_current_accum(struct ds2760_device_info *di,
+ unsigned int acr_val)
+{
+ unsigned char acr[2];
+
+ /* acr is in units of 0.25 mAh */
+ acr_val *= 4L;
+ acr_val /= 1000;
+
+ acr[0] = acr_val >> 8;
+ acr[1] = acr_val & 0xff;
+
+ if (w1_ds2760_write(di->w1_dev, acr, DS2760_CURRENT_ACCUM_MSB, 2) < 2)
+ dev_warn(di->dev, "ACR write failed\n");
+}
+
static void ds2760_battery_update_status(struct ds2760_device_info *di)
{
int old_charge_status = di->charge_status;
@@ -246,21 +266,9 @@ static void ds2760_battery_update_status(struct ds2760_device_info *di)
if (di->full_counter < 2) {
di->charge_status = POWER_SUPPLY_STATUS_CHARGING;
} else {
- unsigned char acr[2];
- int acr_val;
-
- /* acr is in units of 0.25 mAh */
- acr_val = di->full_active_uAh * 4L / 1000;
-
- acr[0] = acr_val >> 8;
- acr[1] = acr_val & 0xff;
-
- if (w1_ds2760_write(di->w1_dev, acr,
- DS2760_CURRENT_ACCUM_MSB, 2) < 2)
- dev_warn(di->dev,
- "ACR reset failed\n");
-
di->charge_status = POWER_SUPPLY_STATUS_FULL;
+ ds2760_battery_set_current_accum(di,
+ di->full_active_uAh);
}
}
} else {
@@ -423,6 +431,11 @@ static int ds2760_battery_probe(struct platform_device *pdev)
if (rated_capacity)
ds2760_battery_write_rated_capacity(di, rated_capacity);

+ /* set current accumulator if given as parameter.
+ * this should only be done for bootstrapping the value */
+ if (current_accum)
+ ds2760_battery_set_current_accum(di, current_accum);
+
retval = power_supply_register(&pdev->dev, &di->bat);
if (retval) {
dev_err(di->dev, "failed to register battery\n");
--
1.6.3.1