| 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
| ||
|
Message-ID: <CACRpkdZQhJ9hBxD93FHjpCngugTZ2EFgJFwWObGd1YmEtfnxFA@mail.gmail.com>
Date: Thu, 23 Feb 2012 06:54:46 +0100
From: Linus Walleij <linus.walleij@...aro.org>
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 Mon, Feb 20, 2012 at 7:45 AM, Stephen Warren <swarren@...dia.com> 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>
Overall this is excellent and exactly what we have discussed and
just what we want, good job!
> static const struct pinctrl_map __initdata mapping[] = {
> {
> + .dev_name = "foo-spi.0",
> + .name = "default",
I think we should introduce a
#define PINCTRL_DEFAULT_STATE "default"
And use that #define in examples and code from day 1.
That will ease the transition to generalized states like
PINCTRL_IDLE_STATE etc further down the road.
> {
> + .dev_name = "foo-mmc.0",
> .name = "2bit"
> .ctrl_dev_name = "pinctrl-foo",
> .function = "mmc0",
> .group = "mmc0_1_grp",
> - .dev_name = "foo-mmc.0",
> },
> {
> + .dev_name = "foo-mmc.0",
> .name = "4bit"
> .ctrl_dev_name = "pinctrl-foo",
> .function = "mmc0",
> .group = "mmc0_1_grp",
> - .dev_name = "foo-mmc.0",
> },
> {
> + .dev_name = "foo-mmc.0",
> .name = "4bit"
> .ctrl_dev_name = "pinctrl-foo",
> .function = "mmc0",
> .group = "mmc0_2_grp",
> - .dev_name = "foo-mmc.0",
> },
> {
> + .dev_name = "foo-mmc.0",
> .name = "8bit"
> .ctrl_dev_name = "pinctrl-foo",
> + .function = "mmc0",
> .group = "mmc0_1_grp",
> - .dev_name = "foo-mmc.0",
> },
> {
> + .dev_name = "foo-mmc.0",
> .name = "8bit"
> .ctrl_dev_name = "pinctrl-foo",
> .function = "mmc0",
> .group = "mmc0_2_grp",
> - .dev_name = "foo-mmc.0",
> },
> {
> + .dev_name = "foo-mmc.0",
> .name = "8bit"
> .ctrl_dev_name = "pinctrl-foo",
> .function = "mmc0",
> .group = "mmc0_3_grp",
> - .dev_name = "foo-mmc.0",
> },
Seems like random reordering but no big deal, could go in separate
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");
Ohhh sweet! I like pinctrl_get_select()!
> 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",
Again define a standard name, I think PINCTRL_STATE_DEFAULT
should be used for this too, since it means it defaults to active.
> -PIN_MAP_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
> +PIN_MAP_SYS_HOG("active", "pinctrl-foo", "power_func")
Then use that PINCTRL_STATE_DEFAULT
> This snippet first muxes the function in the pins defined by group A, enables
> it, disables and releases it, and muxes it in on the pins defined by group B:
> @@ -1017,23 +1045,36 @@ it, disables and releases it, and muxes it in on the pins defined by group B:
> foo_switch()
> {
> struct pinctrl *p;
> + struct pinctrl_state *s1, *s2;
> +
> + /* Setup */
> + p = pinctrl_get(&device);
> + if (IS_ERR(p))
> + ...
> +
> + s1 = pinctrl_lookup_state(foo->p, "pos-A");
> + if (IS_ERR(s1))
> + ...
> +
> + s2 = pinctrl_lookup_state(foo->p, "pos-B");
> + if (IS_ERR(s2))
> + ...
>
> /* Enable on position A */
> - p = pinctrl_get(&device, "spi0-pos-A");
> - if IS_ERR(p)
> - return PTR_ERR(p);
> - pinctrl_enable(p);
> + ret = pinctrl_select_state(s);
> + if (ret < 0)
> + ...
>
> - /* This releases the pins again */
> - pinctrl_disable(p);
> - pinctrl_put(p);
> + ...
>
> /* Enable on position B */
> - p = pinctrl_get(&device, "spi0-pos-B");
> - if IS_ERR(p)
> - return PTR_ERR(p);
> - pinctrl_enable(p);
> + ret = pinctrl_select_state(s);
> + if (ret < 0)
> + ...
> +
> ...
> +
> + pinctrl_put(p);
> }
This is looking real good. I think recent discussion with Russell
also ironed out the problems with potential gpio interaction.
> - PIN_MAP_SYS_HOG("POWER", "pinmux-u300", "power"),
> - PIN_MAP_SYS_HOG("EMIF0", "pinmux-u300", "emif0"),
> - PIN_MAP_SYS_HOG("EMIF1", "pinmux-u300", "emif1"),
> + PIN_MAP_SYS_HOG("default", "pinmux-u300", "power"),
> + PIN_MAP_SYS_HOG("default", "pinmux-u300", "emif0"),
> + PIN_MAP_SYS_HOG("default", "pinmux-u300", "emif1"),
> /* per-device maps for MMC/SD, SPI and UART */
> - PIN_MAP("MMCSD", "pinmux-u300", "mmc0", "mmci"),
> - PIN_MAP("SPI", "pinmux-u300", "spi0", "pl022"),
> - PIN_MAP("UART0", "pinmux-u300", "uart0", "uart0"),
> + PIN_MAP("default", "pinmux-u300", "mmc0", "mmci"),
> + PIN_MAP("default", "pinmux-u300", "spi0", "pl022"),
> + PIN_MAP("default", "pinmux-u300", "uart0", "uart0"),
> };
PINCTRL_STATE_DEFAULT
So far I really like this. I should look closer at the code but mainly
I just have to trust you on it and fix any bugs later, this change is
way more valuable as is.
Please move this patch as far to the head of the series as possible
so we can get it in, I guess you need the state lookup in struct pinctrl
to proceed but could we e.g. puy the locking changes after this?
Yours,
Linus Walleij
--
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