[PATCH] i2c: Use generic subsystem-level power management callbacks

[PATCH] i2c: Use generic subsystem-level power management callbacks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>

There are three issues with the i2c's power management callbacks at
the moment.  First, they don't include any hibernate callbacks,
although they should at least include the .restore() callback
(there's no guarantee that the driver will be in memory before
loading the image kernel and we must restore the pre-hibernation
state of the device).  Second, the "legacy" callbacks are not going
to be invoked by the PM core since the bus type's pm object is not
NULL.  Finally, the system power transition (ie. suspend/resume)
callbacks don't check if the device hasn't been already suspended
at run time, in which case they should skip suspending it and
handle it in a special way during resume.  Also, it looks like the
i2c bus type itself doesn't need any power management handling beyond
that provided by the generic subsystem-level PM callbacks.

For these reasons, remove the power management callbacks defined by
the i2c bus type and make it use the generic subsystem-level power
management callbacks.

Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
---
Jean,

I know I said I wanted to implement the generic callbacks later, but they
were implemented in the meantime anyway, so I think we can use them now.

Please note that the "legacy" callbacks removed by this patch are already
effectively dead, because they are not going to be called by the PM core
anyway.  I'm not sure if this is what we want, though, so please let me know
in case it's not.

Thanks,
Rafael
---
 drivers/i2c/i2c-core.c |  113 -------------------------------------------------
 1 file changed, 1 insertion(+), 112 deletions(-)

Index: linux-2.6/drivers/i2c/i2c-core.c
===================================================================
--- linux-2.6.orig/drivers/i2c/i2c-core.c
+++ linux-2.6/drivers/i2c/i2c-core.c
@@ -156,107 +156,6 @@ static void i2c_device_shutdown(struct d
 		driver->shutdown(client);
 }
 
-#ifdef CONFIG_SUSPEND
-static int i2c_device_pm_suspend(struct device *dev)
-{
-	const struct dev_pm_ops *pm;
-
-	if (!dev->driver)
-		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->suspend)
-		return 0;
-	return pm->suspend(dev);
-}
-
-static int i2c_device_pm_resume(struct device *dev)
-{
-	const struct dev_pm_ops *pm;
-
-	if (!dev->driver)
-		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->resume)
-		return 0;
-	return pm->resume(dev);
-}
-#else
-#define i2c_device_pm_suspend	NULL
-#define i2c_device_pm_resume	NULL
-#endif
-
-#ifdef CONFIG_PM_RUNTIME
-static int i2c_device_runtime_suspend(struct device *dev)
-{
-	const struct dev_pm_ops *pm;
-
-	if (!dev->driver)
-		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->runtime_suspend)
-		return 0;
-	return pm->runtime_suspend(dev);
-}
-
-static int i2c_device_runtime_resume(struct device *dev)
-{
-	const struct dev_pm_ops *pm;
-
-	if (!dev->driver)
-		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->runtime_resume)
-		return 0;
-	return pm->runtime_resume(dev);
-}
-
-static int i2c_device_runtime_idle(struct device *dev)
-{
-	const struct dev_pm_ops *pm = NULL;
-	int ret;
-
-	if (dev->driver)
-		pm = dev->driver->pm;
-	if (pm && pm->runtime_idle) {
-		ret = pm->runtime_idle(dev);
-		if (ret)
-			return ret;
-	}
-
-	return pm_runtime_suspend(dev);
-}
-#else
-#define i2c_device_runtime_suspend	NULL
-#define i2c_device_runtime_resume	NULL
-#define i2c_device_runtime_idle		NULL
-#endif
-
-static int i2c_device_suspend(struct device *dev, pm_message_t mesg)
-{
-	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_driver *driver;
-
-	if (!client || !dev->driver)
-		return 0;
-	driver = to_i2c_driver(dev->driver);
-	if (!driver->suspend)
-		return 0;
-	return driver->suspend(client, mesg);
-}
-
-static int i2c_device_resume(struct device *dev)
-{
-	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_driver *driver;
-
-	if (!client || !dev->driver)
-		return 0;
-	driver = to_i2c_driver(dev->driver);
-	if (!driver->resume)
-		return 0;
-	return driver->resume(client);
-}
-
 static void i2c_client_dev_release(struct device *dev)
 {
 	kfree(to_i2c_client(dev));
@@ -295,23 +194,13 @@ static const struct attribute_group *i2c
 	NULL
 };
 
-static const struct dev_pm_ops i2c_device_pm_ops = {
-	.suspend = i2c_device_pm_suspend,
-	.resume = i2c_device_pm_resume,
-	.runtime_suspend = i2c_device_runtime_suspend,
-	.runtime_resume = i2c_device_runtime_resume,
-	.runtime_idle = i2c_device_runtime_idle,
-};
-
 struct bus_type i2c_bus_type = {
 	.name		= "i2c",
 	.match		= i2c_device_match,
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
 	.shutdown	= i2c_device_shutdown,
-	.suspend	= i2c_device_suspend,
-	.resume		= i2c_device_resume,
-	.pm		= &i2c_device_pm_ops,
+	.pm		= GENERIC_SUBSYS_PM_OPS,
 };
 EXPORT_SYMBOL_GPL(i2c_bus_type);
 

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Alan Stern]

On Sat, 13 Mar 2010, Rafael J. Wysocki wrote:

> Please note that the "legacy" callbacks removed by this patch are already
> effectively dead, because they are not going to be called by the PM core
> anyway.  I'm not sure if this is what we want, though, so please let me know
> in case it's not.

You know, maybe we should allow bus types to use both the old and new
interfaces.  It would make life easier for other subsystems in addition
to i2c.

This doesn't mean that the core would end up calling two sets of
suspend routines.  If the bus type uses legacy routines then all the
non-runtime entries in the pm_ops structure would be empty.

The changes to the PM core necessary to do this are quite small.  Does 
it seem like a reasonable thing to do?

Alan Stern

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Rafael J. Wysocki]

On Saturday 13 March 2010, Alan Stern wrote:
> On Sat, 13 Mar 2010, Rafael J. Wysocki wrote:
> 
> > Please note that the "legacy" callbacks removed by this patch are already
> > effectively dead, because they are not going to be called by the PM core
> > anyway.  I'm not sure if this is what we want, though, so please let me know
> > in case it's not.
> 
> You know, maybe we should allow bus types to use both the old and new
> interfaces.  It would make life easier for other subsystems in addition
> to i2c.
> 
> This doesn't mean that the core would end up calling two sets of
> suspend routines.  If the bus type uses legacy routines then all the
> non-runtime entries in the pm_ops structure would be empty.
> 
> The changes to the PM core necessary to do this are quite small.

Not really.  The detection that the particular callback is not present happens
in pm_op(), while the decision which framework to use is made at the
device_[suspend|resume]() level.

> Does it seem like a reasonable thing to do?

Well, if someone spends time on implementing new callbacks for a bus type,
writing them in such a way that they will call the "legacy" callbacks from
drivers if necessary is not really a big deal IMO.

The problem in this particular case is that I don't know whether or not we
_want_ the drivers' "legacy" callbacks to be invoked at all.  The $subject
patch assumes we don't, but if we do in fact, I'll rework it to do so.

I need that piece of information, though. :-)

Rafael

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Alan Stern]

On Sun, 14 Mar 2010, Rafael J. Wysocki wrote:

> > You know, maybe we should allow bus types to use both the old and new
> > interfaces.  It would make life easier for other subsystems in addition
> > to i2c.
> > 
> > This doesn't mean that the core would end up calling two sets of
> > suspend routines.  If the bus type uses legacy routines then all the
> > non-runtime entries in the pm_ops structure would be empty.
> > 
> > The changes to the PM core necessary to do this are quite small.
> 
> Not really.  The detection that the particular callback is not present happens
> in pm_op(), while the decision which framework to use is made at the
> device_[suspend|resume]() level.

All you have to do is change the "else if" lines in
device_[suspend|resume]() to "if".

> > Does it seem like a reasonable thing to do?
> 
> Well, if someone spends time on implementing new callbacks for a bus type,
> writing them in such a way that they will call the "legacy" callbacks from
> drivers if necessary is not really a big deal IMO.

Sure.  But suppose you _don't_ want to spend the time implementing new
callbacks to replace the existing legacy suspend and resume methods,
whereas you _do_ want to implement runtime PM.  Runtime PM forces the
bus type to have a pm_ops member, which as you point out, will prevent
the legacy methods from being called.

Alan Stern

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Rafael J. Wysocki]

On Sunday 14 March 2010, Alan Stern wrote:
> On Sun, 14 Mar 2010, Rafael J. Wysocki wrote:
> 
> > > You know, maybe we should allow bus types to use both the old and new
> > > interfaces.  It would make life easier for other subsystems in addition
> > > to i2c.
> > > 
> > > This doesn't mean that the core would end up calling two sets of
> > > suspend routines.  If the bus type uses legacy routines then all the
> > > non-runtime entries in the pm_ops structure would be empty.
> > > 
> > > The changes to the PM core necessary to do this are quite small.
> > 
> > Not really.  The detection that the particular callback is not present happens
> > in pm_op(), while the decision which framework to use is made at the
> > device_[suspend|resume]() level.
> 
> All you have to do is change the "else if" lines in
> device_[suspend|resume]() to "if".

Then, if a bus type implements both "new" and "old" callbacks, we'll end up
calling both.  Not nice.

> > > Does it seem like a reasonable thing to do?
> > 
> > Well, if someone spends time on implementing new callbacks for a bus type,
> > writing them in such a way that they will call the "legacy" callbacks from
> > drivers if necessary is not really a big deal IMO.
> 
> Sure.  But suppose you _don't_ want to spend the time implementing new
> callbacks to replace the existing legacy suspend and resume methods,
> whereas you _do_ want to implement runtime PM.  Runtime PM forces the
> bus type to have a pm_ops member,

Which is very much on purpose, because the legacy suspend and resume have no
idea about the runtime stuff.

> which as you point out, will prevent the legacy methods from being called.

Yes, that's intentional.

That said, I think we might modify the generic callbacks in generic_ops.c to
call the drivers' legacy callbacks if the "new" ones are not defined.

Rafael

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Alan Stern]

On Sun, 14 Mar 2010, Rafael J. Wysocki wrote:

> On Sunday 14 March 2010, Alan Stern wrote:
> > On Sun, 14 Mar 2010, Rafael J. Wysocki wrote:
> > 
> > > > You know, maybe we should allow bus types to use both the old and new
> > > > interfaces.  It would make life easier for other subsystems in addition
> > > > to i2c.
> > > > 
> > > > This doesn't mean that the core would end up calling two sets of
> > > > suspend routines.  If the bus type uses legacy routines then all the
> > > > non-runtime entries in the pm_ops structure would be empty.
> > > > 
> > > > The changes to the PM core necessary to do this are quite small.
> > > 
> > > Not really.  The detection that the particular callback is not present happens
> > > in pm_op(), while the decision which framework to use is made at the
> > > device_[suspend|resume]() level.
> > 
> > All you have to do is change the "else if" lines in
> > device_[suspend|resume]() to "if".
> 
> Then, if a bus type implements both "new" and "old" callbacks, we'll end up
> calling both.  Not nice.

Why would a bus type implement both?  Or looked at another way, if it 
did implement both then wouldn't it want both sets of callbacks to be 
invoked?

If a bus type does define both types of callback by mistake and only 
wants one of them to be used, that's a bug -- pure and simple.  It's 
not the PM core's fault if something goes wrong under those 
circumstances.

> > Sure.  But suppose you _don't_ want to spend the time implementing new
> > callbacks to replace the existing legacy suspend and resume methods,
> > whereas you _do_ want to implement runtime PM.  Runtime PM forces the
> > bus type to have a pm_ops member,
> 
> Which is very much on purpose, because the legacy suspend and resume have no
> idea about the runtime stuff.
> 
> > which as you point out, will prevent the legacy methods from being called.
> 
> Yes, that's intentional.
> 
> That said, I think we might modify the generic callbacks in generic_ops.c to
> call the drivers' legacy callbacks if the "new" ones are not defined.

That's no good -- the routines in generic_ops.c invoke the _driver's_
callbacks, not the _bus type's_ callbacks.

I suppose you could write a new set of generic "legacy" callbacks that
would work just as though the new ops weren't defined.  Removing those
"else"s would be a lot simpler, though.

Alan Stern

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Rafael J. Wysocki]

On Monday 15 March 2010, Alan Stern wrote:
> On Sun, 14 Mar 2010, Rafael J. Wysocki wrote:
> 
> > On Sunday 14 March 2010, Alan Stern wrote:
> > > On Sun, 14 Mar 2010, Rafael J. Wysocki wrote:
> > > 
> > > > > You know, maybe we should allow bus types to use both the old and new
> > > > > interfaces.  It would make life easier for other subsystems in addition
> > > > > to i2c.
> > > > > 
> > > > > This doesn't mean that the core would end up calling two sets of
> > > > > suspend routines.  If the bus type uses legacy routines then all the
> > > > > non-runtime entries in the pm_ops structure would be empty.
> > > > > 
> > > > > The changes to the PM core necessary to do this are quite small.
> > > > 
> > > > Not really.  The detection that the particular callback is not present happens
> > > > in pm_op(), while the decision which framework to use is made at the
> > > > device_[suspend|resume]() level.
> > > 
> > > All you have to do is change the "else if" lines in
> > > device_[suspend|resume]() to "if".
> > 
> > Then, if a bus type implements both "new" and "old" callbacks, we'll end up
> > calling both.  Not nice.
> 
> Why would a bus type implement both?  Or looked at another way, if it 
> did implement both then wouldn't it want both sets of callbacks to be 
> invoked?
> 
> If a bus type does define both types of callback by mistake and only 
> wants one of them to be used, that's a bug -- pure and simple.  It's 
> not the PM core's fault if something goes wrong under those 
> circumstances.

In fact, I'd like the "legacy" callbacks to go, at least at the subsystem
level, so I'd prefer not to make it easier to keep them around.

How many bus types are there and how many of them actually implement power
management callbacks?  I don't really think the issue is so big.

Rafael

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Alan Stern]

On Mon, 15 Mar 2010, Rafael J. Wysocki wrote:

> In fact, I'd like the "legacy" callbacks to go, at least at the subsystem
> level, so I'd prefer not to make it easier to keep them around.
> 
> How many bus types are there and how many of them actually implement power
> management callbacks?  I don't really think the issue is so big.

I have no idea.  But I'm in the middle of implementing runtime PM for
SCSI.  SCSI already has bus-level legacy callbacks, so to add the
runtime methods without changing the existing system sleep behavior
requires something like this.

The alternative is to write a bunch of tiny "shim" functions, sort of
like what I ended up doing in the USB stack (see usb_dev_prepare() and
the following routines in drivers/usb/core/usb.c).

Alan Stern

Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks

[Rafael J. Wysocki]

On Monday 15 March 2010, Alan Stern wrote:
> On Mon, 15 Mar 2010, Rafael J. Wysocki wrote:
> 
> > In fact, I'd like the "legacy" callbacks to go, at least at the subsystem
> > level, so I'd prefer not to make it easier to keep them around.
> > 
> > How many bus types are there and how many of them actually implement power
> > management callbacks?  I don't really think the issue is so big.
> 
> I have no idea.  But I'm in the middle of implementing runtime PM for
> SCSI.  SCSI already has bus-level legacy callbacks, so to add the
> runtime methods without changing the existing system sleep behavior
> requires something like this.
> 
> The alternative is to write a bunch of tiny "shim" functions, sort of
> like what I ended up doing in the USB stack (see usb_dev_prepare() and
> the following routines in drivers/usb/core/usb.c).

I prefer this approach to changing the core.  If we changed the core, it would
defeat the whole purpose of introducing the new framework.

Rafael

[PATCH] i2c: Fix bus-level power management callbacks

[Rafael J. Wysocki]

On Saturday 13 March 2010, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> 
> There are three issues with the i2c's power management callbacks at
> the moment.  First, they don't include any hibernate callbacks,
> although they should at least include the .restore() callback
> (there's no guarantee that the driver will be in memory before
> loading the image kernel and we must restore the pre-hibernation
> state of the device).  Second, the "legacy" callbacks are not going
> to be invoked by the PM core since the bus type's pm object is not
> NULL.  Finally, the system power transition (ie. suspend/resume)
> callbacks don't check if the device hasn't been already suspended
> at run time, in which case they should skip suspending it and
> handle it in a special way during resume.  Also, it looks like the
> i2c bus type itself doesn't need any power management handling beyond
> that provided by the generic subsystem-level PM callbacks.
> 
> For these reasons, remove the power management callbacks defined by
> the i2c bus type and make it use the generic subsystem-level power
> management callbacks.

I assume the lack of responses except for the Alan's one means the patch
wasn't correct, so below is one that I think is better.  It fixes all of the
issues described above without breaking backwards compatibility.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
Subject: i2c: Fix bus-level power management callbacks

There are three issues with the i2c bus type's power management
callbacks at the moment.  First, they don't include any hibernate
callbacks, although they should at least include the .restore()
callback (there's no guarantee that the driver will be present in
memory before loading the image kernel and we must restore the
pre-hibernation state of the device).  Second, the "legacy"
callbacks are not going to be invoked by the PM core since the bus
type's pm object is not NULL.  Finally, the system sleep PM
(ie. suspend/resume) callbacks don't check if the device has been
already suspended at run time, in which case they should skip
suspending it.  Also, it looks like the i2c bus type can use the
generic subsystem-level runtime PM callbacks.

For these reasons, rework the system sleep PM callbacks provided by
the i2c bus type to handle hibernation correctly and to invoke the
"legacy" callbacks for drivers that provide them.  In addition to
that make the i2c bus type use the generic subsystem-level runtime
PM callbacks.

Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
---
 drivers/i2c/i2c-core.c     |  168 ++++++++++++++++++++++++++-------------------
 include/linux/pm_runtime.h |    7 +
 2 files changed, 105 insertions(+), 70 deletions(-)

Index: linux-2.6/drivers/i2c/i2c-core.c
===================================================================
--- linux-2.6.orig/drivers/i2c/i2c-core.c
+++ linux-2.6/drivers/i2c/i2c-core.c
@@ -156,107 +156,131 @@ static void i2c_device_shutdown(struct d
 		driver->shutdown(client);
 }
 
-#ifdef CONFIG_SUSPEND
-static int i2c_device_pm_suspend(struct device *dev)
+static int i2c_legacy_suspend(struct device *dev, pm_message_t mesg)
 {
-	const struct dev_pm_ops *pm;
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_driver *driver;
 
-	if (!dev->driver)
+	if (!client || !dev->driver)
 		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->suspend)
+	driver = to_i2c_driver(dev->driver);
+	if (!driver->suspend)
 		return 0;
-	return pm->suspend(dev);
+	return driver->suspend(client, mesg);
 }
 
-static int i2c_device_pm_resume(struct device *dev)
+static int i2c_legacy_resume(struct device *dev)
 {
-	const struct dev_pm_ops *pm;
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_driver *driver;
 
-	if (!dev->driver)
+	if (!client || !dev->driver)
 		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->resume)
+	driver = to_i2c_driver(dev->driver);
+	if (!driver->resume)
 		return 0;
-	return pm->resume(dev);
+	return driver->resume(client);
 }
-#else
-#define i2c_device_pm_suspend	NULL
-#define i2c_device_pm_resume	NULL
-#endif
 
-#ifdef CONFIG_PM_RUNTIME
-static int i2c_device_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int i2c_device_pm_suspend(struct device *dev)
 {
-	const struct dev_pm_ops *pm;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (!dev->driver)
-		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->runtime_suspend)
+	if (pm_runtime_suspended(dev))
 		return 0;
-	return pm->runtime_suspend(dev);
-}
 
-static int i2c_device_runtime_resume(struct device *dev)
-{
-	const struct dev_pm_ops *pm;
+	if (pm)
+		return pm->suspend ? pm->suspend(dev) : 0;
 
-	if (!dev->driver)
-		return 0;
-	pm = dev->driver->pm;
-	if (!pm || !pm->runtime_resume)
-		return 0;
-	return pm->runtime_resume(dev);
+	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
 }
 
-static int i2c_device_runtime_idle(struct device *dev)
+static int i2c_device_pm_resume(struct device *dev)
 {
-	const struct dev_pm_ops *pm = NULL;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int ret;
 
-	if (dev->driver)
-		pm = dev->driver->pm;
-	if (pm && pm->runtime_idle) {
-		ret = pm->runtime_idle(dev);
-		if (ret)
-			return ret;
+	if (pm)
+		ret = pm->resume ? pm->resume(dev) : 0;
+	else
+		ret = i2c_legacy_resume(dev);
+
+	if (!ret) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
 	}
 
-	return pm_runtime_suspend(dev);
-}
-#else
-#define i2c_device_runtime_suspend	NULL
-#define i2c_device_runtime_resume	NULL
-#define i2c_device_runtime_idle		NULL
-#endif
+	return ret;
+}
 
-static int i2c_device_suspend(struct device *dev, pm_message_t mesg)
+static int i2c_device_pm_freeze(struct device *dev)
 {
-	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_driver *driver;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (!client || !dev->driver)
+	if (pm_runtime_suspended(dev))
 		return 0;
-	driver = to_i2c_driver(dev->driver);
-	if (!driver->suspend)
-		return 0;
-	return driver->suspend(client, mesg);
+
+	if (pm)
+		return pm->freeze ? pm->freeze(dev) : 0;
+
+	return i2c_legacy_suspend(dev, PMSG_FREEZE);
 }
 
-static int i2c_device_resume(struct device *dev)
+static int i2c_device_pm_thaw(struct device *dev)
 {
-	struct i2c_client *client = i2c_verify_client(dev);
-	struct i2c_driver *driver;
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
-	if (!client || !dev->driver)
+	if (pm_runtime_suspended(dev))
 		return 0;
-	driver = to_i2c_driver(dev->driver);
-	if (!driver->resume)
+
+	if (pm)
+		return pm->thaw ? pm->thaw(dev) : 0;
+
+	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_runtime_suspended(dev))
 		return 0;
-	return driver->resume(client);
+
+	if (pm)
+		return pm->poweroff ? pm->poweroff(dev) : 0;
+
+	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;
+	else
+		ret = i2c_legacy_resume(dev);
+
+	if (!ret) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return ret;
+}
+#else /* !CONFIG_PM_SLEEP */
+#define i2c_device_pm_suspend	NULL
+#define i2c_device_pm_resume	NULL
+#define i2c_device_pm_freeze	NULL
+#define i2c_device_pm_thaw	NULL
+#define i2c_device_pm_poweroff	NULL
+#define i2c_device_pm_restore	NULL
+#endif /* !CONFIG_PM_SLEEP */
+
 static void i2c_client_dev_release(struct device *dev)
 {
 	kfree(to_i2c_client(dev));
@@ -298,9 +322,15 @@ static const struct attribute_group *i2c
 static const struct dev_pm_ops i2c_device_pm_ops = {
 	.suspend = i2c_device_pm_suspend,
 	.resume = i2c_device_pm_resume,
-	.runtime_suspend = i2c_device_runtime_suspend,
-	.runtime_resume = i2c_device_runtime_resume,
-	.runtime_idle = i2c_device_runtime_idle,
+	.freeze = i2c_device_pm_freeze,
+	.thaw = i2c_device_pm_thaw,
+	.poweroff = i2c_device_pm_poweroff,
+	.restore = i2c_device_pm_restore,
+	SET_RUNTIME_PM_OPS(
+		pm_generic_runtime_suspend,
+		pm_generic_runtime_resume,
+		pm_generic_runtime_idle
+	)
 };
 
 struct bus_type i2c_bus_type = {
@@ -309,8 +339,6 @@ struct bus_type i2c_bus_type = {
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
 	.shutdown	= i2c_device_shutdown,
-	.suspend	= i2c_device_suspend,
-	.resume		= i2c_device_resume,
 	.pm		= &i2c_device_pm_ops,
 };
 EXPORT_SYMBOL_GPL(i2c_bus_type);
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
@@ -30,6 +30,9 @@ extern void pm_runtime_enable(struct dev
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
+extern int pm_generic_runtime_idle(struct device *dev);
+extern int pm_generic_runtime_suspend(struct device *dev);
+extern int pm_generic_runtime_resume(struct device *dev);
 
 static inline bool pm_children_suspended(struct device *dev)
 {
@@ -96,6 +99,10 @@ static inline bool device_run_wake(struc
 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 int pm_generic_runtime_idle(struct device *dev) { return 0; }
+static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
+static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
+
 #endif /* !CONFIG_PM_RUNTIME */
 
 static inline int pm_runtime_get(struct device *dev)

Re: [PATCH] i2c: Fix bus-level power management callbacks

[Jean Delvare]

Hi Rafael,

On Fri, 19 Mar 2010 23:44:08 +0100, Rafael J. Wysocki wrote:
> I assume the lack of responses except for the Alan's one means the patch
> wasn't correct, so below is one that I think is better.  It fixes all of the
> issues described above without breaking backwards compatibility.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> Subject: i2c: Fix bus-level power management callbacks
> 
> There are three issues with the i2c bus type's power management
> callbacks at the moment.  First, they don't include any hibernate
> callbacks, although they should at least include the .restore()
> callback (there's no guarantee that the driver will be present in
> memory before loading the image kernel and we must restore the
> pre-hibernation state of the device).  Second, the "legacy"
> callbacks are not going to be invoked by the PM core since the bus
> type's pm object is not NULL.  Finally, the system sleep PM
> (ie. suspend/resume) callbacks don't check if the device has been
> already suspended at run time, in which case they should skip
> suspending it.  Also, it looks like the i2c bus type can use the
> generic subsystem-level runtime PM callbacks.
> 
> For these reasons, rework the system sleep PM callbacks provided by
> the i2c bus type to handle hibernation correctly and to invoke the
> "legacy" callbacks for drivers that provide them.  In addition to
> that make the i2c bus type use the generic subsystem-level runtime
> PM callbacks.
> 
> Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>

Mark, you contributed the initial runtime PM support for the i2c
subsystem, I thought you would have comments on Rafael's
reimplementation?

> ---
>  drivers/i2c/i2c-core.c     |  168 ++++++++++++++++++++++++++-------------------
>  include/linux/pm_runtime.h |    7 +

I am a little surprised to see changes to a generic header file here,
how is the i2c subsystem so special that we have needs other subsystems
did not?

>  2 files changed, 105 insertions(+), 70 deletions(-)
> 
> Index: linux-2.6/drivers/i2c/i2c-core.c
> ===================================================================
> --- linux-2.6.orig/drivers/i2c/i2c-core.c
> +++ linux-2.6/drivers/i2c/i2c-core.c
> @@ -156,107 +156,131 @@ static void i2c_device_shutdown(struct d
>  		driver->shutdown(client);
>  }
>  
> -#ifdef CONFIG_SUSPEND
> -static int i2c_device_pm_suspend(struct device *dev)
> +static int i2c_legacy_suspend(struct device *dev, pm_message_t mesg)
>  {
> -	const struct dev_pm_ops *pm;
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_driver *driver;
>  
> -	if (!dev->driver)
> +	if (!client || !dev->driver)
>  		return 0;
> -	pm = dev->driver->pm;
> -	if (!pm || !pm->suspend)
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->suspend)
>  		return 0;
> -	return pm->suspend(dev);
> +	return driver->suspend(client, mesg);
>  }
>  
> -static int i2c_device_pm_resume(struct device *dev)
> +static int i2c_legacy_resume(struct device *dev)
>  {
> -	const struct dev_pm_ops *pm;
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_driver *driver;
>  
> -	if (!dev->driver)
> +	if (!client || !dev->driver)
>  		return 0;
> -	pm = dev->driver->pm;
> -	if (!pm || !pm->resume)
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver->resume)
>  		return 0;
> -	return pm->resume(dev);
> +	return driver->resume(client);
>  }
> -#else
> -#define i2c_device_pm_suspend	NULL
> -#define i2c_device_pm_resume	NULL
> -#endif

I fail to see why the functions above are outside of the #ifdef
CONFIG_PM_SLEEP scope. They are only called by functions which are
inside the #ifdef CONFIG_PM_SLEEP scope, so you'll get a build-time
warning if CONFIG_PM_SLEEP isn't set.

Is there a plan to get rid of the above legacy functions at some point
in time?

>  
> -#ifdef CONFIG_PM_RUNTIME
> -static int i2c_device_runtime_suspend(struct device *dev)
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_device_pm_suspend(struct device *dev)
>  {
> -	const struct dev_pm_ops *pm;
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> -	if (!dev->driver)
> -		return 0;
> -	pm = dev->driver->pm;
> -	if (!pm || !pm->runtime_suspend)
> +	if (pm_runtime_suspended(dev))
>  		return 0;
> -	return pm->runtime_suspend(dev);
> -}
> (...)

Apart from the above, the code looks sane to me, but then again I don't
know a thing about power management. I'll keep this patch in my i2c
tree, scheduled for merge in 2.6.35. If there are any updates, please
send them over, either as a new patch or as incremental changes which I
will merge myself.

-- 
Jean Delvare

Re: [PATCH] i2c: Fix bus-level power management callbacks

[Mark Brown]

On Thu, Mar 25, 2010 at 09:57:20AM +0100, Jean Delvare wrote:

> Mark, you contributed the initial runtime PM support for the i2c
> subsystem, I thought you would have comments on Rafael's
> reimplementation?

Yeah, see below...

> >  drivers/i2c/i2c-core.c     |  168 ++++++++++++++++++++++++++-------------------
> >  include/linux/pm_runtime.h |    7 +

> I am a little surprised to see changes to a generic header file here,
> how is the i2c subsystem so special that we have needs other subsystems
> did not?

Very few subsystems actually support runtime PM thus far - I think it's
more the case that I2C is an early user than anything else.

> Apart from the above, the code looks sane to me, but then again I don't
> know a thing about power management. I'll keep this patch in my i2c
> tree, scheduled for merge in 2.6.35. If there are any updates, please
> send them over, either as a new patch or as incremental changes which I
> will merge myself.

I'm in a similar position - the code looks fine except I'm not 100% sure
I follow all the possible ifdefs and I've never actually used hibernate
(s2disk isn't supported on ARM) but for what it's worth:

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

Re: [PATCH] i2c: Fix bus-level power management callbacks

[Rafael J. Wysocki]

On Thursday 25 March 2010, Mark Brown wrote:
> On Thu, Mar 25, 2010 at 09:57:20AM +0100, Jean Delvare wrote:
> 
> > Mark, you contributed the initial runtime PM support for the i2c
> > subsystem, I thought you would have comments on Rafael's
> > reimplementation?
> 
> Yeah, see below...
> 
> > >  drivers/i2c/i2c-core.c     |  168 ++++++++++++++++++++++++++-------------------
> > >  include/linux/pm_runtime.h |    7 +
> 
> > I am a little surprised to see changes to a generic header file here,
> > how is the i2c subsystem so special that we have needs other subsystems
> > did not?
> 
> Very few subsystems actually support runtime PM thus far - I think it's
> more the case that I2C is an early user than anything else.

That's the case indeed.

> > Apart from the above, the code looks sane to me, but then again I don't
> > know a thing about power management. I'll keep this patch in my i2c
> > tree, scheduled for merge in 2.6.35. If there are any updates, please
> > send them over, either as a new patch or as incremental changes which I
> > will merge myself.
> 
> I'm in a similar position - the code looks fine except I'm not 100% sure
> I follow all the possible ifdefs and I've never actually used hibernate
> (s2disk isn't supported on ARM) but for what it's worth:
> 
> Acked-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

Thanks!

Rafael