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@xxxxxxxxxx> wrote:
> On 26 March 2015 at 02:18, NeilBrown <neilb@xxxxxxx> wrote:
>> On Thu, 26 Mar 2015 08:43:37 +1100 NeilBrown <neil@xxxxxxxxxx> 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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/