| Message ID | 1370938616-5952-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 11, 2013 at 10:16:56AM +0200, Linus Walleij wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > In addition to the recently introduced pinctrl core > control, the PM runtime pin control for the OMAP platforms > require a fourth state in addtition to the default, idle and > sleep states already handled by the core: an explicit "active" > state. Let's introduce this to the core in addition to the > other states already defined. > > Cc: Hebbar Gururaja <gururaja.hebbar@ti.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Mark Brown <broonie@kernel.org> > Cc: Kevin Hilman <khilman@linaro.org> > Suggested-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Greg: need your ACK on this to merge it through the pinctrl > tree. > --- > drivers/base/pinctrl.c | 8 +++++++- > drivers/pinctrl/core.c | 15 +++++++++++++++ For anything that just touches drivers/base/pinctrl.c, you don't need my ack, that's your code, you can do whatever you want with it :) But here you go anyway: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On 06/11/2013 02:16 AM, Linus Walleij wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > In addition to the recently introduced pinctrl core > control, the PM runtime pin control for the OMAP platforms > require a fourth state in addtition to the default, idle and > sleep states already handled by the core: an explicit "active" > state. Let's introduce this to the core in addition to the > other states already defined. Surely "default" /is/ "active"? That's what it's meant so far. Since the pinctrl states are represented in DT, the DT bindings of all devices potentially get affected by changes like this. They'd need to be documented in a DT binding document, and the exact semantics of the different states clearly explained. It may be better for struct device, struct platform_driver, or similar, to contain a list of the states that are required by the driver or its binding. That way, the list of states (or states beyond the basic default) is driver-/DT-binding- specific, and custom stuff like this for OMAP wouldn't require explicit code in drivers/base/pinctrl.c, but rather simply iterating over a custom list. Related to that, I'm not sure we should be deriving what we put into DT based on the runtime PM requirements of drivers; DT is supposed to be driven by HW definitions, although I suppose you could argue that the drivers implement what they do because they're implementing the HW requirements.
* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [130611 09:08]: > On Tue, Jun 11, 2013 at 10:16:56AM +0200, Linus Walleij wrote: > > From: Linus Walleij <linus.walleij@linaro.org> > > > > In addition to the recently introduced pinctrl core > > control, the PM runtime pin control for the OMAP platforms > > require a fourth state in addtition to the default, idle and > > sleep states already handled by the core: an explicit "active" > > state. Let's introduce this to the core in addition to the > > other states already defined. > > > > Cc: Hebbar Gururaja <gururaja.hebbar@ti.com> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Stephen Warren <swarren@wwwdotorg.org> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Mark Brown <broonie@kernel.org> > > Cc: Kevin Hilman <khilman@linaro.org> > > Suggested-by: Tony Lindgren <tony@atomide.com> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > Greg: need your ACK on this to merge it through the pinctrl > > tree. > > --- > > drivers/base/pinctrl.c | 8 +++++++- > > drivers/pinctrl/core.c | 15 +++++++++++++++ > > For anything that just touches drivers/base/pinctrl.c, you don't need my > ack, that's your code, you can do whatever you want with it :) > > But here you go anyway: > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Acked-by: Tony Lindgren <tony@atomide.com>
On Tue, Jun 11, 2013 at 10:16 AM, Linus Walleij <linus.walleij@stericsson.com> wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > In addition to the recently introduced pinctrl core > control, the PM runtime pin control for the OMAP platforms > require a fourth state in addtition to the default, idle and > sleep states already handled by the core: an explicit "active" > state. Let's introduce this to the core in addition to the > other states already defined. > > Cc: Hebbar Gururaja <gururaja.hebbar@ti.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Mark Brown <broonie@kernel.org> > Cc: Kevin Hilman <khilman@linaro.org> > Suggested-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> It seems this thing needs another round of discussion so I've pulled this patch out of the branch with stuff for -next and rebased dependencies. Yours, Linus Walleij
diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c index 5fb74b4..833d7cd 100644 --- a/drivers/base/pinctrl.c +++ b/drivers/base/pinctrl.c @@ -51,9 +51,15 @@ int pinctrl_bind_pins(struct device *dev) #ifdef CONFIG_PM /* * If power management is enabled, we also look for the optional - * sleep and idle pin states, with semantics as defined in + * active, sleep and idle pin states, with semantics as defined in * <linux/pinctrl/pinctrl-state.h> */ + dev->pins->active_state = pinctrl_lookup_state(dev->pins->p, + PINCTRL_STATE_ACTIVE); + if (IS_ERR(dev->pins->active_state)) + /* Not supplying this state is perfectly legal */ + dev_dbg(dev, "no active pinctrl state\n"); + dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p, PINCTRL_STATE_SLEEP); if (IS_ERR(dev->pins->sleep_state)) diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index dca9208..ff01b8f 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1217,6 +1217,21 @@ int pinctrl_pm_select_default_state(struct device *dev) return ret; } +int pinctrl_pm_select_active_state(struct device *dev) +{ + struct dev_pin_info *pins = dev->pins; + int ret; + + if (!pins) + return 0; + if (IS_ERR(pins->active_state)) + return 0; /* No active state */ + ret = pinctrl_select_state(pins->p, pins->active_state); + if (ret) + dev_err(dev, "failed to activate pinctrl active state\n"); + return ret; +} + /** * pinctrl_pm_select_sleep_state() - select sleep pinctrl state for PM * @dev: device to select sleep state for diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h index 0f32f10..18cdbb1 100644 --- a/include/linux/pinctrl/consumer.h +++ b/include/linux/pinctrl/consumer.h @@ -42,6 +42,7 @@ extern void devm_pinctrl_put(struct pinctrl *p); #ifdef CONFIG_PM extern int pinctrl_pm_select_default_state(struct device *dev); +extern int pinctrl_pm_select_active_state(struct device *dev); extern int pinctrl_pm_select_sleep_state(struct device *dev); extern int pinctrl_pm_select_idle_state(struct device *dev); #else @@ -49,6 +50,10 @@ static inline int pinctrl_pm_select_default_state(struct device *dev) { return 0; } +static inline int pinctrl_pm_select_active_state(struct device *dev) +{ + return 0; +} static inline int pinctrl_pm_select_sleep_state(struct device *dev) { return 0; @@ -223,6 +228,11 @@ static inline int pinctrl_pm_select_default_state(struct device *dev) return 0; } +static inline int pinctrl_pm_select_active_state(struct device *dev) +{ + return 0; +} + static inline int pinctrl_pm_select_sleep_state(struct device *dev) { return 0; diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h index 281cb91..a61e6a5 100644 --- a/include/linux/pinctrl/devinfo.h +++ b/include/linux/pinctrl/devinfo.h @@ -24,11 +24,15 @@ * struct dev_pin_info - pin state container for devices * @p: pinctrl handle for the containing device * @default_state: the default state for the handle, if found + * @active_state: the active state for the handle, if found + * @sleep_state: the sleep state for the handle, if found + * @idle_state: the idle state for the handle, if found */ struct dev_pin_info { struct pinctrl *p; struct pinctrl_state *default_state; #ifdef CONFIG_PM + struct pinctrl_state *active_state; struct pinctrl_state *sleep_state; struct pinctrl_state *idle_state; #endif diff --git a/include/linux/pinctrl/pinctrl-state.h b/include/linux/pinctrl/pinctrl-state.h index b5919f8..37d0cd1 100644 --- a/include/linux/pinctrl/pinctrl-state.h +++ b/include/linux/pinctrl/pinctrl-state.h @@ -9,6 +9,10 @@ * 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_ACTIVE: the state the pinctrl handle shall be put + * into for explicitly active mode, typically in .pm_runtime_resume() + * and other occasions where we want to be sure that the pins are + * ready to roll. * @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 @@ -20,5 +24,6 @@ * ordinary .suspend() function. */ #define PINCTRL_STATE_DEFAULT "default" +#define PINCTRL_STATE_ACTIVE "active" #define PINCTRL_STATE_IDLE "idle" #define PINCTRL_STATE_SLEEP "sleep"