On Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@xxxxxx> wrote:
> Balaji T K <balajitk@xxxxxx> writes:
>
>> @@ -1880,18 +1873,12 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>
>> mmc->caps |= MMC_CAP_DISABLE;
>>
>> - if (clk_enable(host->iclk) != 0) {
>> - clk_put(host->iclk);
>> - clk_put(host->fclk);
>> - goto err1;
>> - }
>> -
>> - if (mmc_host_enable(host->mmc) != 0) {
>> - clk_disable(host->iclk);
>> - clk_put(host->iclk);
>> - clk_put(host->fclk);
>> - goto err1;
>> - }
>> + pm_runtime_enable(host->dev);
>> + pm_runtime_allow(host->dev);
>> + pm_runtime_get_sync(host->dev);
>> + pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
>> + pm_runtime_use_autosuspend(host->dev);
>> + pm_suspend_ignore_children(host->dev, 1);
>
> Why is ignore_children needed for this device? Is this device the
> parent of other devices? If it is, why should it ignore it's
> children?
>
No, I will remove. Added it for testing only.
>> if (cpu_is_omap2430()) {
>> host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
>> @@ -2018,6 +2005,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>> }
>>
>> omap_hsmmc_debugfs(mmc);
>> + pm_runtime_mark_last_busy(host->dev);
>> + pm_runtime_put_autosuspend(host->dev);
>>
>> return 0;
>>
>> @@ -2033,8 +2022,8 @@ err_reg:
>> err_irq_cd_init:
>> free_irq(host->irq, host);
>> err_irq:
>> - mmc_host_disable(host->mmc);
>> - clk_disable(host->iclk);
>> + pm_runtime_mark_last_busy(host->dev);
>> + pm_runtime_put_autosuspend(host->dev);
>> clk_put(host->fclk);
>> clk_put(host->iclk);
>> if (host->got_dbclk) {
>> @@ -2058,7 +2047,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>> struct resource *res;
>>
>> if (host) {
>> - mmc_host_enable(host->mmc);
>> + pm_runtime_get_sync(host->dev);
>> mmc_remove_host(host->mmc);
>> if (host->use_reg)
>> omap_hsmmc_reg_put(host);
>> @@ -2069,8 +2058,9 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>> free_irq(mmc_slot(host).card_detect_irq, host);
>> flush_work_sync(&host->mmc_carddetect_work);
>>
>> - mmc_host_disable(host->mmc);
>> - clk_disable(host->iclk);
>> + pm_runtime_put_sync(host->dev);
>> + pm_runtime_forbid(host->dev);
>
> Why?
>
Added for balancing pm_runtime_allow added in _probe.
But forbid also resume the device on remove.
Should this be removed, keeping _allow in _probe ?
>> + pm_runtime_disable(host->dev);
>> clk_put(host->fclk);
>> clk_put(host->iclk);
>> if (host->got_dbclk) {
>> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev)
>> return 0;
>>
>> if (host) {
>> + /* FIXME: TODO move get_sync to proper dev_pm_ops function */
>
> what does this mean?
get_sync is needed to enable clock before accessing the registers but
the discusssion @
http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg50819.html
suggested to move runtime get_sync calls to .prepare
Haven't tried it yet.
>
>> + pm_runtime_get_sync(host->dev);
>> host->suspended = 1;
>> if (host->pdata->suspend) {
>> ret = host->pdata->suspend(&pdev->dev,
>> @@ -2116,13 +2108,11 @@ static int omap_hsmmc_suspend(struct device *dev)
>> }
>> cancel_work_sync(&host->mmc_carddetect_work);
>> ret = mmc_suspend_host(host->mmc);
>> - mmc_host_enable(host->mmc);
>> +
>> if (ret == 0) {
>> omap_hsmmc_disable_irq(host);
>> 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 {
>> @@ -2134,8 +2124,9 @@ static int omap_hsmmc_suspend(struct device *dev)
>> dev_dbg(mmc_dev(host->mmc),
>> "Unmask interrupt failed\n");
>> }
>> - mmc_host_disable(host->mmc);
>> }
>> + /* FIXME: TODO move put_sync to proper dev_pm_ops function */
>
> ditto
>
>> + pm_runtime_put_sync(host->dev);
>>
>> }
>> return ret;
>> @@ -2152,14 +2143,8 @@ static int omap_hsmmc_resume(struct device *dev)
>> return 0;
>>
>> if (host) {
>> - ret = clk_enable(host->iclk);
>> - if (ret)
>> - goto clk_en_err;
>> -
>> - if (mmc_host_enable(host->mmc) != 0) {
>> - clk_disable(host->iclk);
>> - goto clk_en_err;
>> - }
>> + /* FIXME: TODO move put_sync to proper dev_pm_ops function */
>
> comment says put_sync, code says get_sync, but again, comment doesn't
> make any sense.
>
>> + pm_runtime_get_sync(host->dev);
>>
>> if (host->got_dbclk)
>> clk_enable(host->dbclk);
>> @@ -2179,14 +2164,14 @@ static int omap_hsmmc_resume(struct device *dev)
>> ret = mmc_resume_host(host->mmc);
>> if (ret == 0)
>> host->suspended = 0;
>> +
>> + /* FIXME: TODO move put_sync to proper dev_pm_ops function */
>> + pm_runtime_mark_last_busy(host->dev);
>> + pm_runtime_put_autosuspend(host->dev);
>> }
>>
>> return ret;
>>
>> -clk_en_err:
>> - dev_dbg(mmc_dev(host->mmc),
>> - "Failed to enable MMC clocks during resume\n");
>> - return ret;
>> }
>>
>> #else
>> @@ -2194,9 +2179,35 @@ clk_en_err:
>> #define omap_hsmmc_resume NULL
>> #endif
>>
>> +static int omap_hsmmc_runtime_suspend(struct device *dev)
>> +{
>> + struct omap_hsmmc_host *host;
>> +
>> +
>
> extra blank line
Removed
>
>> + host = platform_get_drvdata(to_platform_device(dev));
>> + omap_hsmmc_context_save(host);
>> + dev_dbg(mmc_dev(host->mmc), "disabled\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int omap_hsmmc_runtime_resume(struct device *dev)
>> +{
>> + struct omap_hsmmc_host *host;
>> +
>> +
>
> extra blank line
Removed
--
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
