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

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

[Daniel Mack]

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

For userspace tools and daemons, it might be necessary to adjust
the charge_now and charge_full_design 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 was 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 |   57 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/power/ds2760_battery.c b/drivers/power/ds2760_battery.c
index 3bf8d1f..14df424 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,51 @@ 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)
+{
+	unsigned char tmp;
+	struct ds2760_device_info *di = to_ds2760_device_info(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		/* 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;
+		w1_ds2760_write(di->w1_dev, &tmp, DS2760_CURRENT_ACCUM_LSB, 1);
+
+		tmp = val->intval >> 8;
+		w1_ds2760_write(di->w1_dev, &tmp, DS2760_CURRENT_ACCUM_MSB, 1);
+
+		break;
+
+	default:
+		return -EPERM;
+	}
+
+	return -EINVAL;
+}
+
+static int ds2760_battery_property_is_writeable(struct power_supply *psy,
+						enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		return 1;
+
+	default:
+		return 0;
+	}
+
+	return 0;
+}
+
 static enum power_supply_property ds2760_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -460,6 +507,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 +526,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.6.3.3

[PATCH 3/3] [RFC] power/ds2760_battery: use factor of 50 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.

Change the factor to 20 to broaden that range. Note that due to
RATED_CAPACITY_FACTOR, the internal 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 14df424..66d8dae 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.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 2/3] power/ds2760_battery: make charge_now and charge_full_design writeable

[Mark Brown]

On Fri, May 07, 2010 at 07:52:12PM +0200, Daniel Mack wrote:
> For userspace tools and daemons, it might be necessary to adjust
> the charge_now and charge_full_design properties of the ds2760 battery
> monitor, for example for unavoidable corrections due to aging batteries.

Changing charge_full_design seems really odd - I'd really expect that to
be something which is constant throughout the lifetime of the system.  A
separate property showing the degraded capacity would be a bit less
surprising.

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

[Daniel Mack]

On Mon, May 10, 2010 at 10:52:11AM +0100, Mark Brown wrote:
> On Fri, May 07, 2010 at 07:52:12PM +0200, Daniel Mack wrote:
> > For userspace tools and daemons, it might be necessary to adjust
> > the charge_now and charge_full_design properties of the ds2760 battery
> > monitor, for example for unavoidable corrections due to aging batteries.
> 
> Changing charge_full_design seems really odd - I'd really expect that to
> be something which is constant throughout the lifetime of the system.  A
> separate property showing the degraded capacity would be a bit less
> surprising.

There's 'carge_full' as well. I'll use this one instead.

Thanks,
Daniel

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

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

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

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

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