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 DC245CA1018 for ; Sat, 31 Aug 2024 10:25:59 +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:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Ih8LT8l7YTynZUDZqg9sn3LBFAsvNNfGvvNbeyzO78M=; b=2Nle2TCgqZhuXTSpWMU1OFt0j7 eg7/9pNr8ubt53+Ls07HyjR4i/aptUuJ42IgpAjOnpb8+TdTbIq6iRyNyUDHysLh5XIkooZ7i8TnE Y8iXwZZJYoiMH0ty2w/Dzc0ZGnDH/WJ4pXVmj6NtsJl5yafsZG2BHc79XzCibkqI0YIS+jEQIC/xo O91lFAngRArmwVbhAYaeLRh6hNFe0tyM/FD3vwq93D9NJwQPcgy7ZGa8I6TW/GUhF8FPn+I9frzsf pRr/H/PQxV1K2+/7QyejX2laelGOXtht4W5iJkDT8fpZlW8tdqKkuWE4PjbDO7unwCO0/RKM4e0L5 BzEEi0CA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1skLIj-000000097Ai-11Ji; Sat, 31 Aug 2024 10:25:49 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1skLHq-0000000975w-42pS for linux-arm-kernel@lists.infradead.org; Sat, 31 Aug 2024 10:24:56 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 37E2DA4014D; Sat, 31 Aug 2024 10:24:46 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 19E4EC4CEC0; Sat, 31 Aug 2024 10:24:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1725099892; bh=JaObuWEZUK4FTWTxsswtHtStwRyczcdv8rTcV+HpaGw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=chdsIsiz5ob3VHxzLH2w1Ue1FUzWxdHf8pxZb/hEMYYHtKTACz1kk5v+tVOtEDhi3 QOK3MpTA5YITJixvJWIETE+nPnqIpqf5+xFatx4WtgBTLOfJs5wFTg+euCElNUSZ4z STrsTrKhWgcg7XgpOaeOyzPj9kkb6C6JEoWQYm/AtnN0pOWn4Dy+BAB1cmXVFVa6M0 xZXKht1h+0JRsmMVrGXxPb4deZ9UNN1uCN3SyFQWIBGQiLEOnJC3TtFiSsE6nu56nb r17wOuT47QqNhhcDRS8E9u9/RureflYXHNc+7ZLOE5pIbJbDFcCaKAAlMCfw8NDiXp A9V2K43lw9BSQ== Date: Sat, 31 Aug 2024 11:24:40 +0100 From: Jonathan Cameron To: Rob Herring Cc: Kuninori Morimoto , 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: <20240831112440.3fa997a1@jic23-huawei> In-Reply-To: References: <87cylwqa12.wl-kuninori.morimoto.gx@renesas.com> <87a5h0qa0g.wl-kuninori.morimoto.gx@renesas.com> <20240826154009.GA300981-robh@kernel.org> <87bk1ebz59.wl-kuninori.morimoto.gx@renesas.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240831_032455_160522_1B165F38 X-CRM114-Status: GOOD ( 41.85 ) 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 Tue, 27 Aug 2024 08:54:51 -0500 Rob Herring wrote: > +Jonathan C for the naming >=20 > On Mon, Aug 26, 2024 at 7:14=E2=80=AFPM Kuninori Morimoto > wrote: > > > > > > Hi Rob > > =20 > > > > We already have of_graph_get_next_endpoint(), but it is not > > > > intuitive to use in some case. =20 > > > > > > 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. =20 > > > > Hmm... > > of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port", > > but new helper doesn't have such feature. =20 >=20 > Right, but the "feature" is somewhat awkward as you said. You > generally should know what port you are operating on. >=20 > > Even though I try to replace it with new helper, I guess it will be > > almost same as current of_graph_get_next_endpoint() anyway. > > > > Alternative idea is... > > One of the big user of of_graph_get_next_endpoint() is > > for_each_endpoint_of_node() loop. > > > > So we can replace it to.. > > > > - for_each_endpoint_of_node(parent, endpoint) { > > + for_each_of_graph_port(parent, port) { > > + for_each_of_graph_port_endpoint(port, endpoint) { > > > > Above is possible, but it replaces single loop to multi loops. > > > > And, we still need to consider about of_fwnode_graph_get_next_endpoint() > > which is the last user of of_graph_get_next_endpoint() =20 >=20 > I missed fwnode_graph_get_next_endpoint() which has lots of users. > Though almost all of those are just "get the endpoint" and assume > there is only 1. In any case, it's a lot more than 3, so nevermind for > now. >=20 > > > > +struct device_node *of_graph_get_next_port_endpoint(const struct d= evice_node *port, > > > > + struct device_node = *prev) > > > > +{ > > > > + do { > > > > + prev =3D of_get_next_child(port, prev); > > > > + if (!prev) > > > > + break; > > > > + } while (!of_node_name_eq(prev, "endpoint")); =20 > > > > > > 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(). = =20 > > > > OK, will do in v4 > > =20 > > > > +/** > > > > + * for_each_of_graph_port_endpoint - iterate over every endpoint i= n 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 cal= led manually. =20 > > > > > > No need for this requirement anymore. Use cleanup.h so this is > > > automatic. =20 > > > > Do you mean it should include __free() inside this loop, like _scoped()= ? =20 >=20 > Yes. >=20 > > #define for_each_child_of_node_scoped(parent, child) \ > > for (struct device_node *child __free(device_node) =3D = \ > > of_get_next_child(parent, NULL); = \ > > child !=3D NULL; = \ > > child =3D of_get_next_child(parent, child)) > > > > In such case, I wonder does it need to have _scoped() in loop name ? =20 >=20 > Well, we added that to avoid changing all the users at once. >=20 > > And in such case, I think we want to have non _scoped() loop too ? =20 >=20 > Do we have a user? I don't think we need it because anywhere we need > the node iterator pointer outside the loop that can be done explicitly > (no_free_ptr()). >=20 > So back to the name, I don't think we need _scoped in it. I think if > any user treats the iterator like it's the old style, the compiler is > going to complain. Hmm. Up to you but I'd be concerned that the scoping stuff is non obvious enough that it is worth making people really really aware it is going on. However I don't feel strongly about it. For the other _scoped iterators there is some push back on the churn using them is causing so I doubt we'll ever get rid of the non scoped variants. For something new that's not a concern. Jonathan >=20 > > For example, when user want to use the param. > > > > for_each_of_graph_port_endpoint(port, endpoint) > > if (xxx =3D=3D yyy) > > return endpoint; > > > > for_each_of_graph_port_endpoint_scoped(port, endpoint) > > if (xxx =3D=3D yyy) > > return of_node_get(endpoint) =20 >=20 > Actually, you would do "return_ptr(endpoint)" here. >=20 > Rob