[Update,v5,1/3] PM: i2c-designware-platdrv: Clean up PM handling in probe

Commit Message

Rafael J. Wysocki Sept. 25, 2017, 9:10 p.m. UTC

From: Rafael J. Wysocki <[e-mail]>

The power management handling in dw_i2c_plat_probe() is somewhat
messy and it is rather hard to figure out the code intention for
the case when pm_disabled is set.  In that case, the driver doesn't
enable runtime PM at all, but in addition to that it calls
pm_runtime_forbid() as though it wasn't sure if runtime PM might
be enabled for the device later by someone else.

Although that concern doesn't seem to be actually valid, the
device is clearly still expected to be PM-capable even in the
pm_disabled set case, so a better approach would be to enable
runtime PM for it unconditionally and prevent it from being
runtime-suspended by using pm_runtime_get_noresume().

Make the driver do that.

Signed-off-by: Rafael J. Wysocki <[e-mail]>
---

v4 -> v5: Address the Ulf's comment that pm_runtime_forbid() was not
  suitable for preventing runtime suspends of the device from occurring
  (as it could be overridden by user space) and update the changelog.

  Also drop the tags as the patch changed (although that change shouldn't
  matter on systems where pm_disabled is not set).

v3 -> v4: Added Tested-by tags.

-> v3: Small update of the changelog.

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   34 ++++++++++++++++++----------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Wolfram Sang Oct. 5, 2017, 11 a.m. UTC | #1

On Mon, Sep 25, 2017 at 11:10:06PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
> The power management handling in dw_i2c_plat_probe() is somewhat
> messy and it is rather hard to figure out the code intention for
> the case when pm_disabled is set.  In that case, the driver doesn't
> enable runtime PM at all, but in addition to that it calls
> pm_runtime_forbid() as though it wasn't sure if runtime PM might
> be enabled for the device later by someone else.
> 
> Although that concern doesn't seem to be actually valid, the
> device is clearly still expected to be PM-capable even in the
> pm_disabled set case, so a better approach would be to enable
> runtime PM for it unconditionally and prevent it from being
> runtime-suspended by using pm_runtime_get_noresume().
> 
> Make the driver do that.
> 
> Signed-off-by: Rafael J. Wysocki <[e-mail]>

Applied to for-next, thanks!

Patch

Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
===================================================================
--- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
+++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -249,6 +249,14 @@  static void dw_i2c_set_fifo_size(struct
 	}
 }
 
+static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
+{
+	pm_runtime_disable(dev->dev);
+
+	if (dev->pm_disabled)
+		pm_runtime_put_noidle(dev->dev);
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -362,14 +370,17 @@  static int dw_i2c_plat_probe(struct plat
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
-	if (dev->pm_disabled) {
-		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+	/* The code below assumes runtime PM to be disabled. */
+	WARN_ON(pm_runtime_enabled(&pdev->dev));
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+
+	if (dev->pm_disabled)
+		pm_runtime_get_noresume(&pdev->dev);
+
+	pm_runtime_enable(&pdev->dev);
 
 	if (dev->mode == DW_IC_SLAVE)
 		ret = i2c_dw_probe_slave(dev);
@@ -382,8 +393,7 @@  static int dw_i2c_plat_probe(struct plat
 	return ret;
 
 exit_probe:
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
 exit_reset:
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
@@ -402,8 +412,8 @@  static int dw_i2c_plat_remove(struct pla
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
+
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);