On Sun, 24 Nov 2019 14:17:27 +0100 Boris Brezillon boris.brezillon@collabora.com wrote:
@@ -1687,16 +1705,18 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, drm_encoder_init(drm_dev, encoder, &exynos_dsi_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);
- drm_encoder_helper_add(encoder, &exynos_dsi_encoder_helper_funcs);
- ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD); if (ret < 0) return ret;
- /* Declare ourself as the first bridge element. */
- dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
- drm_bridge_attach(encoder, &dsi->bridge, NULL);
- if (dsi->in_bridge_node) { in_bridge = of_drm_find_bridge(dsi->in_bridge_node); if (in_bridge)
drm_bridge_attach(encoder, in_bridge, NULL);
drm_bridge_attach(encoder, in_bridge, &dsi->bridge);Same as for patch 01/21, maybe this could be moved to this bridge's attach operation ? Actually, now that I've read the code, this in_bridge part looks weird. Why would the DSI encoder have an input bridge that is has to manage itself ?
Yes, I know, it doesn't make any sense. Either we're dealing with a bridge which can be chained to other bridges (can be placed in the middle of a chain as well), or we're dealing with an encoder which precedes any bridges. In the latter case (which is how exynos_dsi is implemented) in_bridge doesn't have any meaning, and that's even worse since we're placing the so-called input bridge (AKA previous bridge) after our encoder (that's what drm_bridge_attach(encoder, in_bridge, NULL) does).
More on that topic: I checked the exynos dts we have in mainline and no one is making use of the ports part of the exynos_dsim bindings, so maybe we should just deprecate it. The other option would be to patch the driver to act as a real bridge, but I can't do that without someone testing my changes and I didn't get much feedback from Exynos maintainers so far...