| 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
| ||
|
Date: Mon, 27 Feb 2012 17:07:22 +0800
From: Dong Aisheng <aisheng.dong@...escale.com>
To: Stephen Warren <swarren@...dia.com>
CC: Linus Walleij <linus.walleij@...ricsson.com>,
<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 19/20] pinctrl: API changes to support multiple states
per device
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@...dia.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
--
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/
Powered by blists - more mailing lists