* Linus Walleij <linus.walleij@xxxxxxxxxx> [130611 01:00]: > On Mon, Jun 10, 2013 at 6:23 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote: > > > We only should remux the pins that need remuxing as that's done > > every time hitting idle. So I think we should have the following > > default groups: > > > > default Static pins that don't change, no need to remux > > configured in consumer driver probe like we already > > do > > > > active Optional dynamic pins remuxed for runtime, can be > > configured and selected in consumer driver probe. > > The consumer driver may also want to select this > > state in PM runtime resume. > > > > idle Optional dynamic pins remuxed for idle. The consumer > > driver may also want to select this state in PM > > runtime suspend depending on device_can_wakeup() > > and driver specific needs. > > The one thing I don't understand is why a driver would select the > active state in probe(), unless it's a driver that does not support > runtime PM. (But maybe that's what you mean.) Yes you're right, there should not be any need to select active state in probe, that should be selected by PM runtime. > Compare this to <linus/pinctrl/pinctrl-state.h>: > > /** > * @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put > * into as default, usually this means the pins are up and ready to > * be used by the device driver. This state is commonly used by > * hogs to configure muxing and pins at boot, and also as a state > * to go into when returning from sleep and idle in > * .pm_runtime_resume() or ordinary .resume() for example. > * @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into > * when the pins are idle. This is a state where the system is relaxed > * but not fully sleeping - some power may be on but clocks gated for > * example. Could typically be set from a pm_runtime_suspend() or > * pm_runtime_idle() operation. > * @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into > * when the pins are sleeping. This is a state where the system is in > * its lowest sleep state. Could typically be set from an > * ordinary .suspend() function. > */ > #define PINCTRL_STATE_DEFAULT "default" > #define PINCTRL_STATE_IDLE "idle" > #define PINCTRL_STATE_SLEEP "sleep" > > The way I currently use these in e.g. > drivers/spi/spi-pl022 is: > > probe: > -> default > > runtime_suspend: > -> idle > > runtime_resume: > -> default > > suspend: > -> sleep > > resume: > -> default > -> idle > > Notice that we go to default then idle on probe and > runtime resume. This is because the idle state is > optional (as is the sleep state). > > So I guess if we should extend this terminology to match > what you are using for the OMAP it would rather be like > this: > > probe: > -> default > > runtime_suspend: > -> idle > > runtime_resume: > -> default > -> active At least for omaps, there's no need to select default in runtime_resume as the default pins stay that way. > suspend: > -> sleep For omaps, we would just select idle pins again in the suspend case. > resume: > -> default > -> idle And for omaps, there's no need to select default in resume either. Just selecting active would do the trick for resume. So for omaps, the sequence would be: probe: -> default (typically all device pins except rx pin) runtime_suspend: suspend: -> idle (remux rx pin from device to gpio input for wake) runtime_resume: resume: -> active (remux rx pin from gpio input to device) > Just one more optional "active" state in runtime resume. > Correct? Yes the "active" is needed, but "sleep" would be unused for omaps. > If we can agree on this I will add the active state to the > state table and add a container in the core for this as well > as pinctrl_pm_select_active_state() so we can skip all the > pointless boilerplate also in the OMAP drivers, plus increase > the readability and portability quite a bit. Sounds good to me as long as we don't always need to select the default pins over and over in PM runtime_resume. > >> However in this case I *suspect* that what you really want > >> to do it to rename the state called "default" to "sleep" > >> (it appears the default state is sleepy) and then rename > >> the "active" state to "default" (as this is the defined semantic > >> meaning of "default" from <linux/pinctrl/pinctrl-state.h>. > > > > The idle state above could also be called sleep instead of idle > > if you prefer that. > > No I think we should reserve that name for the pin state > associated with suspend(). Let's leave it like this. OK > > I think the confusion is caused by the fact that we need three > > mux groups, not just two :) The toggling between active and idle > > is the hotpath as that can potentially happen for multiple drivers > > every time we enter and exit idle. > > Actually we have the same thing, it's just that our "default" > and "active" are the same thing. But it seems we need to > add your granularity to this. Well the difference seems to be that you need to remux all the device pins for runtime_suspend and resume while in most of the cases I know of only one device pins needs to be toggled and the rest can be selected in driver probe. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
