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

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

[Jagan Teki]

Some OF graphs don't require 'port' or '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 is neither a 'port' nor a 'ports'
node.

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 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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 59d368ea006b..aeddd39b8df6 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 	if (panel)
 		*panel = NULL;
 
+	/**
+	 * Some OF graphs don't require 'port' or '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 is
+	 * neither a 'port' nor a 'ports' node.
+	 */
+	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 +275,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 v3] drm: of: Lookup if child node has panel or bridge

[Maxime Ripard]

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

On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> Some OF graphs don't require 'port' or 'ports' to represent the
> downstream panel or bridge; instead it simply adds a child node
> on a given parent node.

All bindings using OF graph nodes require either port or ports.

DSI Host however don't have to use the OF graph, and that's what you're
talking about.

> 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 is neither a 'port' nor a 'ports'
> node.
> 
> 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>;
>         };
> };

I can't see how that one makes sense. The endpoint seems to have a
single output, yet you also have a panel under it which is also an
output? You should have at least the virtual channel of that endpoint
somewhere to differentiate data between the panel and whatever is
connected on the other side of that endpoint.

> 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 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 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 59d368ea006b..aeddd39b8df6 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  	if (panel)
>  		*panel = NULL;
>  
> +	/**
> +	 * Some OF graphs don't require 'port' or 'ports' to represent the
> +	 * downstream panel or bridge; instead it simply adds a child node
> +	 * on a given parent node.

All OF graphs require either port or ports. DSI hosts can just have a
child node.

Maxime

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

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

[Jagan Teki]

On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> > Some OF graphs don't require 'port' or 'ports' to represent the
> > downstream panel or bridge; instead it simply adds a child node
> > on a given parent node.
>
> All bindings using OF graph nodes require either port or ports.
>
> DSI Host however don't have to use the OF graph, and that's what you're
> talking about.

Yes, right now I can see DSI but this change is generic to any OF graph.

>
> > 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 is neither a 'port' nor a 'ports'
> > node.
> >
> > 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>;
> >         };
> > };
>
> I can't see how that one makes sense. The endpoint seems to have a
> single output, yet you also have a panel under it which is also an
> output? You should have at least the virtual channel of that endpoint
> somewhere to differentiate data between the panel and whatever is
> connected on the other side of that endpoint.

Same that I understood so far (based on v2 change), However we have
exynos5433 has this pipeline and Andrzej mentioned it is valid
pipeline on other thread.

May be Andrzej, can give more conclusive evidence for it.

>
> > 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 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 | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 59d368ea006b..aeddd39b8df6 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >       if (panel)
> >               *panel = NULL;
> >
> > +     /**
> > +      * Some OF graphs don't require 'port' or 'ports' to represent the
> > +      * downstream panel or bridge; instead it simply adds a child node
> > +      * on a given parent node.
>
> All OF graphs require either port or ports. DSI hosts can just have a
> child node.

As commented above, it can be DSI or any host which has child nodes
and are looking to find panel/bridge.

Thanks,
Jagan.

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

[Andrzej Hajda]

On 12.01.2022 11:14, Jagan Teki wrote:
> On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@cerno.tech> wrote:
>> On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
>>> Some OF graphs don't require 'port' or 'ports' to represent the
>>> downstream panel or bridge; instead it simply adds a child node
>>> on a given parent node.
>> All bindings using OF graph nodes require either port or ports.
>>
>> DSI Host however don't have to use the OF graph, and that's what you're
>> talking about.
> Yes, right now I can see DSI but this change is generic to any OF graph.
>
>>> 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 is neither a 'port' nor a 'ports'
>>> node.
>>>
>>> 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>;
>>>          };
>>> };
>> I can't see how that one makes sense. The endpoint seems to have a
>> single output, yet you also have a panel under it which is also an
>> output? You should have at least the virtual channel of that endpoint
>> somewhere to differentiate data between the panel and whatever is
>> connected on the other side of that endpoint.
> Same that I understood so far (based on v2 change), However we have
> exynos5433 has this pipeline and Andrzej mentioned it is valid
> pipeline on other thread.
>
> May be Andrzej, can give more conclusive evidence for it.


Hmm, this is DSI bridge (or encoder), which has one input (connected to 
mic, described by port 0) and one output (connected to DSI panel 
described by child relationship).

It looks for me quite natural.


Regards

Andrzej


>
>>> 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 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 | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>>> index 59d368ea006b..aeddd39b8df6 100644
>>> --- a/drivers/gpu/drm/drm_of.c
>>> +++ b/drivers/gpu/drm/drm_of.c
>>> @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>>>        if (panel)
>>>                *panel = NULL;
>>>
>>> +     /**
>>> +      * Some OF graphs don't require 'port' or 'ports' to represent the
>>> +      * downstream panel or bridge; instead it simply adds a child node
>>> +      * on a given parent node.
>> All OF graphs require either port or ports. DSI hosts can just have a
>> child node.
> As commented above, it can be DSI or any host which has child nodes
> and are looking to find panel/bridge.
>
> Thanks,
> Jagan.

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

[Maxime Ripard]

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

On Wed, Jan 12, 2022 at 12:45:23PM +0100, Andrzej Hajda wrote:
> 
> On 12.01.2022 11:14, Jagan Teki wrote:
> > On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> > > > Some OF graphs don't require 'port' or 'ports' to represent the
> > > > downstream panel or bridge; instead it simply adds a child node
> > > > on a given parent node.
> > > All bindings using OF graph nodes require either port or ports.
> > > 
> > > DSI Host however don't have to use the OF graph, and that's what you're
> > > talking about.
> > Yes, right now I can see DSI but this change is generic to any OF graph.
> > 
> > > > 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 is neither a 'port' nor a 'ports'
> > > > node.
> > > > 
> > > > 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>;
> > > >          };
> > > > };
> > > I can't see how that one makes sense. The endpoint seems to have a
> > > single output, yet you also have a panel under it which is also an
> > > output? You should have at least the virtual channel of that endpoint
> > > somewhere to differentiate data between the panel and whatever is
> > > connected on the other side of that endpoint.
> > Same that I understood so far (based on v2 change), However we have
> > exynos5433 has this pipeline and Andrzej mentioned it is valid
> > pipeline on other thread.
> > 
> > May be Andrzej, can give more conclusive evidence for it.
> 
> 
> Hmm, this is DSI bridge (or encoder), which has one input (connected to mic,
> described by port 0) and one output (connected to DSI panel described by
> child relationship).
> 
> It looks for me quite natural.

Yeah, the dsi_to_mic feels weird if DSI is the receiver here, but it
makes sense, thanks

Maxime

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

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

[Maxime Ripard]

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

On Wed, Jan 12, 2022 at 03:44:00PM +0530, Jagan Teki wrote:
> On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> > > Some OF graphs don't require 'port' or 'ports' to represent the
> > > downstream panel or bridge; instead it simply adds a child node
> > > on a given parent node.
> >
> > All bindings using OF graph nodes require either port or ports.
> >
> > DSI Host however don't have to use the OF graph, and that's what you're
> > talking about.
> 
> Yes, right now I can see DSI but this change is generic to any OF graph.

Not really? Generic to all users of drm_of_find_panel_or_bridge sure,
but DSI is the only use case where we could have bridges or panels that
would be child of the user.

> > > 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 is neither a 'port' nor a 'ports'
> > > node.
> > >
> > > 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>;
> > >         };
> > > };
> >
> > I can't see how that one makes sense. The endpoint seems to have a
> > single output, yet you also have a panel under it which is also an
> > output? You should have at least the virtual channel of that endpoint
> > somewhere to differentiate data between the panel and whatever is
> > connected on the other side of that endpoint.
> 
> Same that I understood so far (based on v2 change), However we have
> exynos5433 has this pipeline and Andrzej mentioned it is valid
> pipeline on other thread.
> 
> May be Andrzej, can give more conclusive evidence for it.
> 
> >
> > > 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 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 | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > index 59d368ea006b..aeddd39b8df6 100644
> > > --- a/drivers/gpu/drm/drm_of.c
> > > +++ b/drivers/gpu/drm/drm_of.c
> > > @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > >       if (panel)
> > >               *panel = NULL;
> > >
> > > +     /**
> > > +      * Some OF graphs don't require 'port' or 'ports' to represent the
> > > +      * downstream panel or bridge; instead it simply adds a child node
> > > +      * on a given parent node.
> >
> > All OF graphs require either port or ports. DSI hosts can just have a
> > child node.
> 
> As commented above, it can be DSI or any host which has child nodes
> and are looking to find panel/bridge.

DSI is the only case in this situation, but ok. It still has nothing to
do with OF graph. All bindings using OF graph will require either port
or ports, and the DSI binding doesn't mandate to use OF graph. So the
comment above is misleading at best.

Maxime

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

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

[Jagan Teki]

On Wed, Jan 12, 2022 at 6:37 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Jan 12, 2022 at 03:44:00PM +0530, Jagan Teki wrote:
> > On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> > > > Some OF graphs don't require 'port' or 'ports' to represent the
> > > > downstream panel or bridge; instead it simply adds a child node
> > > > on a given parent node.
> > >
> > > All bindings using OF graph nodes require either port or ports.
> > >
> > > DSI Host however don't have to use the OF graph, and that's what you're
> > > talking about.
> >
> > Yes, right now I can see DSI but this change is generic to any OF graph.
>
> Not really? Generic to all users of drm_of_find_panel_or_bridge sure,
> but DSI is the only use case where we could have bridges or panels that
> would be child of the user.
>
> > > > 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 is neither a 'port' nor a 'ports'
> > > > node.
> > > >
> > > > 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>;
> > > >         };
> > > > };
> > >
> > > I can't see how that one makes sense. The endpoint seems to have a
> > > single output, yet you also have a panel under it which is also an
> > > output? You should have at least the virtual channel of that endpoint
> > > somewhere to differentiate data between the panel and whatever is
> > > connected on the other side of that endpoint.
> >
> > Same that I understood so far (based on v2 change), However we have
> > exynos5433 has this pipeline and Andrzej mentioned it is valid
> > pipeline on other thread.
> >
> > May be Andrzej, can give more conclusive evidence for it.
> >
> > >
> > > > 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 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 | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > > index 59d368ea006b..aeddd39b8df6 100644
> > > > --- a/drivers/gpu/drm/drm_of.c
> > > > +++ b/drivers/gpu/drm/drm_of.c
> > > > @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > >       if (panel)
> > > >               *panel = NULL;
> > > >
> > > > +     /**
> > > > +      * Some OF graphs don't require 'port' or 'ports' to represent the
> > > > +      * downstream panel or bridge; instead it simply adds a child node
> > > > +      * on a given parent node.
> > >
> > > All OF graphs require either port or ports. DSI hosts can just have a
> > > child node.
> >
> > As commented above, it can be DSI or any host which has child nodes
> > and are looking to find panel/bridge.
>
> DSI is the only case in this situation, but ok. It still has nothing to
> do with OF graph. All bindings using OF graph will require either port
> or ports, and the DSI binding doesn't mandate to use OF graph. So the
> comment above is misleading at best.

Okay. How about this updated comment?

        /**
         * All bindings using OF graph will require either 'port' or 'ports'
         * to represent the downstream panel or bridge however the DSI binding
         * doesn't mandate to use OF graph; instead it simply adds a panel or
         * bridge in child node.
         *
         * Lookup that child node for a given parent however that child is
         * neither a 'port' nor a 'ports' node.
         */

Thanks,
Jagan.

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

[Maxime Ripard]

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

On Wed, Jan 12, 2022 at 11:03:55PM +0530, Jagan Teki wrote:
> On Wed, Jan 12, 2022 at 6:37 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Jan 12, 2022 at 03:44:00PM +0530, Jagan Teki wrote:
> > > On Wed, Jan 12, 2022 at 3:33 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > On Wed, Jan 12, 2022 at 12:01:52AM +0530, Jagan Teki wrote:
> > > > > Some OF graphs don't require 'port' or 'ports' to represent the
> > > > > downstream panel or bridge; instead it simply adds a child node
> > > > > on a given parent node.
> > > >
> > > > All bindings using OF graph nodes require either port or ports.
> > > >
> > > > DSI Host however don't have to use the OF graph, and that's what you're
> > > > talking about.
> > >
> > > Yes, right now I can see DSI but this change is generic to any OF graph.
> >
> > Not really? Generic to all users of drm_of_find_panel_or_bridge sure,
> > but DSI is the only use case where we could have bridges or panels that
> > would be child of the user.
> >
> > > > > 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 is neither a 'port' nor a 'ports'
> > > > > node.
> > > > >
> > > > > 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>;
> > > > >         };
> > > > > };
> > > >
> > > > I can't see how that one makes sense. The endpoint seems to have a
> > > > single output, yet you also have a panel under it which is also an
> > > > output? You should have at least the virtual channel of that endpoint
> > > > somewhere to differentiate data between the panel and whatever is
> > > > connected on the other side of that endpoint.
> > >
> > > Same that I understood so far (based on v2 change), However we have
> > > exynos5433 has this pipeline and Andrzej mentioned it is valid
> > > pipeline on other thread.
> > >
> > > May be Andrzej, can give more conclusive evidence for it.
> > >
> > > >
> > > > > 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 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 | 18 ++++++++++++++++++
> > > > >  1 file changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > > > > index 59d368ea006b..aeddd39b8df6 100644
> > > > > --- a/drivers/gpu/drm/drm_of.c
> > > > > +++ b/drivers/gpu/drm/drm_of.c
> > > > > @@ -249,6 +249,22 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > > > >       if (panel)
> > > > >               *panel = NULL;
> > > > >
> > > > > +     /**
> > > > > +      * Some OF graphs don't require 'port' or 'ports' to represent the
> > > > > +      * downstream panel or bridge; instead it simply adds a child node
> > > > > +      * on a given parent node.
> > > >
> > > > All OF graphs require either port or ports. DSI hosts can just have a
> > > > child node.
> > >
> > > As commented above, it can be DSI or any host which has child nodes
> > > and are looking to find panel/bridge.
> >
> > DSI is the only case in this situation, but ok. It still has nothing to
> > do with OF graph. All bindings using OF graph will require either port
> > or ports, and the DSI binding doesn't mandate to use OF graph. So the
> > comment above is misleading at best.
> 
> Okay. How about this updated comment?
> 
>         /**
>          * All bindings using OF graph will require either 'port' or 'ports'
>          * to represent the downstream panel or bridge however the DSI binding
>          * doesn't mandate to use OF graph; instead it simply adds a panel or
>          * bridge in child node.
>          *
>          * Lookup that child node for a given parent however that child is
>          * neither a 'port' nor a 'ports' node.
>          */

That's still a bit misleading. The OF graph is here to model the data
path, while the rest of the tree is about the control path (ie, which
bus you need to use to access one of the device registers).

So there's no equivalency between OF graph and child nodes. The first
one is here to represent where you get the pixels out, and the second
how you control that device.

Having a child node makes it obvious where the data is going though, so
it would be completely redundant to have both a child node and an OF
graph endpoint.

What about something like:

/*
 * Devices can also be child nodes when we also control that device through
 * the upstream device (ie, MIPI-DCS for a MIPI-DSI device).
 *
 * Search for a child node of the given parent that isn't either port or
 * ports.
 */

Maxime

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