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

Devices can also be child nodes when we also control that device
through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).

drm_of_find_panel_or_bridge can lookup panel or bridge for a given
device has port and endpoint and it fails to lookup if the device
has a child nodes.

This patch add support to lookup for a child node of the given parent
that isn't either port or ports.

Example OF graph representation of DSI host, which has port but
not has ports and has child panel node.

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 has ports but
not has port and has child panel node.

dsi {
        compatible = "samsung,exynos5433-mipi-dsi";
        #address-cells = <1>;
        #size-cells = <0>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;

                	dsi_to_mic: endpoint {
                        	remote-endpoint = <&mic_to_dsi>;
                	};
                };
        };

        panel@0 {
                reg = <0>;
        };
};

Example OF graph representation of DSI host, which has neither a port
nor a ports but has child panel node.

dsi0 {
	compatible = "ste,mcde-dsi";
	#address-cells = <1>;
	#size-cells = <0>;

	panel@0 {
		reg = <0>;
	};
};

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- update comments and commit message
Changes for v3:
- updated based on other usecase where 'ports' used along with child
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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
  

On Wed, Feb 2, 2022 at 9:34 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Devices can also be child nodes when we also control that device
> through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
>
> drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> device has port and endpoint and it fails to lookup if the device
> has a child nodes.
>
> This patch add support to lookup for a child node of the given parent
> that isn't either port or ports.
>
> Example OF graph representation of DSI host, which has port but
> not has ports and has child panel node.
>
> 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 has ports but
> not has port and has child panel node.
>
> dsi {
>         compatible = "samsung,exynos5433-mipi-dsi";
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>
>                         dsi_to_mic: endpoint {
>                                 remote-endpoint = <&mic_to_dsi>;
>                         };
>                 };
>         };
>
>         panel@0 {
>                 reg = <0>;
>         };
> };
>
> Example OF graph representation of DSI host, which has neither a port
> nor a ports but has child panel node.
>
> dsi0 {
>         compatible = "ste,mcde-dsi";
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         panel@0 {
>                 reg = <0>;
>         };
> };
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v4:
> - update comments and commit message
> Changes for v3:
> - updated based on other usecase where 'ports' used along with child
> 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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 59d368ea006b..9d90cd75c457 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -249,6 +249,21 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>         if (panel)
>                 *panel = NULL;
>
> +       /**
> +        * Devices can also be child nodes when we also control that device
> +        * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> +        *
> +        * Lookup for a child node of the given parent that isn't either port
> +        * or ports.
> +        */
> +       for_each_available_child_of_node(np, remote) {
> +               if (of_node_name_eq(remote, "port") ||
> +                   of_node_name_eq(remote, "ports"))
> +                       continue;
> +
> +               goto of_find_panel_or_bridge;
> +       }
> +
>         /*
>          * 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 +274,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
>

Any further comments on this?

Thanks,
Jagan.
  

On Wed, Feb 2, 2022 at 5:04 PM Jagan Teki <jagan@amarulasolutions.com> wrote:

> Devices can also be child nodes when we also control that device
> through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
>
> drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> device has port and endpoint and it fails to lookup if the device
> has a child nodes.
>
> This patch add support to lookup for a child node of the given parent
> that isn't either port or ports.
>
> Example OF graph representation of DSI host, which has port but
> not has ports and has child panel node.
>
> 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 has ports but
> not has port and has child panel node.
>
> dsi {
>         compatible = "samsung,exynos5433-mipi-dsi";
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>
>                         dsi_to_mic: endpoint {
>                                 remote-endpoint = <&mic_to_dsi>;
>                         };
>                 };
>         };
>
>         panel@0 {
>                 reg = <0>;
>         };
> };
>
> Example OF graph representation of DSI host, which has neither a port
> nor a ports but has child panel node.
>
> dsi0 {
>         compatible = "ste,mcde-dsi";
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         panel@0 {
>                 reg = <0>;
>         };
> };
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v4:
> - update comments and commit message

Looks good to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
  

On Tue, Feb 22, 2022 at 04:32:44PM +0100, Linus Walleij wrote:
> On Wed, Feb 2, 2022 at 5:04 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> 
> > Devices can also be child nodes when we also control that device
> > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> >
> > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > device has port and endpoint and it fails to lookup if the device
> > has a child nodes.
> >
> > This patch add support to lookup for a child node of the given parent
> > that isn't either port or ports.
> >
> > Example OF graph representation of DSI host, which has port but
> > not has ports and has child panel node.
> >
> > 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 has ports but
> > not has port and has child panel node.
> >
> > dsi {
> >         compatible = "samsung,exynos5433-mipi-dsi";
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         ports {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 port@0 {
> >                         reg = <0>;
> >
> >                         dsi_to_mic: endpoint {
> >                                 remote-endpoint = <&mic_to_dsi>;
> >                         };
> >                 };
> >         };
> >
> >         panel@0 {
> >                 reg = <0>;
> >         };
> > };
> >
> > Example OF graph representation of DSI host, which has neither a port
> > nor a ports but has child panel node.
> >
> > dsi0 {
> >         compatible = "ste,mcde-dsi";
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         panel@0 {
> >                 reg = <0>;
> >         };
> > };
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v4:
> > - update comments and commit message
> 
> Looks good to me.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied, thanks
Maxime
  

Hi Jagan,

On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> Devices can also be child nodes when we also control that device
> through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> 
> drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> device has port and endpoint and it fails to lookup if the device
> has a child nodes.

This patch breaks the logicvc drm driver that I'm currently developping.
The symptom is that drm_of_find_panel_or_bridge now always returns
-EPROBE_DEFER even after the panel has probed and is running well.
It seems that the function can no longer find the panel.

I haven't figured out the details, but reverting your patch makes
it work again. I suspect other drivers might be affected as well, so
it would probably be a good idea to revert the patch until the root
cause is clearly understood and the patch can be adapted accordingly.

Here is what the device-tree looks like:

/ {
	panel: panel-lvds {
		compatible = "panel-lvds";

		[...]

		port {
			#address-cells = <1>;
			#size-cells = <0>;

			panel_input: endpoint@0 {
				reg = <0>;
				remote-endpoint = <&logicvc_output>;
			};
		};
	};
};

&amba {
	logicvc: logicvc@43c00000 {
		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
		reg = <0x43c00000 0x6000>;

		#address-cells = <1>;
		#size-cells = <1>;

		[...]

		logicvc_display: display-engine@0 {
			compatible = "xylon,logicvc-4.01.a-display";

			[...]

			port {
				#address-cells = <1>;
				#size-ce/lls = <0>;

				logicvc_output: endpoint@0 {
					reg = <0>;
					remote-endpoint = <&panel_input>;
				};
			};
		};
	};
};

Cheers,

Paul

> This patch add support to lookup for a child node of the given parent
> that isn't either port or ports.
> 
> Example OF graph representation of DSI host, which has port but
> not has ports and has child panel node.
> 
> 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 has ports but
> not has port and has child panel node.
> 
> dsi {
>         compatible = "samsung,exynos5433-mipi-dsi";
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 
>                 	dsi_to_mic: endpoint {
>                         	remote-endpoint = <&mic_to_dsi>;
>                 	};
>                 };
>         };
> 
>         panel@0 {
>                 reg = <0>;
>         };
> };
> 
> Example OF graph representation of DSI host, which has neither a port
> nor a ports but has child panel node.
> 
> dsi0 {
> 	compatible = "ste,mcde-dsi";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	panel@0 {
> 		reg = <0>;
> 	};
> };
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes for v4:
> - update comments and commit message
> Changes for v3:
> - updated based on other usecase where 'ports' used along with child
> 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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 59d368ea006b..9d90cd75c457 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -249,6 +249,21 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  	if (panel)
>  		*panel = NULL;
>  
> +	/**
> +	 * Devices can also be child nodes when we also control that device
> +	 * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> +	 *
> +	 * Lookup for a child node of the given parent that isn't either port
> +	 * or ports.
> +	 */
> +	for_each_available_child_of_node(np, remote) {
> +		if (of_node_name_eq(remote, "port") ||
> +		    of_node_name_eq(remote, "ports"))
> +			continue;
> +
> +		goto of_find_panel_or_bridge;
> +	}
> +
>  	/*
>  	 * 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 +274,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;
>
  

Hi Paul,

On Thu, Mar 03, 2022 at 09:26:30PM +0100, Paul Kocialkowski wrote:
> On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> > Devices can also be child nodes when we also control that device
> > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > 
> > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > device has port and endpoint and it fails to lookup if the device
> > has a child nodes.
> 
> This patch breaks the logicvc drm driver that I'm currently developping.
> The symptom is that drm_of_find_panel_or_bridge now always returns
> -EPROBE_DEFER even after the panel has probed and is running well.
> It seems that the function can no longer find the panel.
> 
> I haven't figured out the details, but reverting your patch makes
> it work again. I suspect other drivers might be affected as well, so
> it would probably be a good idea to revert the patch until the root
> cause is clearly understood and the patch can be adapted accordingly.
> 
> Here is what the device-tree looks like:
> 
> / {
> 	panel: panel-lvds {
> 		compatible = "panel-lvds";
> 
> 		[...]
> 
> 		port {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			panel_input: endpoint@0 {
> 				reg = <0>;
> 				remote-endpoint = <&logicvc_output>;
> 			};
> 		};
> 	};
> };
> 
> &amba {
> 	logicvc: logicvc@43c00000 {
> 		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> 		reg = <0x43c00000 0x6000>;
> 
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		[...]
> 
> 		logicvc_display: display-engine@0 {
> 			compatible = "xylon,logicvc-4.01.a-display";
> 
> 			[...]

I think the issue lies in what you left out here: you have another node
aside from the port one, called layers. I *think* the issue is that the
code will now pick up the layers node, and try to use it as a panel,
which will never probe.

I've had a look at all the other bindings though, it seems like this
driver is the only one that can be affected: the anx7625 seems to be the
only other driver that has a child node that isn't either a port or a
panel (aux-bus) but it doesn't use drm_of_find_panel_or_bridge either.

Maxime
  

Hi Maxime,

On Fri 04 Mar 22, 09:54, Maxime Ripard wrote:
> Hi Paul,
> 
> On Thu, Mar 03, 2022 at 09:26:30PM +0100, Paul Kocialkowski wrote:
> > On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> > > Devices can also be child nodes when we also control that device
> > > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > 
> > > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > > device has port and endpoint and it fails to lookup if the device
> > > has a child nodes.
> > 
> > This patch breaks the logicvc drm driver that I'm currently developping.
> > The symptom is that drm_of_find_panel_or_bridge now always returns
> > -EPROBE_DEFER even after the panel has probed and is running well.
> > It seems that the function can no longer find the panel.
> > 
> > I haven't figured out the details, but reverting your patch makes
> > it work again. I suspect other drivers might be affected as well, so
> > it would probably be a good idea to revert the patch until the root
> > cause is clearly understood and the patch can be adapted accordingly.
> > 
> > Here is what the device-tree looks like:
> > 
> > / {
> > 	panel: panel-lvds {
> > 		compatible = "panel-lvds";
> > 
> > 		[...]
> > 
> > 		port {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			panel_input: endpoint@0 {
> > 				reg = <0>;
> > 				remote-endpoint = <&logicvc_output>;
> > 			};
> > 		};
> > 	};
> > };
> > 
> > &amba {
> > 	logicvc: logicvc@43c00000 {
> > 		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> > 		reg = <0x43c00000 0x6000>;
> > 
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 
> > 		[...]
> > 
> > 		logicvc_display: display-engine@0 {
> > 			compatible = "xylon,logicvc-4.01.a-display";
> > 
> > 			[...]
> 
> I think the issue lies in what you left out here: you have another node
> aside from the port one, called layers. I *think* the issue is that the
> code will now pick up the layers node, and try to use it as a panel,
> which will never probe.
> 
> I've had a look at all the other bindings though, it seems like this
> driver is the only one that can be affected: the anx7625 seems to be the
> only other driver that has a child node that isn't either a port or a
> panel (aux-bus) but it doesn't use drm_of_find_panel_or_bridge either.

Thanks a lot for looking into this so quickly!

After some testing it clearly appears that you're right and the layers
node is the one conflicting with the patch. Removing it brings the
behavior back to normal. I'll try to dig-in a bit more to understand
why this is happening since it's really not obvious when just looking
at the patch.

Cheers,

Paul
  

On Fri 04 Mar 22, 12:00, Paul Kocialkowski wrote:
> Hi Maxime,
> 
> On Fri 04 Mar 22, 09:54, Maxime Ripard wrote:
> > Hi Paul,
> > 
> > On Thu, Mar 03, 2022 at 09:26:30PM +0100, Paul Kocialkowski wrote:
> > > On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> > > > Devices can also be child nodes when we also control that device
> > > > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > > 
> > > > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > > > device has port and endpoint and it fails to lookup if the device
> > > > has a child nodes.
> > > 
> > > This patch breaks the logicvc drm driver that I'm currently developping.
> > > The symptom is that drm_of_find_panel_or_bridge now always returns
> > > -EPROBE_DEFER even after the panel has probed and is running well.
> > > It seems that the function can no longer find the panel.
> > > 
> > > I haven't figured out the details, but reverting your patch makes
> > > it work again. I suspect other drivers might be affected as well, so
> > > it would probably be a good idea to revert the patch until the root
> > > cause is clearly understood and the patch can be adapted accordingly.
> > > 
> > > Here is what the device-tree looks like:
> > > 
> > > / {
> > > 	panel: panel-lvds {
> > > 		compatible = "panel-lvds";
> > > 
> > > 		[...]
> > > 
> > > 		port {
> > > 			#address-cells = <1>;
> > > 			#size-cells = <0>;
> > > 
> > > 			panel_input: endpoint@0 {
> > > 				reg = <0>;
> > > 				remote-endpoint = <&logicvc_output>;
> > > 			};
> > > 		};
> > > 	};
> > > };
> > > 
> > > &amba {
> > > 	logicvc: logicvc@43c00000 {
> > > 		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> > > 		reg = <0x43c00000 0x6000>;
> > > 
> > > 		#address-cells = <1>;
> > > 		#size-cells = <1>;
> > > 
> > > 		[...]
> > > 
> > > 		logicvc_display: display-engine@0 {
> > > 			compatible = "xylon,logicvc-4.01.a-display";
> > > 
> > > 			[...]
> > 
> > I think the issue lies in what you left out here: you have another node
> > aside from the port one, called layers. I *think* the issue is that the
> > code will now pick up the layers node, and try to use it as a panel,
> > which will never probe.
> > 
> > I've had a look at all the other bindings though, it seems like this
> > driver is the only one that can be affected: the anx7625 seems to be the
> > only other driver that has a child node that isn't either a port or a
> > panel (aux-bus) but it doesn't use drm_of_find_panel_or_bridge either.
> 
> Thanks a lot for looking into this so quickly!
> 
> After some testing it clearly appears that you're right and the layers
> node is the one conflicting with the patch. Removing it brings the
> behavior back to normal. I'll try to dig-in a bit more to understand
> why this is happening since it's really not obvious when just looking
> at the patch.

Ah wait I do understand it actually. The patch will take the *first* node
that doesn't have ports/port in it and use that as remote instead of
of_graph_get_remote_node.

So maybe the fix would be to first look via of_graph_get_remote_node and
if nothing is returned then it should try to use the first node as remote.
tl;dr just inverting the order of the logic.

Do you think that would work?

Cheers,

Paul
  

On Fri, Mar 04, 2022 at 12:05:14PM +0100, Paul Kocialkowski wrote:
> On Fri 04 Mar 22, 12:00, Paul Kocialkowski wrote:
> > Hi Maxime,
> > 
> > On Fri 04 Mar 22, 09:54, Maxime Ripard wrote:
> > > Hi Paul,
> > > 
> > > On Thu, Mar 03, 2022 at 09:26:30PM +0100, Paul Kocialkowski wrote:
> > > > On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> > > > > Devices can also be child nodes when we also control that device
> > > > > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > > > 
> > > > > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > > > > device has port and endpoint and it fails to lookup if the device
> > > > > has a child nodes.
> > > > 
> > > > This patch breaks the logicvc drm driver that I'm currently developping.
> > > > The symptom is that drm_of_find_panel_or_bridge now always returns
> > > > -EPROBE_DEFER even after the panel has probed and is running well.
> > > > It seems that the function can no longer find the panel.
> > > > 
> > > > I haven't figured out the details, but reverting your patch makes
> > > > it work again. I suspect other drivers might be affected as well, so
> > > > it would probably be a good idea to revert the patch until the root
> > > > cause is clearly understood and the patch can be adapted accordingly.
> > > > 
> > > > Here is what the device-tree looks like:
> > > > 
> > > > / {
> > > > 	panel: panel-lvds {
> > > > 		compatible = "panel-lvds";
> > > > 
> > > > 		[...]
> > > > 
> > > > 		port {
> > > > 			#address-cells = <1>;
> > > > 			#size-cells = <0>;
> > > > 
> > > > 			panel_input: endpoint@0 {
> > > > 				reg = <0>;
> > > > 				remote-endpoint = <&logicvc_output>;
> > > > 			};
> > > > 		};
> > > > 	};
> > > > };
> > > > 
> > > > &amba {
> > > > 	logicvc: logicvc@43c00000 {
> > > > 		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> > > > 		reg = <0x43c00000 0x6000>;
> > > > 
> > > > 		#address-cells = <1>;
> > > > 		#size-cells = <1>;
> > > > 
> > > > 		[...]
> > > > 
> > > > 		logicvc_display: display-engine@0 {
> > > > 			compatible = "xylon,logicvc-4.01.a-display";
> > > > 
> > > > 			[...]
> > > 
> > > I think the issue lies in what you left out here: you have another node
> > > aside from the port one, called layers. I *think* the issue is that the
> > > code will now pick up the layers node, and try to use it as a panel,
> > > which will never probe.
> > > 
> > > I've had a look at all the other bindings though, it seems like this
> > > driver is the only one that can be affected: the anx7625 seems to be the
> > > only other driver that has a child node that isn't either a port or a
> > > panel (aux-bus) but it doesn't use drm_of_find_panel_or_bridge either.
> > 
> > Thanks a lot for looking into this so quickly!
> > 
> > After some testing it clearly appears that you're right and the layers
> > node is the one conflicting with the patch. Removing it brings the
> > behavior back to normal. I'll try to dig-in a bit more to understand
> > why this is happening since it's really not obvious when just looking
> > at the patch.
> 
> Ah wait I do understand it actually. The patch will take the *first* node
> that doesn't have ports/port in it and use that as remote instead of
> of_graph_get_remote_node.
> 
> So maybe the fix would be to first look via of_graph_get_remote_node and
> if nothing is returned then it should try to use the first node as remote.
> tl;dr just inverting the order of the logic.
> 
> Do you think that would work?

We can have multiple strategies here. The one you have in mind does work
indeed, but relying on the node order is still fairly fragile.

I think it would work fine then if:

  - We first lookup any endpoint, and see if we have a panel or bridge.
    If so, we return it.

  - Then, we look at any available child node, and see if we have a
    panel or bridge attached. If so, we return it.

  - we return -EPROBE_DEFER

That way, even if we have something like:

node {
     totally-not-a-panel {
     }

     panel {
     }
}

It would work fine, without relying on the node name (well, except for
port(s)?)

What do you think?
Maxime
  

Hi Maxime,

On Fri 04 Mar 22, 12:38, Maxime Ripard wrote:
> On Fri, Mar 04, 2022 at 12:05:14PM +0100, Paul Kocialkowski wrote:
> > On Fri 04 Mar 22, 12:00, Paul Kocialkowski wrote:
> > > Hi Maxime,
> > > 
> > > On Fri 04 Mar 22, 09:54, Maxime Ripard wrote:
> > > > Hi Paul,
> > > > 
> > > > On Thu, Mar 03, 2022 at 09:26:30PM +0100, Paul Kocialkowski wrote:
> > > > > On Wed 02 Feb 22, 21:34, Jagan Teki wrote:
> > > > > > Devices can also be child nodes when we also control that device
> > > > > > through the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
> > > > > > 
> > > > > > drm_of_find_panel_or_bridge can lookup panel or bridge for a given
> > > > > > device has port and endpoint and it fails to lookup if the device
> > > > > > has a child nodes.
> > > > > 
> > > > > This patch breaks the logicvc drm driver that I'm currently developping.
> > > > > The symptom is that drm_of_find_panel_or_bridge now always returns
> > > > > -EPROBE_DEFER even after the panel has probed and is running well.
> > > > > It seems that the function can no longer find the panel.
> > > > > 
> > > > > I haven't figured out the details, but reverting your patch makes
> > > > > it work again. I suspect other drivers might be affected as well, so
> > > > > it would probably be a good idea to revert the patch until the root
> > > > > cause is clearly understood and the patch can be adapted accordingly.
> > > > > 
> > > > > Here is what the device-tree looks like:
> > > > > 
> > > > > / {
> > > > > 	panel: panel-lvds {
> > > > > 		compatible = "panel-lvds";
> > > > > 
> > > > > 		[...]
> > > > > 
> > > > > 		port {
> > > > > 			#address-cells = <1>;
> > > > > 			#size-cells = <0>;
> > > > > 
> > > > > 			panel_input: endpoint@0 {
> > > > > 				reg = <0>;
> > > > > 				remote-endpoint = <&logicvc_output>;
> > > > > 			};
> > > > > 		};
> > > > > 	};
> > > > > };
> > > > > 
> > > > > &amba {
> > > > > 	logicvc: logicvc@43c00000 {
> > > > > 		compatible = "xylon,logicvc-3.02.a", "syscon", "simple-mfd";
> > > > > 		reg = <0x43c00000 0x6000>;
> > > > > 
> > > > > 		#address-cells = <1>;
> > > > > 		#size-cells = <1>;
> > > > > 
> > > > > 		[...]
> > > > > 
> > > > > 		logicvc_display: display-engine@0 {
> > > > > 			compatible = "xylon,logicvc-4.01.a-display";
> > > > > 
> > > > > 			[...]
> > > > 
> > > > I think the issue lies in what you left out here: you have another node
> > > > aside from the port one, called layers. I *think* the issue is that the
> > > > code will now pick up the layers node, and try to use it as a panel,
> > > > which will never probe.
> > > > 
> > > > I've had a look at all the other bindings though, it seems like this
> > > > driver is the only one that can be affected: the anx7625 seems to be the
> > > > only other driver that has a child node that isn't either a port or a
> > > > panel (aux-bus) but it doesn't use drm_of_find_panel_or_bridge either.
> > > 
> > > Thanks a lot for looking into this so quickly!
> > > 
> > > After some testing it clearly appears that you're right and the layers
> > > node is the one conflicting with the patch. Removing it brings the
> > > behavior back to normal. I'll try to dig-in a bit more to understand
> > > why this is happening since it's really not obvious when just looking
> > > at the patch.
> > 
> > Ah wait I do understand it actually. The patch will take the *first* node
> > that doesn't have ports/port in it and use that as remote instead of
> > of_graph_get_remote_node.
> > 
> > So maybe the fix would be to first look via of_graph_get_remote_node and
> > if nothing is returned then it should try to use the first node as remote.
> > tl;dr just inverting the order of the logic.
> > 
> > Do you think that would work?
> 
> We can have multiple strategies here. The one you have in mind does work
> indeed, but relying on the node order is still fairly fragile.
> 
> I think it would work fine then if:
> 
>   - We first lookup any endpoint, and see if we have a panel or bridge.
>     If so, we return it.
> 
>   - Then, we look at any available child node, and see if we have a
>     panel or bridge attached. If so, we return it.
> 
>   - we return -EPROBE_DEFER
> 
> That way, even if we have something like:
> 
> node {
>      totally-not-a-panel {
>      }
> 
>      panel {
>      }
> }
> 
> It would work fine, without relying on the node name (well, except for
> port(s)?)
> 
> What do you think?

Yes it would definitely be better to try all possible cases, including when
one of them fails instead of just selecting one to check and failing if
the panel/bridge nodes aren't there.

I'll have a try at this and send a fixup patch soon!

Cheers,

Paul