Re: [PATCH] drm: exynos: Add runtime PM support to MIC driver

[Inki Dae <inki.dae@samsung.com>]

2017년 01월 11일 10:39에 Chanwoo Choi 이(가) 쓴 글:
> Hi Marek,
> 
> On 2017년 01월 10일 21:57, Marek Szyprowski wrote:
>> This patch adds pm_runtime_get/put calls to notify device core when MIC
>> device is really in use. This is needed to let power domain with this
>> device to be turned off when display is turned off.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_mic.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
>> index a0def0be6d65..f643c380cb9a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/of_graph.h>
>>  #include <linux/clk.h>
>>  #include <linux/component.h>
>> +#include <linux/pm_runtime.h>
>>  #include <drm/drmP.h>
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/regmap.h>
>> @@ -323,6 +324,7 @@ static void mic_post_disable(struct drm_bridge *bridge)
>>  	for (i = NUM_CLKS - 1; i > -1; i--)
>>  		clk_disable_unprepare(mic->clks[i]);
>>  
>> +	pm_runtime_put(mic->dev);
>>  	mic->enabled = 0;
> 
> It is minor comment.
> 
> How about calling the pm_runtime_put() after 'mic->enabled = 0'?

This means it changes device status to 'disable' even through real hardware status is still 'enable'. so it would be better to update device status after making sure to disable real hardware.

Than that, how about adding dev_pm_ops callbacks - runtime_suspend and runtime_resume - and moving clock contol codes into the callback functions?

Thanks.

> I think that you better to call the runtime_pm funtcion after completing
> the handle of data of mic device driver. Because this patch just notifies
> the status.
> 
>>  
>>  already_disabled:
>> @@ -338,6 +340,8 @@ static void mic_pre_enable(struct drm_bridge *bridge)
>>  	if (mic->enabled)
>>  		goto already_enabled;
>>  
>> +	pm_runtime_get_sync(mic->dev);
>> +
>>  	for (i = 0; i < NUM_CLKS; i++) {
>>  		ret = clk_prepare_enable(mic->clks[i]);
>>  		if (ret < 0) {
>> @@ -473,8 +477,18 @@ static int exynos_mic_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, mic);
>>  
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = component_add(dev, &exynos_mic_component_ops);
>> +	if (ret)
>> +		goto err_pm;
>> +
>>  	DRM_DEBUG_KMS("MIC has been probed\n");
>> -	return component_add(dev, &exynos_mic_component_ops);
>> +
>> +	return 0;
>> +
>> +err_pm:
>> +	pm_runtime_disable(dev);
>>  
>>  err:
>>  	return ret;
>> @@ -483,6 +497,7 @@ static int exynos_mic_probe(struct platform_device *pdev)
>>  static int exynos_mic_remove(struct platform_device *pdev)
>>  {
>>  	component_del(&pdev->dev, &exynos_mic_component_ops);
>> +	pm_runtime_disable(&pdev->dev);
>>  	return 0;
>>  }
>>  
>>
> 
> If this patch just notifies the status(enabled or disabled)
> of mic device with pm runtime interface, looks good to me.
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel