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

login
register
mail settings
Submitter Nicolas Ferre
Date June 30, 2011, 1:49 p.m.
Message ID <1309441781-11012-1-git-send-email-nicolas.ferre@atmel.com>
Download mbox | patch
Permalink /patch/102755/
State New
Headers show

Comments

Uwe Kleine-König - June 30, 2011, 1:13 p.m.
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 - June 30, 2011, 1:49 p.m.
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(-)
Nicolas Ferre - July 1, 2011, 2:22 p.m.
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.
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>
Felipe Balbi - July 4, 2011, 11:06 a.m.
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
Nicolas Ferre - July 4, 2011, 12:43 p.m.
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,
Felipe Balbi - July 6, 2011, 8:28 a.m.
Hi,

On Wed, Jul 06, 2011 at 11:31:36AM +0200, Nicolas Ferre wrote:
> Take care of slots while going to suspend state.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>

FWIW:

Reviewed-by: Felipe Balbi <balbi@ti.com>

> @@ -1878,10 +1879,72 @@ 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);
> +		}

just one small nitpicking which you can ignore if you like, but you
don't really need the else branch here. If you fall into if (ret < 0)
you will return early anyway, so you can save two lines and an
indentation level :-p
Chris Ball - July 9, 2011, 8:58 p.m.
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.
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!

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,
 	},
 };