Messages in this thread Patch in this message | | | Date | Thu, 1 Mar 2012 16:46:18 +0800 | From | Dong Aisheng <> | Subject | Re: [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
| |