lkml.org 
[lkml]   [2012]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCH V2 5/6] pinctrl: Enhance mapping table to support pin config operations
    Dong Aisheng wrote at Thursday, March 01, 2012 1:46 AM:
    > 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?

    Exactly yes.

    > > -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?

    Thanks, fixed.

    > > 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.

    The group parameter can't be removed, since in general, the mapping table
    needs some way to indicate which group to activate the function on. For
    example, consider an I2C controller that can be muxed onto two different
    sets of pins based on the board layout - you need to specify which to use.

    I suppose there could be yet more and more macros for muxing; one set
    that hard-codes group to NULL internally and hence doesn't take a parameter,
    and another that takes the group from a parameter. The set of macros and
    their names would start getting extremely unwieldy then though.

    > > 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.

    Well, that's a limitation of how the pinctrl core works right now. It'd
    be entirely possible to fix this, I think, and we can easily expand
    that enum if that happens.

    > > +#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?

    Oh yes, the macro names should either have "MUX" /or/ "CONFIG", not both.
    I'll fix that.

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

    Well yes that's true, but is it really worth fixing? If we fix this,
    Then note that PIN_MUX_DUMMY_STATE also shares the first 3 of those 4
    lines, so we'll end up with a crazy maze of macros by removing all
    possible duplication

    > > + .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.

    Well, it's technically possible.

    But then, you'll need macros that just define a mux setting, just configs
    for groups, or both mux setting and config settings. Then, cross-product
    that with whether the state name is default or not, and cross-product that
    with whether the macro should hard-code group to NULL or not etc. etc...
    It seems like somewhat of a nightmare. Parsing this table is in the slow-
    path of pinctrl_get() anyway, so I don't think it's an enormous deal.

    One alternative might be to represent mux settings as just another config
    on the group though. But that'd require the pin config namespace to have
    some standardized values and some driver-specific values.

    > 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)

    That's exactly what I've done for the Tegra:

    #define TEGRA_MAP_MUX(_group_, _function_) \
    PIN_MAP_MUX_GROUP_HOG_DEFAULT(PINMUX_DEV, _group_, _function_)

    #define TEGRA_MAP_CONF(_group_, _pull_, _drive_) \
    PIN_MAP_CONFIGS_GROUP_HOG_DEFAULT(PINMUX_DEV, _group_, tegra_pincfg_pull##_pull_##_##_drive_)

    #define TEGRA_MAP_MUXCONF(_group_, _function_, _pull_, _drive_) \
    TEGRA_MAP_MUX(_group_, _function_), \
    TEGRA_MAP_CONF(_group_, _pull_, _drive_)

    And the mapping table arrays in the board files use TEGRA_MAP_MUXCONF().

    I'm not convinced it's a good idea to try and come up with common macros
    for this in <pinctrl/machine.h> due to the massive number of macros it'd
    take to represent all possible combinations. And even with such a common
    combo macro, I'd still need to define a custom TEGRA_MAP_MUXCONF in order
    to select which one of the common macros to use (in order to use a shorter
    name in board files than the long common macro names), hard-code the pin
    controller driver name, hard-code the configs array name, etc.

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

    If you know that your chip muxes per pin, then presumably the pin and
    group names are the same, so your combination macro can simply "call"
    PIN_MAP_CONFIGS_PIN_*() instead of PIN_MAP_CONFIGS_GROUP_*().

    Another solution may be to extend the pinctrl core to allow muxing per
    pin where the HW does, so you could use pin-based macros for both mux
    and config. But, lets tackle that after this current round of patches
    is merged.

    --
    nvpublic



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