[PATCH 0/2] Remove mmc_host enable/disable methods.

[PATCH 0/2] Remove mmc_host enable/disable methods.

[NeilBrown]

Only omap_hsmmc uses enable and disable, and this seems
to be largely for historical reasons and is no longer
necessary.

I have tested these patches with an OMAP3 with an
uSD card on mmc0 and a wifi SDIO device on mmc1.

NeilBrown


---

NeilBrown (2):
      mmc: omap_hsmmc: stop using .enable and .disable method.
      mmc: remove enable/disable methods.


 drivers/mmc/core/core.c       |    5 -----
 drivers/mmc/host/omap_hsmmc.c |   24 +++---------------------
 include/linux/mmc/host.h      |    6 ------
 3 files changed, 3 insertions(+), 32 deletions(-)

--
Signature

[PATCH 1/2] mmc: omap_hsmmc: stop using .enable and .disable method.

[NeilBrown]

enable and disable are only used to get and put
runtime pm references.  .set_ios already does this
itself, and other drivers just do it in set_ios
and .request without using enable/disable.

So add pm_runtime get/put to omap_hsmmc_request(),
and discard the enable/disable methods.

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/mmc/host/omap_hsmmc.c |   24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index f84cfb01716d..092bcecd73e6 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -882,6 +882,8 @@ static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req
 		return;
 	host->mrq = NULL;
 	mmc_request_done(host->mmc, mrq);
+	pm_runtime_mark_last_busy(host->dev);
+	pm_runtime_put_autosuspend(host->dev);
 }
 
 /*
@@ -1537,6 +1539,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 
 	BUG_ON(host->req_in_progress);
 	BUG_ON(host->dma_ch != -1);
+	pm_runtime_get_sync(host->dev);
 	if (host->protect_card) {
 		if (host->reqs_blocked < 3) {
 			/*
@@ -1778,25 +1781,6 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
 	set_sd_bus_power(host);
 }
 
-static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
-{
-	struct omap_hsmmc_host *host = mmc_priv(mmc);
-
-	pm_runtime_get_sync(host->dev);
-
-	return 0;
-}
-
-static int omap_hsmmc_disable_fclk(struct mmc_host *mmc)
-{
-	struct omap_hsmmc_host *host = mmc_priv(mmc);
-
-	pm_runtime_mark_last_busy(host->dev);
-	pm_runtime_put_autosuspend(host->dev);
-
-	return 0;
-}
-
 static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
 				     unsigned int direction, int blk_size)
 {
@@ -1808,8 +1792,6 @@ static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
 }
 
 static struct mmc_host_ops omap_hsmmc_ops = {
-	.enable = omap_hsmmc_enable_fclk,
-	.disable = omap_hsmmc_disable_fclk,
 	.post_req = omap_hsmmc_post_req,
 	.pre_req = omap_hsmmc_pre_req,
 	.request = omap_hsmmc_request,

[PATCH 2/2] mmc: remove enable/disable methods.

[NeilBrown]

The 'enable' and 'disable' methods are "deprecated"
according to host.h, and are no longer used.  So
discard them.

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/mmc/core/core.c  |    5 -----
 include/linux/mmc/host.h |    6 ------
 2 files changed, 11 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 23f10f72e5f3..709ada9f26b5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -920,8 +920,6 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 		wake_up(&host->wq);
 	spin_unlock_irqrestore(&host->lock, flags);
 	remove_wait_queue(&host->wq, &wait);
-	if (host->ops->enable && !stop && host->claim_cnt == 1)
-		host->ops->enable(host);
 	return stop;
 }
 
@@ -940,9 +938,6 @@ void mmc_release_host(struct mmc_host *host)
 
 	WARN_ON(!host->claimed);
 
-	if (host->ops->disable && host->claim_cnt == 1)
-		host->ops->disable(host);
-
 	spin_lock_irqsave(&host->lock, flags);
 	if (--host->claim_cnt) {
 		/* Release for nested claim */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c8cbe5d1550..b5bedaec6223 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -80,12 +80,6 @@ struct mmc_ios {
 
 struct mmc_host_ops {
 	/*
-	 * 'enable' is called when the host is claimed and 'disable' is called
-	 * when the host is released. 'enable' and 'disable' are deprecated.
-	 */
-	int (*enable)(struct mmc_host *host);
-	int (*disable)(struct mmc_host *host);
-	/*
 	 * It is optional for the host to implement pre_req and post_req in
 	 * order to support double buffering of requests (prepare one
 	 * request while another request is active).

Re: [PATCH 1/2] mmc: omap_hsmmc: stop using .enable and .disable method.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

On Thu, 26 Mar 2015 08:43:37 +1100 NeilBrown <neil@brown.name> wrote:

> enable and disable are only used to get and put
> runtime pm references.  .set_ios already does this
> itself, and other drivers just do it in set_ios
> and .request without using enable/disable.
> 
> So add pm_runtime get/put to omap_hsmmc_request(),
> and discard the enable/disable methods.
> 
> Signed-off-by: NeilBrown <neil@brown.name>

Sorry, that patch wasn't much good.  This one I have really tested properly
and checked that the 'put' match the 'get's and  that the device to regularly
go to sleep as expected.

NeilBrown


From: NeilBrown <neil@brown.name>
Date: Thu, 26 Mar 2015 08:28:43 +1100
Subject: [PATCH] mmc: omap_hsmmc: stop using .enable and .disable method.

enable and disable are only used to get and put
runtime pm references.  .set_ios already does this
itself, and other drivers just do it in set_ios
and .request without using enable/disable.

So add pm_runtime get/put to omap_hsmmc_request(),
and discard the enable/disable methods.

Signed-off-by: NeilBrown <neil@brown.name>

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index f84cfb01716d..2cb81538a612 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -882,6 +882,8 @@ static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req
 		return;
 	host->mrq = NULL;
 	mmc_request_done(host->mmc, mrq);
+	pm_runtime_mark_last_busy(host->dev);
+	pm_runtime_put_autosuspend(host->dev);
 }
 
 /*
@@ -1305,6 +1307,8 @@ static void omap_hsmmc_dma_callback(void *param)
 
 		host->mrq = NULL;
 		mmc_request_done(host->mmc, mrq);
+		pm_runtime_mark_last_busy(host->dev);
+		pm_runtime_put_autosuspend(host->dev);
 	}
 }
 
@@ -1537,6 +1541,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 
 	BUG_ON(host->req_in_progress);
 	BUG_ON(host->dma_ch != -1);
+	pm_runtime_get_sync(host->dev);
 	if (host->protect_card) {
 		if (host->reqs_blocked < 3) {
 			/*
@@ -1553,6 +1558,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 			req->data->error = -EBADF;
 		req->cmd->retries = 0;
 		mmc_request_done(mmc, req);
+		pm_runtime_mark_last_busy(host->dev);
+		pm_runtime_put_autosuspend(host->dev);
 		return;
 	} else if (host->reqs_blocked)
 		host->reqs_blocked = 0;
@@ -1566,6 +1573,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 			req->data->error = err;
 		host->mrq = NULL;
 		mmc_request_done(mmc, req);
+		pm_runtime_mark_last_busy(host->dev);
+		pm_runtime_put_autosuspend(host->dev);
 		return;
 	}
 	if (req->sbc && !(host->flags & AUTO_CMD23)) {
@@ -1778,25 +1787,6 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
 	set_sd_bus_power(host);
 }
 
-static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
-{
-	struct omap_hsmmc_host *host = mmc_priv(mmc);
-
-	pm_runtime_get_sync(host->dev);
-
-	return 0;
-}
-
-static int omap_hsmmc_disable_fclk(struct mmc_host *mmc)
-{
-	struct omap_hsmmc_host *host = mmc_priv(mmc);
-
-	pm_runtime_mark_last_busy(host->dev);
-	pm_runtime_put_autosuspend(host->dev);
-
-	return 0;
-}
-
 static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
 				     unsigned int direction, int blk_size)
 {
@@ -1808,8 +1798,6 @@ static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
 }
 
 static struct mmc_host_ops omap_hsmmc_ops = {
-	.enable = omap_hsmmc_enable_fclk,
-	.disable = omap_hsmmc_disable_fclk,
 	.post_req = omap_hsmmc_post_req,
 	.pre_req = omap_hsmmc_pre_req,
 	.request = omap_hsmmc_request,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/2] mmc: omap_hsmmc: stop using .enable and .disable method.

[Ulf Hansson]

On 26 March 2015 at 02:18, NeilBrown <neilb@suse.de> wrote:
> On Thu, 26 Mar 2015 08:43:37 +1100 NeilBrown <neil@brown.name> wrote:
>
>> enable and disable are only used to get and put
>> runtime pm references.  .set_ios already does this
>> itself, and other drivers just do it in set_ios
>> and .request without using enable/disable.
>>
>> So add pm_runtime get/put to omap_hsmmc_request(),
>> and discard the enable/disable methods.
>>
>> Signed-off-by: NeilBrown <neil@brown.name>
>
> Sorry, that patch wasn't much good.  This one I have really tested properly
> and checked that the 'put' match the 'get's and  that the device to regularly
> go to sleep as expected.
>
> NeilBrown
>
>
> From: NeilBrown <neil@brown.name>
> Date: Thu, 26 Mar 2015 08:28:43 +1100
> Subject: [PATCH] mmc: omap_hsmmc: stop using .enable and .disable method.
>
> enable and disable are only used to get and put
> runtime pm references.  .set_ios already does this
> itself, and other drivers just do it in set_ios
> and .request without using enable/disable.
>
> So add pm_runtime get/put to omap_hsmmc_request(),
> and discard the enable/disable methods.
>
> Signed-off-by: NeilBrown <neil@brown.name>
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index f84cfb01716d..2cb81538a612 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -882,6 +882,8 @@ static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req
>                 return;
>         host->mrq = NULL;
>         mmc_request_done(host->mmc, mrq);
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);
>  }

How about adding a function like this:

_omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req)
{
  mmc_request_done(host->mmc, mrq);
  pm_runtime_mark_last_busy(host->dev);
  pm_runtime_put_autosuspend(host->dev);
}

Then just invoke it from those places were needed?

>
>  /*
> @@ -1305,6 +1307,8 @@ static void omap_hsmmc_dma_callback(void *param)
>
>                 host->mrq = NULL;
>                 mmc_request_done(host->mmc, mrq);
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
>         }
>  }
>
> @@ -1537,6 +1541,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>
>         BUG_ON(host->req_in_progress);
>         BUG_ON(host->dma_ch != -1);
> +       pm_runtime_get_sync(host->dev);
>         if (host->protect_card) {
>                 if (host->reqs_blocked < 3) {
>                         /*
> @@ -1553,6 +1558,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>                         req->data->error = -EBADF;
>                 req->cmd->retries = 0;
>                 mmc_request_done(mmc, req);
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
>                 return;
>         } else if (host->reqs_blocked)
>                 host->reqs_blocked = 0;
> @@ -1566,6 +1573,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>                         req->data->error = err;
>                 host->mrq = NULL;
>                 mmc_request_done(mmc, req);
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
>                 return;
>         }
>         if (req->sbc && !(host->flags & AUTO_CMD23)) {
> @@ -1778,25 +1787,6 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
>         set_sd_bus_power(host);
>  }
>
> -static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
> -{
> -       struct omap_hsmmc_host *host = mmc_priv(mmc);
> -
> -       pm_runtime_get_sync(host->dev);
> -
> -       return 0;
> -}
> -
> -static int omap_hsmmc_disable_fclk(struct mmc_host *mmc)
> -{
> -       struct omap_hsmmc_host *host = mmc_priv(mmc);
> -
> -       pm_runtime_mark_last_busy(host->dev);
> -       pm_runtime_put_autosuspend(host->dev);
> -
> -       return 0;
> -}
> -
>  static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
>                                      unsigned int direction, int blk_size)
>  {
> @@ -1808,8 +1798,6 @@ static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
>  }
>
>  static struct mmc_host_ops omap_hsmmc_ops = {
> -       .enable = omap_hsmmc_enable_fclk,
> -       .disable = omap_hsmmc_disable_fclk,
>         .post_req = omap_hsmmc_post_req,
>         .pre_req = omap_hsmmc_pre_req,
>         .request = omap_hsmmc_request,

Finally, I think you have forgotten to deal with pm_runtime_get|put*()
in omap_hsmmc_enable_sdio_irq().

Kind regards
Uffe

Re: [PATCH 1/2] mmc: omap_hsmmc: stop using .enable and .disable method.

[Ulf Hansson]

On 26 March 2015 at 08:38, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 26 March 2015 at 02:18, NeilBrown <neilb@suse.de> wrote:
>> On Thu, 26 Mar 2015 08:43:37 +1100 NeilBrown <neil@brown.name> wrote:
>>
>>> enable and disable are only used to get and put
>>> runtime pm references.  .set_ios already does this
>>> itself, and other drivers just do it in set_ios
>>> and .request without using enable/disable.
>>>
>>> So add pm_runtime get/put to omap_hsmmc_request(),
>>> and discard the enable/disable methods.
>>>
>>> Signed-off-by: NeilBrown <neil@brown.name>
>>
>> Sorry, that patch wasn't much good.  This one I have really tested properly
>> and checked that the 'put' match the 'get's and  that the device to regularly
>> go to sleep as expected.
>>
>> NeilBrown
>>
>>
>> From: NeilBrown <neil@brown.name>
>> Date: Thu, 26 Mar 2015 08:28:43 +1100
>> Subject: [PATCH] mmc: omap_hsmmc: stop using .enable and .disable method.
>>
>> enable and disable are only used to get and put
>> runtime pm references.  .set_ios already does this
>> itself, and other drivers just do it in set_ios
>> and .request without using enable/disable.
>>
>> So add pm_runtime get/put to omap_hsmmc_request(),
>> and discard the enable/disable methods.
>>
>> Signed-off-by: NeilBrown <neil@brown.name>
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index f84cfb01716d..2cb81538a612 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -882,6 +882,8 @@ static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req
>>                 return;
>>         host->mrq = NULL;
>>         mmc_request_done(host->mmc, mrq);
>> +       pm_runtime_mark_last_busy(host->dev);
>> +       pm_runtime_put_autosuspend(host->dev);
>>  }
>
> How about adding a function like this:
>
> _omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req)
> {
>   mmc_request_done(host->mmc, mrq);
>   pm_runtime_mark_last_busy(host->dev);
>   pm_runtime_put_autosuspend(host->dev);
> }
>
> Then just invoke it from those places were needed?
>
>>
>>  /*
>> @@ -1305,6 +1307,8 @@ static void omap_hsmmc_dma_callback(void *param)
>>
>>                 host->mrq = NULL;
>>                 mmc_request_done(host->mmc, mrq);
>> +               pm_runtime_mark_last_busy(host->dev);
>> +               pm_runtime_put_autosuspend(host->dev);
>>         }
>>  }
>>
>> @@ -1537,6 +1541,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>>
>>         BUG_ON(host->req_in_progress);
>>         BUG_ON(host->dma_ch != -1);
>> +       pm_runtime_get_sync(host->dev);
>>         if (host->protect_card) {
>>                 if (host->reqs_blocked < 3) {
>>                         /*
>> @@ -1553,6 +1558,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>>                         req->data->error = -EBADF;
>>                 req->cmd->retries = 0;
>>                 mmc_request_done(mmc, req);
>> +               pm_runtime_mark_last_busy(host->dev);
>> +               pm_runtime_put_autosuspend(host->dev);
>>                 return;
>>         } else if (host->reqs_blocked)
>>                 host->reqs_blocked = 0;
>> @@ -1566,6 +1573,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>>                         req->data->error = err;
>>                 host->mrq = NULL;
>>                 mmc_request_done(mmc, req);
>> +               pm_runtime_mark_last_busy(host->dev);
>> +               pm_runtime_put_autosuspend(host->dev);
>>                 return;
>>         }
>>         if (req->sbc && !(host->flags & AUTO_CMD23)) {
>> @@ -1778,25 +1787,6 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
>>         set_sd_bus_power(host);
>>  }
>>
>> -static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
>> -{
>> -       struct omap_hsmmc_host *host = mmc_priv(mmc);
>> -
>> -       pm_runtime_get_sync(host->dev);
>> -
>> -       return 0;
>> -}
>> -
>> -static int omap_hsmmc_disable_fclk(struct mmc_host *mmc)
>> -{
>> -       struct omap_hsmmc_host *host = mmc_priv(mmc);
>> -
>> -       pm_runtime_mark_last_busy(host->dev);
>> -       pm_runtime_put_autosuspend(host->dev);
>> -
>> -       return 0;
>> -}
>> -
>>  static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
>>                                      unsigned int direction, int blk_size)
>>  {
>> @@ -1808,8 +1798,6 @@ static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
>>  }
>>
>>  static struct mmc_host_ops omap_hsmmc_ops = {
>> -       .enable = omap_hsmmc_enable_fclk,
>> -       .disable = omap_hsmmc_disable_fclk,
>>         .post_req = omap_hsmmc_post_req,
>>         .pre_req = omap_hsmmc_pre_req,
>>         .request = omap_hsmmc_request,
>
> Finally, I think you have forgotten to deal with pm_runtime_get|put*()
> in omap_hsmmc_enable_sdio_irq().

No, _me_ totally forgot that SDIO IRQ is a special case and from the
above mentioned function we mustn't deal with runtime PM, since that
function is executed in IRQ context. :-)

Also, since omap's runtime PM ->suspend() callback disables regular
SDIO IRQS and enables wakeups, this case is already covered. Please
ignore my comment above and sorry for the noise.

Kind regards
Uffe

Re: [PATCH 0/2] Remove mmc_host enable/disable methods.

[Ulf Hansson]

On 25 March 2015 at 22:43, NeilBrown <neil@brown.name> wrote:
> Only omap_hsmmc uses enable and disable, and this seems
> to be largely for historical reasons and is no longer
> necessary.
>
> I have tested these patches with an OMAP3 with an
> uSD card on mmc0 and a wifi SDIO device on mmc1.
>
> NeilBrown
>
>
> ---
>
> NeilBrown (2):
>       mmc: omap_hsmmc: stop using .enable and .disable method.
>       mmc: remove enable/disable methods.
>
>
>  drivers/mmc/core/core.c       |    5 -----
>  drivers/mmc/host/omap_hsmmc.c |   24 +++---------------------
>  include/linux/mmc/host.h      |    6 ------
>  3 files changed, 3 insertions(+), 32 deletions(-)
>
> --

Applied both patches (the later version of patch 1). Also I took the
liberty to update the commit messages.

If you care about my comments for patch1, let's deal with that as new
separate "cleanup-patch".

Thanks!

Kind regards
Uffe