[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