[PATCH 1/3] pda-power: add set_charged functionaltity

[PATCH 1/3] pda-power: add set_charged functionaltity

[Daniel Mack]

This adds a function to let the power supply core know that the battery
is fully charged now. This can be done by watching a GPIO line, for
example.

Some battery drivers might need that information to calibrate
themselves.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Ian Molton <spyro@f2s.com>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: Matt Reimer <mreimer@vpop.net>
---
 drivers/power/power_supply_core.c |   16 ++++++++++++++++
 include/linux/power_supply.h      |    3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 5520040..7303c45 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -111,6 +111,22 @@ int power_supply_is_system_supplied(void)
 	return error;
 }
 
+static int __power_supply_set_battery_charged(struct device *dev, void *data)
+{
+	struct power_supply *psy = dev_get_drvdata(dev);
+
+	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged)
+		psy->set_charged(psy);
+
+	return 0;
+}
+
+void power_supply_set_battery_charged(void)
+{
+	class_for_each_device(power_supply_class, NULL, NULL,
+				__power_supply_set_battery_charged);
+}
+
 int power_supply_register(struct device *parent, struct power_supply *psy)
 {
 	int rc = 0;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 594c494..b2b39e2 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -126,6 +126,7 @@ struct power_supply {
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
 	void (*external_power_changed)(struct power_supply *psy);
+	void (*set_charged)(struct power_supply *psy);
 
 	/* For APM emulation, think legacy userspace. */
 	int use_for_apm;
@@ -170,8 +171,10 @@ extern int power_supply_am_i_supplied(struct power_supply *psy);
 
 #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
 extern int power_supply_is_system_supplied(void);
+extern void power_supply_set_battery_charged(void);
 #else
 static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
+static inline void power_supply_set_battery_charged(void) {}
 #endif
 
 extern int power_supply_register(struct device *parent,
-- 
1.6.3.1

[PATCH 2/3] pda-power: EXPORT_SYMBOL cleanups

[Daniel Mack]

While I'm at it, cleanup the power supply code so that EXPORT_SYMBOL_GPL
appears directly after the symbole declaration. checkpatch.pl wants it
that way.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Ian Molton <spyro@f2s.com>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: Matt Reimer <mreimer@vpop.net>
---
 drivers/power/power_supply_core.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 7303c45..0a10799 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -20,6 +20,9 @@
 
 struct class *power_supply_class;
 
+/* exported for the APM Power driver, APM emulation */
+EXPORT_SYMBOL_GPL(power_supply_class);
+
 static int __power_supply_changed_work(struct device *dev, void *data)
 {
 	struct power_supply *psy = (struct power_supply *)data;
@@ -55,6 +58,7 @@ void power_supply_changed(struct power_supply *psy)
 
 	schedule_work(&psy->changed_work);
 }
+EXPORT_SYMBOL_GPL(power_supply_changed);
 
 static int __power_supply_am_i_supplied(struct device *dev, void *data)
 {
@@ -86,6 +90,7 @@ int power_supply_am_i_supplied(struct power_supply *psy)
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(power_supply_am_i_supplied);
 
 static int __power_supply_is_system_supplied(struct device *dev, void *data)
 {
@@ -110,6 +115,7 @@ int power_supply_is_system_supplied(void)
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
 
 static int __power_supply_set_battery_charged(struct device *dev, void *data)
 {
@@ -160,6 +166,7 @@ dev_create_failed:
 success:
 	return rc;
 }
+EXPORT_SYMBOL_GPL(power_supply_register);
 
 void power_supply_unregister(struct power_supply *psy)
 {
@@ -168,6 +175,7 @@ void power_supply_unregister(struct power_supply *psy)
 	power_supply_remove_attrs(psy);
 	device_unregister(psy->dev);
 }
+EXPORT_SYMBOL_GPL(power_supply_unregister);
 
 static int __init power_supply_class_init(void)
 {
@@ -186,15 +194,6 @@ static void __exit power_supply_class_exit(void)
 	class_destroy(power_supply_class);
 }
 
-EXPORT_SYMBOL_GPL(power_supply_changed);
-EXPORT_SYMBOL_GPL(power_supply_am_i_supplied);
-EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
-EXPORT_SYMBOL_GPL(power_supply_register);
-EXPORT_SYMBOL_GPL(power_supply_unregister);
-
-/* exported for the APM Power driver, APM emulation */
-EXPORT_SYMBOL_GPL(power_supply_class);
-
 subsys_initcall(power_supply_class_init);
 module_exit(power_supply_class_exit);
 
-- 
1.6.3.1

[PATCH 3/3] ds2760: implement set_charged() feature

[Daniel Mack]

The ds2760's internal current meter is not reliable enough as it has an
inacurracy of around ~15%. Without any correction for that error, the
current accumulator is couting up all the time, even though the battery
is already fully charged and hence destroys the static information. The
longer it is connected, the worse is the aberration.

Fortunately, this can be corrected by the DS2760_CURRENT_OFFSET_BIAS
register. Using the external power_supply_set_battery_charged()
function, this register is now gauging the measurement.

A delayed work is used to debounce flaky GPIO signals and to let the
current value settle. Also see Maxim's application note AN4188.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Szabolcs Gyurko <szabolcs.gyurko@tlt.hu>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Anton Vorontsov <cbou@mail.ru>
---
 drivers/power/ds2760_battery.c |   47 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index f4a9258..30ebd82 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -56,6 +56,7 @@ struct ds2760_device_info {
 	struct device *w1_dev;
 	struct workqueue_struct *monitor_wqueue;
 	struct delayed_work monitor_work;
+	struct delayed_work set_charged_work;
 };
 
 static unsigned int cache_time = 1000;
@@ -327,6 +328,48 @@ static void ds2760_battery_external_power_changed(struct power_supply *psy)
 	queue_delayed_work(di->monitor_wqueue, &di->monitor_work, HZ/10);
 }
 
+
+static void ds2760_battery_set_charged_work(struct work_struct *work)
+{
+	char bias;
+	struct ds2760_device_info *di = container_of(work,
+		struct ds2760_device_info, set_charged_work.work);
+
+	dev_dbg(di->dev, "%s\n", __func__);
+
+	ds2760_battery_read_status(di);
+
+	/* When we get notified by external circuitry that the battery is
+	 * considered fully charged now, we know that there is no current
+	 * flow any more. However, the ds2760's internal current meter is
+	 * too inaccurate to rely on - spec say something ~15% failure.
+	 * Hence, we use the current offset bias register to compensate
+	 * that error.
+	 */
+
+	if (!power_supply_am_i_supplied(&di->bat))
+		return;
+
+	bias = di->current_raw +
+		(signed char) di->raw[DS2760_CURRENT_OFFSET_BIAS];
+
+	dev_dbg(di->dev, "%s: bias = %d\n", __func__, bias);
+
+	w1_ds2760_write(di->w1_dev, &bias, DS2760_CURRENT_OFFSET_BIAS, 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_set_charged(struct power_supply *psy)
+{
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	/* postpone the actual work by 20 secs. This is for debouncing GPIO
+	 * signals and to let the current value settle. See AN4188. */
+	cancel_delayed_work(&di->set_charged_work);
+	queue_delayed_work(di->monitor_wqueue, &di->set_charged_work, HZ * 20);
+}
+
 static int ds2760_battery_get_property(struct power_supply *psy,
 				       enum power_supply_property psp,
 				       union power_supply_propval *val)
@@ -412,6 +455,7 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 	di->bat.properties	= ds2760_battery_props;
 	di->bat.num_properties	= ARRAY_SIZE(ds2760_battery_props);
 	di->bat.get_property	= ds2760_battery_get_property;
+	di->bat.set_charged	= ds2760_battery_set_charged;
 	di->bat.external_power_changed =
 				  ds2760_battery_external_power_changed;
 
@@ -443,6 +487,7 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 	}
 
 	INIT_DELAYED_WORK(&di->monitor_work, ds2760_battery_work);
+	INIT_DELAYED_WORK(&di->set_charged_work, ds2760_battery_set_charged_work);
 	di->monitor_wqueue = create_singlethread_workqueue(dev_name(&pdev->dev));
 	if (!di->monitor_wqueue) {
 		retval = -ESRCH;
@@ -467,6 +512,8 @@ static int ds2760_battery_remove(struct platform_device *pdev)
 
 	cancel_rearming_delayed_workqueue(di->monitor_wqueue,
 					  &di->monitor_work);
+	cancel_rearming_delayed_workqueue(di->monitor_wqueue,
+					  &di->set_charged_work);
 	destroy_workqueue(di->monitor_wqueue);
 	power_supply_unregister(&di->bat);
 
-- 
1.6.3.1

Re: [PATCH 3/3] ds2760: implement set_charged() feature

[Daniel Mack]

On Thu, Jul 16, 2009 at 05:44:04PM +0200, Daniel Mack wrote:
> The ds2760's internal current meter is not reliable enough as it has an
> inacurracy of around ~15%. Without any correction for that error, the
> current accumulator is couting up all the time, even though the battery
> is already fully charged and hence destroys the static information. The
> longer it is connected, the worse is the aberration.
> 
> Fortunately, this can be corrected by the DS2760_CURRENT_OFFSET_BIAS
> register. Using the external power_supply_set_battery_charged()
> function, this register is now gauging the measurement.
> 
> A delayed work is used to debounce flaky GPIO signals and to let the
> current value settle. Also see Maxim's application note AN4188.

One more small correction - we need to manually write back the BIAS
level to the di->raw buffer as ds2760_battery_read_status() won't do it
for us after the first time.

Daniel


commit ebb1173a2c6e1334b1afc7cd119b39eca2350351
Author: Daniel Mack <daniel@caiaq.de>
Date:   Thu Jul 16 17:28:56 2009 +0200

    ds2760: implement set_charged() feature
    
    The ds2760's internal current meter is not reliable enough as it has an
    inacurracy of around ~15%. Without any correction for that error, the
    current accumulator is couting up all the time, even though the battery
    is already fully charged and hence destroys the static information. The
    longer it is connected, the worse is the aberration.
    
    Fortunately, this can be corrected by the DS2760_CURRENT_OFFSET_BIAS
    register. Using the external power_supply_set_battery_charged()
    function, this register is now gauging the measurement.
    
    A delayed work is used to debounce flaky GPIO signals and to let the
    current value settle. Also see Maxim's application note AN4188.
    
    Signed-off-by: Daniel Mack <daniel@caiaq.de>
    Cc: Szabolcs Gyurko <szabolcs.gyurko@tlt.hu>
    Cc: Matt Reimer <mreimer@vpop.net>
    Cc: Anton Vorontsov <cbou@mail.ru>

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index f4a9258..9a95bc1 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -56,6 +56,7 @@ struct ds2760_device_info {
 	struct device *w1_dev;
 	struct workqueue_struct *monitor_wqueue;
 	struct delayed_work monitor_work;
+	struct delayed_work set_charged_work;
 };
 
 static unsigned int cache_time = 1000;
@@ -327,6 +328,52 @@ static void ds2760_battery_external_power_changed(struct power_supply *psy)
 	queue_delayed_work(di->monitor_wqueue, &di->monitor_work, HZ/10);
 }
 
+
+static void ds2760_battery_set_charged_work(struct work_struct *work)
+{
+	char bias;
+	struct ds2760_device_info *di = container_of(work,
+		struct ds2760_device_info, set_charged_work.work);
+
+	dev_dbg(di->dev, "%s\n", __func__);
+
+	ds2760_battery_read_status(di);
+
+	/* When we get notified by external circuitry that the battery is
+	 * considered fully charged now, we know that there is no current
+	 * flow any more. However, the ds2760's internal current meter is
+	 * too inaccurate to rely on - spec say something ~15% failure.
+	 * Hence, we use the current offset bias register to compensate
+	 * that error.
+	 */
+
+	if (!power_supply_am_i_supplied(&di->bat))
+		return;
+
+	bias = di->current_raw +
+		(signed char) di->raw[DS2760_CURRENT_OFFSET_BIAS];
+
+	dev_dbg(di->dev, "%s: bias = %d\n", __func__, bias);
+
+	w1_ds2760_write(di->w1_dev, &bias, DS2760_CURRENT_OFFSET_BIAS, 1);
+	w1_ds2760_store_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
+	w1_ds2760_recall_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
+
+	/* Write to the di->raw[] buffer directly - the CURRENT_OFFSET_BIAS
+	 * value won't be read back by ds2760_battery_read_status() */
+	di->raw[DS2760_CURRENT_OFFSET_BIAS] = bias;
+}
+
+static void ds2760_battery_set_charged(struct power_supply *psy)
+{
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	/* postpone the actual work by 20 secs. This is for debouncing GPIO
+	 * signals and to let the current value settle. See AN4188. */
+	cancel_delayed_work(&di->set_charged_work);
+	queue_delayed_work(di->monitor_wqueue, &di->set_charged_work, HZ * 20);
+}
+
 static int ds2760_battery_get_property(struct power_supply *psy,
 				       enum power_supply_property psp,
 				       union power_supply_propval *val)
@@ -412,6 +459,7 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 	di->bat.properties	= ds2760_battery_props;
 	di->bat.num_properties	= ARRAY_SIZE(ds2760_battery_props);
 	di->bat.get_property	= ds2760_battery_get_property;
+	di->bat.set_charged	= ds2760_battery_set_charged;
 	di->bat.external_power_changed =
 				  ds2760_battery_external_power_changed;
 
@@ -443,6 +491,7 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 	}
 
 	INIT_DELAYED_WORK(&di->monitor_work, ds2760_battery_work);
+	INIT_DELAYED_WORK(&di->set_charged_work, ds2760_battery_set_charged_work);
 	di->monitor_wqueue = create_singlethread_workqueue(dev_name(&pdev->dev));
 	if (!di->monitor_wqueue) {
 		retval = -ESRCH;
@@ -467,6 +516,8 @@ static int ds2760_battery_remove(struct platform_device *pdev)
 
 	cancel_rearming_delayed_workqueue(di->monitor_wqueue,
 					  &di->monitor_work);
+	cancel_rearming_delayed_workqueue(di->monitor_wqueue,
+					  &di->set_charged_work);
 	destroy_workqueue(di->monitor_wqueue);
 	power_supply_unregister(&di->bat);
 

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Daniel Mack]

On Thu, Jul 16, 2009 at 05:44:02PM +0200, Daniel Mack wrote:
> This adds a function to let the power supply core know that the battery
> is fully charged now. This can be done by watching a GPIO line, for
> example.
> 
> Some battery drivers might need that information to calibrate
> themselves.

Any comments about the last patch series? I've sent one inlined in a
reply and three more apart from it, which might have been confusing. Do
you want me to resend it as one block?

Thanks,
Daniel

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Matt Reimer]

On Jul 16, 2009, at 8:44 AM, Daniel Mack wrote:

> This adds a function to let the power supply core know that the  
> battery
> is fully charged now. This can be done by watching a GPIO line, for
> example.
>
> Some battery drivers might need that information to calibrate
> themselves.

Looks fine to me.

Acked-by: Matt Reimer <mreimer@vpop.net>

Matt

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Anton Vorontsov]

On Thu, Jul 16, 2009 at 05:44:02PM +0200, Daniel Mack wrote:
> This adds a function to let the power supply core know that the battery
> is fully charged now. This can be done by watching a GPIO line, for
> example.
> 
> Some battery drivers might need that information to calibrate
> themselves.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Ian Molton <spyro@f2s.com>
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: Matt Reimer <mreimer@vpop.net>
> ---
>  drivers/power/power_supply_core.c |   16 ++++++++++++++++
>  include/linux/power_supply.h      |    3 +++

The patch is missing pda_power.c part?

>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 5520040..7303c45 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -111,6 +111,22 @@ int power_supply_is_system_supplied(void)
>  	return error;
>  }
>  
> +static int __power_supply_set_battery_charged(struct device *dev, void *data)
> +{
> +	struct power_supply *psy = dev_get_drvdata(dev);
> +
> +	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged)
> +		psy->set_charged(psy);
> +
> +	return 0;
> +}
> +
> +void power_supply_set_battery_charged(void)
> +{
> +	class_for_each_device(power_supply_class, NULL, NULL,
> +				__power_supply_set_battery_charged);
> +}
> +
>  int power_supply_register(struct device *parent, struct power_supply *psy)
>  {
>  	int rc = 0;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 594c494..b2b39e2 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -126,6 +126,7 @@ struct power_supply {
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val);
>  	void (*external_power_changed)(struct power_supply *psy);
> +	void (*set_charged)(struct power_supply *psy);
>  
>  	/* For APM emulation, think legacy userspace. */
>  	int use_for_apm;
> @@ -170,8 +171,10 @@ extern int power_supply_am_i_supplied(struct power_supply *psy);
>  
>  #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
>  extern int power_supply_is_system_supplied(void);
> +extern void power_supply_set_battery_charged(void);

I don't quite like this interface. Note that system might want
to know if it's supplied (hence global is_system_supplied(void) call),
but having set_battery_charged(void) interface makes little sense
(well, it makes sense for small embedded systems that have just
one battery and one charger)...

There is a supplied_to psy field that might help you there...

The cleanup patch looks OK though, I'll apply it.

Thanks,

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

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Daniel Mack]

On Mon, Jul 20, 2009 at 10:27:52PM +0400, Anton Vorontsov wrote:
> On Thu, Jul 16, 2009 at 05:44:02PM +0200, Daniel Mack wrote:
> > This adds a function to let the power supply core know that the battery
> > is fully charged now. This can be done by watching a GPIO line, for
> > example.
> > 
> > Some battery drivers might need that information to calibrate
> > themselves.
> > 
> > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > Cc: Ian Molton <spyro@f2s.com>
> > Cc: Anton Vorontsov <cbou@mail.ru>
> > Cc: Matt Reimer <mreimer@vpop.net>
> > ---
> >  drivers/power/power_supply_core.c |   16 ++++++++++++++++
> >  include/linux/power_supply.h      |    3 +++
> 
> The patch is missing pda_power.c part?

Erm, does it? What would you expect it to do in there?

> >  #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
> >  extern int power_supply_is_system_supplied(void);
> > +extern void power_supply_set_battery_charged(void);
> 
> I don't quite like this interface. Note that system might want
> to know if it's supplied (hence global is_system_supplied(void) call),
> but having set_battery_charged(void) interface makes little sense
> (well, it makes sense for small embedded systems that have just
> one battery and one charger)...
> 
> There is a supplied_to psy field that might help you there...

Hmm. I thought about that too, but didn't find a way to access the
single members of the pda-power device and find the right one to
access. They're registered on a string base and matched magically once
the battery monitor is probed when found on the one-wire bus. Any
proposal how an interface could look like that does that?

All I need is a way to pass this information from my platform code down
to the battery driver.

> The cleanup patch looks OK though, I'll apply it.

Thanks. What about the one adding support for the bootstrapping of the
current_accum value? I've sent that inline as a reply to the other patch
series (4/4) last week (which was probably not a good idea).

Daniel

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Anton Vorontsov]

On Mon, Jul 20, 2009 at 08:37:11PM +0200, Daniel Mack wrote:
> On Mon, Jul 20, 2009 at 10:27:52PM +0400, Anton Vorontsov wrote:
> > On Thu, Jul 16, 2009 at 05:44:02PM +0200, Daniel Mack wrote:
> > > This adds a function to let the power supply core know that the battery
> > > is fully charged now. This can be done by watching a GPIO line, for
> > > example.
> > > 
> > > Some battery drivers might need that information to calibrate
> > > themselves.
> > > 
> > > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > > Cc: Ian Molton <spyro@f2s.com>
> > > Cc: Anton Vorontsov <cbou@mail.ru>
> > > Cc: Matt Reimer <mreimer@vpop.net>
> > > ---
> > >  drivers/power/power_supply_core.c |   16 ++++++++++++++++
> > >  include/linux/power_supply.h      |    3 +++
> > 
> > The patch is missing pda_power.c part?
> 
> Erm, does it? What would you expect it to do in there?

Um... then the patch subject is misleading. It says "pda-power".

Is there any user of this new call, btw? I mean a mainline user,
or soon to be?

> > >  #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
> > >  extern int power_supply_is_system_supplied(void);
> > > +extern void power_supply_set_battery_charged(void);
> > 
> > I don't quite like this interface. Note that system might want
> > to know if it's supplied (hence global is_system_supplied(void) call),
> > but having set_battery_charged(void) interface makes little sense
> > (well, it makes sense for small embedded systems that have just
> > one battery and one charger)...
> > 
> > There is a supplied_to psy field that might help you there...
> 
> Hmm. I thought about that too, but didn't find a way to access the
> single members of the pda-power device and find the right one to
> access. They're registered on a string base and matched magically once
> the battery monitor is probed when found on the one-wire bus. Any
> proposal how an interface could look like that does that?
> 
> All I need is a way to pass this information from my platform code down
> to the battery driver.

Yes, I see the problem, but I don't have a solution off-hand. :-/

Let me think about it for awhile.

> > The cleanup patch looks OK though, I'll apply it.
> 
> Thanks. What about the one adding support for the bootstrapping of the
> current_accum value? I've sent that inline as a reply to the other patch
> series (4/4) last week (which was probably not a good idea).

"ds2760: add current_accum module parameter"? Looks perfectly OK.

Thanks!

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

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Daniel Mack]

On Mon, Jul 20, 2009 at 11:00:34PM +0400, Anton Vorontsov wrote:
> > Erm, does it? What would you expect it to do in there?
> 
> Um... then the patch subject is misleading. It says "pda-power".

Well, that's the subsystem it addresses :)

> Is there any user of this new call, btw? I mean a mainline user,
> or soon to be?

No, not yet. We might be merging our platform code mainline some day,
but at the moment it doesn't make sense to anyone to have have it. The
only prototypes tha exist are on our desks :)

Anyway - the surrounding code is perfectly simple. It just registers an
IRQ and calls set_battery_charged() in it. The battery driver cares for
debouncing and the like.

> > Hmm. I thought about that too, but didn't find a way to access the
> > single members of the pda-power device and find the right one to
> > access. They're registered on a string base and matched magically once
> > the battery monitor is probed when found on the one-wire bus. Any
> > proposal how an interface could look like that does that?
> > 
> > All I need is a way to pass this information from my platform code down
> > to the battery driver.
> 
> Yes, I see the problem, but I don't have a solution off-hand. :-/
> 
> Let me think about it for awhile.

So will I :)

> > Thanks. What about the one adding support for the bootstrapping of the
> > current_accum value? I've sent that inline as a reply to the other patch
> > series (4/4) last week (which was probably not a good idea).
> 
> "ds2760: add current_accum module parameter"? Looks perfectly OK.

That one, yes. Thanks for applying.

Daniel

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Daniel Mack]

On Mon, Jul 20, 2009 at 11:00:34PM +0400, Anton Vorontsov wrote:
> > Hmm. I thought about that too, but didn't find a way to access the
> > single members of the pda-power device and find the right one to
> > access. They're registered on a string base and matched magically once
> > the battery monitor is probed when found on the one-wire bus. Any
> > proposal how an interface could look like that does that?
> > 
> > All I need is a way to pass this information from my platform code down
> > to the battery driver.
> 
> Yes, I see the problem, but I don't have a solution off-hand. :-/
> 
> Let me think about it for awhile.

What about something like the patch below?

The platform code interface would look like

	struct power_supply *psy =
		power_support_get_by_name("ds2760.0");
	if (psy)
		power_supply_set_battery_charged(psy);

Couldn't test it yet as my hardware is missing, but I would still like
to hear your comments about it :)

Daniel



commit 1a865b0023619c93de095fdb35823e57a496fc0b
Author: Daniel Mack <daniel@caiaq.de>
Date:   Thu Jul 16 15:54:46 2009 +0200

    pwoer_supply: get_by_name and set_charged functionaltity
    
    This adds a function to get a power_supply device from the class of
    registered devices by name reference. It can be used to find a specific
    battery to call power_supply_set_battery_charged() on.
    
    Some battery drivers might need that information to calibrate
    themselves.
    
    Signed-off-by: Daniel Mack <daniel@caiaq.de>
    Cc: Ian Molton <spyro@f2s.com>
    Cc: Anton Vorontsov <cbou@mail.ru>
    Cc: Matt Reimer <mreimer@vpop.net>

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 58f10ac..ddb4800 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -117,6 +117,34 @@ int power_supply_is_system_supplied(void)
 }
 EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
 
+int power_supply_set_battery_charged(struct power_supply *psy)
+{
+	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged) {
+		psy->set_charged(psy);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(power_supply_set_battery_charged);
+
+static int power_supply_match_device_by_name(struct device *dev, void *data)
+{
+	const char *name = data;
+	struct power_supply *psy = dev_get_drvdata(dev);
+
+	return strcmp(psy->name, name) == 0;
+}
+
+struct power_supply *power_supply_get_by_name(char *name)
+{
+	struct device *dev = class_find_device(power_supply_class, NULL, name,
+					power_supply_match_device_by_name);
+
+	return dev ? dev_get_drvdata(dev) : NULL;
+}
+EXPORT_SYMBOL_GPL(power_supply_get_by_name);
+
 int power_supply_register(struct device *parent, struct power_supply *psy)
 {
 	int rc = 0;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 594c494..49f3296 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -126,6 +126,7 @@ struct power_supply {
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
 	void (*external_power_changed)(struct power_supply *psy);
+	void (*set_charged)(struct power_supply *psy);
 
 	/* For APM emulation, think legacy userspace. */
 	int use_for_apm;
@@ -165,8 +166,10 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+extern struct power_supply *power_supply_get_by_name(char *name);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
+extern int power_supply_set_battery_charged(struct power_supply *psy);
 
 #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
 extern int power_supply_is_system_supplied(void);

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Anton Vorontsov]

On Wed, Jul 22, 2009 at 07:20:45PM +0200, Daniel Mack wrote:
> On Mon, Jul 20, 2009 at 11:00:34PM +0400, Anton Vorontsov wrote:
> > > Hmm. I thought about that too, but didn't find a way to access the
> > > single members of the pda-power device and find the right one to
> > > access. They're registered on a string base and matched magically once
> > > the battery monitor is probed when found on the one-wire bus. Any
> > > proposal how an interface could look like that does that?
> > > 
> > > All I need is a way to pass this information from my platform code down
> > > to the battery driver.
> > 
> > Yes, I see the problem, but I don't have a solution off-hand. :-/
> > 
> > Let me think about it for awhile.
> 
> What about something like the patch below?
> 
> The platform code interface would look like
> 
> 	struct power_supply *psy =
> 		power_support_get_by_name("ds2760.0");
> 	if (psy)
> 		power_supply_set_battery_charged(psy);
> 
> Couldn't test it yet as my hardware is missing, but I would still like
> to hear your comments about it :)

Wow, looks perfect. Let me know when you test it, and I'll
apply that patch.

Thanks!


> Daniel
> 
> 
> 
> commit 1a865b0023619c93de095fdb35823e57a496fc0b
> Author: Daniel Mack <daniel@caiaq.de>
> Date:   Thu Jul 16 15:54:46 2009 +0200
> 
>     pwoer_supply: get_by_name and set_charged functionaltity
>     
>     This adds a function to get a power_supply device from the class of
>     registered devices by name reference. It can be used to find a specific
>     battery to call power_supply_set_battery_charged() on.
>     
>     Some battery drivers might need that information to calibrate
>     themselves.
>     
>     Signed-off-by: Daniel Mack <daniel@caiaq.de>
>     Cc: Ian Molton <spyro@f2s.com>
>     Cc: Anton Vorontsov <cbou@mail.ru>
>     Cc: Matt Reimer <mreimer@vpop.net>
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 58f10ac..ddb4800 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -117,6 +117,34 @@ int power_supply_is_system_supplied(void)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
>  
> +int power_supply_set_battery_charged(struct power_supply *psy)
> +{
> +	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged) {
> +		psy->set_charged(psy);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(power_supply_set_battery_charged);
> +
> +static int power_supply_match_device_by_name(struct device *dev, void *data)
> +{
> +	const char *name = data;
> +	struct power_supply *psy = dev_get_drvdata(dev);
> +
> +	return strcmp(psy->name, name) == 0;
> +}
> +
> +struct power_supply *power_supply_get_by_name(char *name)
> +{
> +	struct device *dev = class_find_device(power_supply_class, NULL, name,
> +					power_supply_match_device_by_name);
> +
> +	return dev ? dev_get_drvdata(dev) : NULL;
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_by_name);
> +
>  int power_supply_register(struct device *parent, struct power_supply *psy)
>  {
>  	int rc = 0;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 594c494..49f3296 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -126,6 +126,7 @@ struct power_supply {
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val);
>  	void (*external_power_changed)(struct power_supply *psy);
> +	void (*set_charged)(struct power_supply *psy);
>  
>  	/* For APM emulation, think legacy userspace. */
>  	int use_for_apm;
> @@ -165,8 +166,10 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +extern struct power_supply *power_supply_get_by_name(char *name);
>  extern void power_supply_changed(struct power_supply *psy);
>  extern int power_supply_am_i_supplied(struct power_supply *psy);
> +extern int power_supply_set_battery_charged(struct power_supply *psy);
>  
>  #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
>  extern int power_supply_is_system_supplied(void);

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

Re: [PATCH 1/3] pda-power: add set_charged functionaltity

[Daniel Mack]

On Wed, Jul 22, 2009 at 09:41:11PM +0400, Anton Vorontsov wrote:
> On Wed, Jul 22, 2009 at 07:20:45PM +0200, Daniel Mack wrote:
> > What about something like the patch below?
> > 
> > The platform code interface would look like
> > 
> > 	struct power_supply *psy =
> > 		power_support_get_by_name("ds2760.0");
> > 	if (psy)
> > 		power_supply_set_battery_charged(psy);
> > 
> > Couldn't test it yet as my hardware is missing, but I would still like
> > to hear your comments about it :)
> 
> Wow, looks perfect. Let me know when you test it, and I'll
> apply that patch.

That seems to work well. I'm still resending the last two remaining
patches as a follow-up to this one - the changelog had a typo.

Thanks,
Daniel

[PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Daniel Mack]

This adds a function to get a power_supply device from the class of
registered devices by name reference. It can be used to find a specific
battery to call power_supply_set_battery_charged() on.

Some battery drivers might need that information to calibrate
themselves.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Ian Molton <spyro@f2s.com>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: Matt Reimer <mreimer@vpop.net>
---
 drivers/power/power_supply_core.c |   28 ++++++++++++++++++++++++++++
 include/linux/power_supply.h      |    3 +++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 58f10ac..ddb4800 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -117,6 +117,34 @@ int power_supply_is_system_supplied(void)
 }
 EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
 
+int power_supply_set_battery_charged(struct power_supply *psy)
+{
+	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged) {
+		psy->set_charged(psy);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(power_supply_set_battery_charged);
+
+static int power_supply_match_device_by_name(struct device *dev, void *data)
+{
+	const char *name = data;
+	struct power_supply *psy = dev_get_drvdata(dev);
+
+	return strcmp(psy->name, name) == 0;
+}
+
+struct power_supply *power_supply_get_by_name(char *name)
+{
+	struct device *dev = class_find_device(power_supply_class, NULL, name,
+					power_supply_match_device_by_name);
+
+	return dev ? dev_get_drvdata(dev) : NULL;
+}
+EXPORT_SYMBOL_GPL(power_supply_get_by_name);
+
 int power_supply_register(struct device *parent, struct power_supply *psy)
 {
 	int rc = 0;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 594c494..49f3296 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -126,6 +126,7 @@ struct power_supply {
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
 	void (*external_power_changed)(struct power_supply *psy);
+	void (*set_charged)(struct power_supply *psy);
 
 	/* For APM emulation, think legacy userspace. */
 	int use_for_apm;
@@ -165,8 +166,10 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+extern struct power_supply *power_supply_get_by_name(char *name);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
+extern int power_supply_set_battery_charged(struct power_supply *psy);
 
 #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
 extern int power_supply_is_system_supplied(void);
-- 
1.6.3.1

[PATCH 2/2] ds2760: implement set_charged() feature

[Daniel Mack]

The ds2760's internal current meter is not reliable enough as it has an
inacurracy of around ~15%. Without any correction for that error, the
current accumulator is couting up all the time, even though the battery
is already fully charged and hence destroys the static information. The
longer it is connected, the worse is the aberration.

Fortunately, this can be corrected by the DS2760_CURRENT_OFFSET_BIAS
register. Using the external power_supply_set_battery_charged()
function, this register is now gauging the measurement.

A delayed work is used to debounce flaky GPIO signals and to let the
current value settle. Also see Maxim's application note AN4188.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Szabolcs Gyurko <szabolcs.gyurko@tlt.hu>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Anton Vorontsov <cbou@mail.ru>
---
 drivers/power/ds2760_battery.c |   51 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index f4a9258..d545fee 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -56,6 +56,7 @@ struct ds2760_device_info {
 	struct device *w1_dev;
 	struct workqueue_struct *monitor_wqueue;
 	struct delayed_work monitor_work;
+	struct delayed_work set_charged_work;
 };
 
 static unsigned int cache_time = 1000;
@@ -327,6 +328,52 @@ static void ds2760_battery_external_power_changed(struct power_supply *psy)
 	queue_delayed_work(di->monitor_wqueue, &di->monitor_work, HZ/10);
 }
 
+
+static void ds2760_battery_set_charged_work(struct work_struct *work)
+{
+	char bias;
+	struct ds2760_device_info *di = container_of(work,
+		struct ds2760_device_info, set_charged_work.work);
+
+	dev_dbg(di->dev, "%s\n", __func__);
+
+	ds2760_battery_read_status(di);
+
+	/* When we get notified by external circuitry that the battery is
+	 * considered fully charged now, we know that there is no current
+	 * flow any more. However, the ds2760's internal current meter is
+	 * too inaccurate to rely on - spec say something ~15% failure.
+	 * Hence, we use the current offset bias register to compensate
+	 * that error.
+	 */
+
+	if (!power_supply_am_i_supplied(&di->bat))
+		return;
+
+	bias = (signed char) di->current_raw +
+		(signed char) di->raw[DS2760_CURRENT_OFFSET_BIAS];
+
+	dev_dbg(di->dev, "%s: bias = %d\n", __func__, bias);
+
+	w1_ds2760_write(di->w1_dev, &bias, DS2760_CURRENT_OFFSET_BIAS, 1);
+	w1_ds2760_store_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
+	w1_ds2760_recall_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
+
+	/* Write to the di->raw[] buffer directly - the CURRENT_OFFSET_BIAS
+	 * value won't be read back by ds2760_battery_read_status() */
+	di->raw[DS2760_CURRENT_OFFSET_BIAS] = bias;
+}
+
+static void ds2760_battery_set_charged(struct power_supply *psy)
+{
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	/* postpone the actual work by 20 secs. This is for debouncing GPIO
+	 * signals and to let the current value settle. See AN4188. */
+	cancel_delayed_work(&di->set_charged_work);
+	queue_delayed_work(di->monitor_wqueue, &di->set_charged_work, HZ * 20);
+}
+
 static int ds2760_battery_get_property(struct power_supply *psy,
 				       enum power_supply_property psp,
 				       union power_supply_propval *val)
@@ -412,6 +459,7 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 	di->bat.properties	= ds2760_battery_props;
 	di->bat.num_properties	= ARRAY_SIZE(ds2760_battery_props);
 	di->bat.get_property	= ds2760_battery_get_property;
+	di->bat.set_charged	= ds2760_battery_set_charged;
 	di->bat.external_power_changed =
 				  ds2760_battery_external_power_changed;
 
@@ -443,6 +491,7 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 	}
 
 	INIT_DELAYED_WORK(&di->monitor_work, ds2760_battery_work);
+	INIT_DELAYED_WORK(&di->set_charged_work, ds2760_battery_set_charged_work);
 	di->monitor_wqueue = create_singlethread_workqueue(dev_name(&pdev->dev));
 	if (!di->monitor_wqueue) {
 		retval = -ESRCH;
@@ -467,6 +516,8 @@ static int ds2760_battery_remove(struct platform_device *pdev)
 
 	cancel_rearming_delayed_workqueue(di->monitor_wqueue,
 					  &di->monitor_work);
+	cancel_rearming_delayed_workqueue(di->monitor_wqueue,
+					  &di->set_charged_work);
 	destroy_workqueue(di->monitor_wqueue);
 	power_supply_unregister(&di->bat);
 
-- 
1.6.3.1

Re: [PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Daniel Mack]

ping?

On Thu, Jul 23, 2009 at 08:35:53PM +0200, Daniel Mack wrote:
> This adds a function to get a power_supply device from the class of
> registered devices by name reference. It can be used to find a specific
> battery to call power_supply_set_battery_charged() on.
> 
> Some battery drivers might need that information to calibrate
> themselves.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Ian Molton <spyro@f2s.com>
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: Matt Reimer <mreimer@vpop.net>
> ---
>  drivers/power/power_supply_core.c |   28 ++++++++++++++++++++++++++++
>  include/linux/power_supply.h      |    3 +++
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 58f10ac..ddb4800 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -117,6 +117,34 @@ int power_supply_is_system_supplied(void)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
>  
> +int power_supply_set_battery_charged(struct power_supply *psy)
> +{
> +	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged) {
> +		psy->set_charged(psy);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(power_supply_set_battery_charged);
> +
> +static int power_supply_match_device_by_name(struct device *dev, void *data)
> +{
> +	const char *name = data;
> +	struct power_supply *psy = dev_get_drvdata(dev);
> +
> +	return strcmp(psy->name, name) == 0;
> +}
> +
> +struct power_supply *power_supply_get_by_name(char *name)
> +{
> +	struct device *dev = class_find_device(power_supply_class, NULL, name,
> +					power_supply_match_device_by_name);
> +
> +	return dev ? dev_get_drvdata(dev) : NULL;
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_by_name);
> +
>  int power_supply_register(struct device *parent, struct power_supply *psy)
>  {
>  	int rc = 0;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 594c494..49f3296 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -126,6 +126,7 @@ struct power_supply {
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val);
>  	void (*external_power_changed)(struct power_supply *psy);
> +	void (*set_charged)(struct power_supply *psy);
>  
>  	/* For APM emulation, think legacy userspace. */
>  	int use_for_apm;
> @@ -165,8 +166,10 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +extern struct power_supply *power_supply_get_by_name(char *name);
>  extern void power_supply_changed(struct power_supply *psy);
>  extern int power_supply_am_i_supplied(struct power_supply *psy);
> +extern int power_supply_set_battery_charged(struct power_supply *psy);
>  
>  #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
>  extern int power_supply_is_system_supplied(void);
> -- 
> 1.6.3.1
> 

Re: [PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Ian molton]

Daniel Mack wrote:
> ping?

Looks ok. the only comment I have right now is that the comment doesnt 
quite match the patch - it suggests that the patch doesnt include the 
power_supply_set_battery_charged() function, when it does in fact.

> On Thu, Jul 23, 2009 at 08:35:53PM +0200, Daniel Mack wrote:
>> This adds a function to get a power_supply device from the class of
>> registered devices by name reference. It can be used to find a specific
>> battery to call power_supply_set_battery_charged() on.
>>
>> Some battery drivers might need that information to calibrate
>> themselves.
>>
>> Signed-off-by: Daniel Mack <daniel@caiaq.de>
>> Cc: Ian Molton <spyro@f2s.com>
>> Cc: Anton Vorontsov <cbou@mail.ru>
>> Cc: Matt Reimer <mreimer@vpop.net>
>> ---
>>  drivers/power/power_supply_core.c |   28 ++++++++++++++++++++++++++++
>>  include/linux/power_supply.h      |    3 +++
>>  2 files changed, 31 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>> index 58f10ac..ddb4800 100644
>> --- a/drivers/power/power_supply_core.c
>> +++ b/drivers/power/power_supply_core.c
>> @@ -117,6 +117,34 @@ int power_supply_is_system_supplied(void)
>>  }
>>  EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
>>  
>> +int power_supply_set_battery_charged(struct power_supply *psy)
>> +{
>> +	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged) {
>> +		psy->set_charged(psy);
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_set_battery_charged);
>> +
>> +static int power_supply_match_device_by_name(struct device *dev, void *data)
>> +{
>> +	const char *name = data;
>> +	struct power_supply *psy = dev_get_drvdata(dev);
>> +
>> +	return strcmp(psy->name, name) == 0;
>> +}
>> +
>> +struct power_supply *power_supply_get_by_name(char *name)
>> +{
>> +	struct device *dev = class_find_device(power_supply_class, NULL, name,
>> +					power_supply_match_device_by_name);
>> +
>> +	return dev ? dev_get_drvdata(dev) : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>> +
>>  int power_supply_register(struct device *parent, struct power_supply *psy)
>>  {
>>  	int rc = 0;
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index 594c494..49f3296 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -126,6 +126,7 @@ struct power_supply {
>>  			    enum power_supply_property psp,
>>  			    union power_supply_propval *val);
>>  	void (*external_power_changed)(struct power_supply *psy);
>> +	void (*set_charged)(struct power_supply *psy);
>>  
>>  	/* For APM emulation, think legacy userspace. */
>>  	int use_for_apm;
>> @@ -165,8 +166,10 @@ struct power_supply_info {
>>  	int use_for_apm;
>>  };
>>  
>> +extern struct power_supply *power_supply_get_by_name(char *name);
>>  extern void power_supply_changed(struct power_supply *psy);
>>  extern int power_supply_am_i_supplied(struct power_supply *psy);
>> +extern int power_supply_set_battery_charged(struct power_supply *psy);
>>  
>>  #if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
>>  extern int power_supply_is_system_supplied(void);
>> -- 
>> 1.6.3.1
>>
> 
> 

Re: [PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Daniel Mack]

On Wed, Jul 29, 2009 at 11:29:48AM +0100, Ian molton wrote:
> Looks ok. the only comment I have right now is that the comment doesnt  
> quite match the patch - it suggests that the patch doesnt include the  
> power_supply_set_battery_charged() function, when it does in fact.

The subject mentions it, and the body references it again. Hmm. Anyway.
Want me to resend? Or could the commiter add another sentence to point
that out? I think that would be Anton, as he agreed to pick all the
other patches I recently sent in.

Thanks,
Daniel


>> On Thu, Jul 23, 2009 at 08:35:53PM +0200, Daniel Mack wrote:
>>> This adds a function to get a power_supply device from the class of
>>> registered devices by name reference. It can be used to find a specific
>>> battery to call power_supply_set_battery_charged() on.
>>>
>>> Some battery drivers might need that information to calibrate
>>> themselves.
>>>
>>> Signed-off-by: Daniel Mack <daniel@caiaq.de>
>>> Cc: Ian Molton <spyro@f2s.com>
>>> Cc: Anton Vorontsov <cbou@mail.ru>
>>> Cc: Matt Reimer <mreimer@vpop.net>
>>> ---
>>>  drivers/power/power_supply_core.c |   28 ++++++++++++++++++++++++++++
>>>  include/linux/power_supply.h      |    3 +++
>>>  2 files changed, 31 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>>> index 58f10ac..ddb4800 100644
>>> --- a/drivers/power/power_supply_core.c
>>> +++ b/drivers/power/power_supply_core.c
>>> @@ -117,6 +117,34 @@ int power_supply_is_system_supplied(void)
>>>  }
>>>  EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
>>>  +int power_supply_set_battery_charged(struct power_supply *psy)
>>> +{
>>> +	if (psy->type == POWER_SUPPLY_TYPE_BATTERY && psy->set_charged) {
>>> +		psy->set_charged(psy);
>>> +		return 0;
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(power_supply_set_battery_charged);
>>> +
>>> +static int power_supply_match_device_by_name(struct device *dev, void *data)
>>> +{
>>> +	const char *name = data;
>>> +	struct power_supply *psy = dev_get_drvdata(dev);
>>> +
>>> +	return strcmp(psy->name, name) == 0;
>>> +}
>>> +
>>> +struct power_supply *power_supply_get_by_name(char *name)
>>> +{
>>> +	struct device *dev = class_find_device(power_supply_class, NULL, name,
>>> +					power_supply_match_device_by_name);
>>> +
>>> +	return dev ? dev_get_drvdata(dev) : NULL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>>> +
>>>  int power_supply_register(struct device *parent, struct power_supply *psy)
>>>  {
>>>  	int rc = 0;
>>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>>> index 594c494..49f3296 100644
>>> --- a/include/linux/power_supply.h
>>> +++ b/include/linux/power_supply.h
>>> @@ -126,6 +126,7 @@ struct power_supply {
>>>  			    enum power_supply_property psp,
>>>  			    union power_supply_propval *val);
>>>  	void (*external_power_changed)(struct power_supply *psy);
>>> +	void (*set_charged)(struct power_supply *psy);
>>>   	/* For APM emulation, think legacy userspace. */
>>>  	int use_for_apm;
>>> @@ -165,8 +166,10 @@ struct power_supply_info {
>>>  	int use_for_apm;
>>>  };
>>>  +extern struct power_supply *power_supply_get_by_name(char *name);
>>>  extern void power_supply_changed(struct power_supply *psy);
>>>  extern int power_supply_am_i_supplied(struct power_supply *psy);
>>> +extern int power_supply_set_battery_charged(struct power_supply *psy);
>>>   #if defined(CONFIG_POWER_SUPPLY) || 
>>> defined(CONFIG_POWER_SUPPLY_MODULE)
>>>  extern int power_supply_is_system_supplied(void);
>>> -- 
>

Re: [PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Ian Molton]

Daniel Mack wrote:
> On Wed, Jul 29, 2009 at 11:29:48AM +0100, Ian molton wrote:
>> Looks ok. the only comment I have right now is that the comment doesnt  
>> quite match the patch - it suggests that the patch doesnt include the  
>> power_supply_set_battery_charged() function, when it does in fact.
> 
> The subject mentions it, and the body references it again. Hmm. Anyway.
> Want me to resend? Or could the commiter add another sentence to point
> that out? I think that would be Anton, as he agreed to pick all the
> other patches I recently sent in.

I'm not that fussed - you just seemed to want feedback, so I had a read ;-)

If you change it, I'd write something like:

This adds a function that indicates that a battery is fully charged. It 
also includes functions to get a power_supply device from the class of 
registered devices by name reference. These can be used to find a 
specific battery to call power_supply_set_battery_charged() on.

Some battery drivers might need this information to calibrate
themselves.

Or something.

Re: [PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Anton Vorontsov]

On Wed, Jul 29, 2009 at 01:45:05PM +0100, Ian Molton wrote:
> Daniel Mack wrote:
> >On Wed, Jul 29, 2009 at 11:29:48AM +0100, Ian molton wrote:
> >>Looks ok. the only comment I have right now is that the comment
> >>doesnt  quite match the patch - it suggests that the patch
> >>doesnt include the  power_supply_set_battery_charged() function,
> >>when it does in fact.
> >
> >The subject mentions it, and the body references it again. Hmm. Anyway.
> >Want me to resend? Or could the commiter add another sentence to point
> >that out? I think that would be Anton, as he agreed to pick all the
> >other patches I recently sent in.
> 
> I'm not that fussed - you just seemed to want feedback, so I had a read ;-)
> 
> If you change it, I'd write something like:
> 
> This adds a function that indicates that a battery is fully charged.
> It also includes functions to get a power_supply device from the
> class of registered devices by name reference. These can be used to
> find a specific battery to call power_supply_set_battery_charged()
> on.
> 
> Some battery drivers might need this information to calibrate
> themselves.

Ian, Matt, much thanks for reviews and acks!
Daniel, thanks for the patches, and for the patience. :-)


Following patches applied to battery-2.6.git (for -next):

Daniel Mack (8):
      ds2760_battery: delay power supply registration
      ds2760_battery: export more features
      ds2760_battery: add rated_capacity module parameter
      ds2760_battery: handle full_active_uAh == 0 case correctly
      ds2760_battery: add current_accum module parameter
      power_supply: EXPORT_SYMBOL cleanups
      power_supply: get_by_name and set_charged functionality
      ds2760_battery: implement set_charged() feature

Let me know if I missed anything.

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

Re: [PATCH 1/2] power_supply: get_by_name and set_charged functionality

[Daniel Mack]

On Thu, Jul 30, 2009 at 06:11:34PM +0400, Anton Vorontsov wrote:
> Ian, Matt, much thanks for reviews and acks!
> Daniel, thanks for the patches, and for the patience. :-)
> 
> 
> Following patches applied to battery-2.6.git (for -next):
> 
> Daniel Mack (8):
>       ds2760_battery: delay power supply registration
>       ds2760_battery: export more features
>       ds2760_battery: add rated_capacity module parameter
>       ds2760_battery: handle full_active_uAh == 0 case correctly
>       ds2760_battery: add current_accum module parameter
>       power_supply: EXPORT_SYMBOL cleanups
>       power_supply: get_by_name and set_charged functionality
>       ds2760_battery: implement set_charged() feature
> 
> Let me know if I missed anything.

Looks good. Thanks a lot :) There might be some more coming up the next
days.

Daniel