| Message ID | 1309441781-11012-1-git-send-email-nicolas.ferre@atmel.com |
|---|---|
| State | New |
| Headers | show |
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, > }, > };
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,
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>
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
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,
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
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.
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 --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, }, };