[v3,1/3] drm/exynos: dp: add of_graph dt binding support for panel
diff mbox

Message ID 1449135010-8111-2-git-send-email-inki.dae@samsung.com
State New
Headers show

Commit Message

Inki Dae Dec. 3, 2015, 9:30 a.m. UTC
This patch adds of_graph dt binding support for panel device
and also keeps the backward compatibility.

i.e.,
The dts file for Exynos5800 based peach pi board
has a panel property so we need to keep the backward compatibility.

Changelog v3:
- bind only one of two nodes outbound - panel or bridge.

Changelog v2:
- return -EINVAL if getting a port node failed.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Javier Martinez Canillas Dec. 3, 2015, 1:55 p.m. UTC | #1
Hello Inki,

I found that v2 of this patch is alredy in your exynos-drm for-next branch so
so I had to revert it in linux-next to apply this one to test. You shouldn't
push patches that were still not reviewed (specially the ones that weren't
tested like this one) to your branch that gets pulled by -next. The idea of
-next is to have some test coverage but it should be as stable as possible.

On 12/03/2015 06:30 AM, Inki Dae wrote:
> This patch adds of_graph dt binding support for panel device
> and also keeps the backward compatibility.
> 
> i.e.,
> The dts file for Exynos5800 based peach pi board
> has a panel property so we need to keep the backward compatibility.
> 
> Changelog v3:
> - bind only one of two nodes outbound - panel or bridge.
>

This patch fixes one of the comments I had for v2 but I've another
comment below.
 
> Changelog v2:
> - return -EINVAL if getting a port node failed.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_dp_core.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index 94f02a0..60260a0 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -1392,7 +1392,7 @@ static const struct component_ops exynos_dp_ops = {
>  static int exynos_dp_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *panel_node, *bridge_node, *endpoint;
> +	struct device_node *panel_node = NULL, *bridge_node, *endpoint = NULL;
>  	struct exynos_dp_device *dp;
>  	int ret;
>  
> @@ -1403,14 +1403,32 @@ static int exynos_dp_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, dp);
>  
> +	/* This is for the backward compatibility. */
>  	panel_node = of_parse_phandle(dev->of_node, "panel", 0);
>  	if (panel_node) {
>  		dp->panel = of_drm_find_panel(panel_node);
>  		of_node_put(panel_node);
>  		if (!dp->panel)
>  			return -EPROBE_DEFER;
> +	} else {
> +		endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> +		if (endpoint) {
> +			panel_node = of_graph_get_remote_port_parent(endpoint);

Here is asssumed that the endpoint will be a panel but it could be that there
is no "panel" phandle but the port is for a bridge chip (i.e: Peach Pit) so
this assumption seems fragile to me.

That's what I meant when I said "Assuming you can make a distinction if the
endpoint is a panel or a bridge" in the other thread.

> +			if (panel_node) {
> +				dp->panel = of_drm_find_panel(panel_node);
> +				of_node_put(panel_node);
> +				if (!dp->panel)
> +					return -EPROBE_DEFER;
> +			} else {
> +				DRM_ERROR("no port node for panel device.\n");
> +				return -EINVAL;
> +			}
> +		}
>  	}
>  
> +	if (endpoint)
> +		goto out;
> +

Ok, so IIUC what's done here is to test if the panel lookup failed, then the
endpoint is looked up again but this time a call to of_drm_find_bridge() is
made instead of a call to of_drm_find_panel(). I wonder if there is a better
way to do this...

In any case then I think you should test if (panel_node) instead of endpoint.

>  	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>  	if (endpoint) {
>  		bridge_node = of_graph_get_remote_port_parent(endpoint);
> @@ -1423,6 +1441,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
>  			return -EPROBE_DEFER;
>  	}
>  
> +out:
>  	pm_runtime_enable(dev);
>  
>  	ret = component_add(&pdev->dev, &exynos_dp_ops);
> 

I can't think of a better way to lookup either a panel or a bridge though
and I'm not that familiar with DRM so with the correct check, the patch
looks good to me.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Also on an Exynos5800 Peach Pi with the DTS patch I shared, the display
is working correctly and also I tested without the DTS patch to make
sure that it does not cause a regression for older DTBs.

Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Inki Dae Dec. 4, 2015, 9 a.m. UTC | #2
Hi Javier,

2015? 12? 03? 22:55? Javier Martinez Canillas ?(?) ? ?:
> Hello Inki,
> 
> I found that v2 of this patch is alredy in your exynos-drm for-next branch so
> so I had to revert it in linux-next to apply this one to test. You shouldn't
> push patches that were still not reviewed (specially the ones that weren't
> tested like this one) to your branch that gets pulled by -next. The idea of
> -next is to have some test coverage but it should be as stable as possible.

exynos-drm/for-next branch is not really for-next branch. This branch is used
only for integration test. As you know, there are many exynos maintainers
and they have their own repository. So we would need to test the integration.
For this, exynos-drm/for-next is merged to linux-next not Dave's tree.
Only exynos-drm-next branch will be merged to Dave's tree so only reviewed and
tested patches will be merged to exynos-drm-next.

> 
> On 12/03/2015 06:30 AM, Inki Dae wrote:
>> This patch adds of_graph dt binding support for panel device
>> and also keeps the backward compatibility.
>>
>> i.e.,
>> The dts file for Exynos5800 based peach pi board
>> has a panel property so we need to keep the backward compatibility.
>>
>> Changelog v3:
>> - bind only one of two nodes outbound - panel or bridge.
>>
> 
> This patch fixes one of the comments I had for v2 but I've another
> comment below.
>  
>> Changelog v2:
>> - return -EINVAL if getting a port node failed.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_dp_core.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> index 94f02a0..60260a0 100644
>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> @@ -1392,7 +1392,7 @@ static const struct component_ops exynos_dp_ops = {
>>  static int exynos_dp_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> -	struct device_node *panel_node, *bridge_node, *endpoint;
>> +	struct device_node *panel_node = NULL, *bridge_node, *endpoint = NULL;
>>  	struct exynos_dp_device *dp;
>>  	int ret;
>>  
>> @@ -1403,14 +1403,32 @@ static int exynos_dp_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, dp);
>>  
>> +	/* This is for the backward compatibility. */
>>  	panel_node = of_parse_phandle(dev->of_node, "panel", 0);
>>  	if (panel_node) {
>>  		dp->panel = of_drm_find_panel(panel_node);
>>  		of_node_put(panel_node);
>>  		if (!dp->panel)
>>  			return -EPROBE_DEFER;
>> +	} else {
>> +		endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>> +		if (endpoint) {
>> +			panel_node = of_graph_get_remote_port_parent(endpoint);
> 
> Here is asssumed that the endpoint will be a panel but it could be that there
> is no "panel" phandle but the port is for a bridge chip (i.e: Peach Pit) so
> this assumption seems fragile to me.
> 
> That's what I meant when I said "Assuming you can make a distinction if the
> endpoint is a panel or a bridge" in the other thread.
> 
>> +			if (panel_node) {
>> +				dp->panel = of_drm_find_panel(panel_node);
>> +				of_node_put(panel_node);
>> +				if (!dp->panel)
>> +					return -EPROBE_DEFER;
>> +			} else {
>> +				DRM_ERROR("no port node for panel device.\n");
>> +				return -EINVAL;
>> +			}
>> +		}
>>  	}
>>  
>> +	if (endpoint)
>> +		goto out;
>> +
> 
> Ok, so IIUC what's done here is to test if the panel lookup failed, then the
> endpoint is looked up again but this time a call to of_drm_find_bridge() is
> made instead of a call to of_drm_find_panel(). I wonder if there is a better
> way to do this...
> 
> In any case then I think you should test if (panel_node) instead of endpoint.
> 
>>  	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>>  	if (endpoint) {
>>  		bridge_node = of_graph_get_remote_port_parent(endpoint);
>> @@ -1423,6 +1441,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
>>  			return -EPROBE_DEFER;
>>  	}
>>  
>> +out:
>>  	pm_runtime_enable(dev);
>>  
>>  	ret = component_add(&pdev->dev, &exynos_dp_ops);
>>
> 
> I can't think of a better way to lookup either a panel or a bridge though
> and I'm not that familiar with DRM so with the correct check, the patch
> looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> Also on an Exynos5800 Peach Pi with the DTS patch I shared, the display
> is working correctly and also I tested without the DTS patch to make
> sure that it does not cause a regression for older DTBs.
> 
> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Thanks,
Inki Dae

> 
> Best regards,
>
Javier Martinez Canillas Dec. 4, 2015, 12:38 p.m. UTC | #3
Hello Inki,

On 12/04/2015 06:00 AM, Inki Dae wrote:
> Hi Javier,
> 
> 2015? 12? 03? 22:55? Javier Martinez Canillas ?(?) ? ?:
>> Hello Inki,
>>
>> I found that v2 of this patch is alredy in your exynos-drm for-next branch so
>> so I had to revert it in linux-next to apply this one to test. You shouldn't
>> push patches that were still not reviewed (specially the ones that weren't
>> tested like this one) to your branch that gets pulled by -next. The idea of
>> -next is to have some test coverage but it should be as stable as possible.
> 
> exynos-drm/for-next branch is not really for-next branch. This branch is used

Well, it is a for-next branch because is pulled by -next. It is listed in:
http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Next/Trees

drm-exynos	git	git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git#exynos-drm/for-next

> only for integration test. As you know, there are many exynos maintainers
> and they have their own repository. So we would need to test the integration.

Integration testing is of course very needed and linux-next is for that but
what should be tested are the patches that are targeted to mainline.

> For this, exynos-drm/for-next is merged to linux-next not Dave's tree.
> Only exynos-drm-next branch will be merged to Dave's tree so only reviewed and
> tested patches will be merged to exynos-drm-next.
>

In that case, exynos-drm-next is what should be pulled by linux-next, no the
for-next branch. Linux-next is a simulation of what Torvalds would do next
so problems are found earlier, ideally before the patches land into mainline.

Best regards,
Inki Dae Dec. 4, 2015, 2:57 p.m. UTC | #4
Hi Javier,

2015-12-04 21:38 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> Hello Inki,
>
> On 12/04/2015 06:00 AM, Inki Dae wrote:
>> Hi Javier,
>>
>> 2015? 12? 03? 22:55? Javier Martinez Canillas ?(?) ? ?:
>>> Hello Inki,
>>>
>>> I found that v2 of this patch is alredy in your exynos-drm for-next branch so
>>> so I had to revert it in linux-next to apply this one to test. You shouldn't
>>> push patches that were still not reviewed (specially the ones that weren't
>>> tested like this one) to your branch that gets pulled by -next. The idea of
>>> -next is to have some test coverage but it should be as stable as possible.
>>
>> exynos-drm/for-next branch is not really for-next branch. This branch is used
>
> Well, it is a for-next branch because is pulled by -next. It is listed in:
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Next/Trees
>
> drm-exynos      git     git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git#exynos-drm/for-next
>
>> only for integration test. As you know, there are many exynos maintainers
>> and they have their own repository. So we would need to test the integration.
>
> Integration testing is of course very needed and linux-next is for that but
> what should be tested are the patches that are targeted to mainline.
>
>> For this, exynos-drm/for-next is merged to linux-next not Dave's tree.
>> Only exynos-drm-next branch will be merged to Dave's tree so only reviewed and
>> tested patches will be merged to exynos-drm-next.
>>
>
> In that case, exynos-drm-next is what should be pulled by linux-next, no the
> for-next branch. Linux-next is a simulation of what Torvalds would do next
> so problems are found earlier, ideally before the patches land into mainline.

Seems I didn't comment enough. exynos-drm/for-next branch includes all
patches of exynos-drm-next
and actually, they keep same patches for most time but sometimes, I
merge additional patches only to
exynos-drm/for-next, which should be more tested with other trees
before going to Dave's tree.

One more thing, there is other difference between exynos-drm-next and
exynos-drm/for-next.
That is all patches of exynos-drm-next are merged on top of based on
Dave's drm-next branch.
On the other hand, all of exynos-drm/for-next are merged on top of
mainline. So if exynos-drm-next
is merged to linux-next then all patches will be conflicted with
Dave's tree because his branch
is also merged to linux-next.

I'm not sure that other maintainers always keep only the for-next
branch in their repository but
in my case, I use exynos-drm/for-next branch for the test before going
to drm-next.
Anyway, exynos-drm-next will go to Dave's tree and then Dave's tree
will also be pulled by
linux-next, which would allow exynos-drm-next to be tested altogether
again before going to mainline.

Thanks,
Inki Dae

>
> Best regards,
> --
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Javier Martinez Canillas Dec. 4, 2015, 4:18 p.m. UTC | #5
Hello Inki,

On 12/04/2015 11:57 AM, Inki Dae wrote:
> Hi Javier,
> 
> 2015-12-04 21:38 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>> Hello Inki,
>>
>> On 12/04/2015 06:00 AM, Inki Dae wrote:
>>> Hi Javier,
>>>
>>> 2015? 12? 03? 22:55? Javier Martinez Canillas ?(?) ? ?:
>>>> Hello Inki,
>>>>
>>>> I found that v2 of this patch is alredy in your exynos-drm for-next branch so
>>>> so I had to revert it in linux-next to apply this one to test. You shouldn't
>>>> push patches that were still not reviewed (specially the ones that weren't
>>>> tested like this one) to your branch that gets pulled by -next. The idea of
>>>> -next is to have some test coverage but it should be as stable as possible.
>>>
>>> exynos-drm/for-next branch is not really for-next branch. This branch is used
>>
>> Well, it is a for-next branch because is pulled by -next. It is listed in:
>> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Next/Trees
>>
>> drm-exynos      git     git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git#exynos-drm/for-next
>>
>>> only for integration test. As you know, there are many exynos maintainers
>>> and they have their own repository. So we would need to test the integration.
>>
>> Integration testing is of course very needed and linux-next is for that but
>> what should be tested are the patches that are targeted to mainline.
>>
>>> For this, exynos-drm/for-next is merged to linux-next not Dave's tree.
>>> Only exynos-drm-next branch will be merged to Dave's tree so only reviewed and
>>> tested patches will be merged to exynos-drm-next.
>>>
>>
>> In that case, exynos-drm-next is what should be pulled by linux-next, no the
>> for-next branch. Linux-next is a simulation of what Torvalds would do next
>> so problems are found earlier, ideally before the patches land into mainline.
> 
> Seems I didn't comment enough. exynos-drm/for-next branch includes all
> patches of exynos-drm-next
> and actually, they keep same patches for most time but sometimes, I
> merge additional patches only to
> exynos-drm/for-next, which should be more tested with other trees
> before going to Dave's tree.
> 

Ok, but unreviewed and more importantly untested patches should not
go to to exynos-drm/for-next because those will be made available in
linux-next and can cause issues.

Only patches that are known to be good and have been reviewed/acked
should go there.

> One more thing, there is other difference between exynos-drm-next and
> exynos-drm/for-next.
> That is all patches of exynos-drm-next are merged on top of based on
> Dave's drm-next branch.
> On the other hand, all of exynos-drm/for-next are merged on top of
> mainline. So if exynos-drm-next

It's OK if you keep a different branch because you need a different
base before sending your pull request but IMHO the patches in both
branches should always be the same.

> is merged to linux-next then all patches will be conflicted with
> Dave's tree because his branch
> is also merged to linux-next.
>
> I'm not sure that other maintainers always keep only the for-next
> branch in their repository but
> in my case, I use exynos-drm/for-next branch for the test before going
> to drm-next.
> Anyway, exynos-drm-next will go to Dave's tree and then Dave's tree
> will also be pulled by
> linux-next, which would allow exynos-drm-next to be tested altogether
> again before going to mainline.
>

This should be a common problem for subsystems with different levels
of maintainership. I'm not a subsystem maintainer so I don't know how
this should be solved but I thought that git merge would take care
of this when both trees are pulled by linux-next.

Maybe Krzysztof can comment on this since he has to do the same for
the Exynos SoC support? He maintains a for-next branch and has to
send pull request to Kukjin (and sometimes may need to rebase on top
of Kukjin's tree) but both trees are pulled by linux-next to test.

> Thanks,
> Inki Dae
> 

Best regards,

Patch
diff mbox

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 94f02a0..60260a0 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -1392,7 +1392,7 @@  static const struct component_ops exynos_dp_ops = {
 static int exynos_dp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *panel_node, *bridge_node, *endpoint;
+	struct device_node *panel_node = NULL, *bridge_node, *endpoint = NULL;
 	struct exynos_dp_device *dp;
 	int ret;
 
@@ -1403,14 +1403,32 @@  static int exynos_dp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dp);
 
+	/* This is for the backward compatibility. */
 	panel_node = of_parse_phandle(dev->of_node, "panel", 0);
 	if (panel_node) {
 		dp->panel = of_drm_find_panel(panel_node);
 		of_node_put(panel_node);
 		if (!dp->panel)
 			return -EPROBE_DEFER;
+	} else {
+		endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+		if (endpoint) {
+			panel_node = of_graph_get_remote_port_parent(endpoint);
+			if (panel_node) {
+				dp->panel = of_drm_find_panel(panel_node);
+				of_node_put(panel_node);
+				if (!dp->panel)
+					return -EPROBE_DEFER;
+			} else {
+				DRM_ERROR("no port node for panel device.\n");
+				return -EINVAL;
+			}
+		}
 	}
 
+	if (endpoint)
+		goto out;
+
 	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
 	if (endpoint) {
 		bridge_node = of_graph_get_remote_port_parent(endpoint);
@@ -1423,6 +1441,7 @@  static int exynos_dp_probe(struct platform_device *pdev)
 			return -EPROBE_DEFER;
 	}
 
+out:
 	pm_runtime_enable(dev);
 
 	ret = component_add(&pdev->dev, &exynos_dp_ops);