diff mbox

[V2] MMC: PM: add suspend/resume in atmel-mci

Message ID 1309441781-11012-1-git-send-email-nicolas.ferre@atmel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nicolas Ferre June 30, 2011, 1:49 p.m. UTC
Take care of slots while going to suspend state.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
V2: move to pm_ops

 drivers/mmc/host/atmel-mci.c |   47 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

Comments

Uwe Kleine-König June 30, 2011, 1:13 p.m. UTC | #1
Hello Nicolas,

On Thu, Jun 30, 2011 at 03:49:41PM +0200, Nicolas Ferre wrote:
> Take care of slots while going to suspend state.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> V2: move to pm_ops
> 
>  drivers/mmc/host/atmel-mci.c |   47 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index aa8039f..058f1842 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -1878,10 +1878,57 @@ 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);
> +	struct atmel_mci_slot *slot;
> +	int i, ret;
slot and ret can have a more local scope.

> +
> +	 for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
> +		slot = host->slot[i];
> +		if (!slot)
> +			continue;
> +		ret = mmc_suspend_host(slot->mmc);
> +		if (ret < 0) {
> +			while (--i >= 0) {
> +				slot = host->slot[i];
> +				if (slot)
> +					mmc_resume_host(host->slot[i]->mmc);
hmm, mmc_resume_host could fail. But probably you cannot handle that in
a sane way, do you?

> +			}
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int atmci_resume(struct device *dev)
> +{
> +	struct atmel_mci *host = dev_get_drvdata(dev);
> +	struct atmel_mci_slot *slot;
> +	int i, ret;
> +
> +	for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
> +		slot = host->slot[i];
> +		if (!slot)
> +			continue;
> +		ret = mmc_resume_host(slot->mmc);
> +		if (ret < 0)
> +			return ret;
Maybe you should try to resume host 5 even if resuming host 4 failed?

> +	}
> +
> +	return 0;
> +}
> +#endif
> +static SIMPLE_DEV_PM_OPS(atmci_pm, atmci_suspend, atmci_resume);
> +
> +
For my taste a single empty line is enough.

>  static struct platform_driver atmci_driver = {
>  	.remove		= __exit_p(atmci_remove),
>  	.driver		= {
>  		.name		= "atmel_mci",
> +		.pm		= &atmci_pm,
>  	},
>  };
Nicolas Ferre July 1, 2011, 2:22 p.m. UTC | #2
Le 30/06/2011 15:13, Uwe Kleine-König :
> Hello Nicolas,
> 
> On Thu, Jun 30, 2011 at 03:49:41PM +0200, Nicolas Ferre wrote:
>> Take care of slots while going to suspend state.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> V2: move to pm_ops
>>
>>  drivers/mmc/host/atmel-mci.c |   47 ++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 47 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
>> index aa8039f..058f1842 100644
>> --- a/drivers/mmc/host/atmel-mci.c
>> +++ b/drivers/mmc/host/atmel-mci.c
>> @@ -1878,10 +1878,57 @@ 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);
>> +	struct atmel_mci_slot *slot;
>> +	int i, ret;
> slot and ret can have a more local scope.

ok.

>> +
>> +	 for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
>> +		slot = host->slot[i];
>> +		if (!slot)
>> +			continue;
>> +		ret = mmc_suspend_host(slot->mmc);
>> +		if (ret < 0) {
>> +			while (--i >= 0) {
>> +				slot = host->slot[i];
>> +				if (slot)
>> +					mmc_resume_host(host->slot[i]->mmc);
> hmm, mmc_resume_host could fail. But probably you cannot handle that in
> a sane way, do you?

Well, actually for the current implementation the only return code is... 0.
And here I try to have a king of best effort approach ;-)

>> +			}
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int atmci_resume(struct device *dev)
>> +{
>> +	struct atmel_mci *host = dev_get_drvdata(dev);
>> +	struct atmel_mci_slot *slot;
>> +	int i, ret;
>> +
>> +	for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
>> +		slot = host->slot[i];
>> +		if (!slot)
>> +			continue;
>> +		ret = mmc_resume_host(slot->mmc);
>> +		if (ret < 0)
>> +			return ret;
> Maybe you should try to resume host 5 even if resuming host 4 failed?

In fact all other drivers that are dealing with multiple slots are doing
the same... So I have difficulties to know the truth.
The question is: is it better to return an error to the "resume"
function so that we are called later again or do we have to do our best
to thaw everything out?

>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
>> +static SIMPLE_DEV_PM_OPS(atmci_pm, atmci_suspend, atmci_resume);
>> +
>> +
> For my taste a single empty line is enough.

Ok.

>>  static struct platform_driver atmci_driver = {
>>  	.remove		= __exit_p(atmci_remove),
>>  	.driver		= {
>>  		.name		= "atmel_mci",
>> +		.pm		= &atmci_pm,
>>  	},
>>  };
> 

Thanks, best regards,
Hans-Christian Egtvedt July 1, 2011, 2:32 p.m. UTC | #3
On Fri, 2011-07-01 at 16:22 +0200, Nicolas Ferre wrote:
> Le 30/06/2011 15:13, Uwe Kleine-König :

<snipp>

> Well, actually for the current implementation the only return code is... 0.
> And here I try to have a king of best effort approach ;-)
> 
> >> +			}
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int atmci_resume(struct device *dev)
> >> +{
> >> +	struct atmel_mci *host = dev_get_drvdata(dev);
> >> +	struct atmel_mci_slot *slot;
> >> +	int i, ret;
> >> +
> >> +	for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
> >> +		slot = host->slot[i];
> >> +		if (!slot)
> >> +			continue;
> >> +		ret = mmc_resume_host(slot->mmc);
> >> +		if (ret < 0)
> >> +			return ret;
> > Maybe you should try to resume host 5 even if resuming host 4 failed?
> 
> In fact all other drivers that are dealing with multiple slots are doing
> the same... So I have difficulties to know the truth.
> The question is: is it better to return an error to the "resume"
> function so that we are called later again or do we have to do our best
> to thaw everything out?

Detect failures and return the error value, but also try to resume the
other slots.

You must however then keep track of which are restored for later calls
to resume AFAICT.

<snipp>
Chris Ball July 9, 2011, 8:58 p.m. UTC | #4
Hi,

On Mon, Jul 04 2011, 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

Thanks, I've merged v3 of this patch for mmc-next/3.1 with Felipe's
Reviewed-by, and your [2/2] kconfig change.

- Chris.
Nicolas Ferre July 11, 2011, 9:47 a.m. UTC | #5
Le 09/07/2011 22:58, Chris Ball :
> Hi,
> 
> On Mon, Jul 04 2011, 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
> 
> Thanks, I've merged v3 of this patch for mmc-next/3.1 with Felipe's

You mean v4, I have checked: everything is ok.

> Reviewed-by, and your [2/2] kconfig change.

Perfect, thanks a lot Chris!
diff mbox

Patch

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index aa8039f..058f1842 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -1878,10 +1878,57 @@  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);
+	struct atmel_mci_slot *slot;
+	int i, ret;
+
+	 for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
+		slot = host->slot[i];
+		if (!slot)
+			continue;
+		ret = mmc_suspend_host(slot->mmc);
+		if (ret < 0) {
+			while (--i >= 0) {
+				slot = host->slot[i];
+				if (slot)
+					mmc_resume_host(host->slot[i]->mmc);
+			}
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int atmci_resume(struct device *dev)
+{
+	struct atmel_mci *host = dev_get_drvdata(dev);
+	struct atmel_mci_slot *slot;
+	int i, ret;
+
+	for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
+		slot = host->slot[i];
+		if (!slot)
+			continue;
+		ret = mmc_resume_host(slot->mmc);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+#endif
+static SIMPLE_DEV_PM_OPS(atmci_pm, atmci_suspend, atmci_resume);
+
+
 static struct platform_driver atmci_driver = {
 	.remove		= __exit_p(atmci_remove),
 	.driver		= {
 		.name		= "atmel_mci",
+		.pm		= &atmci_pm,
 	},
 };