On Tue, Sep 20, 2011 at 11:58 PM, Stephen Warren swarren@nvidia.com wrote:
Linus Walleij wrote at Friday, September 16, 2011 6:13 AM:
This creates a subsystem for handling of pin control devices. These are devices that control different aspects of package pins.
I've read through the documentation and header files, but not the .c files, and this looks almost perfect as far as I can tell right now. I'll try to review the .c files sometime too.
Great, I'm hunting your Acked-by/Reviewed-by ...
I will likely request inclusion into linux-next soon-ish.
I just have one comment:
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
...
+/* External interface to pinmux */ +extern int pinmux_request_gpio(unsigned gpio); +extern void pinmux_free_gpio(unsigned gpio); +extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *name); +extern void pinmux_put(struct pinmux *pmx); +extern int pinmux_enable(struct pinmux *pmx); +extern void pinmux_disable(struct pinmux *pmx); +extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data);
That definition of pinmux_config doesn't seem as useful as it could be;
It should be removed. It's just there in the header file, I killed off the implementation because specific control of a mux doesn't make sense. We want to do stuff to pin groups directly, not related to muxing, so that kind of thing needs to be in the generic pinctrl interface.
I'd like the ability to execute pinmux_config on a /named/ group, and I can certainly see a use-case for applying it to /named/ pins too.
That sounds correct to me.
To abstract things the stuff we can do with the group should be something enumerated too. So:
pinctrl_config_group(const char *pinctrl_device, const char *group, const char *mode); pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);
So the driver need an API to enumerate pin and group modes.
I might want to save this thing for post-merge of the basic API and pinmux stuff though so we don't try to push too much upfront design at once.
The issues with applying pinmux_config to a mapping table entry are:
- When there are multiple mapping table entries referenced by one
pinmux_get, you don't necessarily want to apply the same configuration to all of the groups; think of a bus with a combination of low-speed output control signals and high-speed input data signals or something like that.
- When muxing works in groups, you may want to apply the configuration
to individual pins rather than the whole groups using in the mapping table.
Yeah, we kill this old interface.
Thanks, Linus Walleij