diff mbox

drm: exynos: Add runtime PM support to MIC driver

Message ID 1484053067-31665-1-git-send-email-m.szyprowski@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Marek Szyprowski Jan. 10, 2017, 12:57 p.m. UTC
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(-)

Comments

Chanwoo Choi Jan. 11, 2017, 1:39 a.m. UTC | #1
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'?
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>
Inki Dae Jan. 11, 2017, 5:44 a.m. UTC | #2
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>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 
 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;
 }