| Message ID | 20211014152606.2289528-3-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: rework encoders to pass ATTACH_NO_CONNECTOR | expand |
Hi Neil, one comment below. Other than that Acked-by: Sam Ravnborg <sam@ravnborg.org> Sam On Thu, Oct 14, 2021 at 05:26:01PM +0200, Neil Armstrong wrote: > The initial design was recursive to cover all port/endpoints, but only the first layer > of endpoints should be covered by the components list. > This also breaks the MIPI-DSI init/bridge attach sequence, thus only parse the > first endpoints instead of recursing. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/meson/meson_drv.c | 62 +++++++++++-------------------- > 1 file changed, 21 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c > index bc0d60df04ae..b53606d8825f 100644 > --- a/drivers/gpu/drm/meson/meson_drv.c > +++ b/drivers/gpu/drm/meson/meson_drv.c > @@ -427,46 +427,6 @@ static int compare_of(struct device *dev, void *data) > return dev->of_node == data; > } > > -/* Possible connectors nodes to ignore */ > -static const struct of_device_id connectors_match[] = { > - { .compatible = "composite-video-connector" }, > - { .compatible = "svideo-connector" }, > - { .compatible = "hdmi-connector" }, > - { .compatible = "dvi-connector" }, > - {} > -}; > - > -static int meson_probe_remote(struct platform_device *pdev, > - struct component_match **match, > - struct device_node *parent, > - struct device_node *remote) > -{ > - struct device_node *ep, *remote_node; > - int count = 1; > - > - /* If node is a connector, return and do not add to match table */ > - if (of_match_node(connectors_match, remote)) > - return 1; > - > - component_match_add(&pdev->dev, match, compare_of, remote); > - > - for_each_endpoint_of_node(remote, ep) { > - remote_node = of_graph_get_remote_port_parent(ep); > - if (!remote_node || > - remote_node == parent || /* Ignore parent endpoint */ > - !of_device_is_available(remote_node)) { > - of_node_put(remote_node); > - continue; > - } > - > - count += meson_probe_remote(pdev, match, remote, remote_node); > - > - of_node_put(remote_node); > - } > - > - return count; > -} > - > static void meson_drv_shutdown(struct platform_device *pdev) > { > struct meson_drm *priv = dev_get_drvdata(&pdev->dev); > @@ -478,6 +438,13 @@ static void meson_drv_shutdown(struct platform_device *pdev) > drm_atomic_helper_shutdown(priv->drm); > } > > +/* Possible connectors nodes to ignore */ > +static const struct of_device_id connectors_match[] = { > + { .compatible = "composite-video-connector" }, > + { .compatible = "svideo-connector" }, > + {} > +}; > + > static int meson_drv_probe(struct platform_device *pdev) > { > struct component_match *match = NULL; > @@ -492,8 +459,21 @@ static int meson_drv_probe(struct platform_device *pdev) > continue; > } > > - count += meson_probe_remote(pdev, &match, np, remote); > + /* If an analog connector is detected, count it as an output */ > + if (of_match_node(connectors_match, remote)) { > + ++count; > + of_node_put(remote); > + continue; > + } > + > + DRM_DEBUG_DRIVER("parent %pOF remote match add %pOF parent %s\n", > + np, remote, dev_name(&pdev->dev)); Avoid the deprecated logging functions. Use drm_dbg() or if there is no drm_device just dev_dbg(). I assume the driver uses DRM_xxx all over, so I understand if you keep it as-is. > + > + component_match_add(&pdev->dev, &match, compare_of, remote); > + > of_node_put(remote); > + > + ++count; > } > > if (count && !match) > -- > 2.25.1
Hi Sam, On 14/10/2021 19:49, Sam Ravnborg wrote: > Hi Neil, > > one comment below. Other than that > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > Sam > > On Thu, Oct 14, 2021 at 05:26:01PM +0200, Neil Armstrong wrote: >> The initial design was recursive to cover all port/endpoints, but only the first layer >> of endpoints should be covered by the components list. >> This also breaks the MIPI-DSI init/bridge attach sequence, thus only parse the >> first endpoints instead of recursing. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/meson/meson_drv.c | 62 +++++++++++-------------------- >> 1 file changed, 21 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c >> index bc0d60df04ae..b53606d8825f 100644 >> --- a/drivers/gpu/drm/meson/meson_drv.c >> +++ b/drivers/gpu/drm/meson/meson_drv.c >> @@ -427,46 +427,6 @@ static int compare_of(struct device *dev, void *data) >> return dev->of_node == data; >> } >> >> -/* Possible connectors nodes to ignore */ >> -static const struct of_device_id connectors_match[] = { >> - { .compatible = "composite-video-connector" }, >> - { .compatible = "svideo-connector" }, >> - { .compatible = "hdmi-connector" }, >> - { .compatible = "dvi-connector" }, >> - {} >> -}; >> - >> -static int meson_probe_remote(struct platform_device *pdev, >> - struct component_match **match, >> - struct device_node *parent, >> - struct device_node *remote) >> -{ >> - struct device_node *ep, *remote_node; >> - int count = 1; >> - >> - /* If node is a connector, return and do not add to match table */ >> - if (of_match_node(connectors_match, remote)) >> - return 1; >> - >> - component_match_add(&pdev->dev, match, compare_of, remote); >> - >> - for_each_endpoint_of_node(remote, ep) { >> - remote_node = of_graph_get_remote_port_parent(ep); >> - if (!remote_node || >> - remote_node == parent || /* Ignore parent endpoint */ >> - !of_device_is_available(remote_node)) { >> - of_node_put(remote_node); >> - continue; >> - } >> - >> - count += meson_probe_remote(pdev, match, remote, remote_node); >> - >> - of_node_put(remote_node); >> - } >> - >> - return count; >> -} >> - >> static void meson_drv_shutdown(struct platform_device *pdev) >> { >> struct meson_drm *priv = dev_get_drvdata(&pdev->dev); >> @@ -478,6 +438,13 @@ static void meson_drv_shutdown(struct platform_device *pdev) >> drm_atomic_helper_shutdown(priv->drm); >> } >> >> +/* Possible connectors nodes to ignore */ >> +static const struct of_device_id connectors_match[] = { >> + { .compatible = "composite-video-connector" }, >> + { .compatible = "svideo-connector" }, >> + {} >> +}; >> + >> static int meson_drv_probe(struct platform_device *pdev) >> { >> struct component_match *match = NULL; >> @@ -492,8 +459,21 @@ static int meson_drv_probe(struct platform_device *pdev) >> continue; >> } >> >> - count += meson_probe_remote(pdev, &match, np, remote); >> + /* If an analog connector is detected, count it as an output */ >> + if (of_match_node(connectors_match, remote)) { >> + ++count; >> + of_node_put(remote); >> + continue; >> + } >> + >> + DRM_DEBUG_DRIVER("parent %pOF remote match add %pOF parent %s\n", >> + np, remote, dev_name(&pdev->dev)); > Avoid the deprecated logging functions. > Use drm_dbg() or if there is no drm_device just dev_dbg(). > > I assume the driver uses DRM_xxx all over, so I understand if you keep > it as-is. Since it's in the probe function, I will move to dev_dbg(). I'll probably do a print cleanup all over the driver. > >> + >> + component_match_add(&pdev->dev, &match, compare_of, remote); >> + >> of_node_put(remote); >> + >> + ++count; >> } >> >> if (count && !match) >> -- >> 2.25.1 Thanks, Neil
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index bc0d60df04ae..b53606d8825f 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -427,46 +427,6 @@ static int compare_of(struct device *dev, void *data) return dev->of_node == data; } -/* Possible connectors nodes to ignore */ -static const struct of_device_id connectors_match[] = { - { .compatible = "composite-video-connector" }, - { .compatible = "svideo-connector" }, - { .compatible = "hdmi-connector" }, - { .compatible = "dvi-connector" }, - {} -}; - -static int meson_probe_remote(struct platform_device *pdev, - struct component_match **match, - struct device_node *parent, - struct device_node *remote) -{ - struct device_node *ep, *remote_node; - int count = 1; - - /* If node is a connector, return and do not add to match table */ - if (of_match_node(connectors_match, remote)) - return 1; - - component_match_add(&pdev->dev, match, compare_of, remote); - - for_each_endpoint_of_node(remote, ep) { - remote_node = of_graph_get_remote_port_parent(ep); - if (!remote_node || - remote_node == parent || /* Ignore parent endpoint */ - !of_device_is_available(remote_node)) { - of_node_put(remote_node); - continue; - } - - count += meson_probe_remote(pdev, match, remote, remote_node); - - of_node_put(remote_node); - } - - return count; -} - static void meson_drv_shutdown(struct platform_device *pdev) { struct meson_drm *priv = dev_get_drvdata(&pdev->dev); @@ -478,6 +438,13 @@ static void meson_drv_shutdown(struct platform_device *pdev) drm_atomic_helper_shutdown(priv->drm); } +/* Possible connectors nodes to ignore */ +static const struct of_device_id connectors_match[] = { + { .compatible = "composite-video-connector" }, + { .compatible = "svideo-connector" }, + {} +}; + static int meson_drv_probe(struct platform_device *pdev) { struct component_match *match = NULL; @@ -492,8 +459,21 @@ static int meson_drv_probe(struct platform_device *pdev) continue; } - count += meson_probe_remote(pdev, &match, np, remote); + /* If an analog connector is detected, count it as an output */ + if (of_match_node(connectors_match, remote)) { + ++count; + of_node_put(remote); + continue; + } + + DRM_DEBUG_DRIVER("parent %pOF remote match add %pOF parent %s\n", + np, remote, dev_name(&pdev->dev)); + + component_match_add(&pdev->dev, &match, compare_of, remote); + of_node_put(remote); + + ++count; } if (count && !match)
The initial design was recursive to cover all port/endpoints, but only the first layer of endpoints should be covered by the components list. This also breaks the MIPI-DSI init/bridge attach sequence, thus only parse the first endpoints instead of recursing. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/meson/meson_drv.c | 62 +++++++++++-------------------- 1 file changed, 21 insertions(+), 41 deletions(-)