On 09/22/2012 09:28 AM, Mark Brown wrote:
On Fri, Sep 21, 2012 at 10:23:50AM -0600, Stephen Warren wrote:
On 09/21/2012 07:16 AM, Peter Ujfalusi wrote:
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
Indeed - what about a platform like Tegra which has pinctrl enabled, yet doesn't specify any pinctrl configuration for any device other than the pin controller itself? Do we want to force every device into having to specify an empty pin control configuration? If we take this route, we should really have the driver core or platform bus set up the initial pinctrl state, and I believe that approach was explicitly decided against.
Well, the other options are for pinctrl to either not return an error in the no configuration case or change to a void return type. The current behaviour seems a bit pointless since we can't usefully do anything with the errors, we can just factor the error logs into the core and have done with it since it's not possible to treat errors as such.
(I'm not 100% sure if everyone agrees on this and I'm not sure if it's been explicitly stated, so perhaps this is my personal opinion. However, the more I think about this, the more it makes sense).
I think the errors shouldn't be ignored at all; it's an error for a driver to explicitly go out and request a pinctrl state if that state is not defined. In turn, this means that no generic code should be going out and requesting a pinctrl state on behalf of the driver; it has no idea if the driver has defined that it needs pinctrl set up for it.
I'll explain this in a slightly different way that will probably make more sense.
For any pinctrl configuration that is static, that configuration should be applied one time as the system boots using the "hog" feature of the pin controller itself, i.e. the pin controller driver should apply that state during its own probe(). Individual drivers should only ever request a pinctrl state if they need to dynamically switch between different states at run-time. This is (a) likely to be fairly rare, and (b) the need is likely to be defined directly by the kind of protocol that the driver/HW is implementing.
So in other words, I'd expect to see an almost complete system-wide "default" pinctrl state defined for the pin controller, and quite possibly no other drivers with pinctrl states defined at all.
Drivers that might switch between pinctrl states are say I2C/SPI/MDIO/... bus muxes that use an SoC's pinctrl module to perform the muxing, or perhaps some kind of high-speed data bus that might need different pin configuration (say biasing/equalization/... stuff rather than muxing) based on the runtime-selected bus clock.
Given all this, that probably means that none of your (Peter's) DAI drivers need to touch pinctrl at all, so this patch isn't needed in any form?