From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37369C433FE for ; Mon, 13 Dec 2021 13:35:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232936AbhLMNfs (ORCPT ); Mon, 13 Dec 2021 08:35:48 -0500 Received: from perceval.ideasonboard.com ([213.167.242.64]:51940 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237021AbhLMNfs (ORCPT ); Mon, 13 Dec 2021 08:35:48 -0500 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E3E6E51C; Mon, 13 Dec 2021 14:35:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1639402547; bh=qI+S6XwPUTIapQMkgxu0L3azrMrSTAKv3eEMopD3Dyc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ql4roTcdB0gT62GgBDWjv327wjVtaoocty2bYDxcraWZwy+TQE+BF6fZc4exrCCQY EmSCqitHkYZrGOKzHNWUo5sH2knW2N/LVoYOXuqppAobknCnvLDRDojtBxsZaOX8f4 iIU833zWI0vHXuuMzG/v9jXNr6k9lwDIfkUuHsf0= Date: Mon, 13 Dec 2021 15:35:16 +0200 From: Laurent Pinchart To: Maxime Ripard Cc: Jagan Teki , Rob Herring , Frank Rowand , Maarten Lankhorst , Thomas Zimmermann , Linus Walleij , Andrzej Hajda , Marek Szyprowski , devicetree@vger.kernel.org, linux-amarula@amarulasolutions.com Subject: Re: [PATCH v2] drm: of: Lookup if child node has panel or bridge Message-ID: References: <20211213121613.3377432-1-jagan@amarulasolutions.com> <20211213130936.oz2qywi773dhh3cr@houat> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20211213130936.oz2qywi773dhh3cr@houat> Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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 = ; > > > }; > > > > > > 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 = ; > > > }; > > > > > > 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 > > > --- > > > 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