RE: [PATCH V2 5/6] pinctrl: Enhance mapping table to support pinconfig operations

[Stephen Warren]

Dong Aisheng wrote at Thursday, March 01, 2012 8:47 PM:
> On Thu, Mar 01, 2012 at 08:52:12AM -0800, Stephen Warren wrote:
> > Dong Aisheng wrote at Thursday, March 01, 2012 1:46 AM:
> > > On Tue, Feb 28, 2012 at 01:27:31PM -0700, Stephen Warren wrote:
...
> > > > diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
...
> > > > +	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"),
...
> > > > +	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 did not mean remove the group parameter support, i meant remove the optinal NULL
> group name support to avoid using the macro like:
> PIN_MAP_MUX_GROUP_DEFAULT("uart0", "pinctrl-u300", NULL, "uart0")
> User should be better to specify a group name for this macro to avoid misleading.

Ah right.

Yes, I'd be perfectly happy removing support for not specifying group
as NULL, and requiring a specific group name to be provided in all cases.
I like being specific:-)

Linus,

What are your thoughts here? IIRC, you were the one who wanted to be
able to specify group==NULL, and have the pinctrl core use the "first"
supported group for the function.

Either way, I think that if we did require non-NULL group, this should
be a separate patch we can introduce after merging my series, to avoid
feature creep.

...
> > > > +		.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...
>
> Well, they're indeed a lot.
> But it seems they're not related to whether merging config and mux into one
> map, right?

There's certainly some relationship; see my explanation above re: why
you'd end up with a lot more macros. Of course, if we didn't create all
the special-case macros, this problem wouldn't exist, so I don't think
there would be a problem...

> > 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.
>
> Do you mean treat them all(mux and config) as a config setting?
> Then what benefits can we get?
> Does it have any difference from separate them but still in one map?

I was thinking that we could put the mux values into the config array,
so instead of the mapping table for a combined mux+config entry having
specific fields for:

* pin group to configure
* mux function to set (or set to NULL/-1/... if not used)
* list of config options to set (or NULL if not used)

We could put the mux function into the config array. That way, instead
of having two optional mux and config fields, we'd have 1 mandatory
config field, and the config array may-or-may-not have a mux entry in
it, e.g.:

unsigned long tegra_config_ata[] = {
      TEGRA_PINCONF_PACK(TEGRA_PINCONF_PARAM_MUX, TEGRA_MUX_FUNCTION_SPI1),
	TEGRA_PINCONF_PACK(TEGRA_PINCONF_PARAM_PULL, TEGRA_PINCONFIG_PULL_NONE),
	TEGRA_PINCONF_PACK(TEGRA_PINCONF_PARAM_TRISTATE, TEGRA_PINCONFIG_DRIVEN),
};

However, upon further reflection, that might be a bad idea, because then
there would be no way to share common sets of pin config entries between
multiple mapping table entries, since every pin/group would differ in
their mux setting, i.e. the following couldn't share anything:

unsigned long tegra_config_atb[] = {
      TEGRA_PINCONF_PACK(TEGRA_PINCONF_PARAM_MUX, TEGRA_MUX_FUNCTION_SPI2),
	TEGRA_PINCONF_PACK(TEGRA_PINCONF_PARAM_PULL, TEGRA_PINCONFIG_PULL_NONE),
	TEGRA_PINCONF_PACK(TEGRA_PINCONF_PARAM_TRISTATE, TEGRA_PINCONFIG_DRIVEN),
};

But with the current scheme the mux setting isn't there, so the two arrays
would be the same and could be shared.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/