lkml.org 
[lkml]   [2010]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] pda_power: add support for writeable properties
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


\
 
 \ /
  Last update: 2010-05-12 10:17    [W:0.458 / U:0.292 seconds]
©2003-2014 Jasper Spaans. Advertise on this site