On Tue, Sep 20, 2011 at 11:58 PM, Stephen Warren <swar...@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 _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
