| Message ID | 1398164594-29169-4-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 22, 2014 at 02:03:14PM +0300, Peter Ujfalusi wrote: > Move the pm_runtime_enable/disable to dai driver probe/remove callbacks from > module probe/remove callbacks. > With this change we can remove the platform driver's remove function since > it became NULL. This does mean that if the device gets enumerated but isn't used in the system it won't go to runtime idle since the DAI level probe is only called when we're building a card. That doesn't seem like it's a win. How about creating a devm_pm_runtime_enable() instead?
On 04/22/2014 02:47 PM, Mark Brown wrote: > On Tue, Apr 22, 2014 at 02:03:14PM +0300, Peter Ujfalusi wrote: >> Move the pm_runtime_enable/disable to dai driver probe/remove callbacks from >> module probe/remove callbacks. >> With this change we can remove the platform driver's remove function since >> it became NULL. > > This does mean that if the device gets enumerated but isn't used in the > system it won't go to runtime idle since the DAI level probe is only > called when we're building a card. If the given mcasp is not used as part of a card, it should not have been probed in the first place (at least with DT boot we can control this). If the driver is probed for a mcasp instance and it is not part of any card than it can be left disabled IMHO no need for runtime pm to take care of it. I might missed something related to runtime pm, but this is my understanding. >That doesn't seem like it's a win. > How about creating a devm_pm_runtime_enable() instead? I was also considered to do this but ended up moving the pm_runtime_enable/disable instead.
On Tue, Apr 22, 2014 at 03:20:33PM +0300, Peter Ujfalusi wrote: > On 04/22/2014 02:47 PM, Mark Brown wrote: > > This does mean that if the device gets enumerated but isn't used in the > > system it won't go to runtime idle since the DAI level probe is only > > called when we're building a card. > If the given mcasp is not used as part of a card, it should not have been > probed in the first place (at least with DT boot we can control this). If the > driver is probed for a mcasp instance and it is not part of any card than it > can be left disabled IMHO no need for runtime pm to take care of it. > I might missed something related to runtime pm, but this is my understanding. Sure, but you then also have the cases where for whatever reason the card doesn't probe (some other driver missing for example). Probably almost all the time it's not going to make a practical difference but it just feels like a step in the wrong direction for a minimal gain.
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 14058dc6eaf8..c858c9ccf774 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -778,6 +778,8 @@ static int davinci_mcasp_dai_probe(struct snd_soc_dai *dai) { struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai); + pm_runtime_enable(mcasp->dev); + if (mcasp->version == MCASP_VERSION_4) { /* Using dmaengine PCM */ dai->playback_dma_data = @@ -793,6 +795,15 @@ static int davinci_mcasp_dai_probe(struct snd_soc_dai *dai) return 0; } +static int davinci_mcasp_dai_remove(struct snd_soc_dai *dai) +{ + struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai); + + pm_runtime_disable(mcasp->dev); + + return 0; +} + #ifdef CONFIG_PM_SLEEP static int davinci_mcasp_suspend(struct snd_soc_dai *dai) { @@ -847,6 +858,7 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { { .name = "davinci-mcasp.0", .probe = davinci_mcasp_dai_probe, + .remove = davinci_mcasp_dai_remove, .suspend = davinci_mcasp_suspend, .resume = davinci_mcasp_resume, .playback = { @@ -867,6 +879,7 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { { .name = "davinci-mcasp.1", .probe = davinci_mcasp_dai_probe, + .remove = davinci_mcasp_dai_remove, .playback = { .channels_min = 1, .channels_max = 384, @@ -1122,19 +1135,10 @@ static int davinci_mcasp_probe(struct platform_device *pdev) return -EBUSY; } - pm_runtime_enable(&pdev->dev); - - ret = pm_runtime_get_sync(&pdev->dev); - if (IS_ERR_VALUE(ret)) { - dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n"); - return ret; - } - mcasp->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem)); if (!mcasp->base) { dev_err(&pdev->dev, "ioremap failed\n"); - ret = -ENOMEM; - goto err; + return -ENOMEM; } mcasp->op_mode = pdata->op_mode; @@ -1220,7 +1224,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev) &davinci_mcasp_dai[pdata->op_mode], 1); if (ret != 0) - goto err; + return ret; switch (mcasp->version) { case MCASP_VERSION_1: @@ -1238,30 +1242,11 @@ static int davinci_mcasp_probe(struct platform_device *pdev) break; } - if (ret) { - dev_err(&pdev->dev, "register PCM failed: %d\n", ret); - goto err; - } - - return 0; - -err: - pm_runtime_put_sync(&pdev->dev); - pm_runtime_disable(&pdev->dev); return ret; } -static int davinci_mcasp_remove(struct platform_device *pdev) -{ - pm_runtime_put_sync(&pdev->dev); - pm_runtime_disable(&pdev->dev); - - return 0; -} - static struct platform_driver davinci_mcasp_driver = { .probe = davinci_mcasp_probe, - .remove = davinci_mcasp_remove, .driver = { .name = "davinci-mcasp", .owner = THIS_MODULE,
Move the pm_runtime_enable/disable to dai driver probe/remove callbacks from module probe/remove callbacks. With this change we can remove the platform driver's remove function since it became NULL. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- sound/soc/davinci/davinci-mcasp.c | 45 +++++++++++++-------------------------- 1 file changed, 15 insertions(+), 30 deletions(-)