Message ID | 87bk95klgc.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from am.mirrors.kernel.org ([147.75.80.249]) by linuxtv.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from <linux-media+bounces-4306-patchwork=linuxtv.org@vger.kernel.org>) id 1rUFvg-0001DJ-2i for patchwork@linuxtv.org; Mon, 29 Jan 2024 00:55:21 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id D30EE1F231EE for <patchwork@linuxtv.org>; Mon, 29 Jan 2024 00:55:12 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id BC0E08BFB; Mon, 29 Jan 2024 00:55:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=renesas.com header.i=@renesas.com header.b="HGjJLnxG" X-Original-To: linux-media@vger.kernel.org Received: from JPN01-TYC-obe.outbound.protection.outlook.com (mail-tycjpn01on2099.outbound.protection.outlook.com [40.107.114.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2F819944E; Mon, 29 Jan 2024 00:55:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.114.99 ARC-Seal: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706489704; cv=fail; b=BzI+SRk13oOWj4H5VO7h7MqRdLwHdTu3ZAN7iLO7DDGTAhbtxld11m/WJwIHxWexOn6jRgyd1TuaozxYQOhVAYBIJCk0q+5n2zo8l6O8pps42Ge+4z7+IkThZ0bnk4Tki6egoxrGLVLXJacJRC0ajYbZD9LGwQR4OEo+0m5hrco= ARC-Message-Signature: i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706489704; c=relaxed/simple; bh=+1l+NQrPhoy6N0GQOXrzb6DuHsMWeTvod/7F91rB2Fk=; h=Message-ID:From:Subject:To:Cc:In-Reply-To:References:Content-Type: Date:MIME-Version; b=chbvf+QdDwkUqGOQsvW8lYF2AgRnMkiBf1v1QCSFAKmiSs5ItRpJb+fs3EOBg/Vp/MscogJuzr8+3fZySjROewW9AJHIyaxsUzkBMCwiUzrXVjQpSg6Xm16r6HtZcphGuePVSEPsOpYLKbex4Z9yhjyZZumATS5/2GjswZW6JFM= ARC-Authentication-Results: i=2; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=renesas.com; spf=pass smtp.mailfrom=renesas.com; dkim=pass (1024-bit key) header.d=renesas.com header.i=@renesas.com header.b=HGjJLnxG; arc=fail smtp.client-ip=40.107.114.99 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=renesas.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=renesas.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UjCptBsOMipXeEQODGhbjl72Mq7gHBVPgMXQCxLplArrLNuWMOZcOFCNMXqc5tPItdZRvhq78oPcb69m/BYYL/gZLeRSfP5W40h3iJbGYIVK5GbkGh9lONZN2vxI/SF6r3hVJyYOEp+WTHLpHzNYo+i9uV3iz4uD1+mczW6NvMDZZtXn9Zvhp1gdJljmD+Z2RlPXVfarm+Y+uHuIN6g9areEDsh7HTbI9UlGH4DWl92azYct7OBFvrYRnB59IPNQjg8k+yHu6U5OxnDW6D5L9mUymCkuD6JyJBOGuWx5/3BfQmNVvdq+YOpA2W2S2y8MphX2MU7usUXpnH6PLo7gIQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=53LgFL/tgEMW5iVClRMNKc/fATDPCwYVOacfnZCL7e4=; b=CkJu+LpDx0uTHIVSdCm0cjKQxlS+fAufpu2fl8ribzcnLBXg0IJFxzxG77ExIbkGZqPKBt18bLB6GNbXYeGUJ9x9cg39Qk2kbd5qKS0EqWNPdXljua3sCxKIuKvCoeln2Aa8R1vZRuUSuuxGdJjD3J1cT6m/iGIeeDkoPkWZefJLFbO3R+yG8WBlO6PA7/fH1HiK+FQKKBGDNMWoTJYljBZe8rSdOXnx502KHpOUGNVNTpgguc1x71V3UYIQfllp/uW2miTSDatryI59VZc19+XcrepN2iWhhvMoRijjd+x7Y8vm/qDPzNM82X4rB82jNtIW24TbkLVJESSYdoj/IA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=renesas.com; dmarc=pass action=none header.from=renesas.com; dkim=pass header.d=renesas.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=renesas.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=53LgFL/tgEMW5iVClRMNKc/fATDPCwYVOacfnZCL7e4=; b=HGjJLnxGqABlsvQ0/wYSEkVr5qX+tMMni2gr4zTLLfVpFEZhbjQ0Q4JdH5z6lRJVEP1MNPE4sUa+XCMJFn6Sa3JbpRANoaZFCItljYt8xcWd+QV+tYO6WfpcYuOMBKqp9pHHT5qFjm+2YY3gV+FUcZynLDQ8ePPmE9XyVZQULBw= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=renesas.com; Received: from TYCPR01MB10914.jpnprd01.prod.outlook.com (2603:1096:400:3a9::11) by TYAPR01MB5996.jpnprd01.prod.outlook.com (2603:1096:402:34::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7249.20; Mon, 29 Jan 2024 00:54:59 +0000 Received: from TYCPR01MB10914.jpnprd01.prod.outlook.com ([fe80::ce8:8f5e:99a0:aba4]) by TYCPR01MB10914.jpnprd01.prod.outlook.com ([fe80::ce8:8f5e:99a0:aba4%2]) with mapi id 15.20.7249.017; Mon, 29 Jan 2024 00:54:59 +0000 Message-ID: <87bk95klgc.wl-kuninori.morimoto.gx@renesas.com> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Subject: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw() User-Agent: Wanderlust/2.15.9 Emacs/27.1 Mule/6.0 To: =?iso-8859-1?q?=22Uwe_Kleine-K=C3=B6nig=22?= <u.kleine-koenig@pengutronix.de>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>, Frank Rowand <frowand.list@gmail.com>, Helge Deller <deller@gmx.de>, Jaroslav Kysela <perex@perex.cz>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Liam Girdwood <lgirdwood@gmail.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Mark Brown <broonie@kernel.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Maxime Ripard <mripard@kernel.org>, Michal Simek <michal.simek@amd.com>, Rob Herring <robh+dt@kernel.org>, Saravana Kannan <saravanak@google.com>, Takashi Iwai <tiwai@suse.com>, Thomas Zimmermann <tzimmermann@suse.de>, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Cc: alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-media@vger.kernel.org, linux-sound@vger.kernel.org In-Reply-To: <87fryhklhb.wl-kuninori.morimoto.gx@renesas.com> References: <87fryhklhb.wl-kuninori.morimoto.gx@renesas.com> Content-Type: text/plain; charset=US-ASCII Date: Mon, 29 Jan 2024 00:54:59 +0000 X-ClientProxiedBy: TYCP286CA0351.JPNP286.PROD.OUTLOOK.COM (2603:1096:405:7c::20) To TYCPR01MB10914.jpnprd01.prod.outlook.com (2603:1096:400:3a9::11) Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: TYCPR01MB10914:EE_|TYAPR01MB5996:EE_ X-MS-Office365-Filtering-Correlation-Id: d2462796-9874-4e9b-a8e3-08dc2064eb43 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 5ET4Fe56R4WpB3j55apOMp2zcpaUyZKou2otdPfIpadTY5+Jy/kNcWhavxVfTc+HQcmskCg1jbnlkPrreQ5o6FgoqXrv2YYc5OJDYMlllLrr4q9m3ye3lTtZx06VlJVQJNaUfa7SSXEW6yiQQzfho44ZiMBJmWOdj7jb2DPEh3WRgfeegiSmPa4xVsuJbLEyY0PiGrAHXF0wguWxJNVO/JEAMnUWgQOyZt4eInoQ390MN2Lt3QyDauUeyqUw1QvBZuVsb30WcICQXJXcsEsJ21IGNkqbEnnMFe/lCHpRtH/EYT5Hl85MKGxU6YooulLLRcEwssPPws1PM5HINQqEm5oohJI/oiBx9xyfbDibfNHPIQR/OMVPeQ4L4nlXNkHKJTTrmrXXugtLTG/p6lirTPgZWyXx1Yo60WaJtFOshb3T1KpAm91r1W4/8a3+LvRCTovsZ8GL3QGXiQx5e1ObA0SuDS0mZN5JOtWuAMWElq3lHXwYzBoRfMOHsNh9h1LVGTzPC7qJ8SSTWp2J7b9Nc6c8COUFEfjVohMy7sqyTMU2B3S0MjJ3KMRJrIH4kLgcdw0W2b3+DZcemTeubxopUshxbC7XRCx9A75rMAAw7GnM4U23SKLDeZSNC3QMn9bV1MowAiObYkXLanpXh8Q+D0tkmYkK1mD60utZcNngwbE= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:TYCPR01MB10914.jpnprd01.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(366004)(136003)(346002)(396003)(39860400002)(376002)(230922051799003)(451199024)(186009)(1800799012)(64100799003)(83380400001)(6512007)(7416002)(6486002)(5660300002)(2616005)(6506007)(52116002)(478600001)(26005)(38100700002)(4326008)(8936002)(8676002)(41300700001)(38350700005)(36756003)(86362001)(2906002)(66946007)(316002)(921011)(66556008)(110136005)(66476007)(21314003);DIR:OUT;SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: k0zrijg3C8GA1OUrNQlE6EuPKCK8jL2QTQB2Prcm0UwEb2+JHSynD466HVkAifn7rZjgewOW2ld7ZGxC8u/2K6GX+abs5qP16vJN1BbsbK7Ctp9eWUBgl9DEChWJIUTZgUFrE2hXqsJl9Pd70/uKmq5L5G3LdJczW8QU2uyDo/HN7DU/mcV8xcUk+3lkZ1r5V4BCK7GQyW0NXzMc7DqiLPweI6QSEJrvrlNM0gmoweiPrnPF90lye3Mh5NFoBJk5UpugOqNZ8ZTUTabz1DdPuHuvnexBTk4/8dzaYcyqADPYMRM5erGVLx+20251Hc55YLUGX3jE9lr8ZREg2bdc31/a1fwQOptxqLWJtDETCVJ5EvnbaI2dJ3+sf8qeDCn4xO5o3WZlQLeQbbi71E9rNl8GzVELAWgcbmb+5nWcLT+n2GmWdkaNl+FmelSnCl5Ftr3/wDMu5n+5N0xPjIu8jjRqzGnIacNlZ5m1lTbwrKsSxhagpRilL8JKBEUgpKk95owAXB4UzT1EjEFAX5QQWVjeLpLtg6o5zap40iqHaVoRHMBmRD9qlJjogUCjDH0M6AMeO04pib8bPCCAv9tDjnLSCIk8eIrp6VG6pVx7rtDdP83ozIrYSHOLU0hAitrhicYcSl15HaR2lJW/a9Ma0hYQtJ6n6JVMYZPy5iuh6rrMNxq4whGeSxQj9WFDzvp5VtR6gvYuvhuc6Qf2stfulHi/IXZDQV7syqBXOO6LIbM8UKeyniTqhST9tOMjSspqbyojvpjN6ecNR28koUcFAli2pNrFnU1465BsPXcPbFvkeQUGTQj2b7SCDejai5E5F2sEZNB3XsoXWQP4IoWJhI44cvjdabFKV1IFca9wEww6ya2ZQQZvNQwpAFBE2wqf9LniAVo86uQmZGzE4Vp4vn2Mp6Flum6Qq6dp53iMDqGBsMzsDY7E26JlyrrLIwl3X+bgVS5nkhcvHM0OQFezuwVWs2M872ER/wuBtgWZPzBKqtMpzoVpKshDBpRZ38nE07n/85G7okXuzBLY6W4yCPoj+FDymCUTrdHaZ4gZMwAB9cp/09lssxMBqRSbAvVxR+dCJ+ydOA4AIXAozxLruihEq+WTq6bZ2BySg52bOT9FPl6pEu/3N662WrmoobZYAxlJdx1ErOQeBoOhkt3hiJbhMPNqnbBWTlr2fYDgwcZofi/XDshJu04kus0FChWiHJqdqGs3TkB1trx/3ITxRJRwJaCZmlGnFoHSS92rFKYEjX9faK+PFi5ve/m9pVetxB4By1smBS+g0hRJWBx6YeWNKTjl+ohxacf/YIQYkof6ppkeJUD/uEX+o59D09s+13GlQvfjesR+z01THko7wHIobZ9UsZL2wFw7gqaJAWBktxtwN49puthXmkz8UJsWT6QIJ8XeyjkBtfK/dD9SG7kL3uzwDcyYgJkVIC/p4URtMl5j9FuLZ4M8uC4iXXmgrn+rwXxV3OPRL3GR2RWvS9hoNPkygWChAiaeBUPYw6E9xsC3MVYcvGS30ZZLoEkiRgJDV25XgCFMd9TCGEPh+EXQ7zbz1uNA/36GDwF7UTgwRSWCXjBqbD6X3u+oLTgnOqpyP4nmDD7/7w6bgJ6jsqdGynaLbB5qMcKKJRHX9Kc= X-OriginatorOrg: renesas.com X-MS-Exchange-CrossTenant-Network-Message-Id: d2462796-9874-4e9b-a8e3-08dc2064eb43 X-MS-Exchange-CrossTenant-AuthSource: TYCPR01MB10914.jpnprd01.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 00:54:59.6862 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 53d82571-da19-47e4-9cb4-625a166a4a2a X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: aKzr4mCHETD2SwEsu1d2aEmm/BzaizXtPSq2h3+y6AAB/cqrtVa5qjl5FJ3PnrOU84LUz22GZ7M4XOuTem4cwoJc5hiHkYr7Hq8Zwq6KovEMtR+RdlIAZ8ngjVkfDZr3 X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYAPR01MB5996 X-LSpam-Score: -3.0 (---) X-LSpam-Report: No, score=-3.0 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DMARC_PASS=-0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=ham autolearn_force=no |
Series |
of: property: add port base loop
|
|
Commit Message
Kuninori Morimoto
Jan. 29, 2024, 12:54 a.m. UTC
We already have of_graph_get_next_endpoint(), but it is not intuitive
to use.
(X) node {
(Y) ports {
port@0 { endpoint { remote-endpoint = ...; };};
(A1) port@1 { endpoint { remote-endpoint = ...; };
(A2) endpoint { remote-endpoint = ...; };};
(B) port@2 { endpoint { remote-endpoint = ...; };};
};
};
For example, if I want to handle port@1's 2 endpoints (= A1, A2),
I want to use like below
A1 = of_graph_get_next_endpoint(port1, NULL);
A2 = of_graph_get_next_endpoint(port1, A1);
But 1st one will be error, because of_graph_get_next_endpoint() requested
"parent" means "node" (X) or "ports" (Y), not "port".
Below are OK
of_graph_get_next_endpoint(node, NULL); // node/ports/port@0/endpoint
of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint
In other words, we can't handle A1/A2 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 A1 pointer for some way, and if I want to
handle port@1 things, I would like use it like below
/*
* "endpoint" is now A1, and handle port1 things here,
* but we don't know how many endpoints port1 has.
*
* Because "endpoint" is non NULL, we can use port1
* as of_graph_get_next_endpoint(port1, xxx)
*/
do {
/* do something for port1 specific things here */
} while (endpoint = of_graph_get_next_endpoint(port1, endpoint))
But it also not worked as I expected.
I expect it will be A1 -> A2 -> NULL,
but it will be A1 -> A2 -> B, because of_graph_get_next_endpoint()
will fetch endpoint beyond the port.
It is not useful on generic driver like Generic Sound Card.
It uses of_get_next_child() instead for now, but it is not intuitive,
and not check node name (= "endpoint").
To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw()
of_graph_get_next_endpoint_raw(port1, NULL); // A1
of_graph_get_next_endpoint_raw(port1, A1); // A2
of_graph_get_next_endpoint_raw(port1, A2); // NULL
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
drivers/of/property.c | 26 +++++++++++++++++++++++++-
include/linux/of_graph.h | 2 ++
2 files changed, 27 insertions(+), 1 deletion(-)
Comments
On 29/01/2024 02:54, Kuninori Morimoto wrote: > We already have of_graph_get_next_endpoint(), but it is not intuitive > to use. > > (X) node { > (Y) ports { > port@0 { endpoint { remote-endpoint = ...; };}; > (A1) port@1 { endpoint { remote-endpoint = ...; }; > (A2) endpoint { remote-endpoint = ...; };}; > (B) port@2 { endpoint { remote-endpoint = ...; };}; > }; > }; > > For example, if I want to handle port@1's 2 endpoints (= A1, A2), > I want to use like below > > A1 = of_graph_get_next_endpoint(port1, NULL); > A2 = of_graph_get_next_endpoint(port1, A1); > > But 1st one will be error, because of_graph_get_next_endpoint() requested > "parent" means "node" (X) or "ports" (Y), not "port". > Below are OK > > of_graph_get_next_endpoint(node, NULL); // node/ports/port@0/endpoint > of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint > > In other words, we can't handle A1/A2 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 A1 pointer for some way, and if I want to > handle port@1 things, I would like use it like below > > /* > * "endpoint" is now A1, and handle port1 things here, > * but we don't know how many endpoints port1 has. > * > * Because "endpoint" is non NULL, we can use port1 > * as of_graph_get_next_endpoint(port1, xxx) > */ > do { > /* do something for port1 specific things here */ > } while (endpoint = of_graph_get_next_endpoint(port1, endpoint)) > > But it also not worked as I expected. > I expect it will be A1 -> A2 -> NULL, > but it will be A1 -> A2 -> B, because of_graph_get_next_endpoint() > will fetch endpoint beyond the port. > > It is not useful on generic driver like Generic Sound Card. > It uses of_get_next_child() instead for now, but it is not intuitive, > and not check node name (= "endpoint"). > > To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw() > > of_graph_get_next_endpoint_raw(port1, NULL); // A1 > of_graph_get_next_endpoint_raw(port1, A1); // A2 > of_graph_get_next_endpoint_raw(port1, A2); // NULL > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > drivers/of/property.c | 26 +++++++++++++++++++++++++- > include/linux/of_graph.h | 2 ++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 14ffd199c9b1..37dbb1b0e742 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent, > } > EXPORT_SYMBOL(of_graph_get_next_port); > > +/** > + * of_graph_get_next_endpoint_raw() - get next endpoint node How about "of_graph_get_next_port_endpoint()"? > + * @port: pointer to the target port node > + * @endpoint: current endpoint node, or NULL to get first > + * > + * Return: An 'endpoint' node pointer with refcount incremented. Refcount > + * of the passed @prev node is decremented. > + */ It might be good to highlight here the difference to the of_graph_get_next_endpoint(). > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > + struct device_node *endpoint) > +{ > + if (!port) > + return NULL; > + > + do { > + endpoint = of_get_next_child(port, endpoint); > + if (!endpoint) > + break; > + } while (!of_node_name_eq(endpoint, "endpoint")); > + > + return endpoint; > +} > +EXPORT_SYMBOL(of_graph_get_next_endpoint_raw); > + > /** > * of_graph_get_next_endpoint() - get next endpoint node > * @parent: pointer to the parent device node > @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > * getting the next child. If the previous endpoint is NULL this > * will return the first child. > */ > - endpoint = of_get_next_child(port, prev); > + endpoint = of_graph_get_next_endpoint_raw(port, prev); > if (endpoint) { > of_node_put(port); > return endpoint; > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index fff598640e93..427905a6e8c3 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -57,6 +57,8 @@ int of_graph_get_port_count(const struct device_node *np); > struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id); > struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > struct device_node *previous); > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > + struct device_node *prev); > struct device_node *of_graph_get_next_port(const struct device_node *parent, > struct device_node *previous); > struct device_node *of_graph_get_endpoint_by_regs(
On Mon, Jan 29, 2024 at 02:29:22PM +0200, Tomi Valkeinen wrote: > On 29/01/2024 02:54, Kuninori Morimoto wrote: > > We already have of_graph_get_next_endpoint(), but it is not intuitive > > to use. > > > > (X) node { > > (Y) ports { > > port@0 { endpoint { remote-endpoint = ...; };}; > > (A1) port@1 { endpoint { remote-endpoint = ...; }; > > (A2) endpoint { remote-endpoint = ...; };}; > > (B) port@2 { endpoint { remote-endpoint = ...; };}; > > }; > > }; > > > > For example, if I want to handle port@1's 2 endpoints (= A1, A2), > > I want to use like below > > > > A1 = of_graph_get_next_endpoint(port1, NULL); > > A2 = of_graph_get_next_endpoint(port1, A1); > > > > But 1st one will be error, because of_graph_get_next_endpoint() requested > > "parent" means "node" (X) or "ports" (Y), not "port". > > Below are OK > > > > of_graph_get_next_endpoint(node, NULL); // node/ports/port@0/endpoint > > of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint > > > > In other words, we can't handle A1/A2 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 A1 pointer for some way, and if I want to > > handle port@1 things, I would like use it like below > > > > /* > > * "endpoint" is now A1, and handle port1 things here, > > * but we don't know how many endpoints port1 has. > > * > > * Because "endpoint" is non NULL, we can use port1 > > * as of_graph_get_next_endpoint(port1, xxx) > > */ > > do { > > /* do something for port1 specific things here */ > > } while (endpoint = of_graph_get_next_endpoint(port1, endpoint)) > > > > But it also not worked as I expected. > > I expect it will be A1 -> A2 -> NULL, > > but it will be A1 -> A2 -> B, because of_graph_get_next_endpoint() > > will fetch endpoint beyond the port. > > > > It is not useful on generic driver like Generic Sound Card. > > It uses of_get_next_child() instead for now, but it is not intuitive, > > and not check node name (= "endpoint"). > > > > To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw() > > > > of_graph_get_next_endpoint_raw(port1, NULL); // A1 > > of_graph_get_next_endpoint_raw(port1, A1); // A2 > > of_graph_get_next_endpoint_raw(port1, A2); // NULL > > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > --- > > drivers/of/property.c | 26 +++++++++++++++++++++++++- > > include/linux/of_graph.h | 2 ++ > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 14ffd199c9b1..37dbb1b0e742 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent, > > } > > EXPORT_SYMBOL(of_graph_get_next_port); > > > > +/** > > + * of_graph_get_next_endpoint_raw() - get next endpoint node > > How about "of_graph_get_next_port_endpoint()"? We may want to also rename the existing of_graph_get_next_endpoint() function to of_graph_next_dev_endpoint() then. It would be a tree-wide patch, which is always annoying to get reviewed and merged, so if Rob would prefer avoiding the rename, I'm fine with that. > > + * @port: pointer to the target port node > > + * @endpoint: current endpoint node, or NULL to get first > > + * > > + * Return: An 'endpoint' node pointer with refcount incremented. Refcount > > + * of the passed @prev node is decremented. > > + */ > > It might be good to highlight here the difference to the > of_graph_get_next_endpoint(). Yes, and the documentation of of_graph_get_next_endpoint() shoul also be improved. > > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > > + struct device_node *endpoint) > > +{ > > + if (!port) > > + return NULL; > > + > > + do { > > + endpoint = of_get_next_child(port, endpoint); > > + if (!endpoint) > > + break; > > + } while (!of_node_name_eq(endpoint, "endpoint")); > > + > > + return endpoint; > > +} > > +EXPORT_SYMBOL(of_graph_get_next_endpoint_raw); > > + > > /** > > * of_graph_get_next_endpoint() - get next endpoint node > > * @parent: pointer to the parent device node > > @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > > * getting the next child. If the previous endpoint is NULL this > > * will return the first child. > > */ > > - endpoint = of_get_next_child(port, prev); > > + endpoint = of_graph_get_next_endpoint_raw(port, prev); > > if (endpoint) { > > of_node_put(port); > > return endpoint; > > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > > index fff598640e93..427905a6e8c3 100644 > > --- a/include/linux/of_graph.h > > +++ b/include/linux/of_graph.h > > @@ -57,6 +57,8 @@ int of_graph_get_port_count(const struct device_node *np); > > struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id); > > struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > > struct device_node *previous); > > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > > + struct device_node *prev); > > struct device_node *of_graph_get_next_port(const struct device_node *parent, > > struct device_node *previous); > > struct device_node *of_graph_get_endpoint_by_regs(
Hi Morimoto-san, On Mon, Jan 29, 2024 at 12:54:59AM +0000, Kuninori Morimoto wrote: > We already have of_graph_get_next_endpoint(), but it is not intuitive > to use. > > (X) node { > (Y) ports { > port@0 { endpoint { remote-endpoint = ...; };}; > (A1) port@1 { endpoint { remote-endpoint = ...; }; > (A2) endpoint { remote-endpoint = ...; };}; > (B) port@2 { endpoint { remote-endpoint = ...; };}; > }; > }; > > For example, if I want to handle port@1's 2 endpoints (= A1, A2), > I want to use like below > > A1 = of_graph_get_next_endpoint(port1, NULL); > A2 = of_graph_get_next_endpoint(port1, A1); > > But 1st one will be error, because of_graph_get_next_endpoint() requested > "parent" means "node" (X) or "ports" (Y), not "port". > Below are OK > > of_graph_get_next_endpoint(node, NULL); // node/ports/port@0/endpoint > of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint > > In other words, we can't handle A1/A2 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 A1 pointer for some way, and if I want to > handle port@1 things, I would like use it like below > > /* > * "endpoint" is now A1, and handle port1 things here, > * but we don't know how many endpoints port1 has. > * > * Because "endpoint" is non NULL, we can use port1 > * as of_graph_get_next_endpoint(port1, xxx) > */ > do { > /* do something for port1 specific things here */ > } while (endpoint = of_graph_get_next_endpoint(port1, endpoint)) > > But it also not worked as I expected. > I expect it will be A1 -> A2 -> NULL, > but it will be A1 -> A2 -> B, because of_graph_get_next_endpoint() > will fetch endpoint beyond the port. > > It is not useful on generic driver like Generic Sound Card. > It uses of_get_next_child() instead for now, but it is not intuitive, > and not check node name (= "endpoint"). > > To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw() > > of_graph_get_next_endpoint_raw(port1, NULL); // A1 > of_graph_get_next_endpoint_raw(port1, A1); // A2 > of_graph_get_next_endpoint_raw(port1, A2); // NULL > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > drivers/of/property.c | 26 +++++++++++++++++++++++++- > include/linux/of_graph.h | 2 ++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 14ffd199c9b1..37dbb1b0e742 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent, > } > EXPORT_SYMBOL(of_graph_get_next_port); > > +/** > + * of_graph_get_next_endpoint_raw() - get next endpoint node > + * @port: pointer to the target port node > + * @endpoint: current 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_endpoint_raw(const struct device_node *port, > + struct device_node *endpoint) > +{ > + if (!port) > + return NULL; of_get_next_child() returns NULL if node is NULL, hence there's no need to check this. > + > + do { > + endpoint = of_get_next_child(port, endpoint); > + if (!endpoint) > + break; > + } while (!of_node_name_eq(endpoint, "endpoint")); > + > + return endpoint; > +} > +EXPORT_SYMBOL(of_graph_get_next_endpoint_raw); > + > /** > * of_graph_get_next_endpoint() - get next endpoint node > * @parent: pointer to the parent device node > @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > * getting the next child. If the previous endpoint is NULL this > * will return the first child. > */ > - endpoint = of_get_next_child(port, prev); > + endpoint = of_graph_get_next_endpoint_raw(port, prev); > if (endpoint) { > of_node_put(port); > return endpoint; > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index fff598640e93..427905a6e8c3 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -57,6 +57,8 @@ int of_graph_get_port_count(const struct device_node *np); > struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id); > struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > struct device_node *previous); > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > + struct device_node *prev); > struct device_node *of_graph_get_next_port(const struct device_node *parent, > struct device_node *previous); > struct device_node *of_graph_get_endpoint_by_regs(
Hi Laurent, Tomi Thank you for your review > > > +/** > > > + * of_graph_get_next_endpoint_raw() - get next endpoint node > > > > How about "of_graph_get_next_port_endpoint()"? > > We may want to also rename the existing of_graph_get_next_endpoint() > function to of_graph_next_dev_endpoint() then. It would be a tree-wide > patch, which is always annoying to get reviewed and merged, so if Rob > would prefer avoiding the rename, I'm fine with that. To be honest, from intuitive function naming point of view, I prefer rename existing function name. But yes, it will be big patch. Current of_graph_get_next_endpoint() will get next endpoint beyond the port (A) New function is not get next endpoint beyond the port (B) Something like (A) of_graph_get_next_endpoint() -> of_graph_get_next_port_endpoint() (B) of_graph_get_next_endpoint() > > > + * @port: pointer to the target port node > > > + * @endpoint: current endpoint node, or NULL to get first > > > + * > > > + * Return: An 'endpoint' node pointer with refcount incremented. Refcount > > > + * of the passed @prev node is decremented. > > > + */ > > > > It might be good to highlight here the difference to the > > of_graph_get_next_endpoint(). > > Yes, and the documentation of of_graph_get_next_endpoint() shoul also be > improved. Yes, Indeed. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Sakari > > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > > + struct device_node *endpoint) > > +{ > > + if (!port) > > + return NULL; > > of_get_next_child() returns NULL if node is NULL, hence there's no need to > check this. Thanks, will fix in v3 Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Mon, Jan 29, 2024 at 03:02:19PM +0200, Laurent Pinchart wrote: > On Mon, Jan 29, 2024 at 02:29:22PM +0200, Tomi Valkeinen wrote: > > On 29/01/2024 02:54, Kuninori Morimoto wrote: > > > We already have of_graph_get_next_endpoint(), but it is not intuitive > > > to use. > > > > > > (X) node { > > > (Y) ports { > > > port@0 { endpoint { remote-endpoint = ...; };}; > > > (A1) port@1 { endpoint { remote-endpoint = ...; }; > > > (A2) endpoint { remote-endpoint = ...; };}; > > > (B) port@2 { endpoint { remote-endpoint = ...; };}; > > > }; > > > }; > > > > > > For example, if I want to handle port@1's 2 endpoints (= A1, A2), > > > I want to use like below > > > > > > A1 = of_graph_get_next_endpoint(port1, NULL); > > > A2 = of_graph_get_next_endpoint(port1, A1); > > > > > > But 1st one will be error, because of_graph_get_next_endpoint() requested > > > "parent" means "node" (X) or "ports" (Y), not "port". > > > Below are OK > > > > > > of_graph_get_next_endpoint(node, NULL); // node/ports/port@0/endpoint > > > of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint > > > > > > In other words, we can't handle A1/A2 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 A1 pointer for some way, and if I want to > > > handle port@1 things, I would like use it like below > > > > > > /* > > > * "endpoint" is now A1, and handle port1 things here, > > > * but we don't know how many endpoints port1 has. > > > * > > > * Because "endpoint" is non NULL, we can use port1 > > > * as of_graph_get_next_endpoint(port1, xxx) > > > */ > > > do { > > > /* do something for port1 specific things here */ > > > } while (endpoint = of_graph_get_next_endpoint(port1, endpoint)) > > > > > > But it also not worked as I expected. > > > I expect it will be A1 -> A2 -> NULL, > > > but it will be A1 -> A2 -> B, because of_graph_get_next_endpoint() > > > will fetch endpoint beyond the port. > > > > > > It is not useful on generic driver like Generic Sound Card. > > > It uses of_get_next_child() instead for now, but it is not intuitive, > > > and not check node name (= "endpoint"). > > > > > > To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw() > > > > > > of_graph_get_next_endpoint_raw(port1, NULL); // A1 > > > of_graph_get_next_endpoint_raw(port1, A1); // A2 > > > of_graph_get_next_endpoint_raw(port1, A2); // NULL > > > > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > > --- > > > drivers/of/property.c | 26 +++++++++++++++++++++++++- > > > include/linux/of_graph.h | 2 ++ > > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > index 14ffd199c9b1..37dbb1b0e742 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent, > > > } > > > EXPORT_SYMBOL(of_graph_get_next_port); > > > > > > +/** > > > + * of_graph_get_next_endpoint_raw() - get next endpoint node > > > > How about "of_graph_get_next_port_endpoint()"? > > We may want to also rename the existing of_graph_get_next_endpoint() > function to of_graph_next_dev_endpoint() then. It would be a tree-wide > patch, which is always annoying to get reviewed and merged, so if Rob > would prefer avoiding the rename, I'm fine with that. I think we should get rid of or minimize of_graph_get_next_endpoint() in its current form. In general, drivers should be asking for a specific port number because their function is fixed in the binding. Iterating over endpoints within a port is okay as that's usually a selecting 1 of N operation. Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and wrong. I also added of_graph_get_remote_node() for this reason and cleaned a lot of these (in DRM) up some time ago. Because in the end, a driver generally just wants the remote device it is connected to and details of parsing the graph should be mostly opaque. Wouldn't something like this work for this case: #define for_each_port_endpoint_of_node(parent, port, child) \ for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \ child = of_get_next_child(parent, child)) Rob
Hi Rob > I think we should get rid of or minimize of_graph_get_next_endpoint() in > its current form. In general, drivers should be asking for a specific > port number because their function is fixed in the binding. Iterating > over endpoints within a port is okay as that's usually a selecting 1 of > N operation. > > Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) > which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). > Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and > wrong. > > I also added of_graph_get_remote_node() for this reason and cleaned a > lot of these (in DRM) up some time ago. Because in the end, a driver > generally just wants the remote device it is connected to and details of > parsing the graph should be mostly opaque. > > Wouldn't something like this work for this case: > > #define for_each_port_endpoint_of_node(parent, port, child) \ > for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \ > child = of_get_next_child(parent, child)) I see. I will split this patch-set to like below - patch-set for reduce/remove to using current next_endpoint() - patch-set for rename current next_endpoint() to next_device_endpoint() - patch-set for adding new next_port_endpoint() Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Rob > > #define for_each_port_endpoint_of_node(parent, port, child) \ > > for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \ > > child = of_get_next_child(parent, child)) Hmm... I noticed it is impossible so for. of_graph_get_endpoint_by_regs() (A) is based on for_each_endpoint_of_node() (B). Thus, we can't replace for_each_endpoint_of_node() (B) with by_regs (A) (A) struct device_node *of_graph_get_endpoint_by_regs(...) { ... (B) for_each_endpoint_of_node(parent, node) { ... } return NULL; } > - patch-set for reduce/remove to using current next_endpoint() > - patch-set for rename current next_endpoint() to next_device_endpoint() > - patch-set for adding new next_port_endpoint() So, maybe we can do is, 0) rename current endpoint functions to device_endpoint 1) add new port base functions (port_endpoint) which has for_each_of_graph_port_endpoint() loop. It is for port base endpoint loop (I want to use new naming, using of_graph instead of _of_node). 2) replace above (B) part with port base loops - for_each_endpoint_of_node(parent, node) { + for_each_of_gprah_port(parent, port) { + for_each_of_graph_port_endpoint(port, endpoint) { 3) replace current next_endpoint() by next_endpoint_by_regs(), and remove next_endpoint() What do you think ? Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
diff --git a/drivers/of/property.c b/drivers/of/property.c index 14ffd199c9b1..37dbb1b0e742 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent, } EXPORT_SYMBOL(of_graph_get_next_port); +/** + * of_graph_get_next_endpoint_raw() - get next endpoint node + * @port: pointer to the target port node + * @endpoint: current 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_endpoint_raw(const struct device_node *port, + struct device_node *endpoint) +{ + if (!port) + return NULL; + + do { + endpoint = of_get_next_child(port, endpoint); + if (!endpoint) + break; + } while (!of_node_name_eq(endpoint, "endpoint")); + + return endpoint; +} +EXPORT_SYMBOL(of_graph_get_next_endpoint_raw); + /** * of_graph_get_next_endpoint() - get next endpoint node * @parent: pointer to the parent device node @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, * getting the next child. If the previous endpoint is NULL this * will return the first child. */ - endpoint = of_get_next_child(port, prev); + endpoint = of_graph_get_next_endpoint_raw(port, prev); if (endpoint) { of_node_put(port); return endpoint; diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h index fff598640e93..427905a6e8c3 100644 --- a/include/linux/of_graph.h +++ b/include/linux/of_graph.h @@ -57,6 +57,8 @@ int of_graph_get_port_count(const struct device_node *np); struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id); struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, struct device_node *previous); +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, + struct device_node *prev); struct device_node *of_graph_get_next_port(const struct device_node *parent, struct device_node *previous); struct device_node *of_graph_get_endpoint_by_regs(