Re: [PATCH V2 4/6] pinctrl: API changes to support multiple states per device

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

On Tue, Feb 28, 2012 at 01:27:30PM -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".
> 
> 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>
> ---
> v2: Make use of PINCTRL_STATE_DEFAULT, split out some documentation
> cleanup into an earlier patch. Various minor fixes. Fixes due to
> rebasing on updated earlier patches. Remove usecount field from struct
> pinctrl; allow only one concurrent pinctrl_get() per device.
> 
Glad to hear this.

The whole patch looks pretty good to me.
A few trivial comments, else
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

> +static struct pinctrl *find_pinctrl(struct device *dev)
> +{
> +	struct pinctrl *p;
> +
> +	list_for_each_entry(p, &pinctrldev_list, node)
s/pinctrldev_list/pinctrl_list ?

> -static void pinctrl_disable_locked(struct pinctrl *p)
> +static int pinctrl_select_state_locked(struct pinctrl *p,
> +				       struct pinctrl_state *state)
>  {
> -	struct pinctrl_setting *setting;
> +	struct pinctrl_setting *setting, *setting2;
> +	int ret;
>  
> -	if (p == NULL)
> -		return;
> +	if (p->state == state)
> +		return 0;
>  
> -	if (--p->usecount == 0) {
> -		list_for_each_entry(setting, &p->settings, node)
> -			pinmux_disable_setting(setting);
> +	if (p->state) {
> +		/*
> +		 * The set of groups with a mux configuration in the old state
> +		 * may not be identical to the set of groups with a mux setting
> +		 * in the new state. While this might be unusual, it's entirely
> +		 * possible for the "user"-supplied mapping table to be written
> +		 * that way. For each group that was configured in the old state
> +		 * but not in the new state, this code puts that group into a
> +		 * safe/disabled state.
It means the old state function of some groups may not have been disabled before
enabling the new function mode.
May it be a little error-prone?
Maybe we can not depend on user to disable a function before enable another for
a group when doing pinmux_enable_setting considering they may be different
registers.

> +		 */
> +		list_for_each_entry(setting, &p->state->settings, node) {
> +			bool found = false;
> +			list_for_each_entry(setting2, &state->settings, node) {
> +				if (setting2->group_selector ==
> +						setting->group_selector) {
> +					found = true;
> +					break;
> +				}
> +			}
> +			if (!found)
> +				pinmux_disable_setting(setting);
> +		}
> +	}
> +
> +	p->state = state;
> +
> +	/* Apply all the settings for the new state */
> +	list_for_each_entry(setting, &state->settings, node) {
> +		ret = pinmux_enable_setting(setting);
> +		if (ret < 0) {
> +			/* FIXME: Difficult to return to prev state */
> +			return ret;
> +		}
>  	}
> +
> +	return 0;
>  }
>  
>  /**

Regards
Dong Aisheng