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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5DE6AC5321E for ; Mon, 26 Aug 2024 15:41:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=XoGKEjs7PvpSEK7BL3FgzxPyTLVlQjnBLUp2iAw1epg=; b=2cirs4mW8HqDchOxhKHCeQBKPk 5FI1eu/CRJYyb667vcMBlkrSvvUMYWongVMAgX/Oqydz5MwAQ8oJjdvGg8P+WUlj7ODNi/ESl7ylK vE3eW1uByvM7DJbVWyJ+PkcFtGye9f11KjBrilxzXmouaZDaBK+UDF6qDhVxTlCpQsH/FzUrj+NKC JVMwc8n1MEpQWgIvJw2ut5SeX5Jwlh5X3ZxpH/khqZfI9XVQ7l4raFTswmC7tHXqD9evtgZs7veVG 34kie7dUXmlCaaldYpx1G9q3GN5aZ1BVVkviFi+qnKdJeQCNE+IeroE3xZdDgjrqiuhHlDP2LmdvT vPcKvECw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sibq3-00000007rNx-0Ik1; Mon, 26 Aug 2024 15:41:03 +0000 Received: from nyc.source.kernel.org ([2604:1380:45d1:ec00::3]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sibpG-00000007rEa-1hbI for linux-arm-kernel@lists.infradead.org; Mon, 26 Aug 2024 15:40:15 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 2C29AA4141A; Mon, 26 Aug 2024 15:40:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7F7E6C52FC1; Mon, 26 Aug 2024 15:40:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1724686812; bh=3nPEGv1ScbaB4qHxVjJQ7z0P5oNlCzTkuKeiIaD8G4U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tXEis2oZr+7TJtq6SLWNyCtyZ3RaEWuaa3hHuJShWSnlk4zDlfSTBrLQLLmFddZew yz0j/PDCq4oPjmR4v7fRrPpN1MW0KMRcFRqxLJi7DX+frL/fWFhQJSqygHgo6TAmdY TA3oqAoRjxnlgbPwyWiuG3Ysd1bYMo0OPUfmuyzL7w8xlFurcr8pz/XzrOZ12at/yE 9WpTKkU8bcNvifnyAodtkkSNOFALcUeaqHbsCmu3tdw8wVjKV8seaPVmig2fYNSn6C 2M6yQcWpC3uCJrxYhdNJVVLD2Q9IubAzGQafk4T0uBAJvu8mYUxQzfKrMVzKU50db9 DbkRhZXNAkNjQ== Date: Mon, 26 Aug 2024 10:40:09 -0500 From: Rob Herring To: Kuninori Morimoto Cc: Daniel Vetter , David Airlie , Helge Deller , Jaroslav Kysela , Laurent Pinchart , Liam Girdwood , Maarten Lankhorst , Mark Brown , Mauro Carvalho Chehab , Maxime Ripard , Michal Simek , Saravana Kannan , Takashi Iwai , Thomas Zimmermann , Tomi Valkeinen , devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org, linux-media@vger.kernel.org, linux-omap@vger.kernel.org, linux-sound@vger.kernel.org, Sakari Ailus Subject: Re: [PATCH v3 2/9] of: property: add of_graph_get_next_port_endpoint() Message-ID: <20240826154009.GA300981-robh@kernel.org> References: <87cylwqa12.wl-kuninori.morimoto.gx@renesas.com> <87a5h0qa0g.wl-kuninori.morimoto.gx@renesas.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a5h0qa0g.wl-kuninori.morimoto.gx@renesas.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240826_084014_598909_F0AC0847 X-CRM114-Status: GOOD ( 40.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote: > We already have of_graph_get_next_endpoint(), but it is not > intuitive to use in some case. Can of_graph_get_next_endpoint() users be replaced with your new helpers? I'd really like to get rid of the 3 remaining users. > > (X) node { > (Y) ports { > (P0) port@0 { endpoint { remote-endpoint = ...; };}; > (P10) port@1 { endpoint { remote-endpoint = ...; }; > (P11) endpoint { remote-endpoint = ...; };}; > (P2) port@2 { endpoint { remote-endpoint = ...; };}; > }; > }; > > For example, if I want to handle port@1's 2 endpoints (= P10, P11), > I want to use like below > > P10 = of_graph_get_next_endpoint(port1, NULL); > P11 = of_graph_get_next_endpoint(port1, P10); > > But 1st one will be error, because of_graph_get_next_endpoint() > requested "parent" means "node" (X) or "ports" (Y), not "port". > Below works, but it will get P0 > > /* These will be node/ports/port@0/endpoint */ > P0 = of_graph_get_next_endpoint(node, NULL); > P0 = of_graph_get_next_endpoint(ports, NULL); > > In other words, we can't handle P10/P11 directly via > of_graph_get_next_endpoint() so far. > > There is another non intuitive behavior on of_graph_get_next_endpoint(). > In case of if I could get P10 pointer for some way, and if I want to > handle port@1 things, I would like use it like below > > /* > * "ep" is now P10, and handle port1 things here, > * but we don't know how many endpoints port1 has. > * > * Because "ep" is non NULL now, we can use port1 > * as of_graph_get_next_endpoint(port1, xxx) > */ > do { > /* do something for port1 specific things here */ > } while (ep = of_graph_get_next_endpoint(port1, ep)) > > But it also not worked as I expected. > I expect it will be P10 -> P11 -> NULL, > but it will be P10 -> P11 -> P2, because > of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port". > > It is not useful on generic driver. > It uses of_get_next_child() instead for now, but it is not intuitive. > And it doesn't check node name (= "endpoint"). > > To handle endpoint more intuitive, create of_graph_get_next_port_endpoint() > > of_graph_get_next_port_endpoint(port1, NULL); // P10 > of_graph_get_next_port_endpoint(port1, P10); // P11 > of_graph_get_next_port_endpoint(port1, P11); // NULL > > Signed-off-by: Kuninori Morimoto > --- > drivers/of/property.c | 22 ++++++++++++++++++++++ > include/linux/of_graph.h | 20 ++++++++++++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index aec6ac9f70064..90820e43bc973 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -719,6 +719,28 @@ struct device_node *of_graph_get_next_port(struct device_node *parent, > } > EXPORT_SYMBOL(of_graph_get_next_port); > > +/** > + * of_graph_get_next_port_endpoint() - get next endpoint node in port. > + * If it reached to end of the port, it will return NULL. > + * @port: pointer to the target port node > + * @prev: previous endpoint node, or NULL to get first > + * > + * Return: An 'endpoint' node pointer with refcount incremented. Refcount > + * of the passed @prev node is decremented. > + */ > +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port, > + struct device_node *prev) > +{ > + do { > + prev = of_get_next_child(port, prev); > + if (!prev) > + break; > + } while (!of_node_name_eq(prev, "endpoint")); Really, this check is validation as no other name is valid in a port node. The kernel is not responsible for validation, but okay. However, if we are going to keep this, might as well make it WARN(). > + > + return prev; > +} > +EXPORT_SYMBOL(of_graph_get_next_port_endpoint); > + > /** > * of_graph_get_next_endpoint() - get next endpoint node > * @parent: pointer to the parent device node > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index a6b91577700a8..967ee14a1ff37 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -59,6 +59,17 @@ struct of_endpoint { > for (child = of_graph_get_next_port(parent, NULL); child != NULL; \ > child = of_graph_get_next_port(parent, child)) > > +/** > + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node > + * @parent: parent port node > + * @child: loop variable pointing to the current endpoint node > + * > + * When breaking out of the loop, of_node_put(child) has to be called manually. No need for this requirement anymore. Use cleanup.h so this is automatic. > + */ > +#define for_each_of_graph_port_endpoint(parent, child) \ > + for (child = of_graph_get_next_port_endpoint(parent, NULL); child != NULL; \ > + child = of_graph_get_next_port_endpoint(parent, child)) > + > #ifdef CONFIG_OF > bool of_graph_is_present(const struct device_node *node); > int of_graph_parse_endpoint(const struct device_node *node, > @@ -72,6 +83,8 @@ struct device_node *of_graph_get_next_ports(struct device_node *parent, > struct device_node *ports); > struct device_node *of_graph_get_next_port(struct device_node *parent, > struct device_node *port); > +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port, > + struct device_node *prev); > struct device_node *of_graph_get_endpoint_by_regs( > const struct device_node *parent, int port_reg, int reg); > struct device_node *of_graph_get_remote_endpoint( > @@ -132,6 +145,13 @@ static inline struct device_node *of_graph_get_next_port( > return NULL; > } > > +static inline struct device_node *of_graph_get_next_port_endpoint( > + const struct device_node *parent, > + struct device_node *previous) > +{ > + return NULL; > +} > + > static inline struct device_node *of_graph_get_endpoint_by_regs( > const struct device_node *parent, int port_reg, int reg) > { > -- > 2.43.0 >