| Message ID | 1499066879-13241-2-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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
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); > > > >
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 > > >
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);