Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Maxime,

On Mon, Dec 13, 2021 at 02:42:55PM +0100, Maxime Ripard wrote:
> On Mon, Dec 13, 2021 at 03:35:16PM +0200, Laurent Pinchart wrote:
> > On Mon, Dec 13, 2021 at 02:09:36PM +0100, Maxime Ripard wrote:
> > > On Mon, Dec 13, 2021 at 02:28:39PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Dec 13, 2021 at 05:46:13PM +0530, Jagan Teki wrote:
> > > > > Some OF graphs don't require 'ports' to represent the
> > > > > downstream panel or bridge; instead it simply adds a child
> > > > > node on a given parent node.
> > > > > 
> > > > > drm_of_find_panel_or_bridge can lookup panel or bridge for
> > > > > a given node based on the OF graph port and endpoint and it
> > > > > fails to use if the given node has a child panel or bridge.
> > > > > 
> > > > > This patch add support to lookup that given node has child
> > > > > panel or bridge however that child node cannot be a 'port'
> > > > > alone or it cannot be a 'port' node too.
> > > > > 
> > > > > Example OF graph representation of DSI host, which doesn't
> > > > > have 'ports' and has child panel.
> > > > > 
> > > > > dsi {
> > > > > 	compatible = "allwinner,sun6i-a31-mipi-dsi";
> > > > > 	#address-cells = <1>;
> > > > > 	#size-cells = <0>;
> > > > > 
> > > > > 	port {
> > > > > 		dsi_in_tcon0: endpoint {
> > > > > 			remote-endpoint = <tcon0_out_dsi>;
> > > > > 	};
> > > > > 
> > > > > 	panel@0 {
> > > > > 		reg = <0>;
> > > > > 	};
> > > > > };
> > > > > 
> > > > > Example OF graph representation of DSI host, which doesn't
> > > > > have 'ports' and has child bridge.
> > > > > 
> > > > > dsi {
> > > > > 	compatible = "allwinner,sun6i-a31-mipi-dsi";
> > > > > 	#address-cells = <1>;
> > > > > 	#size-cells = <0>;
> > > > > 
> > > > > 	port {
> > > > > 		dsi_in_tcon0: endpoint {
> > > > > 			remote-endpoint = <tcon0_out_dsi>;
> > > > > 	};
> > > > > 
> > > > > 	bridge@0 {
> > > > > 		reg = <0>;
> > > > > 
> > > > > 		ports {
> > > > > 			#address-cells = <1>;
> > > > > 			#size-cells = <0>;
> > > > > 
> > > > > 			bridge_out: port@1 {
> > > > > 				reg = <1>;
> > > > > 
> > > > > 				bridge_out_panel: endpoint {
> > > > > 					remote-endpoint = <&panel_out_bridge>;
> > > > > 				};
> > > > > 			};
> > > > > 		};
> > > > > 	};
> > > > > };
> > > > > 
> > > > > Example OF graph representation of DSI host, which doesn't
> > > > > have 'ports' or 'port' and has child panel.
> > > > > 
> > > > > dsi0 {
> > > > > 	compatible = "ste,mcde-dsi";
> > > > > 	#address-cells = <1>;
> > > > > 	#size-cells = <0>;
> > > > > 
> > > > > 	panel@0 {
> > > > > 		reg = <0>;
> > > > > 	};
> > > > > };
> > > > > 
> > > > > Example OF graph representation of LTDC host, which doesn't
> > > > > have 'ports' or child panel/bridge and has 'port'.
> > > > > 
> > > > > ltdc {
> > > > > 	compatible = "st,stm32-ltdc";
> > > > > 	#address-cells = <1>;
> > > > > 	#size-cells = <0>;
> > > > > 
> > > > > 	port {
> > > > > 	};
> > > > > };
> > > > > 
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > > Changes for v2:
> > > > > - drop of helper
> > > > > https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@amarulasolutions.com/
> > > > > - support 'port' alone OF graph
> > > > > - updated comments
> > > > > - added simple code
> > > > > 
> > > > >  drivers/gpu/drm/drm_of.c | 23 +++++++++++++++++++++++
> > > > >  1 file changed, 23 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > > > index 59d368ea006b..7d018ff8bc83 100644
> > > > > --- a/drivers/gpu/drm/drm_of.c
> > > > > +++ b/drivers/gpu/drm/drm_of.c
> > > > > @@ -249,6 +249,27 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > > >  	if (panel)
> > > > >  		*panel = NULL;
> > > > >  
> > > > > +	/**
> > > > > +	 * Some OF graphs don't require 'ports' to represent the downstream
> > > > > +	 * panel or bridge; instead it simply adds a child node on a given
> > > > > +	 * parent node.
> > > > > +	 *
> > > > > +	 * Lookup that child node for a given parent however that child
> > > > > +	 * cannot be a 'port' alone or it cannot be a 'port' node too.
> > > > > +	 */
> > > > > +	if (!of_get_child_by_name(np, "ports")) {
> > > > > +		if (of_get_child_by_name(np, "port") && (of_get_child_count(np) == 1))
> > > > 
> > > > This messes up reference counting of device_node.
> > > > 
> > > > > +			goto of_graph_get_remote;
> > > > > +
> > > > > +		for_each_available_child_of_node(np, remote) {
> > > > > +			if (of_node_name_eq(remote, "port"))
> > > > > +				continue;
> > > > > +
> > > > > +			goto of_find_panel_or_bridge;
> > > > > +		}
> > > > > +	}
> > > > 
> > > > This really looks like a hack to me, I'm worried it may cause issues. It
> > > > would be better, I think, to split the drm_of_find_panel_or_bridge()
> > > > function in two, with the of_graph_get_remote_node() call moved to a
> > > > wrapper function, calling an inner function that takes the remote
> > > > device_node pointer. For the DSI use case, you could either look up the
> > > > panel DT node in the display driver and call the inner function
> > > > directly, or implement a DSI-specific wrapper.
> > > 
> > > I disagree. The whole point of drm_of_find_panel_or_bridge was that it
> > > was a helper for the encoder / upstream bridge to retrieve whatever is
> > > there next. It's useful and removes boilerplate.
> > > 
> > > We definitely want to have something just as convenient for DSI.
> > 
> > That could ba a drm_of_find_dsi_panel_or_bridge() :-) My point is that
> > I'd like to avoid making assumptions on node names in the lower layers.
> > 
> > I also have a different use case for a drm_of_find_panel_or_bridge()
> > function ta would take a device_node pointer, so moving the
> > of_graph_get_remote_node() lookup out would be useful there. We could
> > have (names to be bikeshedded)
> > 
> > - __drm_of_find_panel_or_bridge() without of_graph_get_remote_node()
> > - drm_of_find_panel_or_bridge() calling of_graph_get_remote_node() and
> >   __drm_of_find_panel_or_bridge()
> > - drm_of_find_dsi_panel_or_bridge() getting the device_node pointer in a
> >   way specific to DSI devices and calling
> >   __drm_of_find_panel_or_bridge()
> 
> I don't really like the idea of a DSI helper either. Those node names
> are reserved so I'm not sure we'll ever find a conflict, but can we base
> our decision on remote-endpoint (for ports/endpoints) or reg (for DSI
> devices)?
> 
> > Ideally, though, the case where we have no port node should die out
> > slowly, even when DSI devices are children of the DSI controller, there
> > should be ports modelling the data connection.
> 
> I'm not really in favor of that either, it looks like making the DT more
> complex than it needs to be for no particular reason, but I guess it's a
> very subjective matter :)

The reason is to simplify software support, which I think is worth it
:-)

-- 
Regards,

Laurent Pinchart