[PATCH V2] au1xmmc: dev_pm_ops conversion

[PATCH V2] au1xmmc: dev_pm_ops conversion

[Manuel Lauss]

Cc: Frans Pop <elendil@planet.nl>
Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
V1->V2: don't remove CONFIG_PM

 drivers/mmc/host/au1xmmc.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
index d3f5561..2d4e20f 100644
--- a/drivers/mmc/host/au1xmmc.c
+++ b/drivers/mmc/host/au1xmmc.c
@@ -1132,12 +1132,12 @@ static int __devexit au1xmmc_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int au1xmmc_suspend(struct platform_device *pdev, pm_message_t state)
+static int au1xmmc_suspend(struct device *dev)
 {
-	struct au1xmmc_host *host = platform_get_drvdata(pdev);
+	struct au1xmmc_host *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = mmc_suspend_host(host->mmc, state);
+	ret = mmc_suspend_host(host->mmc, PMSG_SUSPEND);
 	if (ret)
 		return ret;
 
@@ -1150,27 +1150,33 @@ static int au1xmmc_suspend(struct platform_device *pdev, pm_message_t state)
 	return 0;
 }
 
-static int au1xmmc_resume(struct platform_device *pdev)
+static int au1xmmc_resume(struct device *dev)
 {
-	struct au1xmmc_host *host = platform_get_drvdata(pdev);
+	struct au1xmmc_host *host = dev_get_drvdata(dev);
 
 	au1xmmc_reset_controller(host);
 
 	return mmc_resume_host(host->mmc);
 }
+
+static struct dev_pm_ops au1xmmc_pmops = {
+	.resume		= au1xmmc_resume,
+	.suspend	= au1xmmc_suspend,
+};
+
+#define AU1XMMC_PMOPS &au1xmmc_pmops
+
 #else
-#define au1xmmc_suspend NULL
-#define au1xmmc_resume NULL
+#define AU1XMMC_PMOPS NULL
 #endif
 
 static struct platform_driver au1xmmc_driver = {
 	.probe         = au1xmmc_probe,
 	.remove        = au1xmmc_remove,
-	.suspend       = au1xmmc_suspend,
-	.resume        = au1xmmc_resume,
 	.driver        = {
 		.name  = DRIVER_NAME,
 		.owner = THIS_MODULE,
+		.pm    = AU1XMMC_PMOPS,
 	},
 };
 
-- 
1.6.3.3

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Dmitry Torokhov]

Hi Manuel,

On Wed, Jul 22, 2009 at 05:18:39PM +0200, Manuel Lauss wrote:
> Cc: Frans Pop <elendil@planet.nl>
> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
>
> +
> +static struct dev_pm_ops au1xmmc_pmops = {
> +	.resume		= au1xmmc_resume,
> +	.suspend	= au1xmmc_suspend,
> +};
> +

Was suspend to disk tested? It requires freeze()/thaw().

-- 
Dmitry

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Manuel Lauss]

On Sat, Jul 25, 2009 at 7:44 PM, Dmitry
Torokhov<dmitry.torokhov@gmail.com> wrote:
> Hi Manuel,
>
> On Wed, Jul 22, 2009 at 05:18:39PM +0200, Manuel Lauss wrote:
>> Cc: Frans Pop <elendil@planet.nl>
>> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
>>
>> +
>> +static struct dev_pm_ops au1xmmc_pmops = {
>> +     .resume         = au1xmmc_resume,
>> +     .suspend        = au1xmmc_suspend,
>> +};
>> +
>
> Was suspend to disk tested? It requires freeze()/thaw().

No, only suspend-to-ram, but that's good to know.
Currently for me only STR is important (and testable),
I'll have to check wheter STD works on MIPS with my test
hardware.

Thank you!
     Manuel Lauss

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Frans Pop]

On Saturday 25 July 2009, Dmitry Torokhov wrote:
> On Wed, Jul 22, 2009 at 05:18:39PM +0200, Manuel Lauss wrote:
> > Cc: Frans Pop <elendil@planet.nl>
> > Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> >
> > +
> > +static struct dev_pm_ops au1xmmc_pmops = {
> > +	.resume		= au1xmmc_resume,
> > +	.suspend	= au1xmmc_suspend,
> > +};
> > +
>
> Was suspend to disk tested? It requires freeze()/thaw().

Is that a regression introduced by this patch then? If so, many more of 
the recent dev_pm_ops conversion patches would need to be revisited.

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Dmitry Torokhov]

On Sat, Jul 25, 2009 at 08:18:58PM +0200, Frans Pop wrote:
> On Saturday 25 July 2009, Dmitry Torokhov wrote:
> > On Wed, Jul 22, 2009 at 05:18:39PM +0200, Manuel Lauss wrote:
> > > Cc: Frans Pop <elendil@planet.nl>
> > > Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> > >
> > > +
> > > +static struct dev_pm_ops au1xmmc_pmops = {
> > > +	.resume		= au1xmmc_resume,
> > > +	.suspend	= au1xmmc_suspend,
> > > +};
> > > +
> >
> > Was suspend to disk tested? It requires freeze()/thaw().
> 
> Is that a regression introduced by this patch then? If so, many more of 
> the recent dev_pm_ops conversion patches would need to be revisited.

Yes, as far as I understand they would. Let's ask Rafael to confirm...

-- 
Dmitry

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Rafael J. Wysocki]

On Saturday 25 July 2009, Dmitry Torokhov wrote:
> On Sat, Jul 25, 2009 at 08:18:58PM +0200, Frans Pop wrote:
> > On Saturday 25 July 2009, Dmitry Torokhov wrote:
> > > On Wed, Jul 22, 2009 at 05:18:39PM +0200, Manuel Lauss wrote:
> > > > Cc: Frans Pop <elendil@planet.nl>
> > > > Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> > > >
> > > > +
> > > > +static struct dev_pm_ops au1xmmc_pmops = {
> > > > +	.resume		= au1xmmc_resume,
> > > > +	.suspend	= au1xmmc_suspend,
> > > > +};
> > > > +
> > >
> > > Was suspend to disk tested? It requires freeze()/thaw().
> > 
> > Is that a regression introduced by this patch then? If so, many more of 
> > the recent dev_pm_ops conversion patches would need to be revisited.
> 
> Yes, as far as I understand they would. Let's ask Rafael to confirm...

Yes, they would.  In general, you'd probably want to do something like this:

static struct dev_pm_ops au1xmmc_pmops = {
	.resume		= au1xmmc_resume,
	.suspend		= au1xmmc_suspend,
	.freeze		= au1xmmc_resume,
	.thaw		= au1xmmc_suspend,
	.restore		= au1xmmc_resume,
	.poweroff	= au1xmmc_suspend,
};

but in this particular case it's probably better to define separate callbacks
for .freeze() and .thaw() at least.

During hibernation we call .freeze() and .thaw() before and after creating
the image, respectively, and then .poweroff() is called right after the image
has been saved.  During resume .freeze() is called after the image has been
loaded and before the control goes to the image kernel, which then calls
.restore().

HTH

I see I forgot about that myself.  I'll fix up the floppy and hp-wmi patches.

Best,
Rafael

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Frans Pop]

On Saturday 25 July 2009, Rafael J. Wysocki wrote:
> > > > Was suspend to disk tested? It requires freeze()/thaw().
> > >
> > > Is that a regression introduced by this patch then? If so, many
> > > more of the recent dev_pm_ops conversion patches would need to be
> > > revisited.
>
> Yes, they would.  In general, you'd probably want to do something like
> this:
>
> static struct dev_pm_ops au1xmmc_pmops = {
>         .resume         = au1xmmc_resume,
>         .suspend                = au1xmmc_suspend,
>         .freeze         = au1xmmc_resume,
>         .thaw           = au1xmmc_suspend,
>         .restore                = au1xmmc_resume,
>         .poweroff       = au1xmmc_suspend,
> };
>
> but in this particular case it's probably better to define separate
> callbacks for .freeze() and .thaw() at least.
>
> During hibernation we call .freeze() and .thaw() before and after
> creating the image, respectively, and then .poweroff() is called right
> after the image has been saved.  During resume .freeze() is called
> after the image has been loaded and before the control goes to the
> image kernel, which then calls .restore().

Yes, I see that in drivers/base/platform.c (legacy) .suspend resp. .resume 
also got called for those cases?
Ouch :-(

I've added others who've submitted dev_pm_ops patches in CC.

> I'll fix up the floppy and hp-wmi patches.

Note that those are already in mainline, as is pcspkr.

Thanks,
FJP

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Frans Pop]

On Saturday 25 July 2009, you wrote:
> > I'll fix up the floppy and hp-wmi patches.
>
> Note that those are already in mainline, as is pcspkr.

Here's an overview of commits (that I could find) already in mainline that 
look to have gotten it wrong:

6daa79b3  drivers/serial/sh-sci.c	(Paul Mundt)
2dbc8a23  drivers/video/hitfb.c		(Paul Mundt)
35db715b  drivers/input/misc/pcspkr.c	(/me)
2702403c  drivers/platform/x86/hp-wmi.c	(/me)
142a735b  drivers/block/floppy.c	(/me)

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Rafael J. Wysocki]

On Saturday 25 July 2009, Frans Pop wrote:
> On Saturday 25 July 2009, you wrote:
> > > I'll fix up the floppy and hp-wmi patches.
> >
> > Note that those are already in mainline, as is pcspkr.
> 
> Here's an overview of commits (that I could find) already in mainline that 
> look to have gotten it wrong:
> 
> 6daa79b3  drivers/serial/sh-sci.c	(Paul Mundt)
> 2dbc8a23  drivers/video/hitfb.c		(Paul Mundt)
> 35db715b  drivers/input/misc/pcspkr.c	(/me)
> 2702403c  drivers/platform/x86/hp-wmi.c	(/me)
> 142a735b  drivers/block/floppy.c	(/me)

Are you sure they're all in mainline?

The last change in mainline floppy.c happened on Jun 30 and I have the last
two (fixed up already) in my linux-next branch.

Best,
Rafael

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Frans Pop]

On Saturday 25 July 2009, Rafael J. Wysocki wrote:
> On Saturday 25 July 2009, Frans Pop wrote:
> > On Saturday 25 July 2009, you wrote:
> > > > I'll fix up the floppy and hp-wmi patches.
> > >
> > > Note that those are already in mainline, as is pcspkr.
> >
> > Here's an overview of commits (that I could find) already in mainline
> > that look to have gotten it wrong:
> >
> > 6daa79b3  drivers/serial/sh-sci.c	(Paul Mundt)
> > 2dbc8a23  drivers/video/hitfb.c		(Paul Mundt)
> > 35db715b  drivers/input/misc/pcspkr.c	(/me)
> > 2702403c  drivers/platform/x86/hp-wmi.c	(/me)
> > 142a735b  drivers/block/floppy.c	(/me)
>
> Are you sure they're all in mainline?

Ugh, you're right. Only the first three are.

For the last two I was confused by the mails from akm that he'd dropped 
them from his queue. I'd not realized that was because you took them.

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Dmitry Torokhov]

On Jul 25, 2009, at 1:21 PM, Frans Pop <elendil@planet.nl> wrote:

> On Saturday 25 July 2009, Rafael J. Wysocki wrote:
>>>>> Was suspend to disk tested? It requires freeze()/thaw().
>>>>
>>>> Is that a regression introduced by this patch then? If so, many
>>>> more of the recent dev_pm_ops conversion patches would need to be
>>>> revisited.
>>
>> Yes, they would.  In general, you'd probably want to do something  
>> like
>> this:
>>
>> static struct dev_pm_ops au1xmmc_pmops = {
>>         .resume         = au1xmmc_resume,
>>         .suspend                = au1xmmc_suspend,
>>         .freeze         = au1xmmc_resume,
>>         .thaw           = au1xmmc_suspend,
>>         .restore                = au1xmmc_resume,
>>         .poweroff       = au1xmmc_suspend,
>> };
>>
>> but in this particular case it's probably better to define separate
>> callbacks for .freeze() and .thaw() at least.
>>
>> During hibernation we call .freeze() and .thaw() before and after
>> creating the image, respectively, and then .poweroff() is called  
>> right
>> after the image has been saved.  During resume .freeze() is called
>> after the image has been loaded and before the control goes to the
>> image kernel, which then calls .restore().
>
> Yes, I see that in drivers/base/platform.c (legacy) .suspend  
> resp. .resume
> also got called for those cases?
> Ouch :-(
>
> I've added others who've submitted dev_pm_ops patches in CC.
>
>> I'll fix up the floppy and hp-wmi patches.
>
> Note that those are already in mainline, as is pcspkr.
>


Pcspkr should be fine as is since we just want to shut it off and with  
s2d it will happen automatically when we power down.

-- 
Dmitry

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Mark Brown]

On Sat, Jul 25, 2009 at 09:39:30PM +0200, Rafael J. Wysocki wrote:

> Yes, they would.  In general, you'd probably want to do something like this:

> static struct dev_pm_ops au1xmmc_pmops = {
> 	.resume		= au1xmmc_resume,
> 	.suspend		= au1xmmc_suspend,
> 	.freeze		= au1xmmc_resume,
> 	.thaw		= au1xmmc_suspend,

I'd have expected freeze and thaw to be the other way around here?

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Rafael J. Wysocki]

On Sunday 26 July 2009, Mark Brown wrote:
> On Sat, Jul 25, 2009 at 09:39:30PM +0200, Rafael J. Wysocki wrote:
> 
> > Yes, they would.  In general, you'd probably want to do something like this:
> 
> > static struct dev_pm_ops au1xmmc_pmops = {
> > 	.resume		= au1xmmc_resume,
> > 	.suspend		= au1xmmc_suspend,
> > 	.freeze		= au1xmmc_resume,
> > 	.thaw		= au1xmmc_suspend,
> 
> I'd have expected freeze and thaw to be the other way around here?

Sure, sorry.

.suspend() corresponds to .freeze() and .poweroff(), while .resume()
corresponds to .thaw() and .restore().

Best,
Rafael

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Albin Tonnerre]

On Sat, Jul 25, 2009, Frans Pop wrote:
> Yes, I see that in drivers/base/platform.c (legacy) .suspend resp. .resume
> also got called for those cases?
> Ouch :-(

This really looks like it's error-prone (I made this mistake for atmel_serial
and wouldn't have noticed if Frans hadn't told me), and AFAICS, for a number of
drivers we'll have suspend = freeze = poweroff and resume = thaw = restore.
Maybe putting something like this in pm.h would help ?

#define PM_OPS(name, suspend, resume) \
struct dev_pm_ops name = { \
	.suspend = suspend, \
	.resume = resume, \
	.freeze = suspend, \
	.thaw = resume, \
	.poweroff = suspend, \
	.restore = resume, \
};

Cheers,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Rafael J. Wysocki]

On Monday 03 August 2009, Albin Tonnerre wrote:
> On Sat, Jul 25, 2009, Frans Pop wrote:
> > Yes, I see that in drivers/base/platform.c (legacy) .suspend resp. .resume
> > also got called for those cases?
> > Ouch :-(
> 
> This really looks like it's error-prone (I made this mistake for atmel_serial
> and wouldn't have noticed if Frans hadn't told me), and AFAICS, for a number of
> drivers we'll have suspend = freeze = poweroff and resume = thaw = restore.
> Maybe putting something like this in pm.h would help ?
> 
> #define PM_OPS(name, suspend, resume) \
> struct dev_pm_ops name = { \
> 	.suspend = suspend, \
> 	.resume = resume, \
> 	.freeze = suspend, \
> 	.thaw = resume, \
> 	.poweroff = suspend, \
> 	.restore = resume, \
> };

That looks like a good idea, but please call that macro SIMPLE_PM_OPS (or
something similar) and submit a patch.

Best,
Rafael

[PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

[Albin Tonnerre]

In a number of cases, the .suspend, .freeze, .poweroff and .resume,
.thaw, .restore functions are identical. However, they all need to be
assigned to avoid regressionsm as the previous code called .suspend
resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with
this case.

Signed-off-by: Albin Tonnerre <albin.tonnerre@free-electrons.com>
---
 include/linux/pm.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index b3f7476..994a62f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -184,6 +184,16 @@ struct dev_pm_ops {
 	int (*restore_noirq)(struct device *dev);
 };
 
+#define SIMPLE_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+	.suspend = suspend_fn, \
+	.resume = resume_fn, \
+	.freeze = suspend_fn, \
+	.thaw = resume_fn, \
+	.poweroff = suspend_fn, \
+	.resume = resume_fn, \
+}
+
 /**
  * PM_EVENT_ messages
  *
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

[Dmitry Torokhov]

On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> In a number of cases, the .suspend, .freeze, .poweroff and .resume,
> .thaw, .restore functions are identical. However, they all need to be
> assigned to avoid regressionsm as the previous code called .suspend
> resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with
> this case.
> 

I'd much rather have conversions done with a bit more analysis now that
our framework is more flexible and we can have specialized routines for
hibernation and suspend.

Maybe we should try changing from run-time to build time warning so that
users are not overly concerned with it?

Thanks.

-- 
Dmitry

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

[Albin Tonnerre]

On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
> On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> > In a number of cases, the .suspend, .freeze, .poweroff and .resume,
> > .thaw, .restore functions are identical. However, they all need to be
> > assigned to avoid regressionsm as the previous code called .suspend
> > resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with
> > this case.


> I'd much rather have conversions done with a bit more analysis now that
> our framework is more flexible and we can have specialized routines for
> hibernation and suspend.

I still think that even though they can, quite a number of drivers won't
/need/ to have different functions for this, but maybe I'm mistaken.

> Maybe we should try changing from run-time to build time warning so that
> users are not overly concerned with it?

I'm not sure that solves the problem. The fact is that even for developers, it's
easy to overlook that assiging only the .suspend and .resume fields is probably
a mistake.

Regards,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

[Rafael J. Wysocki]

On Wednesday 05 August 2009, Albin Tonnerre wrote:
> On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
> > On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> > > In a number of cases, the .suspend, .freeze, .poweroff and .resume,
> > > .thaw, .restore functions are identical. However, they all need to be
> > > assigned to avoid regressionsm as the previous code called .suspend
> > > resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with
> > > this case.
> 
> 
> > I'd much rather have conversions done with a bit more analysis now that
> > our framework is more flexible and we can have specialized routines for
> > hibernation and suspend.
> 
> I still think that even though they can, quite a number of drivers won't
> /need/ to have different functions for this, but maybe I'm mistaken.
> 
> > Maybe we should try changing from run-time to build time warning so that
> > users are not overly concerned with it?
> 
> I'm not sure that solves the problem. The fact is that even for developers, it's
> easy to overlook that assiging only the .suspend and .resume fields is probably
> a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Thanks,
Rafael

[PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Rafael J. Wysocki]

On Wednesday 05 August 2009, Rafael J. Wysocki wrote:
> On Wednesday 05 August 2009, Albin Tonnerre wrote:
> > On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
> > > On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> > > > In a number of cases, the .suspend, .freeze, .poweroff and .resume,
> > > > .thaw, .restore functions are identical. However, they all need to be
> > > > assigned to avoid regressionsm as the previous code called .suspend
> > > > resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with
> > > > this case.
> > 
> > 
> > > I'd much rather have conversions done with a bit more analysis now that
> > > our framework is more flexible and we can have specialized routines for
> > > hibernation and suspend.
> > 
> > I still think that even though they can, quite a number of drivers won't
> > /need/ to have different functions for this, but maybe I'm mistaken.
> > 
> > > Maybe we should try changing from run-time to build time warning so that
> > > users are not overly concerned with it?
> > 
> > I'm not sure that solves the problem. The fact is that even for developers, it's
> > easy to overlook that assiging only the .suspend and .resume fields is probably
> > a mistake.
> 
> I agree, so I'm going to take the patch.
> 
> I'll add a comment describing what the macro is for, though.

Strictly speaking, I'm going to add the appended patch to the linux-next
branch of the suspend-2.6 tree.

Thanks,
Rafael

---
From: Albin Tonnerre <albin.tonnerre@free-electrons.com>
Subject: PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In a number of cases, the .suspend, .freeze, .poweroff and .resume,
.thaw, .restore functions are identical. However, they all need to be
assigned to avoid regressionsm as the previous code called .suspend
resp. .resume in all those cases. SIMPLE_DEV_PM_OPS helps to deal
with this case.

[rjw: Changed the name of the macro and added the comment explaining its
 purpose.]

Signed-off-by: Albin Tonnerre <albin.tonnerre@free-electrons.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/pm.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -184,6 +184,20 @@ struct dev_pm_ops {
 	int (*restore_noirq)(struct device *dev);
 };
 
+/*
+ * Use this if you want to use the same suspend and resume callbacks for suspend
+ * to RAM and hibernation.
+ */
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+	.suspend = suspend_fn, \
+	.resume = resume_fn, \
+	.freeze = suspend_fn, \
+	.thaw = resume_fn, \
+	.poweroff = suspend_fn, \
+	.resume = resume_fn, \
+}
+
 /**
  * PM_EVENT_ messages
  *

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[pHilipp Zabel]

On Wed, Aug 5, 2009 at 10:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Wednesday 05 August 2009, Rafael J. Wysocki wrote:
>> On Wednesday 05 August 2009, Albin Tonnerre wrote:
>> > On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
>> > > On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
>> > > > In a number of cases, the .suspend, .freeze, .poweroff and .resume,
>> > > > .thaw, .restore functions are identical. However, they all need to be
>> > > > assigned to avoid regressionsm as the previous code called .suspend
>> > > > resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with
>> > > > this case.
>> >
>> >
>> > > I'd much rather have conversions done with a bit more analysis now that
>> > > our framework is more flexible and we can have specialized routines for
>> > > hibernation and suspend.
>> >
>> > I still think that even though they can, quite a number of drivers won't
>> > /need/ to have different functions for this, but maybe I'm mistaken.
>> >
>> > > Maybe we should try changing from run-time to build time warning so that
>> > > users are not overly concerned with it?
>> >
>> > I'm not sure that solves the problem. The fact is that even for developers, it's
>> > easy to overlook that assiging only the .suspend and .resume fields is probably
>> > a mistake.
>>
>> I agree, so I'm going to take the patch.
>>
>> I'll add a comment describing what the macro is for, though.
>
> Strictly speaking, I'm going to add the appended patch to the linux-next
> branch of the suspend-2.6 tree.
>
> Thanks,
> Rafael
>
> ---
> From: Albin Tonnerre <albin.tonnerre@free-electrons.com>
> Subject: PM: Add convenience macro to make switching to dev_pm_ops less error-prone
>
> In a number of cases, the .suspend, .freeze, .poweroff and .resume,
> .thaw, .restore functions are identical. However, they all need to be
> assigned to avoid regressionsm as the previous code called .suspend
> resp. .resume in all those cases. SIMPLE_DEV_PM_OPS helps to deal
> with this case.
>
> [rjw: Changed the name of the macro and added the comment explaining its
>  purpose.]
>
> Signed-off-by: Albin Tonnerre <albin.tonnerre@free-electrons.com>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/pm.h |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> Index: linux-2.6/include/linux/pm.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pm.h
> +++ linux-2.6/include/linux/pm.h
> @@ -184,6 +184,20 @@ struct dev_pm_ops {
>        int (*restore_noirq)(struct device *dev);
>  };
>
> +/*
> + * Use this if you want to use the same suspend and resume callbacks for suspend
> + * to RAM and hibernation.
> + */
> +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +struct dev_pm_ops name = { \
> +       .suspend = suspend_fn, \
> +       .resume = resume_fn, \
> +       .freeze = suspend_fn, \
> +       .thaw = resume_fn, \
> +       .poweroff = suspend_fn, \
> +       .resume = resume_fn, \

I think that second .resume was intended to be .restore instead.

regards
Philipp

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Frans Pop]

Rafael J. Wysocki wrote:

> +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +struct dev_pm_ops name = { \
> +       .suspend = suspend_fn, \
> +       .resume = resume_fn, \
> +       .freeze = suspend_fn, \
> +       .thaw = resume_fn, \
> +       .poweroff = suspend_fn, \
> +       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Cheers,
FJP

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Rafael J. Wysocki]

On Wednesday 05 August 2009, Frans Pop wrote:
> Rafael J. Wysocki wrote:
> 
> > +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> > +struct dev_pm_ops name = { \
> > +       .suspend = suspend_fn, \
> > +       .resume = resume_fn, \
> > +       .freeze = suspend_fn, \
> > +       .thaw = resume_fn, \
> > +       .poweroff = suspend_fn, \
> > +       .resume = resume_fn, \
> 
> That defines .resume twice. Guess the last should be restore.

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

Rafael

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Daniel Mack]

On Wed, Aug 05, 2009 at 11:30:07PM +0200, Rafael J. Wysocki wrote:
> On Wednesday 05 August 2009, Frans Pop wrote:
> > Rafael J. Wysocki wrote:
> > 
> > > +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> > > +struct dev_pm_ops name = { \
> > > +       .suspend = suspend_fn, \
> > > +       .resume = resume_fn, \
> > > +       .freeze = suspend_fn, \
> > > +       .thaw = resume_fn, \
> > > +       .poweroff = suspend_fn, \
> > > +       .resume = resume_fn, \
> > 
> > That defines .resume twice. Guess the last should be restore.
> 
> Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

When is a good point to resend patches that use this macro? I guess they
won't make it to .31 anyway, right? So I just wait for the next merge
window?

Daniel

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Rafael J. Wysocki]

On Thursday 06 August 2009, Daniel Mack wrote:
> On Wed, Aug 05, 2009 at 11:30:07PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday 05 August 2009, Frans Pop wrote:
> > > Rafael J. Wysocki wrote:
> > > 
> > > > +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> > > > +struct dev_pm_ops name = { \
> > > > +       .suspend = suspend_fn, \
> > > > +       .resume = resume_fn, \
> > > > +       .freeze = suspend_fn, \
> > > > +       .thaw = resume_fn, \
> > > > +       .poweroff = suspend_fn, \
> > > > +       .resume = resume_fn, \
> > > 
> > > That defines .resume twice. Guess the last should be restore.
> > 
> > Yes, thanks to you and Philipp for noticing that and sorry for the mistake.
> 
> When is a good point to resend patches that use this macro? I guess they
> won't make it to .31 anyway, right? So I just wait for the next merge
> window?

Yes, I think that would be the right thing to do.

Best,
Rafael

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Magnus Damm]

On Thu, Aug 6, 2009 at 9:16 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Thursday 06 August 2009, Daniel Mack wrote:
>> When is a good point to resend patches that use this macro? I guess they
>> won't make it to .31 anyway, right? So I just wait for the next merge
>> window?
>
> Yes, I think that would be the right thing to do.

Is it a good idea to disable the warning before the final 2.6.31 release?

Cheers,

/ magnus

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

[Rafael J. Wysocki]

On Thursday 06 August 2009, Magnus Damm wrote:
> On Thu, Aug 6, 2009 at 9:16 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Thursday 06 August 2009, Daniel Mack wrote:
> >> When is a good point to resend patches that use this macro? I guess they
> >> won't make it to .31 anyway, right? So I just wait for the next merge
> >> window?
> >
> > Yes, I think that would be the right thing to do.
> 
> Is it a good idea to disable the warning before the final 2.6.31 release?

I guess so.  Let's disable it.

Thanks,
Rafael