linux-kernel - Re: [PATCH v7 02/10] property: Add functions to iterate named child

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z9dGX_ckIRUg6fZh@debian-BULLSEYE-live-builder-AMD64>
Date: Sun, 16 Mar 2025 18:45:03 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
	Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v7 02/10] property: Add functions to iterate named child

Hello,

LGTM, few minor comments inline.
With those sorted
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>

On 03/13, Matti Vaittinen wrote:
> There are a few use-cases where child nodes with a specific name need to
> be parsed. Code like:
> 
> fwnode_for_each_child_node()
> 	if (fwnode_name_eq())
> 		...
> 
> can be found from a various drivers/subsystems. Adding a macro for this
> can simplify things a bit.
> 
> In a few cases the data from the found nodes is later added to an array,
> which is allocated based on the number of found nodes. One example of
> such use is the IIO subsystem's ADC channel nodes, where the relevant
> nodes are named as channel[@N].
> 
> Add helpers for iterating and counting device's sub-nodes with certain
> name instead of open-coding this in every user.
> 
> Suggested-by: Jonathan Cameron <jic23@...nel.org>
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> ---
...
> +/**
> + * fwnode_get_named_child_node_count - number of child nodes with given name
> + * @fwnode: Node which child nodes are counted.
> + * @name: String to match child node name against.
> + *
> + * Scan child nodes and count all the nodes with a specific name. Potential
> + * 'number' -ending after the 'at sign' for scanned names is ignored.
> + * E.g.::
> + *   device_get_named_child_node_count(dev, "channel");

The function being documented is fwnode_get_named_child_node_count().
Shouldn't the example be
   fwnode_get_named_child_node_count(fwnode, "channel");
?

> + * would match all the nodes::
> + *   channel { }, channel@0 {}, channel@...bba {}...
> + *
> + * Return: the number of child nodes with a matching name for a given device.
> + */
...
> +#define device_for_each_named_child_node(dev, child, name)		\
> +		device_for_each_child_node(dev, child)			\
> +			if (!fwnode_name_eq(child, name)) { } else

nitpicking: add only one tab for each indentation level.
	device_for_each_child_node(dev, child)			\
		if (!fwnode_name_eq(child, name)) { } else

> +
>  #define device_for_each_child_node_scoped(dev, child)			\
>  	for (struct fwnode_handle *child __free(fwnode_handle) =	\
>  		device_get_next_child_node(dev, NULL);			\
>  	     child; child = device_get_next_child_node(dev, child))
>  
> +#define device_for_each_named_child_node_scoped(dev, child, name)	\
> +		device_for_each_child_node_scoped(dev, child)		\
> +			if (!fwnode_name_eq(child, name)) { } else
> +

nitpicking: instead of two tabs, only one tab before device_for_..._scoped().
	device_for_each_child_node_scoped(dev, child)		\
		if (!fwnode_name_eq(child, name)) { } else

Regards,
Marcelo