2005-11-08 06:41:21

by Pierre Ossman

[permalink] [raw]
Subject: [PATCH] [MMC] wbsd pnp suspend

Allow the wbsd driver to use the new suspend/resume functions added to
the PnP layer.

Signed-off-by: Pierre Ossman <[email protected]>
---

drivers/mmc/wbsd.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/wbsd.c b/drivers/mmc/wbsd.c
--- a/drivers/mmc/wbsd.c
+++ b/drivers/mmc/wbsd.c
@@ -1980,37 +1980,53 @@ static void __devexit wbsd_pnp_remove(st

#ifdef CONFIG_PM

-static int wbsd_suspend(struct device *dev, pm_message_t state)
+static int wbsd_suspend(struct wbsd_host *host, pm_message_t state)
+{
+ BUG_ON(host == NULL);
+
+ return mmc_suspend_host(host->mmc, state);
+}
+
+static int wbsd_resume(struct wbsd_host *host)
+{
+ BUG_ON(host == NULL);
+
+ wbsd_init_device(host);
+
+ return mmc_resume_host(host->mmc);
+}
+
+static int wbsd_platform_suspend(struct device *dev, pm_message_t state)
{
struct mmc_host *mmc = dev_get_drvdata(dev);
struct wbsd_host *host;
int ret;

- if (!mmc)
+ if (mmc == NULL)
return 0;

- DBG("Suspending...\n");
-
- ret = mmc_suspend_host(mmc, state);
- if (!ret)
- return ret;
+ DBGF("Suspending...\n");

host = mmc_priv(mmc);

+ ret = wbsd_suspend(host, state);
+ if (ret)
+ return ret;
+
wbsd_chip_poweroff(host);

return 0;
}

-static int wbsd_resume(struct device *dev)
+static int wbsd_platform_resume(struct device *dev)
{
struct mmc_host *mmc = dev_get_drvdata(dev);
struct wbsd_host *host;

- if (!mmc)
+ if (mmc == NULL)
return 0;

- DBG("Resuming...\n");
+ DBGF("Resuming...\n");

host = mmc_priv(mmc);

@@ -2021,15 +2037,70 @@ static int wbsd_resume(struct device *de
*/
mdelay(5);

- wbsd_init_device(host);
+ return wbsd_resume(host);
+}
+
+#ifdef CONFIG_PNP
+
+static int wbsd_pnp_suspend(struct pnp_dev *pnp_dev, pm_message_t state)
+{
+ struct mmc_host *mmc = dev_get_drvdata(&pnp_dev->dev);
+ struct wbsd_host *host;
+
+ if (mmc == NULL)
+ return 0;
+
+ DBGF("Suspending...\n");
+
+ host = mmc_priv(mmc);

- return mmc_resume_host(mmc);
+ return wbsd_suspend(host, state);
}

+static int wbsd_pnp_resume(struct pnp_dev *pnp_dev)
+{
+ struct mmc_host *mmc = dev_get_drvdata(&pnp_dev->dev);
+ struct wbsd_host *host;
+
+ if (mmc == NULL)
+ return 0;
+
+ DBGF("Resuming...\n");
+
+ host = mmc_priv(mmc);
+
+ /*
+ * See if chip needs to be configured.
+ */
+ if (host->config != 0)
+ {
+ if (!wbsd_chip_validate(host))
+ {
+ printk(KERN_WARNING DRIVER_NAME
+ ": PnP active but chip not configured! "
+ "You probably have a buggy BIOS. "
+ "Configuring chip manually.\n");
+ wbsd_chip_config(host);
+ }
+ }
+
+ /*
+ * Allow device to initialise itself properly.
+ */
+ mdelay(5);
+
+ return wbsd_resume(host);
+}
+
+#endif /* CONFIG_PNP */
+
#else /* CONFIG_PM */

-#define wbsd_suspend NULL
-#define wbsd_resume NULL
+#define wbsd_platform_suspend NULL
+#define wbsd_platform_resume NULL
+
+#define wbsd_pnp_suspend NULL
+#define wbsd_pnp_resume NULL

#endif /* CONFIG_PM */

@@ -2041,8 +2112,8 @@ static struct device_driver wbsd_driver
.probe = wbsd_probe,
.remove = wbsd_remove,

- .suspend = wbsd_suspend,
- .resume = wbsd_resume,
+ .suspend = wbsd_platform_suspend,
+ .resume = wbsd_platform_resume,
};

#ifdef CONFIG_PNP
@@ -2052,6 +2123,9 @@ static struct pnp_driver wbsd_pnp_driver
.id_table = pnp_dev_table,
.probe = wbsd_pnp_probe,
.remove = wbsd_pnp_remove,
+
+ .suspend = wbsd_pnp_suspend,
+ .resume = wbsd_pnp_resume,
};

#endif /* CONFIG_PNP */


2005-11-08 06:50:33

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] [MMC] wbsd pnp suspend

Pierre Ossman <[email protected]> wrote:
>
> Allow the wbsd driver to use the new suspend/resume functions added to
> the PnP layer.
>

Doesn't Russell handle mmc stuff?

> -static int wbsd_suspend(struct device *dev, pm_message_t state)
> +static int wbsd_suspend(struct wbsd_host *host, pm_message_t state)
> +{
> + BUG_ON(host == NULL);
> +
> + return mmc_suspend_host(host->mmc, state);
> +}

There's not much point in this BUG_ON. If host==0 then we'll get a
perfectly good oops in the next statement - it's just as informative.

> +static int wbsd_resume(struct wbsd_host *host)
> +{
> + BUG_ON(host == NULL);

Ditto.

> + if (host->config != 0)
> + {
> + if (!wbsd_chip_validate(host))
> + {

Like:

if (host->config != 0) {
if (!wbsd_chip_validate(host)) {

please.

2005-11-08 06:57:05

by Pierre Ossman

[permalink] [raw]
Subject: Re: [PATCH] [MMC] wbsd pnp suspend

Andrew Morton wrote:
> Pierre Ossman <[email protected]> wrote:
>> Allow the wbsd driver to use the new suspend/resume functions added to
>> the PnP layer.
>>
>
> Doesn't Russell handle mmc stuff?
>

Yup. But this needs the PnP suspend stuff in your patch set.

>> -static int wbsd_suspend(struct device *dev, pm_message_t state)
>> +static int wbsd_suspend(struct wbsd_host *host, pm_message_t state)
>> +{
>> + BUG_ON(host == NULL);
>> +
>> + return mmc_suspend_host(host->mmc, state);
>> +}
>
> There's not much point in this BUG_ON. If host==0 then we'll get a
> perfectly good oops in the next statement - it's just as informative.
>

I suppose. I just have a tendency to scatter assertions all over the
place. :)

>> + if (host->config != 0)
>> + {
>> + if (!wbsd_chip_validate(host))
>> + {
>
> Like:
>
> if (host->config != 0) {
> if (!wbsd_chip_validate(host)) {
>
> please.
>

We had this discussion the last patch for this driver. It's horribly
wrong when it comes to coding style so keeping patches in the same style
as the rest of the driver is the lesser evil (IMHO).

Rgds
Pierre

2005-11-08 07:06:24

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] [MMC] wbsd pnp suspend

On Tuesday 08 November 2005 01:57, Pierre Ossman wrote:
> Andrew Morton wrote:
> > Pierre Ossman <[email protected]> wrote:
> >> + if (host->config != 0)
> >> + {
> >> + if (!wbsd_chip_validate(host))
> >> + {
> >
> > Like:
> >
> > if (host->config != 0) {
> > if (!wbsd_chip_validate(host)) {
> >
> > please.
> >
>
> We had this discussion the last patch for this driver. It's horribly
> wrong when it comes to coding style so keeping patches in the same style
> as the rest of the driver is the lesser evil (IMHO).
>

Aside from unusual brace placement it does not look too bad. How about
running it through Lindent to fix it once and for all?

--
Dmitry

2005-11-08 08:45:32

by Pierre Ossman

[permalink] [raw]
Subject: Re: [PATCH] [MMC] wbsd pnp suspend

Dmitry Torokhov wrote:
>
> Aside from unusual brace placement it does not look too bad. How about
> running it through Lindent to fix it once and for all?
>
>

I could, just haven't gotten around to it. :)

I'll try to remember to have a look at it when there aren't any pending
patches that will get borked by a lindent.

Rgds
Pierre