* [PATCH v2] drm: of: Lookup if child node has panel or bridge @ 2021-12-13 12:16 Jagan Teki 2021-12-13 12:28 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Jagan Teki @ 2021-12-13 12:16 UTC (permalink / raw) To: Rob Herring, Frank Rowand, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Laurent Pinchart, Linus Walleij, Andrzej Hajda, Marek Szyprowski Cc: devicetree, linux-amarula, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge 2021-12-13 12:16 [PATCH v2] drm: of: Lookup if child node has panel or bridge Jagan Teki @ 2021-12-13 12:28 ` Laurent Pinchart 2021-12-13 13:09 ` Maxime Ripard 2022-01-12 9:44 ` Jagan Teki 0 siblings, 2 replies; 7+ messages in thread From: Laurent Pinchart @ 2021-12-13 12:28 UTC (permalink / raw) To: Jagan Teki Cc: Rob Herring, Frank Rowand, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Linus Walleij, Andrzej Hajda, Marek Szyprowski, devicetree, linux-amarula 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge 2021-12-13 12:28 ` Laurent Pinchart @ 2021-12-13 13:09 ` Maxime Ripard 2021-12-13 13:35 ` Laurent Pinchart 2022-01-12 9:44 ` Jagan Teki 1 sibling, 1 reply; 7+ messages in thread From: Maxime Ripard @ 2021-12-13 13:09 UTC (permalink / raw) To: Laurent Pinchart Cc: Jagan Teki, Rob Herring, Frank Rowand, Maarten Lankhorst, Thomas Zimmermann, Linus Walleij, Andrzej Hajda, Marek Szyprowski, devicetree, linux-amarula [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge 2021-12-13 13:09 ` Maxime Ripard @ 2021-12-13 13:35 ` Laurent Pinchart 2021-12-13 13:42 ` Maxime Ripard 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2021-12-13 13:35 UTC (permalink / raw) To: Maxime Ripard Cc: Jagan Teki, Rob Herring, Frank Rowand, Maarten Lankhorst, Thomas Zimmermann, Linus Walleij, Andrzej Hajda, Marek Szyprowski, devicetree, linux-amarula 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge 2021-12-13 13:35 ` Laurent Pinchart @ 2021-12-13 13:42 ` Maxime Ripard 2022-01-12 10:13 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Maxime Ripard @ 2021-12-13 13:42 UTC (permalink / raw) To: Laurent Pinchart Cc: Jagan Teki, Rob Herring, Frank Rowand, Maarten Lankhorst, Thomas Zimmermann, Linus Walleij, Andrzej Hajda, Marek Szyprowski, devicetree, linux-amarula [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge 2021-12-13 13:42 ` Maxime Ripard @ 2022-01-12 10:13 ` Laurent Pinchart 0 siblings, 0 replies; 7+ messages in thread From: Laurent Pinchart @ 2022-01-12 10:13 UTC (permalink / raw) To: Maxime Ripard Cc: Jagan Teki, Rob Herring, Frank Rowand, Maarten Lankhorst, Thomas Zimmermann, Linus Walleij, Andrzej Hajda, Marek Szyprowski, devicetree, linux-amarula 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge 2021-12-13 12:28 ` Laurent Pinchart 2021-12-13 13:09 ` Maxime Ripard @ 2022-01-12 9:44 ` Jagan Teki 1 sibling, 0 replies; 7+ messages in thread From: Jagan Teki @ 2022-01-12 9:44 UTC (permalink / raw) To: Laurent Pinchart Cc: Rob Herring, Frank Rowand, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Linus Walleij, Andrzej Hajda, Marek Szyprowski, devicetree, linux-amarula 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-12 10:13 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-13 12:16 [PATCH v2] drm: of: Lookup if child node has panel or bridge Jagan Teki 2021-12-13 12:28 ` Laurent Pinchart 2021-12-13 13:09 ` Maxime Ripard 2021-12-13 13:35 ` Laurent Pinchart 2021-12-13 13:42 ` Maxime Ripard 2022-01-12 10:13 ` Laurent Pinchart 2022-01-12 9:44 ` Jagan Teki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).