[PATCH] i2c: Factor out runtime suspend checks from PM operations

[PATCH] i2c: Factor out runtime suspend checks from PM operations

[Mark Brown]

When devices use dev_pm_ops the I2C API is implementing standard functionality
for integration with runtime PM and for checking for the presence of a per
device op. The PM core provides pm_generic_ functions implementing this
behaviour - use them to reduce coupling with future PM updates.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---

Depends on my patch to add prototypes for the pm_generic_ operations.

 drivers/i2c/i2c-core.c |   70 ++++++++++++++++++------------------------------
 1 files changed, 26 insertions(+), 44 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6b4cc56..fd3674a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -196,69 +196,50 @@ static int i2c_device_pm_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->suspend ? pm->suspend(dev) : 0;
-	}
-
-	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
+	if (pm)
+		return pm_generic_suspend(dev);
+	else
+		return i2c_legacy_suspend(dev, PMSG_SUSPEND);
 }
 
 static int i2c_device_pm_resume(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int ret;
 
 	if (pm)
-		ret = pm->resume ? pm->resume(dev) : 0;
+		return pm_generic_resume(dev);
 	else
-		ret = i2c_legacy_resume(dev);
-
-	return ret;
+		return i2c_legacy_resume(dev);
 }
 
 static int i2c_device_pm_freeze(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->freeze ? pm->freeze(dev) : 0;
-	}
-
-	return i2c_legacy_suspend(dev, PMSG_FREEZE);
+	if (pm)
+		return pm_generic_freeze(dev);
+	else
+		return i2c_legacy_suspend(dev, PMSG_FREEZE);
 }
 
 static int i2c_device_pm_thaw(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->thaw ? pm->thaw(dev) : 0;
-	}
-
-	return i2c_legacy_resume(dev);
+	if (pm)
+		return pm_generic_thaw(dev);
+	else
+		return i2c_legacy_resume(dev);
 }
 
 static int i2c_device_pm_poweroff(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->poweroff ? pm->poweroff(dev) : 0;
-	}
-
-	return i2c_legacy_suspend(dev, PMSG_HIBERNATE);
+	if (pm)
+		return pm_generic_poweroff(dev);
+	else
+		return i2c_legacy_suspend(dev, PMSG_HIBERNATE);
 }
 
 static int i2c_device_pm_restore(struct device *dev)
@@ -266,15 +247,16 @@ static int i2c_device_pm_restore(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int ret;
 
-	if (pm)
-		ret = pm->restore ? pm->restore(dev) : 0;
-	else
+	if (pm) {
+		ret = pm_generic_restore(dev);
+	} else {
 		ret = i2c_legacy_resume(dev);
 
-	if (!ret) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
+		if (!ret) {
+			pm_runtime_disable(dev);
+			pm_runtime_set_active(dev);
+			pm_runtime_enable(dev);
+		}
 	}
 
 	return ret;
-- 
1.7.2.3

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Rabin Vincent]

On Wed, Dec 22, 2010 at 11:37 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>  static int i2c_device_pm_restore(struct device *dev)
> @@ -266,15 +247,16 @@ static int i2c_device_pm_restore(struct device *dev)
>        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>        int ret;
>
> -       if (pm)
> -               ret = pm->restore ? pm->restore(dev) : 0;
> -       else
> +       if (pm) {
> +               ret = pm_generic_restore(dev);
> +       } else {
>                ret = i2c_legacy_resume(dev);
>
> -       if (!ret) {
> -               pm_runtime_disable(dev);
> -               pm_runtime_set_active(dev);
> -               pm_runtime_enable(dev);
> +               if (!ret) {
> +                       pm_runtime_disable(dev);
> +                       pm_runtime_set_active(dev);
> +                       pm_runtime_enable(dev);
> +               }
>        }

I asked Rafael about the legacy operations earlier on the other thread:

On Fri, Dec 17, 2010 at 5:39 AM, Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org> wrote:
> On Thursday, December 16, 2010, Rabin Vincent wrote:
>> If so, before I send patches to fix them: can it be assumed that only
>> drivers using dev_pm_ops (and not the legacy ops of these busses) will
>> need the interactions between runtime PM and system sleep as done in the
>> generic ops?
>
> Yes, you can make this assumption safely.  The drivers that don't use
> dev_pm_ops can't support runtime PM at all.

And even if we did want to support runtime PM interaction for legacy
ops, the code for restore() above suffers from the problem of setting
active even when no callback exists, like I mentioned in the same email
for resume():

On Thu, Dec 16, 2010 at 11:56 PM, Rabin Vincent <rabin-66gdRtMMWGc@public.gmane.org> wrote:
>  (2) Both I2C and platform do not set the device as active when a
>      pm->resume callback exists and it succeeds.
>
>      This seems to have been done in i2c until recently, but has been
>      removed by 753419f59e ("i2c: Fix for suspend/resume issue").  It
>      seems to me that this removal is incorrect, and instead the real
>      problem with the implementation was that it set the device as
>      active even if no resume callback existed, whereas it should only
>      do so when it exists and returns zero, like the generic ops.

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Mark Brown]

On Thu, Dec 23, 2010 at 12:49:39AM +0530, Rabin Vincent wrote:

> And even if we did want to support runtime PM interaction for legacy
> ops, the code for restore() above suffers from the problem of setting
> active even when no callback exists, like I mentioned in the same email
> for resume():

I agree it's confused, I posted an updated patch which should behave
exactly as the old code did - I think the confusion with the legacy
behaviour should be addressed seperately (and ideally in 2.6.37 or at
least a stable patch rather than 2.6.38 which is where the current patch
is targetted).

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Rafael J. Wysocki]

On Wednesday, December 22, 2010, Mark Brown wrote:
> On Thu, Dec 23, 2010 at 12:49:39AM +0530, Rabin Vincent wrote:
> 
> > And even if we did want to support runtime PM interaction for legacy
> > ops, the code for restore() above suffers from the problem of setting
> > active even when no callback exists, like I mentioned in the same email
> > for resume():
> 
> I agree it's confused, I posted an updated patch which should behave
> exactly as the old code did - I think the confusion with the legacy
> behaviour should be addressed seperately (and ideally in 2.6.37 or at
> least a stable patch rather than 2.6.38 which is where the current patch
> is targetted).

Rabin is right, runtime PM cannot be supported in the "legacy" case.
Still, I see that __pm_generic_resume() has the problem that it sets
RPM_ACTIVE unconditionally, which it shouldn't do if the runtime PM has been
already disabled.

So, I think we need this patch:

---
 drivers/base/power/generic_ops.c |    2 +-
 include/linux/pm_runtime.h       |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/base/power/generic_ops.c
===================================================================
--- linux-2.6.orig/drivers/base/power/generic_ops.c
+++ linux-2.6/drivers/base/power/generic_ops.c
@@ -185,7 +185,7 @@ static int __pm_generic_resume(struct de
 		return 0;
 
 	ret = callback(dev);
-	if (!ret) {
+	if (!ret && pm_runtime_enabled(dev)) {
 		pm_runtime_disable(dev);
 		pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -82,6 +82,11 @@ static inline bool pm_runtime_suspended(
 		&& !dev->power.disable_depth;
 }
 
+static inline bool pm_runtime_enabled(struct device *dev)
+{
+	return !dev->power.disable_depth;
+}
+
 static inline void pm_runtime_mark_last_busy(struct device *dev)
 {
 	ACCESS_ONCE(dev->power.last_busy) = jiffies;
@@ -120,6 +125,7 @@ static inline void pm_runtime_put_noidle
 static inline bool device_run_wake(struct device *dev) { return false; }
 static inline void device_set_run_wake(struct device *dev, bool enable) {}
 static inline bool pm_runtime_suspended(struct device *dev) { return false; }
+static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
 static inline int pm_generic_runtime_idle(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }

[PATCH] i2c: Factor out runtime suspend checks from PM operations

[Mark Brown]

When devices use dev_pm_ops the I2C API is implementing standard functionality
for integration with runtime PM and for checking for the presence of a per
device op. The PM core provides pm_generic_ functions implementing this
behaviour - use them to reduce coupling with future PM updates.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   68 ++++++++++++++---------------------------------
 1 files changed, 20 insertions(+), 48 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6b4cc56..6793c51 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -196,88 +196,60 @@ static int i2c_device_pm_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->suspend ? pm->suspend(dev) : 0;
-	}
-
-	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
+	if (pm)
+		return pm_generic_suspend(dev);
+	else
+		return i2c_legacy_suspend(dev, PMSG_SUSPEND);
 }
 
 static int i2c_device_pm_resume(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int ret;
 
 	if (pm)
-		ret = pm->resume ? pm->resume(dev) : 0;
+		return pm_generic_resume(dev);
 	else
-		ret = i2c_legacy_resume(dev);
-
-	return ret;
+		return i2c_legacy_resume(dev);
 }
 
 static int i2c_device_pm_freeze(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->freeze ? pm->freeze(dev) : 0;
-	}
-
-	return i2c_legacy_suspend(dev, PMSG_FREEZE);
+	if (pm)
+		return pm_generic_freeze(dev);
+	else
+		return i2c_legacy_suspend(dev, PMSG_FREEZE);
 }
 
 static int i2c_device_pm_thaw(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->thaw ? pm->thaw(dev) : 0;
-	}
-
-	return i2c_legacy_resume(dev);
+	if (pm)
+		return pm_generic_thaw(dev);
+	else
+		return i2c_legacy_resume(dev);
 }
 
 static int i2c_device_pm_poweroff(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (pm) {
-		if (pm_runtime_suspended(dev))
-			return 0;
-		else
-			return pm->poweroff ? pm->poweroff(dev) : 0;
-	}
-
-	return i2c_legacy_suspend(dev, PMSG_HIBERNATE);
+	if (pm)
+		return pm_generic_poweroff(dev);
+	else
+		return i2c_legacy_suspend(dev, PMSG_HIBERNATE);
 }
 
 static int i2c_device_pm_restore(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int ret;
 
 	if (pm)
-		ret = pm->restore ? pm->restore(dev) : 0;
+		return pm_generic_restore(dev);
 	else
-		ret = i2c_legacy_resume(dev);
-
-	if (!ret) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
-
-	return ret;
+		return i2c_legacy_resume(dev);
 }
 #else /* !CONFIG_PM_SLEEP */
 #define i2c_device_pm_suspend	NULL
-- 
1.7.2.3

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Abhijeet Dharmapurikar]

Mark Brown wrote:
> When devices use dev_pm_ops the I2C API is implementing standard functionality
> for integration with runtime PM and for checking for the presence of a per
> device op. The PM core provides pm_generic_ functions implementing this
> behaviour - use them to reduce coupling with future PM updates.
> 
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   68 ++++++++++++++---------------------------------
>  1 files changed, 20 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 6b4cc56..6793c51 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -196,88 +196,60 @@ static int i2c_device_pm_suspend(struct device *dev)
>  {
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> -	if (pm) {
> -		if (pm_runtime_suspended(dev))
> -			return 0;
> -		else
> -			return pm->suspend ? pm->suspend(dev) : 0;
> -	}
> -
> -	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> +	if (pm)
> +		return pm_generic_suspend(dev);


pm_generic_suspend() and others needs to be externed in linux/pm_runtime.h?

--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm 
Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Mark Brown]

On Tue, Jan 04, 2011 at 03:07:37PM -0800, Abhijeet Dharmapurikar wrote:
> Mark Brown wrote:

>> device op. The PM core provides pm_generic_ functions implementing this
>> behaviour - use them to reduce coupling with future PM updates.

> pm_generic_suspend() and others needs to be externed in linux/pm_runtime.h?

Yes, there's a patch in -next for this already so this patch would need
to either wait for later in the merge window or go in via the PM tree
with the preceeding one (the original version of this patch was posted
as part of a series with the export patch).

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Rafael J. Wysocki]

On Wednesday, January 05, 2011, Mark Brown wrote:
> On Tue, Jan 04, 2011 at 03:07:37PM -0800, Abhijeet Dharmapurikar wrote:
> > Mark Brown wrote:
> 
> >> device op. The PM core provides pm_generic_ functions implementing this
> >> behaviour - use them to reduce coupling with future PM updates.
> 
> > pm_generic_suspend() and others needs to be externed in linux/pm_runtime.h?
> 
> Yes, there's a patch in -next for this already so this patch would need
> to either wait for later in the merge window or go in via the PM tree
> with the preceeding one (the original version of this patch was posted
> as part of a series with the export patch).

I would take it, but I'm still waiting for a word from Jean.

Thanks,
Rafael

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Jean Delvare]

Hi Rafael,

On Wed, 5 Jan 2011 09:28:53 +0100, Rafael J. Wysocki wrote:
> On Wednesday, January 05, 2011, Mark Brown wrote:
> > On Tue, Jan 04, 2011 at 03:07:37PM -0800, Abhijeet Dharmapurikar wrote:
> > > Mark Brown wrote:
> > 
> > >> device op. The PM core provides pm_generic_ functions implementing this
> > >> behaviour - use them to reduce coupling with future PM updates.
> > 
> > > pm_generic_suspend() and others needs to be externed in linux/pm_runtime.h?
> > 
> > Yes, there's a patch in -next for this already so this patch would need
> > to either wait for later in the merge window or go in via the PM tree
> > with the preceeding one (the original version of this patch was posted
> > as part of a series with the export patch).
> 
> I would take it, but I'm still waiting for a word from Jean.

Sorry for the long silence, I was on vacation. I've just applied this
patch, and I will send it to Linus tomorrow.

-- 
Jean Delvare

Re: [PATCH] i2c: Factor out runtime suspend checks from PM operations

[Rafael J. Wysocki]

On Thursday, January 13, 2011, Jean Delvare wrote:
> Hi Rafael,
> 
> On Wed, 5 Jan 2011 09:28:53 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, January 05, 2011, Mark Brown wrote:
> > > On Tue, Jan 04, 2011 at 03:07:37PM -0800, Abhijeet Dharmapurikar wrote:
> > > > Mark Brown wrote:
> > > 
> > > >> device op. The PM core provides pm_generic_ functions implementing this
> > > >> behaviour - use them to reduce coupling with future PM updates.
> > > 
> > > > pm_generic_suspend() and others needs to be externed in linux/pm_runtime.h?
> > > 
> > > Yes, there's a patch in -next for this already so this patch would need
> > > to either wait for later in the merge window or go in via the PM tree
> > > with the preceeding one (the original version of this patch was posted
> > > as part of a series with the export patch).
> > 
> > I would take it, but I'm still waiting for a word from Jean.
> 
> Sorry for the long silence, I was on vacation. I've just applied this
> patch, and I will send it to Linus tomorrow.

Great, thanks!

Rafael