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-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
