[PATCH] au1xmmc: dev_pm_ops conversion

[PATCH] au1xmmc: dev_pm_ops conversion

[Manuel Lauss]

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
Run-tested on Au1200.

 drivers/mmc/host/au1xmmc.c |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
index d3f5561..70509c9 100644
--- a/drivers/mmc/host/au1xmmc.c
+++ b/drivers/mmc/host/au1xmmc.c
@@ -1131,13 +1131,12 @@ static int __devexit au1xmmc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#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 +1149,27 @@ 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);
 }
-#else
-#define au1xmmc_suspend NULL
-#define au1xmmc_resume NULL
-#endif
+
+static struct dev_pm_ops au1xmmc_pmops = {
+	.resume		= au1xmmc_resume,
+	.suspend	= au1xmmc_suspend,
+};
 
 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

[PATCH] au1xmmc: allow platforms to disable certain host capabilities

[Manuel Lauss]

Although the hardware supports a 4/8bit SD interface and the driver
unconditionally advertises all hardware caps to the MMC core, not all
datalines may actually be wired up.  This patch introduces another
field to au1xmmc platform data allowing platforms to disable certain
advanced host controller features.

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
 arch/mips/include/asm/mach-au1x00/au1100_mmc.h |    1 +
 drivers/mmc/host/au1xmmc.c                     |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/mips/include/asm/mach-au1x00/au1100_mmc.h b/arch/mips/include/asm/mach-au1x00/au1100_mmc.h
index c35e209..a674643 100644
--- a/arch/mips/include/asm/mach-au1x00/au1100_mmc.h
+++ b/arch/mips/include/asm/mach-au1x00/au1100_mmc.h
@@ -46,6 +46,7 @@ struct au1xmmc_platform_data {
 	int(*card_readonly)(void *mmc_host);
 	void(*set_power)(void *mmc_host, int state);
 	struct led_classdev *led;
+	unsigned long mask_host_caps;
 };
 
 #define SD0_BASE	0xB0600000
diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
index 70509c9..d50e7ea 100644
--- a/drivers/mmc/host/au1xmmc.c
+++ b/drivers/mmc/host/au1xmmc.c
@@ -1005,6 +1005,9 @@ static int __devinit au1xmmc_probe(struct platform_device *pdev)
 	mmc->ocr_avail = AU1XMMC_OCR;
 	mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SDIO_IRQ;
 
+	/* platform may not be able to use 4/8 bit datapath */
+	mmc->caps &= ~(host->platdata->mask_host_caps);
+
 	host->status = HOST_S_IDLE;
 
 	/* board-specific carddetect setup, if any */
-- 
1.6.3.3

Re: [PATCH] au1xmmc: dev_pm_ops conversion

[Frans Pop]

> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> ---
> Run-tested on Au1200.
>
>  drivers/mmc/host/au1xmmc.c |   23 +++++++++++------------
>  1 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
> index d3f5561..70509c9 100644
> --- a/drivers/mmc/host/au1xmmc.c
> +++ b/drivers/mmc/host/au1xmmc.c
> @@ -1131,13 +1131,12 @@ static int __devexit au1xmmc_remove(struct
> platform_device *pdev) return 0;
>  }
>  
> -#ifdef CONFIG_PM

Won't the removal of this test cause a build failure if CONFIG_PM is not 
set? If the removal of the test is safe, this should IMHO at least be 
explained in the commit message.

The IMO simplest fix would be to restore the #ifdef ...

> -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 +1149,27 @@ 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);
>  }
> -#else
> -#define au1xmmc_suspend NULL
> -#define au1xmmc_resume NULL

... drop the 3 lines above (as you did) ...

> -#endif

... move this #endif to after the new struct below ...

> +
> +static struct dev_pm_ops au1xmmc_pmops = {
> +       .resume         = au1xmmc_resume,
> +       .suspend        = au1xmmc_suspend,
> +};
>  
>  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,

... and add an #ifdef CONFIG_PM around the new line above.

>         },
>  };

Cheers,
FJP

Re: [PATCH] au1xmmc: dev_pm_ops conversion

[Manuel Lauss]

Good Morning Frans,

On Mon, Jul 20, 2009 at 10:00 PM, Frans Pop<elendil@planet.nl> wrote:
>> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
>> ---
>> Run-tested on Au1200.
>>
>>  drivers/mmc/host/au1xmmc.c |   23 +++++++++++------------
>>  1 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
>> index d3f5561..70509c9 100644
>> --- a/drivers/mmc/host/au1xmmc.c
>> +++ b/drivers/mmc/host/au1xmmc.c
>> @@ -1131,13 +1131,12 @@ static int __devexit au1xmmc_remove(struct
>> platform_device *pdev) return 0;
>>  }
>>
>> -#ifdef CONFIG_PM
>
> Won't the removal of this test cause a build failure if CONFIG_PM is not
> set? If the removal of the test is safe, this should IMHO at least be
> explained in the commit message.

No, it builds just fine without CONFIG_PM; it was there to shave off a
few bytes from the kernel image.  But not everyone tests this driver
with CONFIG_PM=y, because apparently noone really needed PM on
this platform (Alchemy), and a full build of most of the boards using this
driver fails with PM enabled.

This way the PM methods at least get a compile-test in the non-pm case.


>> -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 +1149,27 @@ 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);
>>  }
>> -#else
>> -#define au1xmmc_suspend NULL
>> -#define au1xmmc_resume NULL
>
> ... drop the 3 lines above (as you did) ...
>
>> -#endif
>
> ... move this #endif to after the new struct below ...
>
>> +
>> +static struct dev_pm_ops au1xmmc_pmops = {
>> +       .resume         = au1xmmc_resume,
>> +       .suspend        = au1xmmc_suspend,
>> +};
>>
>>  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,

I like what Magnus Damm did for some of the SuperH drivers:

#ifdef CONFIG_PM
[...]
#define DRIVER_PM_OPS (&driver_pm_ops)
#else
#define DRIVER_PM_OPS NULL
#endif

...
      .driver = {
...
             .pm = DRIVER_PM_OPS,
      }
...

I'd like to keep the pm stuff enabled at all times since it doesn't hurt
in the non-pm case and if kernel size becomes a problem I can add the
#defines back.

What do you think?

Thank you!
      Manuel Lauss

Re: [PATCH] au1xmmc: dev_pm_ops conversion

[Frans Pop]

On Tuesday 21 July 2009, Manuel Lauss wrote:
> >> -#ifdef CONFIG_PM
> >
> > Won't the removal of this test cause a build failure if CONFIG_PM is
> > not set? If the removal of the test is safe, this should IMHO at
> > least be explained in the commit message.
>
> No, it builds just fine without CONFIG_PM; it was there to shave off a
> few bytes from the kernel image.  But not everyone tests this driver
> with CONFIG_PM=y, because apparently noone really needed PM on
> this platform (Alchemy), and a full build of most of the boards using
> this driver fails with PM enabled.

OK.

> This way the PM methods at least get a compile-test in the non-pm case.

Not sure that is a sufficiently valid argument. In any case it *is* a 
separate change to "dev_pm_ops conversion" so it really should at least 
be documented and justified in the commit log.

> I like what Magnus Damm did for some of the SuperH drivers:
>
> #ifdef CONFIG_PM
> [...]
> #define DRIVER_PM_OPS (&driver_pm_ops)
> #else
> #define DRIVER_PM_OPS NULL
> #endif

Yes, that's quite elegant.

> I'd like to keep the pm stuff enabled at all times since it doesn't
> hurt in the non-pm case and if kernel size becomes a problem I can add
> the #defines back.

I guess that's up to the maintainers of the mips port.

Cheers,
FJP