Linus (W),
Some more thoughts on pinmux:
========== GPIO/pinmux API interactions
I'd like to understand how the gpio and pinmux subsystems are intended to interact.
Quoting from Documentation/gpio.txt:
Note that requesting a GPIO does NOT cause it to be configured in any way; it just marks that GPIO as in use. Separate code must handle any pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
Due to this, the current Tegra GPIO driver contains additional non- standard functions:
void tegra_gpio_enable(int gpio); void tegra_gpio_disable(int gpio);
In some cases, those functions are called by board files at boot time, and in other cases by drivers themselves, right before gpio_request().
Is it your intention that such custom functions be replaced by pinmux_request_gpio(), and the Tegra pinmux driver's gpio_request_enable op should call tegra_gpio_enable?
That seems like the right idea to me.
========== pinmux calling GPIO or vice-versa?
Having pinmux call gpio appears to conflict with a couple of things from your recent pinmux patchset:
diff --git a/drivers/Kconfig b/drivers/Kconfig +# pinctrl before gpio - gpio drivers may need it
+source "drivers/pinctrl/Kconfig"
source "drivers/gpio/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile ... +# GPIO must come after pinctrl as gpios may need to mux pins etc +obj-y += pinctrl/
Don't those patches imply that the GPIO controller code is calling into the pinmux code to perform muxing, not the other way around?
========== When pins get marked as in-used
There seems to be a discrepancy in the way the client APIs work: For special functions, the client calls pinmux_get, then later pinmux_enable, whereas for GPIOs, the client just calls pinmux_request_gpio. I'm not sure of the benefit of having separate pinmux_get and pinmux_enable calls; I thought the idea was that a client could:
a = pinmux_get(dev, "funca"); b = pinmux_get(dev, "funcb"); pinmux_enable(a); ... pinmux_disable(a); pinmux_enable(b); ... pinmux_disable(b); pinmux_put(b); pinmux_put(a);
However, since the pins are marked as reserved/in-use when pinmux_get is called rather than pinmux_enable, the code above won't work; it'd need to be:
a = pinmux_get(dev, "funca"); pinmux_enable(a); ... pinmux_disable(a); pinmux_put(a); b = pinmux_get(dev, "funcb"); pinmux_enable(b); ... pinmux_disable(b); pinmux_put(b);
Perhaps pin usage marking should be moved into enable/disable?
========== Matched request/free calls for GPIOs
A couple more comments on your recent pinmux patches in light of this:
From pin_request():
+ /* + * If there is no kind of request function for the pin we just assume + * we got it by default and proceed. + */ + if (gpio && ops->gpio_request_enable) + /* This requests and enables a single GPIO pin */ + status = ops->gpio_request_enable(pmxdev, + pin - pmxdev->desc->base); + else if (ops->request) + status = ops->request(pmxdev, + pin - pmxdev->desc->base); + else + status = 0;
a) I feel the default should be to error out, not succeed; the whole point of the API is for HW where something must be programmed to enable each function. As such, if you don't provide that ops->request* function, that seems like an error.
I think the logic above should be to always call ops->gpio_request_enable if (gpio):
if (gpio) if (ops->gpio_request_enable) /* This requests and enables a single GPIO pin */ status = ops->gpio_request_enable(pmxdev, pin - pmxdev->desc->base); else status = ops->request(pmxdev, pin - pmxdev->desc->base);
(ops->gpio_request_enable could be optional if GPIOs aren't supported on any pinmux pins, whereas ops->request isn't optional, and should be checked during pinmux_register)
Also, ops->gpio_request_enable should also be passed the GPIO number, so the driver can validate that it's the correct one. Often, only one value would be legal, but perhaps some pinmuxs allow 1 of N different GPIOs to be mapped to some pin, so selection of which GPIO is on par with selection of which special function to mux out, yet the driver still doesn't want to expose every GPIO possibility as a pinmux "function" due to explosion of the number of functions if it did that.
b) When ops->gpio_request_enable is called to request a pin, it'd be nice if the core could track this, and specifically call a new ops->gpio_disable to free it, rather than the generic ops->free. There are two cases in HW:
b1) "GPIO" is just another special function in a list, e.g. SPI, I2C, GPIO. In this case, free for a GPIO needs to either do nothing (the next call to request will simply set the desired function), or possibly do something to disable the pin in the same way as any other function. This works fine with a single free function.
b2) GPIO enable/disable is a separate concept from the pinmuxing; on Tegra it'll be implemented by calling tegra_gpio_enable/disable. As such, a single free function would require the pinmux driver to store state indicating whether ops->free should call tegra_gpio_disable, or whether it should do cleanup to the pinmux registers. Since the core is managing GPIO vs. function allocation, I think the storing that state in the core makes sense.
Thanks for reading. Hopefully this email didn't jump about too much.