[PATCH 0/2] drm/exynos: clean up bridge binding

[PATCH 0/2] drm/exynos: clean up bridge binding

[Inki Dae]

This patch series cleans up bridge binding of DSI and MIC driver.

It doesn't have to call of_drm_find_bridge function every time
bind callback of DSI driver is called so moves this function call
into probe function and for this support it adds a bridge
at probe function of MIC driver instead of adding it at bind callback.

Inki Dae (2):
  drm/exynos: dsi: move of_drm_find_bridge call into probe
  drm/exynos: mic: add a bridge at probe

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 23 ++++++++++++-----------
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 17 ++++++++++-------
 2 files changed, 22 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH 1/2] drm/exynos: dsi: move of_drm_find_bridge call into probe

[Inki Dae]

This patch moves of_drm_find_bridge call into probe.

It doesn't need to call of_drm_find_bridge function every time
bind callback is called. It's enough to call this funcation
at probe one time.

Suggested-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index b6a46d9..2412b23 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1661,7 +1661,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 	struct drm_encoder *encoder = dev_get_drvdata(dev);
 	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
 	struct drm_device *drm_dev = data;
-	struct drm_bridge *bridge;
 	int ret;
 
 	ret = exynos_drm_crtc_get_pipe_from_type(drm_dev,
@@ -1685,12 +1684,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	if (dsi->bridge_node) {
-		bridge = of_drm_find_bridge(dsi->bridge_node);
-		if (bridge)
-			drm_bridge_attach(encoder, bridge, NULL);
-	}
-
 	return mipi_dsi_host_register(&dsi->dsi_host);
 }
 
@@ -1798,6 +1791,18 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dsi->encoder);
 
+	if (dsi->bridge_node) {
+		struct drm_bridge *bridge;
+
+		bridge = of_drm_find_bridge(dsi->bridge_node);
+		if (!bridge)
+			return -EPROBE_DEFER;
+
+		of_node_put(dsi->bridge_node);
+		drm_bridge_attach(&dsi->encoder, bridge, NULL);
+	}
+
+
 	pm_runtime_enable(dev);
 
 	return component_add(dev, &exynos_dsi_component_ops);
@@ -1805,10 +1810,6 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 static int exynos_dsi_remove(struct platform_device *pdev)
 {
-	struct exynos_dsi *dsi = platform_get_drvdata(pdev);
-
-	of_node_put(dsi->bridge_node);
-
 	pm_runtime_disable(&pdev->dev);
 
 	component_del(&pdev->dev, &exynos_dsi_component_ops);
-- 
1.9.1

[PATCH 2/2] drm/exynos: mic: add a bridge at probe

[Inki Dae]

This patch moves drm_bridge_add call into probe.

This change is required by DSI driver so that
the brige can be bound at DSI driver's probe.

Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index e457205..5ea6e3d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -340,16 +340,10 @@ static int exynos_mic_bind(struct device *dev, struct device *master,
 			   void *data)
 {
 	struct exynos_mic *mic = dev_get_drvdata(dev);
-	int ret;
 
-	mic->bridge.funcs = &mic_bridge_funcs;
-	mic->bridge.of_node = dev->of_node;
 	mic->bridge.driver_private = mic;
-	ret = drm_bridge_add(&mic->bridge);
-	if (ret)
-		DRM_ERROR("mic: Failed to add MIC to the global bridge list\n");
 
-	return ret;
+	return 0;
 }
 
 static void exynos_mic_unbind(struct device *dev, struct device *master,
@@ -461,6 +455,15 @@ static int exynos_mic_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mic);
 
+	mic->bridge.funcs = &mic_bridge_funcs;
+	mic->bridge.of_node = dev->of_node;
+
+	ret = drm_bridge_add(&mic->bridge);
+	if (ret) {
+		DRM_ERROR("mic: Failed to add MIC to the global bridge list\n");
+		return ret;
+	}
+
 	pm_runtime_enable(dev);
 
 	ret = component_add(dev, &exynos_mic_component_ops);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: dsi: move of_drm_find_bridge call into probe

[Andrzej Hajda]

On 03.07.2017 09:27, Inki Dae wrote:
> This patch moves of_drm_find_bridge call into probe.
>
> It doesn't need to call of_drm_find_bridge function every time
> bind callback is called. It's enough to call this funcation
> at probe one time.
>
> Suggested-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index b6a46d9..2412b23 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1661,7 +1661,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>  	struct drm_encoder *encoder = dev_get_drvdata(dev);
>  	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
>  	struct drm_device *drm_dev = data;
> -	struct drm_bridge *bridge;
>  	int ret;
>  
>  	ret = exynos_drm_crtc_get_pipe_from_type(drm_dev,
> @@ -1685,12 +1684,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>  		return ret;
>  	}
>  
> -	if (dsi->bridge_node) {
> -		bridge = of_drm_find_bridge(dsi->bridge_node);
> -		if (bridge)
> -			drm_bridge_attach(encoder, bridge, NULL);
> -	}
> -
>  	return mipi_dsi_host_register(&dsi->dsi_host);
>  }
>  
> @@ -1798,6 +1791,18 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, &dsi->encoder);
>  
> +	if (dsi->bridge_node) {
> +		struct drm_bridge *bridge;
> +
> +		bridge = of_drm_find_bridge(dsi->bridge_node);
> +		if (!bridge)
> +			return -EPROBE_DEFER;
> +
> +		of_node_put(dsi->bridge_node);
> +		drm_bridge_attach(&dsi->encoder, bridge, NULL);
> +	}
> +
> +

One of benefits of componentized drivers is that they do not need to use
probe deferall to wait for other components. There is guarantee that in
bind callback all components are already probed.
This patch looks like step back - it reintroduces probe deferall despite
of existence of better mechanism.
Another issue is that now drm_bridge_attach is called before drm device
creation, and before encoder is registered, even if it works for now, it
does not seem proper, and it can beat us later.
For sure bridge->dev is unitialized, and it can cause warnings.

If you want to put bridge code together it should be put rather into
bind callback.

Regards
Andrzej

>  	pm_runtime_enable(dev);
>  
>  	return component_add(dev, &exynos_dsi_component_ops);
> @@ -1805,10 +1810,6 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  
>  static int exynos_dsi_remove(struct platform_device *pdev)
>  {
> -	struct exynos_dsi *dsi = platform_get_drvdata(pdev);
> -
> -	of_node_put(dsi->bridge_node);
> -
>  	pm_runtime_disable(&pdev->dev);
>  
>  	component_del(&pdev->dev, &exynos_dsi_component_ops);

Re: [PATCH 2/2] drm/exynos: mic: add a bridge at probe

[Andrzej Hajda]

On 03.07.2017 09:27, Inki Dae wrote:
> This patch moves drm_bridge_add call into probe.
>
> This change is required by DSI driver so that
> the brige can be bound at DSI driver's probe.
>
> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

Re: [PATCH 1/2] drm/exynos: dsi: move of_drm_find_bridge call into probe

[Krzysztof Kozlowski]

On Mon, Jul 3, 2017 at 9:27 AM, Inki Dae <inki.dae@samsung.com> wrote:
> This patch moves of_drm_find_bridge call into probe.
>
> It doesn't need to call of_drm_find_bridge function every time
> bind callback is called. It's enough to call this funcation
> at probe one time.
>
> Suggested-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>

Hi Inki,

Minor note: you're the author of this patch so you cannot suggest it. :)

Best regards,
Krzysztof

Re: [PATCH 1/2] drm/exynos: dsi: move of_drm_find_bridge call into probe

[Inki Dae]

2017년 07월 03일 19:34에 Andrzej Hajda 이(가) 쓴 글:
> On 03.07.2017 09:27, Inki Dae wrote:
>> This patch moves of_drm_find_bridge call into probe.
>>
>> It doesn't need to call of_drm_find_bridge function every time
>> bind callback is called. It's enough to call this funcation
>> at probe one time.
>>
>> Suggested-by: Inki Dae <inki.dae@samsung.com>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> index b6a46d9..2412b23 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> @@ -1661,7 +1661,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>>  	struct drm_encoder *encoder = dev_get_drvdata(dev);
>>  	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
>>  	struct drm_device *drm_dev = data;
>> -	struct drm_bridge *bridge;
>>  	int ret;
>>  
>>  	ret = exynos_drm_crtc_get_pipe_from_type(drm_dev,
>> @@ -1685,12 +1684,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>>  		return ret;
>>  	}
>>  
>> -	if (dsi->bridge_node) {
>> -		bridge = of_drm_find_bridge(dsi->bridge_node);
>> -		if (bridge)
>> -			drm_bridge_attach(encoder, bridge, NULL);
>> -	}
>> -
>>  	return mipi_dsi_host_register(&dsi->dsi_host);
>>  }
>>  
>> @@ -1798,6 +1791,18 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, &dsi->encoder);
>>  
>> +	if (dsi->bridge_node) {
>> +		struct drm_bridge *bridge;
>> +
>> +		bridge = of_drm_find_bridge(dsi->bridge_node);
>> +		if (!bridge)
>> +			return -EPROBE_DEFER;
>> +
>> +		of_node_put(dsi->bridge_node);
>> +		drm_bridge_attach(&dsi->encoder, bridge, NULL);
>> +	}
>> +
>> +
> 
> One of benefits of componentized drivers is that they do not need to use
> probe deferall to wait for other components. There is guarantee that in
> bind callback all components are already probed.
> This patch looks like step back - it reintroduces probe deferall despite
> of existence of better mechanism.

Agree. This patch avoids of_drm_find_bridge function is called repeately and also this makes probe to be deferred.
I will skip this patch.

> Another issue is that now drm_bridge_attach is called before drm device
> creation, and before encoder is registered, even if it works for now, it
> does not seem proper, and it can beat us later.
> For sure bridge->dev is unitialized, and it can cause warnings.
> 
> If you want to put bridge code together it should be put rather into
> bind callback.

Oops, sorry. as I commented[1] at the original patch from Shuah, drm_bridge_attach should be keepped in bind callback.
This is my mistake.

[1] https://patchwork.kernel.org/patch/9808497/


Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
>>  	pm_runtime_enable(dev);
>>  
>>  	return component_add(dev, &exynos_dsi_component_ops);
>> @@ -1805,10 +1810,6 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>  
>>  static int exynos_dsi_remove(struct platform_device *pdev)
>>  {
>> -	struct exynos_dsi *dsi = platform_get_drvdata(pdev);
>> -
>> -	of_node_put(dsi->bridge_node);
>> -
>>  	pm_runtime_disable(&pdev->dev);
>>  
>>  	component_del(&pdev->dev, &exynos_dsi_component_ops);
> 
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos: dsi: move of_drm_find_bridge call into probe

[Inki Dae]

Hi Krzysztof,

2017년 07월 04일 04:05에 Krzysztof Kozlowski 이(가) 쓴 글:
> On Mon, Jul 3, 2017 at 9:27 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> This patch moves of_drm_find_bridge call into probe.
>>
>> It doesn't need to call of_drm_find_bridge function every time
>> bind callback is called. It's enough to call this funcation
>> at probe one time.
>>
>> Suggested-by: Inki Dae <inki.dae@samsung.com>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> 
> Hi Inki,
> 
> Minor note: you're the author of this patch so you cannot suggest it. :)
> 

I just wanted to leave the vestige of the original patch[1] posted by Shuah with my suggestion here.
But yes, looks strange. :)

[1] https://patchwork.kernel.org/patch/9808497/

Thanks,
Inki Dae

> Best regards,
> Krzysztof
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: mic: add a bridge at probe

[Hoegeun Kwon]

On 07/03/2017 04:27 PM, Inki Dae wrote:
> This patch moves drm_bridge_add call into probe.
>
> This change is required by DSI driver so that
> the brige can be bound at DSI driver's probe.
>
> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>

Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>

Best regards,
Hoegeun

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel