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

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

[Jagan Teki]

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))
+			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;
+		}
+	}
+
+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;
 
-- 
2.25.1

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

[Laurent Pinchart]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4578 bytes --]

On Mon, Dec 13, 2021 at 02:28:39PM +0200, Laurent Pinchart wrote:
> 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.

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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Laurent Pinchart]

Hi Maxime,

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()

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.

-- 
Regards,

Laurent Pinchart

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6604 bytes --]

On Mon, Dec 13, 2021 at 03:35:16PM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> 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 :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jagan Teki]

Hi Laurent,

On Mon, Dec 13, 2021 at 5:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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.

I sent the V3 for this, and considered whole video pipeline use cases.
Please have a look and comment.

Thanks,
Jagan.

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

[Laurent Pinchart]

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