* [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-24 0:56 ` Shuah Khan 0 siblings, 0 replies; 10+ messages in thread From: Shuah Khan @ 2017-06-24 0:56 UTC (permalink / raw) To: inki.dae, sw0312.kim, kyungmin.park, airlied, kgene, krzk, javier Cc: Shuah Khan, dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel Fix to call of_node_put() right after of_drm_find_bridge() instead of holding on to it until exynos_dsi_remove(). Suggested-by: Inki Dae <inki.dae@samsung.com> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e337cd2..7513b88 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, if (dsi->bridge_node) { bridge = of_drm_find_bridge(dsi->bridge_node); + of_node_put(dsi->bridge_node); if (bridge) drm_bridge_attach(encoder, bridge, NULL); } @@ -1807,10 +1808,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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-24 0:56 ` Shuah Khan 0 siblings, 0 replies; 10+ messages in thread From: Shuah Khan @ 2017-06-24 0:56 UTC (permalink / raw) To: linux-arm-kernel Fix to call of_node_put() right after of_drm_find_bridge() instead of holding on to it until exynos_dsi_remove(). Suggested-by: Inki Dae <inki.dae@samsung.com> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e337cd2..7513b88 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, if (dsi->bridge_node) { bridge = of_drm_find_bridge(dsi->bridge_node); + of_node_put(dsi->bridge_node); if (bridge) drm_bridge_attach(encoder, bridge, NULL); } @@ -1807,10 +1808,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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() 2017-06-24 0:56 ` Shuah Khan (?) @ 2017-06-26 7:02 ` Andrzej Hajda -1 siblings, 0 replies; 10+ messages in thread From: Andrzej Hajda @ 2017-06-26 7:02 UTC (permalink / raw) To: Shuah Khan, inki.dae, sw0312.kim, kyungmin.park, airlied, kgene, krzk, javier Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-kernel Hi Shuah, On 24.06.2017 02:56, Shuah Khan wrote: > Fix to call of_node_put() right after of_drm_find_bridge() instead of > holding on to it until exynos_dsi_remove(). I think the current implementation is OK, node is get in probe and put in remove. There could be many bind/unbind during lifetime of the bound driver. For example, there is possible sequence: 1. probe - 2. bind 3. unbind 4. bind With this patch on 2nd bind (point 4) driver will call of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). Regards Andrzej > > Suggested-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index e337cd2..7513b88 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, > > if (dsi->bridge_node) { > bridge = of_drm_find_bridge(dsi->bridge_node); > + of_node_put(dsi->bridge_node); > if (bridge) > drm_bridge_attach(encoder, bridge, NULL); > } > @@ -1807,10 +1808,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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-26 7:02 ` Andrzej Hajda 0 siblings, 0 replies; 10+ messages in thread From: Andrzej Hajda @ 2017-06-26 7:02 UTC (permalink / raw) To: linux-arm-kernel Hi Shuah, On 24.06.2017 02:56, Shuah Khan wrote: > Fix to call of_node_put() right after of_drm_find_bridge() instead of > holding on to it until exynos_dsi_remove(). I think the current implementation is OK, node is get in probe and put in remove. There could be many bind/unbind during lifetime of the bound driver. For example, there is possible sequence: 1. probe - 2. bind 3. unbind 4. bind With this patch on 2nd bind (point 4) driver will call of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). Regards Andrzej > > Suggested-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index e337cd2..7513b88 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, > > if (dsi->bridge_node) { > bridge = of_drm_find_bridge(dsi->bridge_node); > + of_node_put(dsi->bridge_node); > if (bridge) > drm_bridge_attach(encoder, bridge, NULL); > } > @@ -1807,10 +1808,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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-26 7:02 ` Andrzej Hajda 0 siblings, 0 replies; 10+ messages in thread From: Andrzej Hajda @ 2017-06-26 7:02 UTC (permalink / raw) To: Shuah Khan, inki.dae, sw0312.kim, kyungmin.park, airlied, kgene, krzk, javier Cc: linux-samsung-soc, dri-devel, linux-arm-kernel, linux-kernel Hi Shuah, On 24.06.2017 02:56, Shuah Khan wrote: > Fix to call of_node_put() right after of_drm_find_bridge() instead of > holding on to it until exynos_dsi_remove(). I think the current implementation is OK, node is get in probe and put in remove. There could be many bind/unbind during lifetime of the bound driver. For example, there is possible sequence: 1. probe - 2. bind 3. unbind 4. bind With this patch on 2nd bind (point 4) driver will call of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). Regards Andrzej > > Suggested-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index e337cd2..7513b88 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, > > if (dsi->bridge_node) { > bridge = of_drm_find_bridge(dsi->bridge_node); > + of_node_put(dsi->bridge_node); > if (bridge) > drm_bridge_attach(encoder, bridge, NULL); > } > @@ -1807,10 +1808,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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() 2017-06-26 7:02 ` Andrzej Hajda @ 2017-06-27 9:13 ` Inki Dae -1 siblings, 0 replies; 10+ messages in thread From: Inki Dae @ 2017-06-27 9:13 UTC (permalink / raw) To: Andrzej Hajda, Shuah Khan, sw0312.kim, kyungmin.park, airlied, kgene, krzk, javier Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-kernel Hi Andrzej, 2017년 06월 26일 16:02에 Andrzej Hajda 이(가) 쓴 글: > Hi Shuah, > > > On 24.06.2017 02:56, Shuah Khan wrote: >> Fix to call of_node_put() right after of_drm_find_bridge() instead of >> holding on to it until exynos_dsi_remove(). > > I think the current implementation is OK, node is get in probe and put > in remove. > There could be many bind/unbind during lifetime of the bound driver. > For example, there is possible sequence: > 1. probe - > 2. bind > 3. unbind > 4. bind > > With this patch on 2nd bind (point 4) driver will call > of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). > Right. This is a problem. How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. Thanks, Inki Dae > Regards > Andrzej > > >> >> Suggested-by: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> index e337cd2..7513b88 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >> >> if (dsi->bridge_node) { >> bridge = of_drm_find_bridge(dsi->bridge_node); >> + of_node_put(dsi->bridge_node); >> if (bridge) >> drm_bridge_attach(encoder, bridge, NULL); >> } >> @@ -1807,10 +1808,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); > > > -- > 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-27 9:13 ` Inki Dae 0 siblings, 0 replies; 10+ messages in thread From: Inki Dae @ 2017-06-27 9:13 UTC (permalink / raw) To: linux-arm-kernel Hi Andrzej, 2017? 06? 26? 16:02? Andrzej Hajda ?(?) ? ?: > Hi Shuah, > > > On 24.06.2017 02:56, Shuah Khan wrote: >> Fix to call of_node_put() right after of_drm_find_bridge() instead of >> holding on to it until exynos_dsi_remove(). > > I think the current implementation is OK, node is get in probe and put > in remove. > There could be many bind/unbind during lifetime of the bound driver. > For example, there is possible sequence: > 1. probe - > 2. bind > 3. unbind > 4. bind > > With this patch on 2nd bind (point 4) driver will call > of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). > Right. This is a problem. How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. Thanks, Inki Dae > Regards > Andrzej > > >> >> Suggested-by: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> index e337cd2..7513b88 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >> >> if (dsi->bridge_node) { >> bridge = of_drm_find_bridge(dsi->bridge_node); >> + of_node_put(dsi->bridge_node); >> if (bridge) >> drm_bridge_attach(encoder, bridge, NULL); >> } >> @@ -1807,10 +1808,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); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() 2017-06-27 9:13 ` Inki Dae (?) @ 2017-06-27 10:07 ` Andrzej Hajda -1 siblings, 0 replies; 10+ messages in thread From: Andrzej Hajda @ 2017-06-27 10:07 UTC (permalink / raw) To: Inki Dae, Shuah Khan, sw0312.kim, kyungmin.park, airlied, kgene, krzk, javier Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-kernel On 27.06.2017 11:13, Inki Dae wrote: > Hi Andrzej, > > 2017년 06월 26일 16:02에 Andrzej Hajda 이(가) 쓴 글: >> Hi Shuah, >> >> >> On 24.06.2017 02:56, Shuah Khan wrote: >>> Fix to call of_node_put() right after of_drm_find_bridge() instead of >>> holding on to it until exynos_dsi_remove(). >> I think the current implementation is OK, node is get in probe and put >> in remove. >> There could be many bind/unbind during lifetime of the bound driver. >> For example, there is possible sequence: >> 1. probe - >> 2. bind >> 3. unbind >> 4. bind >> >> With this patch on 2nd bind (point 4) driver will call >> of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). >> > Right. This is a problem. > > How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? > Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. I think the reason to call of_drm_find_bridge in bind is to avoid probe deferring, and with current code it will probably defer forever, explanation below: The only bridge connected to DSI is bridge provided by MIC driver, and MIC driver is componentized, but it registers his bridge in bind callback. So with drm_bridge_attach moved to probe of DSI initialization would never succeed because: 1. MIC->bind will be called only if all DRM components are present. 2. DSI driver will defer probing because the bridge is not present, thus DSI component will never be present. So the current code looks better to me, but there are two thing which could be improved: A. Maybe it would be good thing to move MIC bridge registration to probe, I see no reason to postpone it to bind phase, this way it should be safer if for some reason DSI->bind callback would be called before MIC->bind. B. Add error checking to of_drm_find_bridge in DSI driver, I know MIC is generally optional, but if the graph link is present (ie. dsi->bridge_node is valid) MIC bridge should be present, if it is not, it indicates some problem. Regards Andrzej > > Thanks, > Inki Dae > >> Regards >> Andrzej >> >> >>> Suggested-by: Inki Dae <inki.dae@samsung.com> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> index e337cd2..7513b88 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>> >>> if (dsi->bridge_node) { >>> bridge = of_drm_find_bridge(dsi->bridge_node); >>> + of_node_put(dsi->bridge_node); >>> if (bridge) >>> drm_bridge_attach(encoder, bridge, NULL); >>> } >>> @@ -1807,10 +1808,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); >> >> -- >> 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 >> >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-27 10:07 ` Andrzej Hajda 0 siblings, 0 replies; 10+ messages in thread From: Andrzej Hajda @ 2017-06-27 10:07 UTC (permalink / raw) To: linux-arm-kernel On 27.06.2017 11:13, Inki Dae wrote: > Hi Andrzej, > > 2017? 06? 26? 16:02? Andrzej Hajda ?(?) ? ?: >> Hi Shuah, >> >> >> On 24.06.2017 02:56, Shuah Khan wrote: >>> Fix to call of_node_put() right after of_drm_find_bridge() instead of >>> holding on to it until exynos_dsi_remove(). >> I think the current implementation is OK, node is get in probe and put >> in remove. >> There could be many bind/unbind during lifetime of the bound driver. >> For example, there is possible sequence: >> 1. probe - >> 2. bind >> 3. unbind >> 4. bind >> >> With this patch on 2nd bind (point 4) driver will call >> of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). >> > Right. This is a problem. > > How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? > Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. I think the reason to call of_drm_find_bridge in bind is to avoid probe deferring, and with current code it will probably defer forever, explanation below: The only bridge connected to DSI is bridge provided by MIC driver, and MIC driver is componentized, but it registers his bridge in bind callback. So with drm_bridge_attach moved to probe of DSI initialization would never succeed because: 1. MIC->bind will be called only if all DRM components are present. 2. DSI driver will defer probing because the bridge is not present, thus DSI component will never be present. So the current code looks better to me, but there are two thing which could be improved: A. Maybe it would be good thing to move MIC bridge registration to probe, I see no reason to postpone it to bind phase, this way it should be safer if for some reason DSI->bind callback would be called before MIC->bind. B. Add error checking to of_drm_find_bridge in DSI driver, I know MIC is generally optional, but if the graph link is present (ie. dsi->bridge_node is valid) MIC bridge should be present, if it is not, it indicates some problem. Regards Andrzej > > Thanks, > Inki Dae > >> Regards >> Andrzej >> >> >>> Suggested-by: Inki Dae <inki.dae@samsung.com> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> index e337cd2..7513b88 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>> >>> if (dsi->bridge_node) { >>> bridge = of_drm_find_bridge(dsi->bridge_node); >>> + of_node_put(dsi->bridge_node); >>> if (bridge) >>> drm_bridge_attach(encoder, bridge, NULL); >>> } >>> @@ -1807,10 +1808,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); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo at vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() @ 2017-06-27 10:07 ` Andrzej Hajda 0 siblings, 0 replies; 10+ messages in thread From: Andrzej Hajda @ 2017-06-27 10:07 UTC (permalink / raw) To: Inki Dae, Shuah Khan, sw0312.kim, kyungmin.park, airlied, kgene, krzk, javier Cc: linux-samsung-soc, dri-devel, linux-arm-kernel, linux-kernel On 27.06.2017 11:13, Inki Dae wrote: > Hi Andrzej, > > 2017년 06월 26일 16:02에 Andrzej Hajda 이(가) 쓴 글: >> Hi Shuah, >> >> >> On 24.06.2017 02:56, Shuah Khan wrote: >>> Fix to call of_node_put() right after of_drm_find_bridge() instead of >>> holding on to it until exynos_dsi_remove(). >> I think the current implementation is OK, node is get in probe and put >> in remove. >> There could be many bind/unbind during lifetime of the bound driver. >> For example, there is possible sequence: >> 1. probe - >> 2. bind >> 3. unbind >> 4. bind >> >> With this patch on 2nd bind (point 4) driver will call >> of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). >> > Right. This is a problem. > > How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? > Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. I think the reason to call of_drm_find_bridge in bind is to avoid probe deferring, and with current code it will probably defer forever, explanation below: The only bridge connected to DSI is bridge provided by MIC driver, and MIC driver is componentized, but it registers his bridge in bind callback. So with drm_bridge_attach moved to probe of DSI initialization would never succeed because: 1. MIC->bind will be called only if all DRM components are present. 2. DSI driver will defer probing because the bridge is not present, thus DSI component will never be present. So the current code looks better to me, but there are two thing which could be improved: A. Maybe it would be good thing to move MIC bridge registration to probe, I see no reason to postpone it to bind phase, this way it should be safer if for some reason DSI->bind callback would be called before MIC->bind. B. Add error checking to of_drm_find_bridge in DSI driver, I know MIC is generally optional, but if the graph link is present (ie. dsi->bridge_node is valid) MIC bridge should be present, if it is not, it indicates some problem. Regards Andrzej > > Thanks, > Inki Dae > >> Regards >> Andrzej >> >> >>> Suggested-by: Inki Dae <inki.dae@samsung.com> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> index e337cd2..7513b88 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>> >>> if (dsi->bridge_node) { >>> bridge = of_drm_find_bridge(dsi->bridge_node); >>> + of_node_put(dsi->bridge_node); >>> if (bridge) >>> drm_bridge_attach(encoder, bridge, NULL); >>> } >>> @@ -1807,10 +1808,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); >> >> -- >> 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 >> >> >> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-06-27 10:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20170626004946epcas4p40785020666ae6d43476822d067604baa@epcas4p4.samsung.com> 2017-06-24 0:56 ` [PATCH] drm: exynos: dsi: release DSI_PORT_OUT node right after of_drm_find_bridge() Shuah Khan 2017-06-24 0:56 ` Shuah Khan 2017-06-26 7:02 ` Andrzej Hajda 2017-06-26 7:02 ` Andrzej Hajda 2017-06-26 7:02 ` Andrzej Hajda 2017-06-27 9:13 ` Inki Dae 2017-06-27 9:13 ` Inki Dae 2017-06-27 10:07 ` Andrzej Hajda 2017-06-27 10:07 ` Andrzej Hajda 2017-06-27 10:07 ` Andrzej Hajda
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.