Re: [PATCH] i2c: rcar: disable runtime PM correctly in slave mode

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

Alan,

> > When we also are I2C slave, we need to disable runtime PM because the
> > address detection mechanism needs to be active all the time. However, we
> > can reenable runtime PM once the slave instance was unregistered. So,
> > use pm_runtime_disable/enable to achieve this, since it has proper
> > refcounting. pm_runtime_allow/forbid is like a global switch which is
> > unsuitable here.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> > 
> > I'd be grateful to get an ACK from a runtime PM expert to verify that my
> > assumptions match reality :)
> 
> Yes, disabling runtime PM will do what you want.  However you might 
> consider using pm_runtime_get_sync() and pm_runtime_put() instead, 
> because pm_runtime_enable() does not check to see if the device is idle 
> and can be suspended right away.  Alternatively, you can call 
> pm_runtime_idle() after pm_runtime_enable().

Thank you for your answer! I think I'll go for the get/put solution
here.

I have another case, may I ask your advice about this, too? When an I2C
bus is marked in DT as multi-master, then RuntimePM also needs to be
disabled, because arbitration detection needs to stay awake. I am
currently implementing this for the i2c-rcar driver:

-	pm_runtime_enable(dev);
+	/* No RuntimePM in multi-master to keep arbitration working */
+	if (!of_get_property(dev->of_node, "multi-master", NULL)) {
+		pm_runtime_enable(dev);
+		priv->flags |= ID_P_PM;
+	}
+
 	pm_runtime_get_sync(dev);
	...

@@ -664,7 +673,8 @@ static int rcar_i2c_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 
 	i2c_del_adapter(&priv->adap);
-	pm_runtime_disable(dev);
+	if (priv->flags & ID_P_PM)
+		pm_runtime_disable(dev);
 
 	return 0;
 }

Here, I'd tend to keep using enable/disable, although get/put would
probably also work. What is the rule of thumb using this pattern or the
other?

> pm_runtime_allow/forbid is even more unsuitable than you said, because
> it can be overridden by the user.

Yeah, I realized his today, too.

Thanks!

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]