lkml.org 
[lkml]   [2012]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH 19/20] pinctrl: API changes to support multiple states per device
Dong Aisheng wrote at Monday, February 27, 2012 2:07 AM:
> On Sun, Feb 19, 2012 at 11:45:59PM -0700, Stephen Warren wrote:

> > - .name = "POWERMAP"
> > + .dev_name = "pinctrl-foo",
> > + .name = "active",
>
> I guess if we really decide to use a fixed state name for hog functions,
> we'd better not let users to write the state name, at least provide a macro.

Yes, that's a good point for adding the special-case macros. I have
added such special-case macros to my local version of patch 20, so they'll
be there in the repost.

> > +static struct pinctrl *pinctrl_get_locked(struct device *dev)
> > +{
> > + struct pinctrl *p;
> >
> > -error:
> > - list_for_each_entry(setting, &p->settings, node)
> > - pinmux_free_setting(setting);
> > + if (WARN_ON(!dev))
> > + return ERR_PTR(-EINVAL);
> >
> > - kfree(p);
> > + p = find_pinctrl(dev);
> > + if (p == NULL)
> > + p = create_pinctrl(dev);
> > + if (IS_ERR(p))
> > + return p;
> > +
> > + p->usecount++;
>
> I still can not understand what's the purpose of p->usecount?
> For allowing multi times calling of pinctrl_get for on the same device?

pinctrl_get() could be called multiple times for the same device. Rather
than create a whole new struct pinctrl each time it's called, we just
reference count the object so that each call returns the same one, and
it won't be destroyed until all users have called pinctrl_put().

Hopefully it is true that multiple different pieces of code won't be
screwing with the same device's pinctrl settings, but it's simple enough
to do this so we may as well. This somewhat of a the moral equivalent of
the old code's p->usecount manipulations in pinctrl_enable()/disable(),
although admittedly a little different.

> > +static inline struct pinctrl * __must_check pinctrl_get_select(
> > + struct device *dev, const char *name)
> > +{
> > + struct pinctrl *p;
> > + struct pinctrl_state *s;
> > + int ret;
> > +
> > + p = pinctrl_get(dev);
> > + if (IS_ERR(p))
> > + return p;
> > +
> > + s = pinctrl_lookup_state(p, name);
> > + if (IS_ERR(s)) {
> > + pinctrl_put(p);
> > + return ERR_PTR(PTR_ERR(s));
> > + }
> > +
> > + ret = pinctrl_select_state(p, s);
> > + if (ret < 0) {
> > + pinctrl_put(p);
> > + return ERR_PTR(ret);
>
> s/ERR_PTR(ret)/ret ?

The function returns a pointer, whereas ret is an int. ERR_PTR() is used
to wrap the int error code into a pointer value so that the function can
return either a valid pointer, or an error-code. See include/linux/err.h.

I've fixed locally up the other issues you pointed out. Thanks.

--
nvpublic



\
 
 \ /
  Last update: 2012-02-27 19:39    [W:0.118 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site