| Message ID | 20180927193812.87462-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: omap: Use noirq system sleep pm ops to idle device for suspend | expand |
On 09/27/2018 02:38 PM, Tony Lindgren wrote: > With runtime PM autosuspend we may have i2c-omap left into enabled > state if an i2c client reconfigures registers for suspend as for > example pixcir_ts driver does. Let's make sure i2c-omap is suspended > by adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. > > Let's also get rid of some ifdefs while at it and replace them with > __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > already deal with the various PM Kconfig options. > > Cc: Dave Gerlach <d-gerlach@ti.com> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Keerthy <j-keerthy@ti.com> > Cc: Tero Kristo <t-kristo@ti.com> > Reported-by: Keerthy <j-keerthy@ti.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > This problem is not yet exposed with the mainline kernel so this can > wait for v4.20 merge window no problem. Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
On Thu, Sep 27, 2018 at 12:38:12PM -0700, Tony Lindgren wrote: > With runtime PM autosuspend we may have i2c-omap left into enabled > state if an i2c client reconfigures registers for suspend as for > example pixcir_ts driver does. Let's make sure i2c-omap is suspended > by adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. I wonder: if we have RPM autosuspend and do a system suspend with the autosuspend timeout not reached yet, shouldn't rather the PM core do this enforcement instead of updating each and every driver?
* Wolfram Sang <wsa@the-dreams.de> [180930 23:20]: > On Thu, Sep 27, 2018 at 12:38:12PM -0700, Tony Lindgren wrote: > > With runtime PM autosuspend we may have i2c-omap left into enabled > > state if an i2c client reconfigures registers for suspend as for > > example pixcir_ts driver does. Let's make sure i2c-omap is suspended > > by adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. > > I wonder: if we have RPM autosuspend and do a system suspend with the > autosuspend timeout not reached yet, shouldn't rather the PM core do > this enforcement instead of updating each and every driver? I've been wondering about that too. Obviously trying to do force suspend automatically when there is no autosuspend timeout is not a safe thing to do for things like RTC :) Adding linux-pm list folks to Cc too. Not sure if we can do it automatically but at least we could document it with something like the following at the end of "6. Runtime PM and System Sleep": Note that if runtime PM autosuspend timeout is used for a bus driver such as an I2C bus controller, it should be paired with suitable struct dev_pm_ops. This is to ensure the bus driver is properly suspended for cases where it's child driver reconfigures registers for suspend. In most cases setting SET_NOIRQ_SYSTEM_SLEEP_PM_OPS with pm_runtime_force_suspend and pm_runtime_force_suspend will be enough if no specific bus driver configuration is needed for suspend. Regards, Tony
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1498,8 +1498,7 @@ static int omap_i2c_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM -static int omap_i2c_runtime_suspend(struct device *dev) +static int __maybe_unused omap_i2c_runtime_suspend(struct device *dev) { struct omap_i2c_dev *omap = dev_get_drvdata(dev); @@ -1525,7 +1524,7 @@ static int omap_i2c_runtime_suspend(struct device *dev) return 0; } -static int omap_i2c_runtime_resume(struct device *dev) +static int __maybe_unused omap_i2c_runtime_resume(struct device *dev) { struct omap_i2c_dev *omap = dev_get_drvdata(dev); @@ -1540,20 +1539,18 @@ static int omap_i2c_runtime_resume(struct device *dev) } static const struct dev_pm_ops omap_i2c_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, omap_i2c_runtime_resume, NULL) }; -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops) -#else -#define OMAP_I2C_PM_OPS NULL -#endif /* CONFIG_PM */ static struct platform_driver omap_i2c_driver = { .probe = omap_i2c_probe, .remove = omap_i2c_remove, .driver = { .name = "omap_i2c", - .pm = OMAP_I2C_PM_OPS, + .pm = &omap_i2c_pm_ops, .of_match_table = of_match_ptr(omap_i2c_of_match), }, };
With runtime PM autosuspend we may have i2c-omap left into enabled state if an i2c client reconfigures registers for suspend as for example pixcir_ts driver does. Let's make sure i2c-omap is suspended by adding SET_NOIRQ_SYSTEM_SLEEP_PM_OPS. Let's also get rid of some ifdefs while at it and replace them with __maybe_unused as SET_RUNTIME_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS already deal with the various PM Kconfig options. Cc: Dave Gerlach <d-gerlach@ti.com> Cc: Grygorii Strashko <grygorii.strashko@ti.com> Cc: Keerthy <j-keerthy@ti.com> Cc: Tero Kristo <t-kristo@ti.com> Reported-by: Keerthy <j-keerthy@ti.com> Signed-off-by: Tony Lindgren <tony@atomide.com> --- This problem is not yet exposed with the mainline kernel so this can wait for v4.20 merge window no problem. --- drivers/i2c/busses/i2c-omap.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)