Re: [PATCH v3 03/21] drm/exynos: Declare the DSI encoder as a bridge element

[Boris Brezillon <boris.brezillon@collabora.com>]

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...