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

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

On Sun, Feb 19, 2012 at 11:45:59PM -0700, Stephen Warren wrote:
> The API model is changed from:
> 
> p = pinctrl_get(dev, "state1");
> pinctrl_enable(p);
> ...
> pinctrl_disable(p);
> pinctrl_put(p);
> p = pinctrl_get(dev, "state2");
> pinctrl_enable(p);
> ...
> pinctrl_disable(p);
> pinctrl_put(p);
> 
> to this:
> 
> p = pinctrl_get(dev);
> s1 = pinctrl_lookup_state(p, "state1");
> s2 = pinctrl_lookup_state(p, "state2");
> pinctrl_select_state(p, s1);
> ...
> pinctrl_select_state(p, s2);
> ...
> pinctrl_put(p);
> 
> This allows devices to directly transition between states without
> disabling the pin controller programming and put()/get()ing the
> configuration data each time. This model will also better suit pinconf
> programming, which doesn't have a concept of "disable".
> 
It's exactly what we discussed and wanted.
Thank for the great and hard work.

> The special-case hogging feature of pin controllers is re-written to use
> the regular APIs instead of special-case code. Hence, the pinmux-hogs
> debugfs file is removed; see the top-level pinctrl-handles files for
> equivalent data.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  Documentation/pinctrl.txt         |  153 ++++++++-----
>  arch/arm/mach-u300/core.c         |   28 +--
>  drivers/pinctrl/core.c            |  491 +++++++++++++++++--------------------
>  drivers/pinctrl/core.h            |   24 ++-
>  drivers/tty/serial/sirfsoc_uart.c |   12 +-
>  include/linux/pinctrl/consumer.h  |   55 ++++-
>  include/linux/pinctrl/machine.h   |   21 +-
>  7 files changed, 408 insertions(+), 376 deletions(-)
> 

>  
> @@ -814,7 +817,7 @@ it even more compact which assumes you want to use pinctrl-foo and position
>  0 for mapping, for example:
>  
>  static struct pinctrl_map __initdata mapping[] = {
> -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> +       PIN_MAP("default", "pinctrl-foo", "i2c0", "foo-i2c.0"),
I remember you said you will also change the PIN_MAP sequence that
place the device name at first.
Will see your further patch.

>  The result of grabbing this mapping from the device with something like
>  this (see next paragraph):
>  
> -	p = pinctrl_get(&device, "8bit");
> +	p = pinctrl_get(dev);
> +	s = pinctrl_lookup_state(p, "8bit");
> +	ret = pinctrl_select_state(p, s);
> +
> +or more simply:
> +
> +	p = pinctrl_get_select(dev, "8bit");
>  
Very glad to see this simply one. :-)

>  foo_probe()
>  {
> -	/* Allocate a state holder named "state" etc */
> -	struct pinctrl p;

> +	/* Allocate a state holder named "foo" etc */
> +	struct foo_state *foo = ...;
>  
> -	p = pinctrl_get(&device, "default");
> -	if IS_ERR(p)
> -		return PTR_ERR(p);
> -	pinctrl_enable(p);
> +	foo->p = pinctrl_get(&device);
> +	if (IS_ERR(foo->p)) {
> +		/* FIXME: clean up "foo" here */
> +		return PTR_ERR(foo->p);
> +	}
> +
> +	foo->s = pinctrl_lookup_state(foo->p, "default");
> +	if (IS_ERR(foo->s)) {
> +		pinctrl_put(foo->p);
> +		/* FIXME: clean up "foo" here */
> +		return PTR_ERR(s);
> +	}
>  
> -	state->p = p;
> +	ret = pinctrl_select_state(s);
s/s/foo->s

> @@ -976,25 +1004,25 @@ System pin control hogging
>  ==========================
>  
>  Pin control map entries can be hogged by the core when the pin controller
> -is registered. This means that the core will attempt to call pinctrl_get() and
> -pinctrl_enable() on it immediately after the pin control device has been
> -registered.
> +is registered. This means that the core will attempt to call pinctrl_get(),
> +lookup_state() and select_state() on it immediately after the pin control
> +device has been registered.
>  
> -This is enabled by simply setting the .dev_name field in the map to the name
> -of the pin controller itself, like this:
> +This occurs for mapping table entries where the client device name is equal
> +to the pin controller device name, and the state name is "active".
>  
>  {
> -	.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.

>  	.ctrl_dev_name = "pinctrl-foo",
>  	.function = "power_func",
> -	.dev_name = "pinctrl-foo",
>  },
>  
>  Since it may be common to request the core to hog a few always-applicable
>  mux settings on the primary pin controller, there is a convenience macro for
>  this:
>  
> -PIN_MAP_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
> +PIN_MAP_SYS_HOG("active", "pinctrl-foo", "power_func")
I remember you had a patch changing this macro a bit.
So here may also need change.

> +static struct pinctrl *find_pinctrl(struct device *dev)
> +{
> +	struct pinctrl *p;
>  
> -	dev_dbg(dev, "pinctrl_get() for state %s\n", name);
> +	list_for_each_entry(p, &pinctrldev_list, node)
s/pinctrldev_list/pinctrl_list ?

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

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

Regards
Dong Aisheng