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


    \
     
     \ /
      Last update: 2010-05-12 10:09    [W:0.023 / U:61.328 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site