[PATCH 1/3] pda_power: add support for writeable properties

[PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

This patch adds support for writeable power supply properties and
exposes them as writeable to sysfs.

A power supply implementation must implement two new function calls in
order to use that feature:

  int set_property(struct power_supply *psy,
                   enum power_supply_property psp,
                   const union power_supply_propval *val);

  int property_is_writeable(struct power_supply *psy,
                            enum power_supply_property psp);

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Andres Salomon <dilinger@collabora.co.uk>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/power/power_supply_sysfs.c |   31 +++++++++++++++++++++++++++++--
 include/linux/power_supply.h       |    5 +++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 5b6e352..5cfb410 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -31,9 +31,9 @@
 
 #define POWER_SUPPLY_ATTR(_name)					\
 {									\
-	.attr = { .name = #_name, .mode = 0444 },	\
+	.attr = { .name = #_name },					\
 	.show = power_supply_show_property,				\
-	.store = NULL,							\
+	.store = power_supply_store_property,				\
 }
 
 static struct device_attribute power_supply_attrs[];
@@ -91,6 +91,25 @@ static ssize_t power_supply_show_property(struct device *dev,
 	return sprintf(buf, "%d\n", value.intval);
 }
 
+static ssize_t power_supply_store_property(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count) {
+	ssize_t ret;
+	struct power_supply *psy = dev_get_drvdata(dev);
+	const ptrdiff_t off = attr - power_supply_attrs;
+	union power_supply_propval value;
+	long long_val;
+
+	/* TODO: support other types than int */
+	ret = strict_strtol(buf, 10, &long_val);
+	if (ret < 0)
+		return ret;
+
+	value.intval = long_val;
+
+	return psy->set_property(psy, off, &value);
+}
+
 /* Must be in the same order as POWER_SUPPLY_PROP_* */
 static struct device_attribute power_supply_attrs[] = {
 	/* Properties of type `int' */
@@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
 	}
 
 	for (j = 0; j < psy->num_properties; j++) {
+		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
+
+		if (psy->property_is_writeable &&
+		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
+			mode |= S_IWUSR;
+
+		power_supply_attrs[psy->properties[j]].attr.mode = mode;
+
 		rc = device_create_file(psy->dev,
 			    &power_supply_attrs[psy->properties[j]]);
 		if (rc)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index ebd2b8f..f02f7fd 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -144,6 +144,11 @@ struct power_supply {
 	int (*get_property)(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val);
+	int (*property_is_writeable)(struct power_supply *psy,
+				     enum power_supply_property psp);
 	void (*external_power_changed)(struct power_supply *psy);
 	void (*set_charged)(struct power_supply *psy);
 
-- 
1.7.1

[PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

[Daniel Mack]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4102 bytes --]

For userspace tools and daemons, it might be necessary to adjust
the charge_now and charge_full properties of the ds2760 battery monitor,
for example for unavoidable corrections due to aging batteries.

For better readbility and to avoid magic numbers in conversion
calculations, RATED_CAPACITY_FACTOR is introduced.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Andres Salomon <dilinger@collabora.co.uk>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/power/ds2760_battery.c |   60 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 3bf8d1f..b82bf92 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -78,6 +78,7 @@ 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. */
+#define RATED_CAPACITY_FACTOR 10
 static int rated_capacities[] = {
 	0,
 	920,	/* Samsung */
@@ -170,7 +171,8 @@ static int ds2760_battery_read_status(struct ds2760_device_info *di)
 		di->rated_capacity = rated_capacities[
 			(unsigned int)di->raw[DS2760_RATED_CAPACITY]];
 	else
-		di->rated_capacity = di->raw[DS2760_RATED_CAPACITY] * 10;
+		di->rated_capacity = di->raw[DS2760_RATED_CAPACITY] *
+						RATED_CAPACITY_FACTOR;
 
 	di->rated_capacity *= 1000; /* convert to µAh */
 
@@ -426,6 +428,54 @@ static int ds2760_battery_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int ds2760_battery_set_property(struct power_supply *psy,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
+{
+	int ret = -EPERM;
+	unsigned char tmp;
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		/* the interface counts in uAh, convert the value */
+		ds2760_battery_write_rated_capacity(di, (val->intval / 1000L) /
+							RATED_CAPACITY_FACTOR);
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		tmp = val->intval & 0xff;
+		ret = w1_ds2760_write(di->w1_dev, &tmp, DS2760_CURRENT_ACCUM_LSB, 1);
+		if (ret)
+			break;
+
+		tmp = val->intval >> 8;
+		ret = w1_ds2760_write(di->w1_dev, &tmp, DS2760_CURRENT_ACCUM_MSB, 1);
+
+		break;
+
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int ds2760_battery_property_is_writeable(struct power_supply *psy,
+						enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		return 1;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static enum power_supply_property ds2760_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -460,6 +510,9 @@ 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_property	= ds2760_battery_set_property;
+	di->bat.property_is_writeable =
+				  ds2760_battery_property_is_writeable;
 	di->bat.set_charged	= ds2760_battery_set_charged;
 	di->bat.external_power_changed =
 				  ds2760_battery_external_power_changed;
@@ -476,9 +529,10 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 
 	ds2760_battery_write_status(di, status);
 
-	/* set rated capacity from module param */
+	/* set rated capacity from module param (given in 10 * mAh) */
 	if (rated_capacity)
-		ds2760_battery_write_rated_capacity(di, rated_capacity);
+		ds2760_battery_write_rated_capacity(di,
+			(rated_capacity * 10) / RATED_CAPACITY_FACTOR);
 
 	/* set current accumulator if given as parameter.
 	 * this should only be done for bootstrapping the value */
-- 
1.7.1

Re: [PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

[Daniel Mack]

On Tue, May 11, 2010 at 06:38:45PM +0200, Daniel Mack wrote:
> For userspace tools and daemons, it might be necessary to adjust
> the charge_now and charge_full properties of the ds2760 battery monitor,
> for example for unavoidable corrections due to aging batteries.
> 
> For better readbility and to avoid magic numbers in conversion
> calculations, RATED_CAPACITY_FACTOR is introduced.

Forgot to squash one hunk on this one. Take the version below, please.

Daniel


>From d1a2c4e5ff53742e7db04bdf5f0009dc013ccbfb Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Tue, 23 Mar 2010 09:00:51 +0100
Subject: [PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

For userspace tools and daemons, it might be necessary to adjust
the charge_now and charge_full properties of the ds2760 battery monitor,
for example for unavoidable corrections due to aging batteries.

For better readbility and to avoid magic numbers in conversion
calculations, RATED_CAPACITY_FACTOR is introduced.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/power/ds2760_battery.c |   60 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 3bf8d1f..4edd253 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -78,6 +78,7 @@ 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. */
+#define RATED_CAPACITY_FACTOR 10
 static int rated_capacities[] = {
 	0,
 	920,	/* Samsung */
@@ -170,7 +171,8 @@ static int ds2760_battery_read_status(struct ds2760_device_info *di)
 		di->rated_capacity = rated_capacities[
 			(unsigned int)di->raw[DS2760_RATED_CAPACITY]];
 	else
-		di->rated_capacity = di->raw[DS2760_RATED_CAPACITY] * 10;
+		di->rated_capacity = di->raw[DS2760_RATED_CAPACITY] *
+						RATED_CAPACITY_FACTOR;
 
 	di->rated_capacity *= 1000; /* convert to µAh */
 
@@ -426,6 +428,54 @@ static int ds2760_battery_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int ds2760_battery_set_property(struct power_supply *psy,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
+{
+	int ret = 0;
+	unsigned char tmp;
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		/* the interface counts in uAh, convert the value */
+		ds2760_battery_write_rated_capacity(di, (val->intval / 1000L) /
+								RATED_CAPACITY_FACTOR);
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		tmp = val->intval & 0xff;
+		ret = w1_ds2760_write(di->w1_dev, &tmp, DS2760_CURRENT_ACCUM_LSB, 1);
+		if (ret)
+			break;
+
+		tmp = val->intval >> 8;
+		ret = w1_ds2760_write(di->w1_dev, &tmp, DS2760_CURRENT_ACCUM_MSB, 1);
+		break;
+
+	default:
+		ret = -EPERM;
+		break;
+	}
+
+	return ret;
+}
+
+static int ds2760_battery_property_is_writeable(struct power_supply *psy,
+						enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		return 1;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static enum power_supply_property ds2760_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -460,6 +510,9 @@ 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_property	= ds2760_battery_set_property;
+	di->bat.property_is_writeable =
+				  ds2760_battery_property_is_writeable;
 	di->bat.set_charged	= ds2760_battery_set_charged;
 	di->bat.external_power_changed =
 				  ds2760_battery_external_power_changed;
@@ -476,9 +529,10 @@ static int ds2760_battery_probe(struct platform_device *pdev)
 
 	ds2760_battery_write_status(di, status);
 
-	/* set rated capacity from module param */
+	/* set rated capacity from module param (given in 10 * mAh) */
 	if (rated_capacity)
-		ds2760_battery_write_rated_capacity(di, rated_capacity);
+		ds2760_battery_write_rated_capacity(di,
+			(rated_capacity * 10) / RATED_CAPACITY_FACTOR);
 
 	/* set current accumulator if given as parameter.
 	 * this should only be done for bootstrapping the value */
-- 
1.7.1

Re: [PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

[Anton Vorontsov]

On Tue, May 11, 2010 at 06:38:45PM +0200, Daniel Mack wrote:
[...]
> +static int ds2760_battery_set_property(struct power_supply *psy,
> +				       enum power_supply_property psp,
> +				       const union power_supply_propval *val)
> +{
> +	int ret = -EPERM;
> +	unsigned char tmp;
> +	struct ds2760_device_info *di = to_ds2760_device_info(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_FULL:
> +		/* the interface counts in uAh, convert the value */
> +		ds2760_battery_write_rated_capacity(di, (val->intval / 1000L) /
> +							RATED_CAPACITY_FACTOR);

OK, Mark hinted that you should not change rated (design)
capacity, ever. You've made CHARGE_FULL writable, which now
actually changes CHARGE_FULL_DESIGN (which reports
rated_capacity). Obviously, this isn't acceptable.

I understand that you might want to change rated capacity for
your custom battery setups, and that's fine. After all, it's
your hardware, and you have a right to break it. :-)

But to do so,

1. Could you at least change this back to
   POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, so it'll be clear what
   exactly this prop is actually changing, and

2. Can we introduce Kconfig symbol BATTERY_DS2760_UNSAFE_OPS,
   and put an #ifdefs for this property? It should be
   'default n', of course. Plus we can #ifdef the rated_capacity
   module param as well.

Thanks!

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

Re: [PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

[Daniel Mack]

On Tue, May 11, 2010 at 09:20:03PM +0400, Anton Vorontsov wrote:
> 
> On Tue, May 11, 2010 at 06:38:45PM +0200, Daniel Mack wrote:
> [...]
> > +static int ds2760_battery_set_property(struct power_supply *psy,
> > +				       enum power_supply_property psp,
> > +				       const union power_supply_propval *val)
> > +{
> > +	int ret = -EPERM;
> > +	unsigned char tmp;
> > +	struct ds2760_device_info *di = to_ds2760_device_info(psy);
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_CHARGE_FULL:
> > +		/* the interface counts in uAh, convert the value */
> > +		ds2760_battery_write_rated_capacity(di, (val->intval / 1000L) /
> > +							RATED_CAPACITY_FACTOR);
> 
> OK, Mark hinted that you should not change rated (design)
> capacity, ever. You've made CHARGE_FULL writable, which now
> actually changes CHARGE_FULL_DESIGN (which reports
> rated_capacity). Obviously, this isn't acceptable.

Of course, sorry.

> I understand that you might want to change rated capacity for
> your custom battery setups, and that's fine. After all, it's
> your hardware, and you have a right to break it. :-)

Well, if it was a hack purely for my hardware, I wouldn't share it beat
it upstream ;) I believe that this feature is something all users of
battery-driven devices need, as all batteries age.

> But to do so,
> 
> 1. Could you at least change this back to
>    POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, so it'll be clear what
>    exactly this prop is actually changing, and

Do you see any other way to store the actual rated capacity (not the one
the battery had in its good days) inside the chip?

> 2. Can we introduce Kconfig symbol BATTERY_DS2760_UNSAFE_OPS,
>    and put an #ifdefs for this property? It should be
>    'default n', of course. Plus we can #ifdef the rated_capacity
>    module param as well.

Hmm, I see that this is one way. I can do this, but I'm hessitant and
would rather go a cleaer way ...


Thanks,
Daniel

Re: [PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

[Anton Vorontsov]

On Tue, May 11, 2010 at 07:28:59PM +0200, Daniel Mack wrote:
[...]
> > 1. Could you at least change this back to
> >    POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, so it'll be clear what
> >    exactly this prop is actually changing, and
> 
> Do you see any other way to store the actual rated capacity (not the one
> the battery had in its good days) inside the chip?

In that case it should be indeed POWER_SUPPLY_PROP_CHARGE_FULL,
which is full_active_uAh, which we get from di->raw[DS2760_ACTIVE_FULL]
and di->raw[DS2760_ACTIVE_FULL + 1].

We fall back to deriving it from rated_capacity if full_active
is 0 (your case?):

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

So, you probably want to make DS2760_ACTIVE_FULL writable, not
DS2760_RATED_CAPACITY.

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

Re: [PATCH 2/3] power/ds2760_battery: make charge_now and charge_full writeable

[Daniel Mack]

(sorry for the late follow-up)

On Tue, May 11, 2010 at 10:05:51PM +0400, Anton Vorontsov wrote:
> On Tue, May 11, 2010 at 07:28:59PM +0200, Daniel Mack wrote:
> [...]
> > > 1. Could you at least change this back to
> > >    POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, so it'll be clear what
> > >    exactly this prop is actually changing, and
> > 
> > Do you see any other way to store the actual rated capacity (not the one
> > the battery had in its good days) inside the chip?
> 
> In that case it should be indeed POWER_SUPPLY_PROP_CHARGE_FULL,
> which is full_active_uAh, which we get from di->raw[DS2760_ACTIVE_FULL]
> and di->raw[DS2760_ACTIVE_FULL + 1].
> 
> We fall back to deriving it from rated_capacity if full_active
> is 0 (your case?):
> 
>         /* 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;
> 
> So, you probably want to make DS2760_ACTIVE_FULL writable, not
> DS2760_RATED_CAPACITY.

Ah of course. And DS2760_ACTIVE_FULL is 16bit wide, so we also get rid
of the range problem.

Thanks,
Daniel

[PATCH 3/3] power/ds2760_battery: use factor of 20 for rated_capacity

[Daniel Mack]

In the ds2760 driver, the currently used factor of 10 to store the rated
battery capacity internally is not sufficient for batteries > 2.55 Ah,
as the 8-bit register will overflow for bigger values.

Change the factor to 20 to broaden that range. Note that due to
RATED_CAPACITY_FACTOR, the external interface won't change, neither for
the writeable sysfs entires nor for the kernel rated_capacity module
parameter.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/power/ds2760_battery.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index b82bf92..7b3043f 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -78,7 +78,7 @@ 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. */
-#define RATED_CAPACITY_FACTOR 10
+#define RATED_CAPACITY_FACTOR 20
 static int rated_capacities[] = {
 	0,
 	920,	/* Samsung */
-- 
1.7.1

Re: [PATCH 3/3] power/ds2760_battery: use factor of 20 for rated_capacity

[Anton Vorontsov]

On Tue, May 11, 2010 at 06:38:46PM +0200, Daniel Mack wrote:
> In the ds2760 driver, the currently used factor of 10 to store the rated
> battery capacity internally is not sufficient for batteries > 2.55 Ah,
> as the 8-bit register will overflow for bigger values.
> 
> Change the factor to 20 to broaden that range. Note that due to
> RATED_CAPACITY_FACTOR, the external interface won't change, neither for
> the writeable sysfs entires nor for the kernel rated_capacity module
> parameter.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
> Cc: Matt Reimer <mreimer@vpop.net>
> Cc: Evgeniy Polyakov <zbr@ioremap.net>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---

Hi Daniel,

Sorry, it took quite a bit to get to your patches.

They look good (as usual), but I have a few concerns.

>  drivers/power/ds2760_battery.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
> index b82bf92..7b3043f 100644
> --- a/drivers/power/ds2760_battery.c
> +++ b/drivers/power/ds2760_battery.c
> @@ -78,7 +78,7 @@ 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. */
> -#define RATED_CAPACITY_FACTOR 10
> +#define RATED_CAPACITY_FACTOR 20

I'm a bit worried about this one.

Shouldn't this confuse batteries that already store rated
capacity with factor of ten? If so, please introduce a module
option.

Also, you don't update comments and module params description,
e.g.

  MODULE_PARM_DESC(rated_capacity, "rated battery capacity, 10*mAh or index");
  ....
  /* set rated capacity from module param (given in 10 * mAh) */

Is that intentionally?

Thanks!

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

Re: [PATCH 3/3] power/ds2760_battery: use factor of 20 for rated_capacity

[Daniel Mack]

On Tue, May 11, 2010 at 09:19:24PM +0400, Anton Vorontsov wrote:
> On Tue, May 11, 2010 at 06:38:46PM +0200, Daniel Mack wrote:
> >  drivers/power/ds2760_battery.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
> > index b82bf92..7b3043f 100644
> > --- a/drivers/power/ds2760_battery.c
> > +++ b/drivers/power/ds2760_battery.c
> > @@ -78,7 +78,7 @@ 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. */
> > -#define RATED_CAPACITY_FACTOR 10
> > +#define RATED_CAPACITY_FACTOR 20
> 
> I'm a bit worried about this one.
> 
> Shouldn't this confuse batteries that already store rated
> capacity with factor of ten? If so, please introduce a module
> option.
> 
> Also, you don't update comments and module params description,
> e.g.
> 
>   MODULE_PARM_DESC(rated_capacity, "rated battery capacity, 10*mAh or index");
>   ....
>   /* set rated capacity from module param (given in 10 * mAh) */
> 
> Is that intentionally?

Well, this parameter doesn't change, and hence the comment is left
untouched. If it is passed as module option, it is interpreted as 10*mAh
and converted to the internal value.

You're right though about your concern about batteries that already
stored a value given in mAh (and not as index) - this will break unless
the module is loaded with a proper module parameter. Don't know whether
this is acceptable.

Daniel

Re: [PATCH 3/3] power/ds2760_battery: use factor of 20 for rated_capacity

[Anton Vorontsov]

On Tue, May 11, 2010 at 07:25:29PM +0200, Daniel Mack wrote:
[...]
> > >  /* Some batteries have their rated capacity stored a N * 10 mAh, while
> > >   * others use an index into this table. */
> > > -#define RATED_CAPACITY_FACTOR 10
> > > +#define RATED_CAPACITY_FACTOR 20
> > 
> > I'm a bit worried about this one.
> > 
> > Shouldn't this confuse batteries that already store rated
> > capacity with factor of ten? If so, please introduce a module
> > option.
> > 
> > Also, you don't update comments and module params description,
> > e.g.
> > 
> >   MODULE_PARM_DESC(rated_capacity, "rated battery capacity, 10*mAh or index");
> >   ....
> >   /* set rated capacity from module param (given in 10 * mAh) */
> > 
> > Is that intentionally?
> 
> Well, this parameter doesn't change, and hence the comment is left
> untouched. If it is passed as module option, it is interpreted as 10*mAh
> and converted to the internal value.
> 
> You're right though about your concern about batteries that already
> stored a value given in mAh (and not as index) - this will break unless
> the module is loaded with a proper module parameter. Don't know whether
> this is acceptable.

Unfortunately, it is not. You have to do a run-time option, i.e.
module parameter (you can set up default value for this module
parameter via Kconfig symbol).

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

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Anton Vorontsov]

On Tue, May 11, 2010 at 06:38:44PM +0200, Daniel Mack wrote:
> This patch adds support for writeable power supply properties and
> exposes them as writeable to sysfs.

A long-awaited feature! Thanks!

> 
> A power supply implementation must implement two new function calls in
> order to use that feature:
> 
>   int set_property(struct power_supply *psy,
>                    enum power_supply_property psp,
>                    const union power_supply_propval *val);
> 
>   int property_is_writeable(struct power_supply *psy,

I'm not a native English speaker, but I think this should be
'writable'.

>                             enum power_supply_property psp);
[...]
>  #define POWER_SUPPLY_ATTR(_name)					\
>  {									\
> -	.attr = { .name = #_name, .mode = 0444 },	\
> +	.attr = { .name = #_name },					\
>  	.show = power_supply_show_property,				\
> -	.store = NULL,							\
> +	.store = power_supply_store_property,				\
>  }
>  
>  static struct device_attribute power_supply_attrs[];
> @@ -91,6 +91,25 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	return sprintf(buf, "%d\n", value.intval);
>  }
>  
> +static ssize_t power_supply_store_property(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t count) {
> +	ssize_t ret;
> +	struct power_supply *psy = dev_get_drvdata(dev);
> +	const ptrdiff_t off = attr - power_supply_attrs;
> +	union power_supply_propval value;
> +	long long_val;
> +
> +	/* TODO: support other types than int */
> +	ret = strict_strtol(buf, 10, &long_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	value.intval = long_val;
> +
> +	return psy->set_property(psy, off, &value);
> +}
> +
>  /* Must be in the same order as POWER_SUPPLY_PROP_* */
>  static struct device_attribute power_supply_attrs[] = {
>  	/* Properties of type `int' */
> @@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
>  	}
>  
>  	for (j = 0; j < psy->num_properties; j++) {
> +		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> +
> +		if (psy->property_is_writeable &&
> +		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
> +			mode |= S_IWUSR;
> +
> +		power_supply_attrs[psy->properties[j]].attr.mode = mode;

This is dangerous. You're changing the attr mode for all power
supplies, including already registered. I have no idea how attr
handling core will cope with that, but we'd better not check
this. :-)

Instead, change the mode to '0644' unconditionally,
and in power_supply_store_property() do something like this:
{
	if (!psy->set_property)
		return -EINVAL; (or EPERM, not sure which is better).
	....
	return psy->set_property(psy, off, &value);
	/* ^^^here set_property() should -EPERM if some property
	 * is read-only.
	 */
}

Plus, that way you don't need is_writable().

Thanks,

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

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

On Tue, May 11, 2010 at 09:47:08PM +0400, Anton Vorontsov wrote:
> On Tue, May 11, 2010 at 06:38:44PM +0200, Daniel Mack wrote:
> > A power supply implementation must implement two new function calls in
> > order to use that feature:
> > 
> >   int set_property(struct power_supply *psy,
> >                    enum power_supply_property psp,
> >                    const union power_supply_propval *val);
> > 
> >   int property_is_writeable(struct power_supply *psy,
> 
> I'm not a native English speaker, but I think this should be
> 'writable'.

Me neither, but I looked it up, and it's in fact both allowed ;)

> > +static ssize_t power_supply_store_property(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   const char *buf, size_t count) {
> > +	ssize_t ret;
> > +	struct power_supply *psy = dev_get_drvdata(dev);
> > +	const ptrdiff_t off = attr - power_supply_attrs;
> > +	union power_supply_propval value;
> > +	long long_val;
> > +
> > +	/* TODO: support other types than int */
> > +	ret = strict_strtol(buf, 10, &long_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	value.intval = long_val;
> > +
> > +	return psy->set_property(psy, off, &value);
> > +}
> > +
> >  /* Must be in the same order as POWER_SUPPLY_PROP_* */
> >  static struct device_attribute power_supply_attrs[] = {
> >  	/* Properties of type `int' */
> > @@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
> >  	}
> >  
> >  	for (j = 0; j < psy->num_properties; j++) {
> > +		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> > +
> > +		if (psy->property_is_writeable &&
> > +		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
> > +			mode |= S_IWUSR;
> > +
> > +		power_supply_attrs[psy->properties[j]].attr.mode = mode;
> 
> This is dangerous. You're changing the attr mode for all power
> supplies, including already registered. I have no idea how attr
> handling core will cope with that, but we'd better not check
> this. :-)

Hmm, no. The code defaults to (S_IRUSR | S_IRGRP | S_IROTH) which is
0444, just like it was before. So there shouldn't be any regression. The
mode is only changed if the psy defines a property_is_writeable()
callback which returns 1. Or do I miss your point?

> Instead, change the mode to '0644' unconditionally,
> and in power_supply_store_property() do something like this:
> {
> 	if (!psy->set_property)
> 		return -EINVAL; (or EPERM, not sure which is better).
> 	....
> 	return psy->set_property(psy, off, &value);
> 	/* ^^^here set_property() should -EPERM if some property
> 	 * is read-only.
> 	 */
> }
> 
> Plus, that way you don't need is_writable().

Ugh, really? I would _much_ prefer to actually _see_ which property is
writeable, just from looking at the file attributes in sysfs.

Thanks,
Daniel

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Anton Vorontsov]

On Tue, May 11, 2010 at 07:58:12PM +0200, Daniel Mack wrote:
[...]
> Hmm, no. The code defaults to (S_IRUSR | S_IRGRP | S_IROTH) which is
> 0444, just like it was before. So there shouldn't be any regression. The
> mode is only changed if the psy defines a property_is_writeable()
> callback which returns 1. Or do I miss your point?

Yes, power_supply_attrs is a global array, and you shouldn't change
it between power_supply_register() calls.

If you don't see why it's a bad idea in general, think about it
other way, a race:

...someone registers psy0 with attr X marked as read-only...
...code flow stops before device_create_file(psy0, global->mode)..
[preempt]
...someone registers psy1 with attr X marked as writable...
...you set up global->mode to 0644...
[preempt again]
...we end up calling device_create_file(psy0, 0644)...

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

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

On Tue, May 11, 2010 at 10:23:47PM +0400, Anton Vorontsov wrote:
> On Tue, May 11, 2010 at 07:58:12PM +0200, Daniel Mack wrote:
> [...]
> > Hmm, no. The code defaults to (S_IRUSR | S_IRGRP | S_IROTH) which is
> > 0444, just like it was before. So there shouldn't be any regression. The
> > mode is only changed if the psy defines a property_is_writeable()
> > callback which returns 1. Or do I miss your point?
> 
> Yes, power_supply_attrs is a global array, and you shouldn't change
> it between power_supply_register() calls.
> 
> If you don't see why it's a bad idea in general, think about it
> other way, a race:
> 
> ...someone registers psy0 with attr X marked as read-only...
> ...code flow stops before device_create_file(psy0, global->mode)..
> [preempt]
> ...someone registers psy1 with attr X marked as writable...
> ...you set up global->mode to 0644...
> [preempt again]
> ...we end up calling device_create_file(psy0, 0644)...

Ah, I see. But the struct passed in is just used as template, right? So
for the particular case you outlined, a simple lock should do the trick,
right? I would prefer this over always-writeable file entries.

Daniel

[PATCH] Introduce {sysfs,device}_create_file_mode

[Anton Vorontsov]

We need to create attributes with different modes across devices.
We can do this by modifying attr.mode between device_create_file
invocations, but that is racy in case of globally defined attrs.

Luckily, there's sysfs_add_file_mode() function that seems to do
exactly what we want, and if we use it, we don't need any locks
to avoid races. Though, it isn't exposed via device-drivers core
API.

Now that we need it, introduce sysfs_create_file_mode()
and device_create_file_mode() functions. With these, creating
attributes with different modes is a joy.

Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
---

On Wed, May 12, 2010 at 12:28:25AM +0200, Daniel Mack wrote:
> On Tue, May 11, 2010 at 10:23:47PM +0400, Anton Vorontsov wrote:
> > Yes, power_supply_attrs is a global array, and you shouldn't change
> > it between power_supply_register() calls.
> > 
> > If you don't see why it's a bad idea in general, think about it
> > other way, a race:
> > 
> > ...someone registers psy0 with attr X marked as read-only...
> > ...code flow stops before device_create_file(psy0, global->mode)..
> > [preempt]
> > ...someone registers psy1 with attr X marked as writable...
> > ...you set up global->mode to 0644...
> > [preempt again]
> > ...we end up calling device_create_file(psy0, 0644)...
> 
> Ah, I see. But the struct passed in is just used as template, right?

In general, you can't assume that it's just a template,
otherwise 'const ptrdiff_t off = attr - power_supply_attrs;'
wouldn't work.

I.e. 'attr' points to exact attribute you specified during
registration. Weather its fields are used as a template or
not, we don't know (well, we know how the sysfs core uses them
*today*, but we don't know how sysfs will use them tomorrow).

> So
> for the particular case you outlined, a simple lock should do the trick,
> right?

Yes, for the particular case.

Still, I don't think that playing with attr->mode is a good
idea.

If we don't want to use attr->mode for mode specifier (and I
think we don't), we must implement a clean API for such a case.

Luckily, there's sysfs_add_file_mode, so if anyone will want
to change attr->mode behaviour, one will have to deal with
that function, and so we'll be safe.

And we must use that function, not just silently play
attr->mode games during registration. That also avoids
any need for locking.


Greg, does the patch look OK? If so, I'd like it to go
via battery-2.6.git tree, along with patches that need
that one.

You can see the original thread here:
http://lkml.org/lkml/2010/5/11/71

See power_supply_create_attrs(), there we'll use
device_create_file_mode() instead of messing with
power_supply_attrs[psy->properties[j]].attr.mode.

 drivers/base/core.c    |   21 +++++++++++++++++++++
 fs/sysfs/file.c        |   16 ++++++++++++++++
 include/linux/device.h |    3 +++
 include/linux/sysfs.h  |   10 ++++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index b56a0ba..1bc6134 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -440,6 +440,27 @@ struct kset *devices_kset;
  * device_create_file - create sysfs attribute file for device.
  * @dev: device.
  * @attr: device attribute descriptor.
+ * @mode: file mode.
+ *
+ * This function is similar to device_create_file(), except that it
+ * doesn't use attr->mode for mode specifier.
+ */
+int device_create_file_mode(struct device *dev,
+			    const struct device_attribute *attr,
+			    mode_t mode)
+{
+	int error = 0;
+
+	if (dev)
+		error = sysfs_create_file_mode(&dev->kobj, &attr->attr, mode);
+	return error;
+}
+EXPORT_SYMBOL_GPL(device_create_file_mode);
+
+/**
+ * device_create_file - create sysfs attribute file for device.
+ * @dev: device.
+ * @attr: device attribute descriptor.
  */
 int device_create_file(struct device *dev,
 		       const struct device_attribute *attr)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index e222b25..7bebb60 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -528,6 +528,22 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd, const struct attribute *attr,
 	return sysfs_add_file_mode(dir_sd, attr, type, attr->mode);
 }
 
+/**
+ *	sysfs_create_file_mode - create an attribute file for an object.
+ *	@kobj:	object we're creating for.
+ *	@attr:	attribute descriptor.
+ *	@mode:	file mode.
+ *
+ *	This function is similar to sysfs_create_file(), except that it
+ *	doesn't use attr->mode for mode specifier.
+ */
+int sysfs_create_file_mode(struct kobject *kobj, const struct attribute *attr,
+			   mode_t mode)
+{
+	BUG_ON(!kobj || !kobj->sd || !attr);
+
+	return sysfs_add_file_mode(kobj->sd, attr, SYSFS_KOBJ_ATTR, mode);
+}
 
 /**
  *	sysfs_create_file - create an attribute file for an object.
diff --git a/include/linux/device.h b/include/linux/device.h
index 1821928..8277364 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -337,6 +337,9 @@ struct device_attribute {
 #define DEVICE_ATTR(_name, _mode, _show, _store) \
 struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)
 
+extern int __must_check device_create_file_mode(struct device *device,
+					const struct device_attribute *entry,
+					mode_t mode);
 extern int __must_check device_create_file(struct device *device,
 					const struct device_attribute *entry);
 extern void device_remove_file(struct device *dev,
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f0496b3..222774f 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -130,6 +130,9 @@ int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);
 int __must_check sysfs_move_dir(struct kobject *kobj,
 				struct kobject *new_parent_kobj);
 
+int __must_check sysfs_create_file_mode(struct kobject *kobj,
+					const struct attribute *attr,
+					mode_t mode);
 int __must_check sysfs_create_file(struct kobject *kobj,
 				   const struct attribute *attr);
 int __must_check sysfs_create_files(struct kobject *kobj,
@@ -202,6 +205,13 @@ static inline int sysfs_move_dir(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_create_file_mode(struct kobject *kobj,
+					 const struct attribute *attr,
+					 mode_t mode)
+{
+	return 0;
+}
+
 static inline int sysfs_create_file(struct kobject *kobj,
 				    const struct attribute *attr)
 {
-- 
1.7.0.5

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Daniel Mack]

On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> We need to create attributes with different modes across devices.
> We can do this by modifying attr.mode between device_create_file
> invocations, but that is racy in case of globally defined attrs.
> 
> Luckily, there's sysfs_add_file_mode() function that seems to do
> exactly what we want, and if we use it, we don't need any locks
> to avoid races. Though, it isn't exposed via device-drivers core
> API.
> 
> Now that we need it, introduce sysfs_create_file_mode()
> and device_create_file_mode() functions. With these, creating
> attributes with different modes is a joy.

Ah, nice. Thanks for caring! If this is accepted, I'll respin my patches
and make them use it.

Thanks,
Daniel


> 
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> ---
> 
> On Wed, May 12, 2010 at 12:28:25AM +0200, Daniel Mack wrote:
> > On Tue, May 11, 2010 at 10:23:47PM +0400, Anton Vorontsov wrote:
> > > Yes, power_supply_attrs is a global array, and you shouldn't change
> > > it between power_supply_register() calls.
> > > 
> > > If you don't see why it's a bad idea in general, think about it
> > > other way, a race:
> > > 
> > > ...someone registers psy0 with attr X marked as read-only...
> > > ...code flow stops before device_create_file(psy0, global->mode)..
> > > [preempt]
> > > ...someone registers psy1 with attr X marked as writable...
> > > ...you set up global->mode to 0644...
> > > [preempt again]
> > > ...we end up calling device_create_file(psy0, 0644)...
> > 
> > Ah, I see. But the struct passed in is just used as template, right?
> 
> In general, you can't assume that it's just a template,
> otherwise 'const ptrdiff_t off = attr - power_supply_attrs;'
> wouldn't work.
> 
> I.e. 'attr' points to exact attribute you specified during
> registration. Weather its fields are used as a template or
> not, we don't know (well, we know how the sysfs core uses them
> *today*, but we don't know how sysfs will use them tomorrow).
> 
> > So
> > for the particular case you outlined, a simple lock should do the trick,
> > right?
> 
> Yes, for the particular case.
> 
> Still, I don't think that playing with attr->mode is a good
> idea.
> 
> If we don't want to use attr->mode for mode specifier (and I
> think we don't), we must implement a clean API for such a case.
> 
> Luckily, there's sysfs_add_file_mode, so if anyone will want
> to change attr->mode behaviour, one will have to deal with
> that function, and so we'll be safe.
> 
> And we must use that function, not just silently play
> attr->mode games during registration. That also avoids
> any need for locking.
> 
> 
> Greg, does the patch look OK? If so, I'd like it to go
> via battery-2.6.git tree, along with patches that need
> that one.
> 
> You can see the original thread here:
> http://lkml.org/lkml/2010/5/11/71
> 
> See power_supply_create_attrs(), there we'll use
> device_create_file_mode() instead of messing with
> power_supply_attrs[psy->properties[j]].attr.mode.
> 
>  drivers/base/core.c    |   21 +++++++++++++++++++++
>  fs/sysfs/file.c        |   16 ++++++++++++++++
>  include/linux/device.h |    3 +++
>  include/linux/sysfs.h  |   10 ++++++++++
>  4 files changed, 50 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b56a0ba..1bc6134 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -440,6 +440,27 @@ struct kset *devices_kset;
>   * device_create_file - create sysfs attribute file for device.
>   * @dev: device.
>   * @attr: device attribute descriptor.
> + * @mode: file mode.
> + *
> + * This function is similar to device_create_file(), except that it
> + * doesn't use attr->mode for mode specifier.
> + */
> +int device_create_file_mode(struct device *dev,
> +			    const struct device_attribute *attr,
> +			    mode_t mode)
> +{
> +	int error = 0;
> +
> +	if (dev)
> +		error = sysfs_create_file_mode(&dev->kobj, &attr->attr, mode);
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(device_create_file_mode);
> +
> +/**
> + * device_create_file - create sysfs attribute file for device.
> + * @dev: device.
> + * @attr: device attribute descriptor.
>   */
>  int device_create_file(struct device *dev,
>  		       const struct device_attribute *attr)
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index e222b25..7bebb60 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -528,6 +528,22 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd, const struct attribute *attr,
>  	return sysfs_add_file_mode(dir_sd, attr, type, attr->mode);
>  }
>  
> +/**
> + *	sysfs_create_file_mode - create an attribute file for an object.
> + *	@kobj:	object we're creating for.
> + *	@attr:	attribute descriptor.
> + *	@mode:	file mode.
> + *
> + *	This function is similar to sysfs_create_file(), except that it
> + *	doesn't use attr->mode for mode specifier.
> + */
> +int sysfs_create_file_mode(struct kobject *kobj, const struct attribute *attr,
> +			   mode_t mode)
> +{
> +	BUG_ON(!kobj || !kobj->sd || !attr);
> +
> +	return sysfs_add_file_mode(kobj->sd, attr, SYSFS_KOBJ_ATTR, mode);
> +}
>  
>  /**
>   *	sysfs_create_file - create an attribute file for an object.
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1821928..8277364 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -337,6 +337,9 @@ struct device_attribute {
>  #define DEVICE_ATTR(_name, _mode, _show, _store) \
>  struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)
>  
> +extern int __must_check device_create_file_mode(struct device *device,
> +					const struct device_attribute *entry,
> +					mode_t mode);
>  extern int __must_check device_create_file(struct device *device,
>  					const struct device_attribute *entry);
>  extern void device_remove_file(struct device *dev,
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index f0496b3..222774f 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -130,6 +130,9 @@ int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);
>  int __must_check sysfs_move_dir(struct kobject *kobj,
>  				struct kobject *new_parent_kobj);
>  
> +int __must_check sysfs_create_file_mode(struct kobject *kobj,
> +					const struct attribute *attr,
> +					mode_t mode);
>  int __must_check sysfs_create_file(struct kobject *kobj,
>  				   const struct attribute *attr);
>  int __must_check sysfs_create_files(struct kobject *kobj,
> @@ -202,6 +205,13 @@ static inline int sysfs_move_dir(struct kobject *kobj,
>  	return 0;
>  }
>  
> +static inline int sysfs_create_file_mode(struct kobject *kobj,
> +					 const struct attribute *attr,
> +					 mode_t mode)
> +{
> +	return 0;
> +}
> +
>  static inline int sysfs_create_file(struct kobject *kobj,
>  				    const struct attribute *attr)
>  {
> -- 
> 1.7.0.5
> 

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Greg KH]

On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> We need to create attributes with different modes across devices.
> We can do this by modifying attr.mode between device_create_file
> invocations, but that is racy in case of globally defined attrs.
> 
> Luckily, there's sysfs_add_file_mode() function that seems to do
> exactly what we want, and if we use it, we don't need any locks
> to avoid races. Though, it isn't exposed via device-drivers core
> API.

But you race the creation of the device notifying userspace, and then
the file being created, right?  Or is that properly taken care of
elsewhere?

> Greg, does the patch look OK? If so, I'd like it to go
> via battery-2.6.git tree, along with patches that need
> that one.

Looks good with one minor problem:

> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -440,6 +440,27 @@ struct kset *devices_kset;
>   * device_create_file - create sysfs attribute file for device.

That should be "device_create_file_mode"

Make that change and then feel free to take it through your tree with
an:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
on it.

thanks,

greg k-h

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Anton Vorontsov]

On Wed, May 12, 2010 at 11:38:06AM -0700, Greg KH wrote:
> On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> > We need to create attributes with different modes across devices.
> > We can do this by modifying attr.mode between device_create_file
> > invocations, but that is racy in case of globally defined attrs.
> > 
> > Luckily, there's sysfs_add_file_mode() function that seems to do
> > exactly what we want, and if we use it, we don't need any locks
> > to avoid races. Though, it isn't exposed via device-drivers core
> > API.
> 
> But you race the creation of the device notifying userspace, and then
> the file being created, right?

Yep, you've raised that question once, like 3 years ago. :-)

http://lkml.org/lkml/2007/4/11/452
http://lkml.org/lkml/2007/4/12/144

In short: we can't use attr groups since the attributes creation
is conditional. And we especially don't want to use the attr groups
for attrs with different modes. But it's not a problem, because...

> Or is that properly taken care of elsewhere?

Yep, userland should just listen to uevents, we're sending
'changed' event at the end of the registration. Attributes
may appear and disappear dynamically, battery handling
userland have to cope with that anyway.

> > Greg, does the patch look OK? If so, I'd like it to go
> > via battery-2.6.git tree, along with patches that need
> > that one.
> 
> Looks good with one minor problem:
> 
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -440,6 +440,27 @@ struct kset *devices_kset;
> >   * device_create_file - create sysfs attribute file for device.
> 
> That should be "device_create_file_mode"

Thanks for catching! Will change.

> Make that change and then feel free to take it through your tree with
> an:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> on it.

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

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Kay Sievers]

On Wed, May 12, 2010 at 21:08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Wed, May 12, 2010 at 11:38:06AM -0700, Greg KH wrote:
>> On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
>> > We need to create attributes with different modes across devices.
>> > We can do this by modifying attr.mode between device_create_file
>> > invocations, but that is racy in case of globally defined attrs.
>> >
>> > Luckily, there's sysfs_add_file_mode() function that seems to do
>> > exactly what we want, and if we use it, we don't need any locks
>> > to avoid races. Though, it isn't exposed via device-drivers core
>> > API.
>>
>> But you race the creation of the device notifying userspace, and then
>> the file being created, right?
>
> Yep, you've raised that question once, like 3 years ago. :-)
>
> http://lkml.org/lkml/2007/4/11/452
> http://lkml.org/lkml/2007/4/12/144
>
> In short: we can't use attr groups since the attributes creation
> is conditional. And we especially don't want to use the attr groups
> for attrs with different modes. But it's not a problem, because...

Groups have a filter callback for every member, to decide if the
attribute should be created or not.

Kay

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Greg KH]

On Wed, May 12, 2010 at 09:12:46PM +0200, Kay Sievers wrote:
> On Wed, May 12, 2010 at 21:08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Wed, May 12, 2010 at 11:38:06AM -0700, Greg KH wrote:
> >> On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> >> > We need to create attributes with different modes across devices.
> >> > We can do this by modifying attr.mode between device_create_file
> >> > invocations, but that is racy in case of globally defined attrs.
> >> >
> >> > Luckily, there's sysfs_add_file_mode() function that seems to do
> >> > exactly what we want, and if we use it, we don't need any locks
> >> > to avoid races. Though, it isn't exposed via device-drivers core
> >> > API.
> >>
> >> But you race the creation of the device notifying userspace, and then
> >> the file being created, right?
> >
> > Yep, you've raised that question once, like 3 years ago. :-)
> >
> > http://lkml.org/lkml/2007/4/11/452
> > http://lkml.org/lkml/2007/4/12/144
> >
> > In short: we can't use attr groups since the attributes creation
> > is conditional. And we especially don't want to use the attr groups
> > for attrs with different modes. But it's not a problem, because...
> 
> Groups have a filter callback for every member, to decide if the
> attribute should be created or not.

Yeah, that's how we solved that issue a long time ago :)

Can you please switch to using attribute groups now, so that userspace
doesn't have to handle 'change' events?

thanks,

greg k-h

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Anton Vorontsov]

On Wed, May 12, 2010 at 12:19:55PM -0700, Greg KH wrote:
> On Wed, May 12, 2010 at 09:12:46PM +0200, Kay Sievers wrote:
> > On Wed, May 12, 2010 at 21:08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > > On Wed, May 12, 2010 at 11:38:06AM -0700, Greg KH wrote:
> > >> On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> > >> > We need to create attributes with different modes across devices.
> > >> > We can do this by modifying attr.mode between device_create_file
> > >> > invocations, but that is racy in case of globally defined attrs.
> > >> >
> > >> > Luckily, there's sysfs_add_file_mode() function that seems to do
> > >> > exactly what we want, and if we use it, we don't need any locks
> > >> > to avoid races. Though, it isn't exposed via device-drivers core
> > >> > API.
> > >>
> > >> But you race the creation of the device notifying userspace, and then
> > >> the file being created, right?
> > >
> > > Yep, you've raised that question once, like 3 years ago. :-)
> > >
> > > http://lkml.org/lkml/2007/4/11/452
> > > http://lkml.org/lkml/2007/4/12/144
> > >
> > > In short: we can't use attr groups since the attributes creation
> > > is conditional. And we especially don't want to use the attr groups
> > > for attrs with different modes. But it's not a problem, because...
> > 
> > Groups have a filter callback for every member, to decide if the
> > attribute should be created or not.
> 
> Yeah, that's how we solved that issue a long time ago :)

But not loud enough for us to notice. ;-)

> Can you please switch to using attribute groups now, so that userspace
> doesn't have to handle 'change' events?

Yep.

Thanks,

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

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Anton Vorontsov]

On Wed, May 12, 2010 at 09:12:46PM +0200, Kay Sievers wrote:
> On Wed, May 12, 2010 at 21:08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Wed, May 12, 2010 at 11:38:06AM -0700, Greg KH wrote:
> >> On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> >> > We need to create attributes with different modes across devices.
> >> > We can do this by modifying attr.mode between device_create_file
> >> > invocations, but that is racy in case of globally defined attrs.
> >> >
> >> > Luckily, there's sysfs_add_file_mode() function that seems to do
> >> > exactly what we want, and if we use it, we don't need any locks
> >> > to avoid races. Though, it isn't exposed via device-drivers core
> >> > API.
> >>
> >> But you race the creation of the device notifying userspace, and then
> >> the file being created, right?
> >
> > Yep, you've raised that question once, like 3 years ago. :-)
> >
> > http://lkml.org/lkml/2007/4/11/452
> > http://lkml.org/lkml/2007/4/12/144
> >
> > In short: we can't use attr groups since the attributes creation
> > is conditional. And we especially don't want to use the attr groups
> > for attrs with different modes. But it's not a problem, because...
> 
> Groups have a filter callback for every member, to decide if the
> attribute should be created or not.

Thanks Kay. It seems the callback was added just a few months
after the discussion above. ;-)

And this commit looks especially cool:

commit 0f4238958d28044b335644b69df6071cdb04b5ce
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Thu Mar 20 20:47:52 2008 -0500

    [SCSI] sysfs: make group is_valid return a mode_t

Daniel, I think that today we can just use the attribute group
mechanism, it has all needed. Do you want me to prepare a patch
to convert existing attributes, or do you to try it yourself?

Just asking to not duplicate efforts.

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

Re: [PATCH] Introduce {sysfs,device}_create_file_mode

[Daniel Mack]

On Wed, May 12, 2010 at 11:30:50PM +0400, Anton Vorontsov wrote:
> On Wed, May 12, 2010 at 09:12:46PM +0200, Kay Sievers wrote:
> > On Wed, May 12, 2010 at 21:08, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > > On Wed, May 12, 2010 at 11:38:06AM -0700, Greg KH wrote:
> > >> On Wed, May 12, 2010 at 10:15:46PM +0400, Anton Vorontsov wrote:
> > >> > We need to create attributes with different modes across devices.
> > >> > We can do this by modifying attr.mode between device_create_file
> > >> > invocations, but that is racy in case of globally defined attrs.
> > >> >
> > >> > Luckily, there's sysfs_add_file_mode() function that seems to do
> > >> > exactly what we want, and if we use it, we don't need any locks
> > >> > to avoid races. Though, it isn't exposed via device-drivers core
> > >> > API.
> > >>
> > >> But you race the creation of the device notifying userspace, and then
> > >> the file being created, right?
> > >
> > > Yep, you've raised that question once, like 3 years ago. :-)
> > >
> > > http://lkml.org/lkml/2007/4/11/452
> > > http://lkml.org/lkml/2007/4/12/144
> > >
> > > In short: we can't use attr groups since the attributes creation
> > > is conditional. And we especially don't want to use the attr groups
> > > for attrs with different modes. But it's not a problem, because...
> > 
> > Groups have a filter callback for every member, to decide if the
> > attribute should be created or not.
> 
> Thanks Kay. It seems the callback was added just a few months
> after the discussion above. ;-)
> 
> And this commit looks especially cool:
> 
> commit 0f4238958d28044b335644b69df6071cdb04b5ce
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Thu Mar 20 20:47:52 2008 -0500
> 
>     [SCSI] sysfs: make group is_valid return a mode_t
> 
> Daniel, I think that today we can just use the attribute group
> mechanism, it has all needed. Do you want me to prepare a patch
> to convert existing attributes, or do you to try it yourself?

Would be good if you did that, just to see how this is interface meant
to be used exactly. I'll adopt my patches accordingly then.

Thanks,
Daniel

[PATCH] power_supply: Use attribute groups

[Anton Vorontsov]

This fixes a race between power supply device and initial
attributes creation, plus makes it possible to implement
writable properties.

Suggested-by: Greg KH <gregkh@suse.de>
Suggested-by: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
---

On Thu, May 13, 2010 at 11:33:04AM +0200, Daniel Mack wrote:
[...]
> > > > In short: we can't use attr groups since the attributes creation
> > > > is conditional. And we especially don't want to use the attr groups
> > > > for attrs with different modes. But it's not a problem, because...
> > > 
> > > Groups have a filter callback for every member, to decide if the
> > > attribute should be created or not.
> > 
> > Thanks Kay. It seems the callback was added just a few months
> > after the discussion above. ;-)
[...]
> > Daniel, I think that today we can just use the attribute group
> > mechanism, it has all needed. Do you want me to prepare a patch
> > to convert existing attributes, or do you to try it yourself?
> 
> Would be good if you did that, just to see how this is interface meant
> to be used exactly. I'll adopt my patches accordingly then.

OK. It took me quite some time as my hx4700 refused to boot.
I still don't know why it doesn't boot, but I implemented
test_power driver and was able to test the draft.

Daniel, it works for me, and implementing writable properties
should be straightforward now (see is_visible callback).

Thanks,

 drivers/power/power_supply.h       |    7 +--
 drivers/power/power_supply_core.c  |   48 +++++++++++-----
 drivers/power/power_supply_sysfs.c |  114 ++++++++++++------------------------
 include/linux/power_supply.h       |    1 +
 4 files changed, 75 insertions(+), 95 deletions(-)

diff --git a/drivers/power/power_supply.h b/drivers/power/power_supply.h
index f38ba48..018de2b 100644
--- a/drivers/power/power_supply.h
+++ b/drivers/power/power_supply.h
@@ -12,15 +12,12 @@
 
 #ifdef CONFIG_SYSFS
 
-extern int power_supply_create_attrs(struct power_supply *psy);
-extern void power_supply_remove_attrs(struct power_supply *psy);
+extern void power_supply_init_attrs(struct device_type *dev_type);
 extern int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env);
 
 #else
 
-static inline int power_supply_create_attrs(struct power_supply *psy)
-{ return 0; }
-static inline void power_supply_remove_attrs(struct power_supply *psy) {}
+static inline void power_supply_init_attrs(struct device_type *dev_type) {}
 #define power_supply_uevent NULL
 
 #endif /* CONFIG_SYSFS */
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index cce75b4..91606bb 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/init.h>
+#include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/power_supply.h>
@@ -22,6 +23,8 @@
 struct class *power_supply_class;
 EXPORT_SYMBOL_GPL(power_supply_class);
 
+static struct device_type power_supply_dev_type;
+
 static int __power_supply_changed_work(struct device *dev, void *data)
 {
 	struct power_supply *psy = (struct power_supply *)data;
@@ -144,22 +147,39 @@ struct power_supply *power_supply_get_by_name(char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+static void power_supply_dev_release(struct device *dev)
+{
+	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
+	kfree(dev);
+}
+
 int power_supply_register(struct device *parent, struct power_supply *psy)
 {
-	int rc = 0;
+	struct device *dev;
+	int rc;
 
-	psy->dev = device_create(power_supply_class, parent, 0, psy,
-				 "%s", psy->name);
-	if (IS_ERR(psy->dev)) {
-		rc = PTR_ERR(psy->dev);
-		goto dev_create_failed;
-	}
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 
-	INIT_WORK(&psy->changed_work, power_supply_changed_work);
+	device_initialize(dev);
 
-	rc = power_supply_create_attrs(psy);
+	dev->class = power_supply_class;
+	dev->type = &power_supply_dev_type;
+	dev->parent = parent;
+	dev->release = power_supply_dev_release;
+	dev_set_drvdata(dev, psy);
+	psy->dev = dev;
+
+	rc = kobject_set_name(&dev->kobj, "%s", psy->name);
+	if (rc)
+		goto kobject_set_name_failed;
+
+	rc = device_add(dev);
 	if (rc)
-		goto create_attrs_failed;
+		goto device_add_failed;
+
+	INIT_WORK(&psy->changed_work, power_supply_changed_work);
 
 	rc = power_supply_create_triggers(psy);
 	if (rc)
@@ -170,10 +190,10 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 	goto success;
 
 create_triggers_failed:
-	power_supply_remove_attrs(psy);
-create_attrs_failed:
 	device_unregister(psy->dev);
-dev_create_failed:
+kobject_set_name_failed:
+device_add_failed:
+	kfree(dev);
 success:
 	return rc;
 }
@@ -183,7 +203,6 @@ void power_supply_unregister(struct power_supply *psy)
 {
 	flush_scheduled_work();
 	power_supply_remove_triggers(psy);
-	power_supply_remove_attrs(psy);
 	device_unregister(psy->dev);
 }
 EXPORT_SYMBOL_GPL(power_supply_unregister);
@@ -196,6 +215,7 @@ static int __init power_supply_class_init(void)
 		return PTR_ERR(power_supply_class);
 
 	power_supply_class->dev_uevent = power_supply_uevent;
+	power_supply_init_attrs(&power_supply_dev_type);
 
 	return 0;
 }
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 5b6e352..f95d934 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -41,6 +41,9 @@ static struct device_attribute power_supply_attrs[];
 static ssize_t power_supply_show_property(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf) {
+	static char *type_text[] = {
+		"Battery", "UPS", "Mains", "USB"
+	};
 	static char *status_text[] = {
 		"Unknown", "Charging", "Discharging", "Not charging", "Full"
 	};
@@ -58,12 +61,15 @@ static ssize_t power_supply_show_property(struct device *dev,
 	static char *capacity_level_text[] = {
 		"Unknown", "Critical", "Low", "Normal", "High", "Full"
 	};
-	ssize_t ret;
+	ssize_t ret = 0;
 	struct power_supply *psy = dev_get_drvdata(dev);
 	const ptrdiff_t off = attr - power_supply_attrs;
 	union power_supply_propval value;
 
-	ret = psy->get_property(psy, off, &value);
+	if (off == POWER_SUPPLY_PROP_TYPE)
+		value.intval = psy->type;
+	else
+		ret = psy->get_property(psy, off, &value);
 
 	if (ret < 0) {
 		if (ret == -ENODATA)
@@ -85,10 +91,13 @@ static ssize_t power_supply_show_property(struct device *dev,
 		return sprintf(buf, "%s\n", technology_text[value.intval]);
 	else if (off == POWER_SUPPLY_PROP_CAPACITY_LEVEL)
 		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
+	else if (off == POWER_SUPPLY_PROP_TYPE)
+		return sprintf(buf, "%s\n", type_text[value.intval]);
 	else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
 		return sprintf(buf, "%s\n", value.strval);
 
 	return sprintf(buf, "%d\n", value.intval);
+return 0;
 }
 
 /* Must be in the same order as POWER_SUPPLY_PROP_* */
@@ -132,67 +141,50 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(time_to_empty_avg),
 	POWER_SUPPLY_ATTR(time_to_full_now),
 	POWER_SUPPLY_ATTR(time_to_full_avg),
+	POWER_SUPPLY_ATTR(type),
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_ATTR(model_name),
 	POWER_SUPPLY_ATTR(manufacturer),
 	POWER_SUPPLY_ATTR(serial_number),
 };
 
-static ssize_t power_supply_show_static_attrs(struct device *dev,
-					      struct device_attribute *attr,
-					      char *buf) {
-	static char *type_text[] = { "Battery", "UPS", "Mains", "USB" };
-	struct power_supply *psy = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%s\n", type_text[psy->type]);
-}
-
-static struct device_attribute power_supply_static_attrs[] = {
-	__ATTR(type, 0444, power_supply_show_static_attrs, NULL),
-};
+static struct attribute *
+__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
 
-int power_supply_create_attrs(struct power_supply *psy)
+static mode_t power_supply_attr_is_visible(struct kobject *kobj,
+					   struct attribute *attr,
+					   int attrno)
 {
-	int rc = 0;
-	int i, j;
-
-	for (i = 0; i < ARRAY_SIZE(power_supply_static_attrs); i++) {
-		rc = device_create_file(psy->dev,
-			    &power_supply_static_attrs[i]);
-		if (rc)
-			goto statics_failed;
-	}
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct power_supply *psy = dev_get_drvdata(dev);
+	int i;
 
-	for (j = 0; j < psy->num_properties; j++) {
-		rc = device_create_file(psy->dev,
-			    &power_supply_attrs[psy->properties[j]]);
-		if (rc)
-			goto dynamics_failed;
+	for (i = 0; i < psy->num_properties; i++) {
+		if (psy->properties[i] == attrno)
+			return 0444;
 	}
 
-	goto succeed;
-
-dynamics_failed:
-	while (j--)
-		device_remove_file(psy->dev,
-			   &power_supply_attrs[psy->properties[j]]);
-statics_failed:
-	while (i--)
-		device_remove_file(psy->dev, &power_supply_static_attrs[i]);
-succeed:
-	return rc;
+	return 0;
 }
 
-void power_supply_remove_attrs(struct power_supply *psy)
+static struct attribute_group power_supply_attr_group = {
+	.attrs = __power_supply_attrs,
+	.is_visible = power_supply_attr_is_visible,
+};
+
+const struct attribute_group *power_supply_attr_groups[] = {
+	&power_supply_attr_group,
+	NULL,
+};
+
+void power_supply_init_attrs(struct device_type *dev_type)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(power_supply_static_attrs); i++)
-		device_remove_file(psy->dev, &power_supply_static_attrs[i]);
+	dev_type->groups = power_supply_attr_groups;
 
-	for (i = 0; i < psy->num_properties; i++)
-		device_remove_file(psy->dev,
-			    &power_supply_attrs[psy->properties[i]]);
+	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++)
+		__power_supply_attrs[i] = &power_supply_attrs[i].attr;
 }
 
 static char *kstruprdup(const char *str, gfp_t gfp)
@@ -236,36 +228,6 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (!prop_buf)
 		return -ENOMEM;
 
-	for (j = 0; j < ARRAY_SIZE(power_supply_static_attrs); j++) {
-		struct device_attribute *attr;
-		char *line;
-
-		attr = &power_supply_static_attrs[j];
-
-		ret = power_supply_show_static_attrs(dev, attr, prop_buf);
-		if (ret < 0)
-			goto out;
-
-		line = strchr(prop_buf, '\n');
-		if (line)
-			*line = 0;
-
-		attrname = kstruprdup(attr->attr.name, GFP_KERNEL);
-		if (!attrname) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		dev_dbg(dev, "Static prop %s=%s\n", attrname, prop_buf);
-
-		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s", attrname, prop_buf);
-		kfree(attrname);
-		if (ret)
-			goto out;
-	}
-
-	dev_dbg(dev, "%zd dynamic props\n", psy->num_properties);
-
 	for (j = 0; j < psy->num_properties; j++) {
 		struct device_attribute *attr;
 		char *line;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index ebd2b8f..c5f73a3 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -114,6 +114,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_AVG,
+	POWER_SUPPLY_PROP_TYPE, /* use power_supply.type instead */
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
-- 
1.7.0.1

Re: [PATCH] power_supply: Use attribute groups

[Daniel Mack]

Hi Anton,

On Mon, May 17, 2010 at 11:40:16PM +0400, Anton Vorontsov wrote:
> This fixes a race between power supply device and initial
> attributes creation, plus makes it possible to implement
> writable properties.

Thanks a lot. This works for me, but I have two minor comments below ...

[...]

> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index 5b6e352..f95d934 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -41,6 +41,9 @@ static struct device_attribute power_supply_attrs[];
>  static ssize_t power_supply_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
> +	static char *type_text[] = {
> +		"Battery", "UPS", "Mains", "USB"
> +	};
>  	static char *status_text[] = {
>  		"Unknown", "Charging", "Discharging", "Not charging", "Full"
>  	};
> @@ -58,12 +61,15 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	static char *capacity_level_text[] = {
>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>  	};
> -	ssize_t ret;
> +	ssize_t ret = 0;
>  	struct power_supply *psy = dev_get_drvdata(dev);
>  	const ptrdiff_t off = attr - power_supply_attrs;
>  	union power_supply_propval value;
>  
> -	ret = psy->get_property(psy, off, &value);
> +	if (off == POWER_SUPPLY_PROP_TYPE)
> +		value.intval = psy->type;
> +	else
> +		ret = psy->get_property(psy, off, &value);
>  
>  	if (ret < 0) {
>  		if (ret == -ENODATA)
> @@ -85,10 +91,13 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		return sprintf(buf, "%s\n", technology_text[value.intval]);
>  	else if (off == POWER_SUPPLY_PROP_CAPACITY_LEVEL)
>  		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
> +	else if (off == POWER_SUPPLY_PROP_TYPE)
> +		return sprintf(buf, "%s\n", type_text[value.intval]);
>  	else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
>  		return sprintf(buf, "%s\n", value.strval);
>  
>  	return sprintf(buf, "%d\n", value.intval);
> +return 0;

This return is superflous :)

And with this approach, we don't need to initialize the .mode field in
the POWER_SUPPLY_ATTR anmore.

I'll then rebase my patches for writeable properties and resend them.

Thanks,
Daniel

[PATCH 1/3] power_supply: Use attribute groups

[Daniel Mack]

From: Anton Vorontsov <cbouatmailru@gmail.com>

This fixes a race between power supply device and initial
attributes creation, plus makes it possible to implement
writable properties.

[Daniel Mack - removed superflous return statement
 and dropped .mode attribute from POWER_SUPPLY_ATTR]

Suggested-by: Greg KH <gregkh@suse.de>
Suggested-by: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
Tested-by: Daniel Mack <daniel@caiaq.de>
---
 drivers/power/power_supply.h       |    7 +--
 drivers/power/power_supply_core.c  |   48 +++++++++++----
 drivers/power/power_supply_sysfs.c |  115 ++++++++++++------------------------
 include/linux/power_supply.h       |    1 +
 4 files changed, 75 insertions(+), 96 deletions(-)

diff --git a/drivers/power/power_supply.h b/drivers/power/power_supply.h
index f38ba48..018de2b 100644
--- a/drivers/power/power_supply.h
+++ b/drivers/power/power_supply.h
@@ -12,15 +12,12 @@
 
 #ifdef CONFIG_SYSFS
 
-extern int power_supply_create_attrs(struct power_supply *psy);
-extern void power_supply_remove_attrs(struct power_supply *psy);
+extern void power_supply_init_attrs(struct device_type *dev_type);
 extern int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env);
 
 #else
 
-static inline int power_supply_create_attrs(struct power_supply *psy)
-{ return 0; }
-static inline void power_supply_remove_attrs(struct power_supply *psy) {}
+static inline void power_supply_init_attrs(struct device_type *dev_type) {}
 #define power_supply_uevent NULL
 
 #endif /* CONFIG_SYSFS */
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index cce75b4..91606bb 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/init.h>
+#include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/power_supply.h>
@@ -22,6 +23,8 @@
 struct class *power_supply_class;
 EXPORT_SYMBOL_GPL(power_supply_class);
 
+static struct device_type power_supply_dev_type;
+
 static int __power_supply_changed_work(struct device *dev, void *data)
 {
 	struct power_supply *psy = (struct power_supply *)data;
@@ -144,22 +147,39 @@ struct power_supply *power_supply_get_by_name(char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+static void power_supply_dev_release(struct device *dev)
+{
+	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
+	kfree(dev);
+}
+
 int power_supply_register(struct device *parent, struct power_supply *psy)
 {
-	int rc = 0;
+	struct device *dev;
+	int rc;
 
-	psy->dev = device_create(power_supply_class, parent, 0, psy,
-				 "%s", psy->name);
-	if (IS_ERR(psy->dev)) {
-		rc = PTR_ERR(psy->dev);
-		goto dev_create_failed;
-	}
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 
-	INIT_WORK(&psy->changed_work, power_supply_changed_work);
+	device_initialize(dev);
 
-	rc = power_supply_create_attrs(psy);
+	dev->class = power_supply_class;
+	dev->type = &power_supply_dev_type;
+	dev->parent = parent;
+	dev->release = power_supply_dev_release;
+	dev_set_drvdata(dev, psy);
+	psy->dev = dev;
+
+	rc = kobject_set_name(&dev->kobj, "%s", psy->name);
+	if (rc)
+		goto kobject_set_name_failed;
+
+	rc = device_add(dev);
 	if (rc)
-		goto create_attrs_failed;
+		goto device_add_failed;
+
+	INIT_WORK(&psy->changed_work, power_supply_changed_work);
 
 	rc = power_supply_create_triggers(psy);
 	if (rc)
@@ -170,10 +190,10 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 	goto success;
 
 create_triggers_failed:
-	power_supply_remove_attrs(psy);
-create_attrs_failed:
 	device_unregister(psy->dev);
-dev_create_failed:
+kobject_set_name_failed:
+device_add_failed:
+	kfree(dev);
 success:
 	return rc;
 }
@@ -183,7 +203,6 @@ void power_supply_unregister(struct power_supply *psy)
 {
 	flush_scheduled_work();
 	power_supply_remove_triggers(psy);
-	power_supply_remove_attrs(psy);
 	device_unregister(psy->dev);
 }
 EXPORT_SYMBOL_GPL(power_supply_unregister);
@@ -196,6 +215,7 @@ static int __init power_supply_class_init(void)
 		return PTR_ERR(power_supply_class);
 
 	power_supply_class->dev_uevent = power_supply_uevent;
+	power_supply_init_attrs(&power_supply_dev_type);
 
 	return 0;
 }
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 5b6e352..6eb49ef 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -31,7 +31,7 @@
 
 #define POWER_SUPPLY_ATTR(_name)					\
 {									\
-	.attr = { .name = #_name, .mode = 0444 },	\
+	.attr = { .name = #_name },					\
 	.show = power_supply_show_property,				\
 	.store = NULL,							\
 }
@@ -41,6 +41,9 @@ static struct device_attribute power_supply_attrs[];
 static ssize_t power_supply_show_property(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf) {
+	static char *type_text[] = {
+		"Battery", "UPS", "Mains", "USB"
+	};
 	static char *status_text[] = {
 		"Unknown", "Charging", "Discharging", "Not charging", "Full"
 	};
@@ -58,12 +61,15 @@ static ssize_t power_supply_show_property(struct device *dev,
 	static char *capacity_level_text[] = {
 		"Unknown", "Critical", "Low", "Normal", "High", "Full"
 	};
-	ssize_t ret;
+	ssize_t ret = 0;
 	struct power_supply *psy = dev_get_drvdata(dev);
 	const ptrdiff_t off = attr - power_supply_attrs;
 	union power_supply_propval value;
 
-	ret = psy->get_property(psy, off, &value);
+	if (off == POWER_SUPPLY_PROP_TYPE)
+		value.intval = psy->type;
+	else
+		ret = psy->get_property(psy, off, &value);
 
 	if (ret < 0) {
 		if (ret == -ENODATA)
@@ -85,6 +91,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 		return sprintf(buf, "%s\n", technology_text[value.intval]);
 	else if (off == POWER_SUPPLY_PROP_CAPACITY_LEVEL)
 		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
+	else if (off == POWER_SUPPLY_PROP_TYPE)
+		return sprintf(buf, "%s\n", type_text[value.intval]);
 	else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
 		return sprintf(buf, "%s\n", value.strval);
 
@@ -132,67 +140,50 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(time_to_empty_avg),
 	POWER_SUPPLY_ATTR(time_to_full_now),
 	POWER_SUPPLY_ATTR(time_to_full_avg),
+	POWER_SUPPLY_ATTR(type),
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_ATTR(model_name),
 	POWER_SUPPLY_ATTR(manufacturer),
 	POWER_SUPPLY_ATTR(serial_number),
 };
 
-static ssize_t power_supply_show_static_attrs(struct device *dev,
-					      struct device_attribute *attr,
-					      char *buf) {
-	static char *type_text[] = { "Battery", "UPS", "Mains", "USB" };
-	struct power_supply *psy = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%s\n", type_text[psy->type]);
-}
-
-static struct device_attribute power_supply_static_attrs[] = {
-	__ATTR(type, 0444, power_supply_show_static_attrs, NULL),
-};
+static struct attribute *
+__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
 
-int power_supply_create_attrs(struct power_supply *psy)
+static mode_t power_supply_attr_is_visible(struct kobject *kobj,
+					   struct attribute *attr,
+					   int attrno)
 {
-	int rc = 0;
-	int i, j;
-
-	for (i = 0; i < ARRAY_SIZE(power_supply_static_attrs); i++) {
-		rc = device_create_file(psy->dev,
-			    &power_supply_static_attrs[i]);
-		if (rc)
-			goto statics_failed;
-	}
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct power_supply *psy = dev_get_drvdata(dev);
+	int i;
 
-	for (j = 0; j < psy->num_properties; j++) {
-		rc = device_create_file(psy->dev,
-			    &power_supply_attrs[psy->properties[j]]);
-		if (rc)
-			goto dynamics_failed;
+	for (i = 0; i < psy->num_properties; i++) {
+		if (psy->properties[i] == attrno)
+			return 0444;
 	}
 
-	goto succeed;
-
-dynamics_failed:
-	while (j--)
-		device_remove_file(psy->dev,
-			   &power_supply_attrs[psy->properties[j]]);
-statics_failed:
-	while (i--)
-		device_remove_file(psy->dev, &power_supply_static_attrs[i]);
-succeed:
-	return rc;
+	return 0;
 }
 
-void power_supply_remove_attrs(struct power_supply *psy)
+static struct attribute_group power_supply_attr_group = {
+	.attrs = __power_supply_attrs,
+	.is_visible = power_supply_attr_is_visible,
+};
+
+const struct attribute_group *power_supply_attr_groups[] = {
+	&power_supply_attr_group,
+	NULL,
+};
+
+void power_supply_init_attrs(struct device_type *dev_type)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(power_supply_static_attrs); i++)
-		device_remove_file(psy->dev, &power_supply_static_attrs[i]);
+	dev_type->groups = power_supply_attr_groups;
 
-	for (i = 0; i < psy->num_properties; i++)
-		device_remove_file(psy->dev,
-			    &power_supply_attrs[psy->properties[i]]);
+	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++)
+		__power_supply_attrs[i] = &power_supply_attrs[i].attr;
 }
 
 static char *kstruprdup(const char *str, gfp_t gfp)
@@ -236,36 +227,6 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (!prop_buf)
 		return -ENOMEM;
 
-	for (j = 0; j < ARRAY_SIZE(power_supply_static_attrs); j++) {
-		struct device_attribute *attr;
-		char *line;
-
-		attr = &power_supply_static_attrs[j];
-
-		ret = power_supply_show_static_attrs(dev, attr, prop_buf);
-		if (ret < 0)
-			goto out;
-
-		line = strchr(prop_buf, '\n');
-		if (line)
-			*line = 0;
-
-		attrname = kstruprdup(attr->attr.name, GFP_KERNEL);
-		if (!attrname) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		dev_dbg(dev, "Static prop %s=%s\n", attrname, prop_buf);
-
-		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s", attrname, prop_buf);
-		kfree(attrname);
-		if (ret)
-			goto out;
-	}
-
-	dev_dbg(dev, "%zd dynamic props\n", psy->num_properties);
-
 	for (j = 0; j < psy->num_properties; j++) {
 		struct device_attribute *attr;
 		char *line;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index ebd2b8f..c5f73a3 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -114,6 +114,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_AVG,
+	POWER_SUPPLY_PROP_TYPE, /* use power_supply.type instead */
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
-- 
1.7.1

[PATCH 2/3] pda_power: add support for writeable properties

[Daniel Mack]

This patch adds support for writeable power supply properties and
exposes them as writeable to sysfs.

A power supply implementation must implement two new function calls in
order to use that feature:

  int set_property(struct power_supply *psy,
                   enum power_supply_property psp,
                   const union power_supply_propval *val);

  int property_is_writeable(struct power_supply *psy,
                            enum power_supply_property psp);

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/power/power_supply_sysfs.c |   38 +++++++++++++++++++++++++++++++++--
 include/linux/power_supply.h       |    5 ++++
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 6eb49ef..6a779fe 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -33,7 +33,7 @@
 {									\
 	.attr = { .name = #_name },					\
 	.show = power_supply_show_property,				\
-	.store = NULL,							\
+	.store = power_supply_store_property,				\
 }
 
 static struct device_attribute power_supply_attrs[];
@@ -99,6 +99,29 @@ static ssize_t power_supply_show_property(struct device *dev,
 	return sprintf(buf, "%d\n", value.intval);
 }
 
+static ssize_t power_supply_store_property(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count) {
+	ssize_t ret;
+	struct power_supply *psy = dev_get_drvdata(dev);
+	const ptrdiff_t off = attr - power_supply_attrs;
+	union power_supply_propval value;
+	long long_val;
+
+	/* TODO: support other types than int */
+	ret = strict_strtol(buf, 10, &long_val);
+	if (ret < 0)
+		return ret;
+
+	value.intval = long_val;
+
+	ret = psy->set_property(psy, off, &value);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
 /* Must be in the same order as POWER_SUPPLY_PROP_* */
 static struct device_attribute power_supply_attrs[] = {
 	/* Properties of type `int' */
@@ -159,8 +182,17 @@ static mode_t power_supply_attr_is_visible(struct kobject *kobj,
 	int i;
 
 	for (i = 0; i < psy->num_properties; i++) {
-		if (psy->properties[i] == attrno)
-			return 0444;
+		int property = psy->properties[i];
+
+		if (property == attrno) {
+			mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
+
+			if (psy->property_is_writeable &&
+			    psy->property_is_writeable(psy, property) > 0)
+				mode |= S_IWUSR;
+
+			return mode;
+		}
 	}
 
 	return 0;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index c5f73a3..30083a8 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -145,6 +145,11 @@ struct power_supply {
 	int (*get_property)(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val);
+	int (*property_is_writeable)(struct power_supply *psy,
+				     enum power_supply_property psp);
 	void (*external_power_changed)(struct power_supply *psy);
 	void (*set_charged)(struct power_supply *psy);
 
-- 
1.7.1

Re: [PATCH 2/3] pda_power: add support for writeable properties

[Greg KH]

On Tue, May 18, 2010 at 09:49:52PM +0200, Daniel Mack wrote:
> This patch adds support for writeable power supply properties and
> exposes them as writeable to sysfs.

Please properly document this in Documentation/ABI as you are changing
the userspace ABI.

thanks,

greg k-h

[PATCH] power/ds2760_battery: document ABI change

[Daniel Mack]

>From 7ed6b078c1cb718509a0faf79ac53f768fa5cfc4 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Tue, 18 May 2010 22:26:40 +0200
Subject: [PATCH] power/ds2760_battery: document ABI change

Add some documentation for the newly added writeable properties.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
---
On Tue, May 18, 2010 at 12:56:24PM -0700, Greg KH wrote:
> On Tue, May 18, 2010 at 09:49:52PM +0200, Daniel Mack wrote:
> > This patch adds support for writeable power supply properties and
> > exposes them as writeable to sysfs.
> 
> Please properly document this in Documentation/ABI as you are changing
> the userspace ABI.
> 
> thanks,
> 
> greg k-h

Is something like the following appropriate?


 Documentation/ABI/testing/sysfs-class-power |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-power

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
new file mode 100644
index 0000000..f46ad66
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -0,0 +1,21 @@
+What:		/sys/class/power/ds2760-battery.*/charge_now
+Date:		May 2010
+KernelVersion:	2.6.35
+Contact:	Daniel Mack <daniel@caiaq.de>
+Description:
+		This file is writeable and can be used to set the current
+		coloumb counter value inside the battery monitor chip. This
+		is needed for unavoidable corrections of aging batteries.
+		A userspace daemon can monitor the battery charging logic
+		and once the counter drops out of considerable bounds, take
+		appropriate action.
+
+What:		/sys/class/power/ds2760-battery.*/charge_full
+Date:		May 2010
+KernelVersion:	2.6.35
+Contact:	Daniel Mack <daniel@caiaq.de>
+Description:
+		This file is writeable and can be used to set the assumed
+		battery 'full level'. As batteries age, this value has to be
+		amended over time.
+
-- 
1.7.1

Re: [PATCH] power/ds2760_battery: document ABI change

[Anton Vorontsov]

On Tue, May 18, 2010 at 10:30:49PM +0200, Daniel Mack wrote:
> >From 7ed6b078c1cb718509a0faf79ac53f768fa5cfc4 Mon Sep 17 00:00:00 2001
> From: Daniel Mack <daniel@caiaq.de>
> Date: Tue, 18 May 2010 22:26:40 +0200
> Subject: [PATCH] power/ds2760_battery: document ABI change
> 
> Add some documentation for the newly added writeable properties.
> 

I added 'Suggested-by: Greg KH <gregkh@suse.de>' tag here.

> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> ---
> On Tue, May 18, 2010 at 12:56:24PM -0700, Greg KH wrote:
> > On Tue, May 18, 2010 at 09:49:52PM +0200, Daniel Mack wrote:
> > > This patch adds support for writeable power supply properties and
> > > exposes them as writeable to sysfs.
> > 
> > Please properly document this in Documentation/ABI as you are changing
> > the userspace ABI.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Is something like the following appropriate?

Looks good to me, as well as patches 1-3 (though in the first patch
I fixed power_supply_attr_groups to be static, it was a negligence
from my original patch).

I applied all these patches to battery-2.6.git, thanks!

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

[PATCH 3/3] power/ds2760_battery: make charge_now and charge_full writeable

[Daniel Mack]

For userspace tools and daemons, it might be necessary to adjust
the charge_now and charge_full properties of the ds2760 battery monitor,
for example for unavoidable corrections due to aging batteries.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/power/ds2760_battery.c |   64 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 3bf8d1f..4d3b272 100644
--- a/drivers/power/ds2760_battery.c
+++ b/drivers/power/ds2760_battery.c
@@ -304,6 +304,28 @@ static void ds2760_battery_write_rated_capacity(struct ds2760_device_info *di,
 	w1_ds2760_recall_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK1);
 }
 
+static void ds2760_battery_write_active_full(struct ds2760_device_info *di,
+					     int active_full)
+{
+	unsigned char tmp[2] = {
+		active_full >> 8,
+		active_full & 0xff
+	};
+
+	if (tmp[0] == di->raw[DS2760_ACTIVE_FULL] &&
+	    tmp[1] == di->raw[DS2760_ACTIVE_FULL + 1])
+		return;
+
+	w1_ds2760_write(di->w1_dev, tmp, DS2760_ACTIVE_FULL, sizeof(tmp));
+	w1_ds2760_store_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK0);
+	w1_ds2760_recall_eeprom(di->w1_dev, DS2760_EEPROM_BLOCK0);
+
+	/* Write to the di->raw[] buffer directly - the DS2760_ACTIVE_FULL
+	 * values won't be read back by ds2760_battery_read_status() */
+	di->raw[DS2760_ACTIVE_FULL] = tmp[0];
+	di->raw[DS2760_ACTIVE_FULL + 1] = tmp[1];
+}
+
 static void ds2760_battery_work(struct work_struct *work)
 {
 	struct ds2760_device_info *di = container_of(work,
@@ -426,6 +448,45 @@ static int ds2760_battery_get_property(struct power_supply *psy,
 	return 0;
 }
 
+static int ds2760_battery_set_property(struct power_supply *psy,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
+{
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		/* the interface counts in uAh, convert the value */
+		ds2760_battery_write_active_full(di, val->intval / 1000L);
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		/* ds2760_battery_set_current_accum() does the conversion */
+		ds2760_battery_set_current_accum(di, val->intval);
+		break;
+
+	default:
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+static int ds2760_battery_property_is_writeable(struct power_supply *psy,
+						enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		return 1;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static enum power_supply_property ds2760_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -460,6 +521,9 @@ 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_property	= ds2760_battery_set_property;
+	di->bat.property_is_writeable =
+				  ds2760_battery_property_is_writeable;
 	di->bat.set_charged	= ds2760_battery_set_charged;
 	di->bat.external_power_changed =
 				  ds2760_battery_external_power_changed;
-- 
1.7.1

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Anton Vorontsov]

On Tue, May 11, 2010 at 07:58:12PM +0200, Daniel Mack wrote:
[...]
> > Plus, that way you don't need is_writable().
> 
> Ugh, really? I would _much_ prefer to actually _see_ which property is
> writeable, just from looking at the file attributes in sysfs.

Well, yeah... that would be also good for userland apps (GUI
apps, in particular). Though, I'm not yet sure how to implement
it without modifying a global array...

To avoid the race that I described in previous email we could
insert some mutex, but I'd like to avoid the global stuff.

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

[PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

This patch adds support for writeable power supply properties and
exposes them as writeable to sysfs.

A power supply implementation must implement two new function calls in
order to use that feature:

  int set_property(struct power_supply *psy,
                   enum power_supply_property psp,
                   const union power_supply_propval *val);

  int property_is_writeable(struct power_supply *psy,
                            enum power_supply_property psp);

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Andres Salomon <dilinger@collabora.co.uk>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/power/power_supply_sysfs.c |   31 +++++++++++++++++++++++++++++--
 include/linux/power_supply.h       |    5 +++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 5b6e352..5cfb410 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -31,9 +31,9 @@
 
 #define POWER_SUPPLY_ATTR(_name)					\
 {									\
-	.attr = { .name = #_name, .mode = 0444 },	\
+	.attr = { .name = #_name },					\
 	.show = power_supply_show_property,				\
-	.store = NULL,							\
+	.store = power_supply_store_property,				\
 }
 
 static struct device_attribute power_supply_attrs[];
@@ -91,6 +91,25 @@ static ssize_t power_supply_show_property(struct device *dev,
 	return sprintf(buf, "%d\n", value.intval);
 }
 
+static ssize_t power_supply_store_property(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count) {
+	ssize_t ret;
+	struct power_supply *psy = dev_get_drvdata(dev);
+	const ptrdiff_t off = attr - power_supply_attrs;
+	union power_supply_propval value;
+	long long_val;
+
+	/* TODO: support other types than int */
+	ret = strict_strtol(buf, 10, &long_val);
+	if (ret < 0)
+		return ret;
+
+	value.intval = long_val;
+
+	return psy->set_property(psy, off, &value);
+}
+
 /* Must be in the same order as POWER_SUPPLY_PROP_* */
 static struct device_attribute power_supply_attrs[] = {
 	/* Properties of type `int' */
@@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
 	}
 
 	for (j = 0; j < psy->num_properties; j++) {
+		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
+
+		if (psy->property_is_writeable &&
+		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
+			mode |= S_IWUSR;
+
+		power_supply_attrs[psy->properties[j]].attr.mode = mode;
+
 		rc = device_create_file(psy->dev,
 			    &power_supply_attrs[psy->properties[j]]);
 		if (rc)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index ebd2b8f..f02f7fd 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -144,6 +144,11 @@ struct power_supply {
 	int (*get_property)(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val);
+	int (*property_is_writeable)(struct power_supply *psy,
+				     enum power_supply_property psp);
 	void (*external_power_changed)(struct power_supply *psy);
 	void (*set_charged)(struct power_supply *psy);
 
-- 
1.6.3.3

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

Sorry for the resend guys. My clock was skewed again, which might cause
people missing the messages. Fixed some commit logs on the way.

Sent from a tired brain, excuse typos :)


On Fri, May 07, 2010 at 07:52:11PM +0200, Daniel Mack wrote:
> This patch adds support for writeable power supply properties and
> exposes them as writeable to sysfs.
> 
> A power supply implementation must implement two new function calls in
> order to use that feature:
> 
>   int set_property(struct power_supply *psy,
>                    enum power_supply_property psp,
>                    const union power_supply_propval *val);
> 
>   int property_is_writeable(struct power_supply *psy,
>                             enum power_supply_property psp);
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Andres Salomon <dilinger@collabora.co.uk>
> Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Matt Reimer <mreimer@vpop.net>
> Cc: Evgeniy Polyakov <zbr@ioremap.net>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  drivers/power/power_supply_sysfs.c |   31 +++++++++++++++++++++++++++++--
>  include/linux/power_supply.h       |    5 +++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index 5b6e352..5cfb410 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -31,9 +31,9 @@
>  
>  #define POWER_SUPPLY_ATTR(_name)					\
>  {									\
> -	.attr = { .name = #_name, .mode = 0444 },	\
> +	.attr = { .name = #_name },					\
>  	.show = power_supply_show_property,				\
> -	.store = NULL,							\
> +	.store = power_supply_store_property,				\
>  }
>  
>  static struct device_attribute power_supply_attrs[];
> @@ -91,6 +91,25 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	return sprintf(buf, "%d\n", value.intval);
>  }
>  
> +static ssize_t power_supply_store_property(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t count) {
> +	ssize_t ret;
> +	struct power_supply *psy = dev_get_drvdata(dev);
> +	const ptrdiff_t off = attr - power_supply_attrs;
> +	union power_supply_propval value;
> +	long long_val;
> +
> +	/* TODO: support other types than int */
> +	ret = strict_strtol(buf, 10, &long_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	value.intval = long_val;
> +
> +	return psy->set_property(psy, off, &value);
> +}
> +
>  /* Must be in the same order as POWER_SUPPLY_PROP_* */
>  static struct device_attribute power_supply_attrs[] = {
>  	/* Properties of type `int' */
> @@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
>  	}
>  
>  	for (j = 0; j < psy->num_properties; j++) {
> +		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> +
> +		if (psy->property_is_writeable &&
> +		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
> +			mode |= S_IWUSR;
> +
> +		power_supply_attrs[psy->properties[j]].attr.mode = mode;
> +
>  		rc = device_create_file(psy->dev,
>  			    &power_supply_attrs[psy->properties[j]]);
>  		if (rc)
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index ebd2b8f..f02f7fd 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -144,6 +144,11 @@ struct power_supply {
>  	int (*get_property)(struct power_supply *psy,
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val);
> +	int (*set_property)(struct power_supply *psy,
> +			    enum power_supply_property psp,
> +			    const union power_supply_propval *val);
> +	int (*property_is_writeable)(struct power_supply *psy,
> +				     enum power_supply_property psp);
>  	void (*external_power_changed)(struct power_supply *psy);
>  	void (*set_charged)(struct power_supply *psy);
>  
> -- 
> 1.6.3.3
> 

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Mark Brown]

On Fri, May 07, 2010 at 07:52:11PM +0200, Daniel Mack wrote:

> +	/* TODO: support other types than int */
> +	ret = strict_strtol(buf, 10, &long_val);
> +	if (ret < 0)
> +		return ret;

It'd be nicer if we could error out if this is used on properties with
an inappropriate type but we don't seem to have type information
anywhere convenient for implementing that, which is a bit unfortunate.

Otherwise this looks reasonable in itself.

Re: [PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

On Mon, May 10, 2010 at 10:48:54AM +0100, Mark Brown wrote:
> On Fri, May 07, 2010 at 07:52:11PM +0200, Daniel Mack wrote:
> 
> > +	/* TODO: support other types than int */
> > +	ret = strict_strtol(buf, 10, &long_val);
> > +	if (ret < 0)
> > +		return ret;
> 
> It'd be nicer if we could error out if this is used on properties with
> an inappropriate type but we don't seem to have type information
> anywhere convenient for implementing that, which is a bit unfortunate.

Yep, hence the 'TODO' :)
So currently the verdict is: if we can't convert the input to an
integer, we bail.

> Otherwise this looks reasonable in itself.

Ok, thanks.

Daniel

[PATCH 1/3] pda_power: add support for writeable properties

[Daniel Mack]

This patch adds support for writeable power supply properties and
exposes them as writeable to sysfs.

A power supply implementation must implement two new function calls in
order to use that feature:

  int set_property(struct power_supply *psy,
                   enum power_supply_property psp,
                   const union power_supply_propval *val);

  int property_is_writeable(struct power_supply *psy,
                            enum power_supply_property psp);

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Andres Salomon <dilinger@collabora.co.uk>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Matt Reimer <mreimer@vpop.net>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/power/power_supply_sysfs.c |   31 +++++++++++++++++++++++++++++--
 include/linux/power_supply.h       |    5 +++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 5b6e352..5cfb410 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -31,9 +31,9 @@
 
 #define POWER_SUPPLY_ATTR(_name)					\
 {									\
-	.attr = { .name = #_name, .mode = 0444 },	\
+	.attr = { .name = #_name },					\
 	.show = power_supply_show_property,				\
-	.store = NULL,							\
+	.store = power_supply_store_property,				\
 }
 
 static struct device_attribute power_supply_attrs[];
@@ -91,6 +91,25 @@ static ssize_t power_supply_show_property(struct device *dev,
 	return sprintf(buf, "%d\n", value.intval);
 }
 
+static ssize_t power_supply_store_property(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t count) {
+	ssize_t ret;
+	struct power_supply *psy = dev_get_drvdata(dev);
+	const ptrdiff_t off = attr - power_supply_attrs;
+	union power_supply_propval value;
+	long long_val;
+
+	/* TODO: support other types than int */
+	ret = strict_strtol(buf, 10, &long_val);
+	if (ret < 0)
+		return ret;
+
+	value.intval = long_val;
+
+	return psy->set_property(psy, off, &value);
+}
+
 /* Must be in the same order as POWER_SUPPLY_PROP_* */
 static struct device_attribute power_supply_attrs[] = {
 	/* Properties of type `int' */
@@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
 	}
 
 	for (j = 0; j < psy->num_properties; j++) {
+		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
+
+		if (psy->property_is_writeable &&
+		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
+			mode |= S_IWUSR;
+
+		power_supply_attrs[psy->properties[j]].attr.mode = mode;
+
 		rc = device_create_file(psy->dev,
 			    &power_supply_attrs[psy->properties[j]]);
 		if (rc)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index ebd2b8f..f02f7fd 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -144,6 +144,11 @@ struct power_supply {
 	int (*get_property)(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val);
+	int (*property_is_writeable)(struct power_supply *psy,
+				     enum power_supply_property psp);
 	void (*external_power_changed)(struct power_supply *psy);
 	void (*set_charged)(struct power_supply *psy);
 
-- 
1.6.3.3