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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120229064633.GA14703@shlinux2.ap.freescale.net>
Date:	Wed, 29 Feb 2012 14:46:34 +0800
From:	Dong Aisheng <aisheng.dong@...escale.com>
To:	Stephen Warren <swarren@...dia.com>
CC:	Linus Walleij <linus.walleij@...ricsson.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	<B29396@...escale.com>, <s.hauer@...gutronix.de>,
	<dongas86@...il.com>, <shawn.guo@...aro.org>,
	<thomas.abraham@...aro.org>, <tony@...mide.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 4/6] pinctrl: API changes to support multiple states
 per device

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@...dia.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@...aro.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/