Re: [PATCH/RFC 4/4] OMAP2/3 MMC: initial conversion to runtime PM

[Kevin Hilman <khilman@deeprootsystems.com>]

"Madhusudhan" <madhu.cr@ti.com> writes:

> <snip>
>
>>  err_irq:
>>  	mmc_host_disable(host->mmc);
>> -	clk_disable(host->iclk);
>> +	pm_runtime_suspend(host->dev);
>
> Why not pm_runtime_put_sync() here? It can replace the calls:
> mmc_host_disable(host->mmc);
> clk_disable(host->iclk);

Not knowing a ton about this driver, I'd rather keep the
mmc_host_disable() since it matches the mmc_host_enable() earlier.

>>  	clk_put(host->fclk);
>>  	clk_put(host->iclk);
>> +
>>  	if (host->got_dbclk) {
>>  		clk_disable(host->dbclk);
>>  		clk_put(host->dbclk);
>> @@ -2216,7 +2164,8 @@ static int omap_hsmmc_remove(struct platform_device
>> *pdev)
>>  		flush_scheduled_work();
>> 
>>  		mmc_host_disable(host->mmc);
>> -		clk_disable(host->iclk);
>> +		pm_runtime_suspend(host->dev);
>> +
>
> Ditto
>
>
>>  		clk_put(host->fclk);
>>  		clk_put(host->iclk);
>>  		if (host->got_dbclk) {
>> @@ -2272,7 +2221,7 @@ static int omap_hsmmc_suspend(struct device *dev)
>>  			OMAP_HSMMC_WRITE(host->base, HCTL,
>>  				OMAP_HSMMC_READ(host->base, HCTL) & ~SDBP);
>>  			mmc_host_disable(host->mmc);
>> -			clk_disable(host->iclk);
>> +
>>  			if (host->got_dbclk)
>>  				clk_disable(host->dbclk);
>>  		} else {
>> @@ -2287,6 +2236,11 @@ static int omap_hsmmc_suspend(struct device *dev)
>>  			mmc_host_disable(host->mmc);
>>  		}
>> 
>> +		/*
>> +		 * HACK: "extra" put to compensate for DPM core keeping
>> +		 *       runtime PM disabled.  -- khilman
>> +		 */
>> +		pm_runtime_put_sync(host->dev);
>>  	}
>>  	return ret;
>>  }
>> @@ -2302,12 +2256,14 @@ static int omap_hsmmc_resume(struct device *dev)
>>  		return 0;
>> 
>>  	if (host) {
>> -		ret = clk_enable(host->iclk);
>> -		if (ret)
>> -			goto clk_en_err;
>> +		/*
>> +		 * HACK: "extra" get to compensate for DPM core keeping
>> +		 *       runtime PM disabled.  -- khilman
>> +		 */
>> +		pm_runtime_get_sync(host->dev);
>> 
>>  		if (mmc_host_enable(host->mmc) != 0) {
>> -			clk_disable(host->iclk);
>> +			pm_runtime_suspend(host->dev);
>>  			goto clk_en_err;
>>  		}
>> 
>> @@ -2346,9 +2302,37 @@ clk_en_err:
>>  #define omap_hsmmc_resume		NULL
>>  #endif
>> 
>> +/* called just before device is disabled */
>> +static int omap_hsmmc_runtime_suspend(struct device *dev)
>> +{
>> +	struct omap_hsmmc_host *host;
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	host = platform_get_drvdata(to_platform_device(dev));
>> +	omap_hsmmc_context_save(host);
>
> The context_save fn is now empty. How does it help here?

It doesn't add anything, but I put it there to show that this is the
only place that context save would be needed, and as an example to other
drivers.

Kevin

>> +
>> +	return 0;
>> +}
>> +
>> +/* called after device is (re)enabled, ONLY if context was lost */
>> +static int omap_hsmmc_runtime_resume(struct device *dev)
>> +{
>> +	struct omap_hsmmc_host *host;
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	host = platform_get_drvdata(to_platform_device(dev));
>> +	omap_hsmmc_context_restore(host);
>> +
>> +	return 0;
>> +}
>> +
>>  static struct dev_pm_ops omap_hsmmc_dev_pm_ops = {
>>  	.suspend	= omap_hsmmc_suspend,
>>  	.resume		= omap_hsmmc_resume,
>> +	.runtime_suspend = omap_hsmmc_runtime_suspend,
>> +	.runtime_resume = omap_hsmmc_runtime_resume,
>>  };
>> 
>>  static struct platform_driver omap_hsmmc_driver = {
>> --
>> 1.6.6
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html