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.
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:
- 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.
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.
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);
Using a pointer does allow passing arbitrary data into the driver, which seems useful. However, this also makes it much more difficult for the core pin controller API to handle the values, e.g. in the mapping table, in a device-tree representation thereof, etc.
Only if it treats "param" as opaque. If we let "param" infer the semantics of "data" it's no problem, and it can easily switch(param) to select the proper semantics for the data.
Having both config_pin and config_group functions:
I buy that straight off, there is obviously a need for that.
Issue 2: Do we need a new "pin settings" table passed from the board to the pin controller core?
Yes I think so, trying to stuff it into the mappings does not look appealing at all. We should try to separate concerns, also it makes for nice separation in the DT metadata I suspect?
Issue 3: Do we need to change "pin settings" on-the-fly, or only at boot?
Both I think, from own experience and from reading the text below...
struct pinmux_map { const char *name; struct device *ctrl_dev; const char *ctrl_dev_name; enum pinmux_map_type type; union { struct { const char *function; const char *group; } mux; struct { u32 param; u32 data; } config; } data; struct device *dev; const char *dev_name; const bool hog_on_boot; };
Such that pinmux_enable would both activate all the mux settings, and also call pin/group_config based on the mapping table.
No thanks :-)
Let's keep these two separate. One mapping, one default pin config or whatever we want to call it.
I'd like to enhance the pinmux core to allow the mapping table to be read from device tree. If we end up with a separate "pin configuration" table, the same applies there. Do you have any thoughts here, or should I just go ahead and create/propose something? I'd probably base this partially on my previous patches that do this within the Tegra pinmux driver itself.
Something is better than nothing, but I cannot claim to be knowledgeable in DT.
Right now I would prefer to create basic stuff first, i.e. create a device tree binding for the stuff we have - the mapping table, get that ready for merge and then move on to the next thing.
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);
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.
The pinmux API currently conflates two different things:
a) A device's ownership of certain pins.
b) The configuration of those pins.
I think we should aim for some mechanism of teasing those two concepts apart, and allowing dynamic transitioning between different configurations for the owned pins without completely releasing them.
I agree with this. So we only need to decide on a proper mechanism. pinmux_set_state() looks appealing to me...
Driver mapping_name configuration settings
i2c - active group i2cp.mux = i2c i2c - active group i2cp.tristate = false i2c - suspend group i2cp.mux = i2c i2c - suspend group i2cp.tristate = true
I would prefer not to try to shoehorn this into the mapping table. Isn't it possible to introduce this in some orthogonal way, like a per-group state table with associated groups?
Right now the pinmux_map** enumerates the valid groups, and pinmux_get()/put() selects one of them, I don't see any problem with pinmux_set_state() changing from one group to another.
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).
Driver probe():
reservation = pinmux_reserve(device, NULL /* mapping_name */) pinmux_select(reservation, "active")
Just pmx = pinmux_get()
Driver suspend(): pinmux_select(reservation, "suspend")
pinmux_set_state(pmx, PINMUX_STATE_SUSPEND);
Driver resume():
pinmux_select(reservation, "active")
pinmux_set_state(pmx, PINMUX_STATE_DEFAULT);
Driver remove(): pinmux_unreserve(reservation)
pinmux_put(pmx);
For reference, I read through all the pinmux drivers I could find in arch/ arm, and made a rough list of all the pin/group_config settings they support:
Thanks for this! I need to go over them too, also read some datasheets I guess, else I can not figure out whether my idea of trying to keep parameters generic makes sense or is just plain raving mad.
Yours, Linus Walleij