Re: [PATCH V3] MMC: PM: add suspend/resume in atmel-mci

Nicolas Ferre
2011-07-04T12:44:06Z

Le 04/07/2011 13:06, Felipe Balbi :
> Hi,
>
> On Mon, Jul 04, 2011 at 01:38:25PM +0200, Nicolas Ferre wrote:
>> Take care of slots while going to suspend state.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> V3: take care of each slot SUSPENDED state
>> (adding a status bit in the slot "flags")
>> V2: move to pm_ops
>>
>> drivers/mmc/host/atmel-mci.c | 60 ++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 60 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
>> index aa8039f..ed63bcd 100644
>> --- a/drivers/mmc/host/atmel-mci.c
>> +++ b/drivers/mmc/host/atmel-mci.c
>> @@ -203,6 +203,7 @@ struct atmel_mci_slot {
>> #define ATMCI_CARD_PRESENT 0
>> #define ATMCI_CARD_NEED_INIT 1
>> #define ATMCI_SHUTDOWN 2
>> +#define ATMCI_SUSPENDED 3
>>
>> int detect_pin;
>> int wp_pin;
>> @@ -1878,10 +1879,69 @@ static int __exit atmci_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_PM
>> +static int atmci_suspend(struct device *dev)
>> +{
>> + struct atmel_mci *host = dev_get_drvdata(dev);
>> + int i;
>> +
>> + for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
>> + struct atmel_mci_slot *slot = host->slot[i];
>> + int ret;
>> +
>> + if (!slot)
>> + continue;
>> + ret = mmc_suspend_host(slot->mmc);
>> + if (ret < 0) {
>> + while (--i >= 0) {
>> + slot = host->slot[i];
>> + if (slot
>> + && test_bit(ATMCI_SUSPENDED, &slot->flags)) {
>> + mmc_resume_host(host->slot[i]->mmc);
>> + clear_bit(ATMCI_SUSPENDED, &slot->flags);
>> + }
>> + }
>> + return ret;
>> + } else {
>> + set_bit(ATMCI_SUSPENDED, &slot->flags);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int atmci_resume(struct device *dev)
>> +{
>> + struct atmel_mci *host = dev_get_drvdata(dev);
>> + int i;
>> + int ret = 0;
>> +
>> + for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
>> + struct atmel_mci_slot *slot = host->slot[i];
>> + int err;
>> +
>> + slot = host->slot[i];
>> + if (!slot)
>> + continue;
>> + if (!test_bit(ATMCI_SUSPENDED, &slot->flags))
>> + continue;
>> + err = mmc_resume_host(slot->mmc);
>> + if (err < 0)
>> + ret = err;
>> + else
>> + clear_bit(ATMCI_SUSPENDED, &slot->flags);
>> + }
>> +
>> + return ret;
>> +}
>> +#endif
>> +static SIMPLE_DEV_PM_OPS(atmci_pm, atmci_suspend, atmci_resume);
>
> if you disable CONFIG_PM this won't work. atmci_resume and atmci_suspend
> will be undefined.
>
>> static struct platform_driver atmci_driver = {
>> .remove = __exit_p(atmci_remove),
>> .driver = {
>> .name = "atmel_mci",
>> + .pm = &atmci_pm,
>
> this pointer will also be invalid.
>
> what most people do is:
>
> #ifdef CONFIG_PM
>
> suspend()
> resume()
>
> static SIMPLE_DEV_PM_OPS(atmci_pm, suspend, resume);
>
> #define DEV_PM_OPS (&atmci_pm)
> #else
> #define DEV_PM_OPS NULL
> #endif

Yes, sure that makes sense.

Thanks, I will cook a V4.

Best regards,
--
Nicolas Ferre