i2c: omap: Use noirq system sleep pm ops to idle device for suspend

Message ID 20180927193812.87462-1-tony@atomide.com
State New
Headers show
Series
  • i2c: omap: Use noirq system sleep pm ops to idle device for suspend
Related show

Commit Message

Tony Lindgren Sept. 27, 2018, 7:38 p.m.
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(-)

Comments

Grygorii Strashko Sept. 28, 2018, 9:32 p.m. | #1
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>
Wolfram Sang Sept. 30, 2018, 11:16 p.m. | #2
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?
Tony Lindgren Oct. 1, 2018, 2:20 p.m. | #3
* 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

Patch

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),
 	},
 };