lkml.org 
[lkml]   [2012]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH V2 5/6] pinctrl: Enhance mapping table to support pin config operations
    On Tue, Feb 28, 2012 at 01:27:31PM -0700, Stephen Warren wrote:
    > The pinctrl mapping table can now contain entries to:
    > * Set the mux function of a pin group
    > * Apply a set of pin config options to a pin or a group
    >
    > This allows pinctrl_select_state() to apply pin configs settings as well
    > as mux settings.
    >
    > Signed-off-by: Stephen Warren <swarren@nvidia.com>
    > ---
    ....
    > +Finally, some devices expect the mapping table to contain certain specific
    > +named states. When running on hardware that doesn't need any pin controller
    > +configuration, the mapping table must still contain those named states, in
    > +order to explicitly indicate that the states were provided and intended to
    > +be empty. Table entry macro PIN_MAP_DUMMY_STATE serves the purpose of defining
    > +a named state without causing any pin controller to be programmed:
    > +
    > +static struct pinctrl_map __initdata mapping[] = {
    > + PIN_MAP_DUMMY_STATE("foo-i2c.0", PINCTRL_STATE_DEFAULT),
    > };
    Is this used for shared devices between different platforms which may need pinmux
    setting while another not?

    >
    > -PIN_MAP_SYS_HOG("pinctrl-foo", "power_func")
    > +PIN_MAP_MUX_GROUP_HOG("pinctrl-foo", NULL /* group */, "power_func")
    Missed state name or should be PIN_MAP_MUX_GROUP_HOG_DEFAULT?

    >
    > This gives the exact same result as the above construction.
    >
    > diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
    > index 18973ca..c965bb5 100644
    > --- a/arch/arm/mach-u300/core.c
    > +++ b/arch/arm/mach-u300/core.c
    > @@ -1569,13 +1569,13 @@ static struct platform_device dma_device = {
    > /* Pinmux settings */
    > static struct pinctrl_map __initdata u300_pinmux_map[] = {
    > /* anonymous maps for chip power and EMIFs */
    > - PIN_MAP_SYS_HOG("pinctrl-u300", "power"),
    > - PIN_MAP_SYS_HOG("pinctrl-u300", "emif0"),
    > - PIN_MAP_SYS_HOG("pinctrl-u300", "emif1"),
    > + PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-u300", NULL, "power"),
    > + PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-u300", NULL, "emif0"),
    > + PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-u300", NULL, "emif1"),
    > /* per-device maps for MMC/SD, SPI and UART */
    > - PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "mmc0", "mmci"),
    > - PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "spi0", "pl022"),
    > - PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "uart0", "uart0"),
    > + PIN_MAP_MUX_GROUP_DEFAULT("mmci", "pinctrl-u300", NULL, "mmc0"),
    > + PIN_MAP_MUX_GROUP_DEFAULT("pl022", "pinctrl-u300", NULL, "spi0"),
    > + PIN_MAP_MUX_GROUP_DEFAULT("uart0", "pinctrl-u300", NULL, "uart0"),
    Although not big issue, but i'm a bit more intended to the original way that
    for the NULL group name using:
    PIN_MAP_MUX_DEFAULT("mmci", "pinctrl-u300", "mmc0"),
    Or using PIN_MAP_MUX_GROUP_DEFAULT, but just totally remove the optional NULL
    group name support to keep consistence and avoid confusing.

    > diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
    > index 05d25c8..b572153 100644
    > --- a/include/linux/pinctrl/machine.h
    > +++ b/include/linux/pinctrl/machine.h
    > @@ -14,6 +14,41 @@
    >
    > #include "pinctrl.h"
    >
    > +enum pinctrl_map_type {
    > + PIN_MAP_TYPE_INVALID,
    > + PIN_MAP_TYPE_DUMMY_STATE,
    > + PIN_MAP_TYPE_MUX_GROUP,
    > + PIN_MAP_TYPE_CONFIGS_PIN,
    > + PIN_MAP_TYPE_CONFIGS_GROUP,

    Basically it causes a bit confusing to me that we have per pin config
    but we do not have per pin mux.
    However, i still have not got a better idea for this issue.

    > +#define PIN_MAP_MUX_CONFIGS_GROUP(dev, state, pinctrl, grp, cfgs) \
    Actually this macro is only for pin config setting,
    maybe PIN_MAP_CONFIGS_GROUP is better, right?

    > + { \
    > + .dev_name = dev, \
    > + .name = state, \
    > + .type = PIN_MAP_TYPE_CONFIGS_GROUP, \
    > + .ctrl_dev_name = pinctrl, \
    We have three entries above duplicated with MUX macro.

    > + .data.configs = { \
    > + .group_or_pin = grp, \
    > + .configs = cfgs, \
    > + .num_configs = ARRAY_SIZE(cfgs), \
    > + }, \
    > + }
    It seems you separate the pin mux setting and pin config setting in
    different maps.
    Can we merge them into one map?
    That will save a lot of map entries and improve searching performance.

    Another question is that:
    the current IMX pinmux setting for non-dt is like:
    #define MX51_I2C_PAD_CTRL (PAD_CTL_SRE_FAST | PAD_CTL_ODE | \
    PAD_CTL_DSE_HIGH | PAD_CTL_PUS_100K_UP | \
    #define MX51_PAD_KEY_COL4__I2C2_SCL \
    IOMUX_PAD(0x65c, 0x26c, 0x13, 0x9b8, 1, MX51_I2C_PAD_CTRL)
    This macro includes both mux and config setting.
    It's very easy and conveniently to use.
    So i'd really like to see a similar using after migrate to pinctrl subsystem
    that using a macro to set both.

    One possible working method i thought is extended macro based on your code:
    #define PIN_MAP_MUX_CONFIG_GROUP(dev, state, pinctrl, grp, func, cfgs) \
    PIN_MAP_MUX_GROUP(dev, state, pinctrl, grp, func), \
    PIN_MAP_CONFIG_GROUP(dev, state, pinctrl, grp, cfgs)

    It works for group.
    However, we can not also do this for per PIN config since PIN_MAP_MUX_*
    is only for group.

    Regards
    Dong Aisheng



    \
     
     \ /
      Last update: 2012-03-01 09:43    [W:7.986 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site