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.025 / U:61.160 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site