Linus Walleij wrote at Monday, October 17, 2011 9:03 AM:> Thanks for a long letter, took som time to read, but as usual it> contains good stuff!> > I think you're talking about two things here:> > 1) The need to configure per-pin "stuff" like biasing, driving,> load capacitance ... whatever> > 2) The need to handle state transitions of pinmux settings.

Yes, that's true.

> I prefer that we try to keep these two separate and not conflate> them too much.> > On Thu, Oct 13, 2011 at 10:59 PM, Stephen Warren <swarren@nvidia.com> wrote:> > > 2) Enhance the pin controller subsystem to support configuring these> > properties.> > This is definately what we want to do. That is why the subsystem was> renamed from pinmux to pinctrl in the first place, and also the rationale> for introducing the abstract pin groups.> > > int (*pin_config)(unsigned pin, u32 param, u32 data);> > int (*group_config)(unsigned group, u32 param, u32 data);> > Do you mean> int (*group_config)(const char *group, u32 param, u32 data);> > On the latter one? We identify groups by name mainly.> The selectors is an internal thing between pinctrl core and> drivers.

That was a proposal for the core->driver API, so I think using an intinstead of a string makes sense?

> > Where "param" is some driver-specific parameter, such as:> >> > #define TEGRA_PINCTRL_PARAM_TRISTATE_ENABLE 0> (...)> > I looked at a bunch of existing pinmux drivers in the mainline kernel.> > There are certainly some common concepts between SoCs. However, the> > details are often different enough that I don't think attempting to> > unify the set of configuration options will be that successful; I> > doubt we could create a generic enough abstraction of these settings> > for a cross-SoC driver to be able to usefully name and select values> > for all the different pin/group options.> > I don't know, I could easily say the same thing about say input devices:> all of them are essentially different, still they opted to create the file> <linux/input.h> with this kind of stuff:> > ...> #define KEY_COMMA 51> #define KEY_DOT 52> #define KEY_SLASH 53> #define KEY_RIGHTSHIFT 54> #define KEY_KPASTERISK 55> ...> > And it has proven to be very useful. I'd rather opt to just remove> the TEGRA_ prefix from all the above, and that we try to create some> common sematics to these calls.

Hmm. Keys seem to have a bit more uniform behavior than the variouspin configuration data. Still, I guess I'm fine defining some unifiednumbering space for all the parameters, so long as we can add Soc-specificentries to the list when they don't match anything from other SoCs.

> > Data being a plain u32 rather than a pointer as your v7 patchset had it:> > Actually it looked like this:> > static inline int pinmux_config(struct pinmux *pmx, u16 param,> unsigned long *data)> > That was supposed to be an unsigned long (not a *pointer), which> is exchangeable to pointer, as per the example known from> interrupt handlers. It can also be used to pass a plain argument.> It was designed to be "ioctl()-like".> > So I prefer we say:> int (*pin_config)(unsigned pin, u32 param, unsigned long data);> int (*group_config)(const char *group, u32 param, unsigned long data);

For the data function parameter, that's the same as my proposal, exceptfor "unsigned long" rather than "u32".

...> > Now let's suppose that we've enhanced the mapping table to support pin/> > group_config values, and that a driver is written to expect a pinmux> > mapping table with the following mappings:> >> > "active"> > "suspend"> > I'm not following why this should be different mappings.> > I would rather contrast it with the similar concept from the> regulator framework, these have modes like these:> > enum regulator_status {> REGULATOR_STATUS_OFF,> REGULATOR_STATUS_ON,> REGULATOR_STATUS_ERROR,> /* fast/normal/idle/standby are flavors of "on" */> REGULATOR_STATUS_FAST,> REGULATOR_STATUS_NORMAL,> REGULATOR_STATUS_IDLE,> REGULATOR_STATUS_STANDBY,> };> > So I think we should have pin group states in a similar> manner:> > enum pinmux_state {> PINMUX_STATE_DEFAULT, /* == active */> PINMUX_STATE_LOWPOWER,> };> > And associated calls:> > pinmux_set_state(const char *group, enum pingroup_state state);

I don't think the pinctrl SoC driver could define what the param valuesare for such a set of states; the set of parameters to vary, and thevalues to set them to, is most likely board-specific.

Now, a board probably could define such a set of "states". Perhaps we'dend up with the board supplying both a mapping table, and a "configstates" table. I suppose if the APIs to change pin config params betweenstates is separate from the API to acquire ownership of the pins and setpin mux, as you proposed later in your email, then having two tables doesmake some sense.

> > Where the only difference between those two mappings is some pin/> > group_config value. When switching between those two settings, the "active"> > mux values will be rolled back to some "safe" values, then the "suspend"> > mux values will be re-programmed. This may cause spurious changes in output> > signals, depending on what the "safe" function is exactly. It might even> > temporarily change some pins from inputs to outputs.> > A pinmux_set_state() function all the way down to the driver> could handle this I think.

Such a function would handle changing config params just fine.

However, what if you want to change between mux settings without goingvia any intermediate "safe" mux settings?

That was the main reason I suggested putting both the mux and configvalues into the same mapping table, so either or both could be switchedusing the same state-switching API.

> If this has to follow some specific order like first go> from group A -> B -> C should probably be handled in> the driver since that's definately HW-specific, else we start> embedding some scripting engine in the pinctrl core> and that would be most unfortunate (IMO).

I'm not sure if the driver is the place to make the decision; I think itwould be board-specific; any requirements for this are driven by what isconnected to the pins in question on a particular board, and hence whichfunctions conflict with the connected HW, and that's something only theboard-specific mapping table could know about. Oh, and the issue is morethat we'd want to go straight from A -> B and not A -> SAFE -> B, whereSAFE would be a requirement if we had to disable(A) enable(B) rather thanjust having A selected, then calling switch_to(B).

I did wonder while writing my original email if the mapping table shouldbecome a sequence of instructions rather than an unordered list of settings.I don't see a need for conditionals, loops, functions, or anything morethan just an ordered sequence of mux or config settings though.