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

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

Hi Jagan,

Thank you for the patch.

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.

> +
> +of_graph_get_remote:
>  	/*
>  	 * of_graph_get_remote_node() produces a noisy error message if port
>  	 * node isn't found and the absence of the port is a legit case here,
> @@ -259,6 +280,8 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  		return -ENODEV;
>  
>  	remote = of_graph_get_remote_node(np, port, endpoint);
> +
> +of_find_panel_or_bridge:
>  	if (!remote)
>  		return -ENODEV;
>  

-- 
Regards,

Laurent Pinchart