Re: [PATCH 19/20] pinctrl: API changes to support multiple states per device

[Dong Aisheng <aisheng.dong@freescale.com>]

On Mon, Feb 27, 2012 at 10:37:16AM -0800, Stephen Warren wrote:
> Dong Aisheng wrote at Monday, February 27, 2012 2:07 AM:
> > On Sun, Feb 19, 2012 at 11:45:59PM -0700, Stephen Warren wrote:
..........
> 
> > > +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.
> 
Hmm, yes, a slight difference after looking the old code.
Oringally the pinmux->usecount was introduced to allow nested call of pinmux_enable
(but not pinmux_get since it will return an error due to pin conflicts).
In the new way, you allow nested call of pinctrl_get.

I still can't find in which case the device will have such requirement
since per my understanding pinctrl is a little different from clock
(clock can be used by different devices but we do not allow pins to be
used by difference devices at the same time).

However it's actually another issue which is not related to this patch since
we're just trying to keep align with the old code.
So it's ok to me for you to keep it here first.

> > > +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.
> 
Hmm, below is what i see in your patch:
+int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 {
+       int ret;
+
        mutex_lock(&pinctrl_mutex);
-       pinctrl_disable_locked(p);
+       ret = pinctrl_select_state_locked(p, state);
        mutex_unlock(&pinctrl_mutex);
+
+       return ret;
 }
It seems pinctrl_select_state does not return a pointer.


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

Regards
Dong Aisheng