Re: [PATCH v2 1/6] libfdt: Add a subnodes iterator macro

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3776 bytes --]

On Mon, Jul 11, 2016 at 09:56:18PM +0200, Maxime Ripard wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The fdt_for_each_subnode() iterator macro provided by this patch can be
> used to iterate over a device tree node's subnodes. At each iteration a
> loop variable will be set to the next subnode.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Code looks ok, but there are some nits in the comments.

> ---
>  libfdt/libfdt.h         | 25 +++++++++++++++++++++++++
>  tests/subnode_iterate.c |  8 ++------
>  2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 36222fd4a6f4..0cf46872b54e 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -168,6 +168,31 @@ int fdt_first_subnode(const void *fdt, int offset);
>   */
>  int fdt_next_subnode(const void *fdt, int offset);
>  
> +/**
> + * fdt_for_each_subnode - iterate over all subnodes of a parent

Parameter descriptions should go here.

> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_subnode(fdt, node, parent) {

Parameter order hasn't been updated for code revisions.

> + *		...
> + *		use node
> + *		...
> + *	}

One gotcha with this is what happens if there's an error during the
iteration.  I suggest adding something like:
	if ((node < 0) && (node != -FDT_ERR_NOTFOUND)) {
		/* error handling */
	}

to the example, to remind users that they should check for this.

> + *
> + * Note that this is implemented as a macro and node is used as iterator in

s/node/@node/ - makes it stand out as a parameter.

> + * the loop. It should therefore be a locally allocated variable. The parent
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Remove for brevity.

> + * variable on the other hand is never modified, so it can be constant or
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Remove for brevity.

> + * even a literal.
> + *
> + * @node:	child node (int)
> + * @fdt:	FDT blob (const void *)
> + * @parent:	parent node (int)
> + */
> +#define fdt_for_each_subnode(node, fdt, parent)		\
> +	for (node = fdt_first_subnode(fdt, parent);	\
> +	     node >= 0;					\
> +	     node = fdt_next_subnode(fdt, node))
> +
>  /**********************************************************************/
>  /* General functions                                                  */
>  /**********************************************************************/
> diff --git a/tests/subnode_iterate.c b/tests/subnode_iterate.c
> index b9f379d5963c..0fb5c901ebd7 100644
> --- a/tests/subnode_iterate.c
> +++ b/tests/subnode_iterate.c
> @@ -48,9 +48,7 @@ static void test_node(void *fdt, int parent_offset)
>  	subnodes = cpu_to_fdt32(*prop);
>  
>  	count = 0;
> -	for (offset = fdt_first_subnode(fdt, parent_offset);
> -	     offset >= 0;
> -	     offset = fdt_next_subnode(fdt, offset))
> +	fdt_for_each_subnode(offset, fdt, parent_offset)
>  		count++;
>  
>  	if (count != subnodes) {
> @@ -65,9 +63,7 @@ static void check_fdt_next_subnode(void *fdt)
>  	int offset;
>  	int count = 0;
>  
> -	for (offset = fdt_first_subnode(fdt, 0);
> -	     offset >= 0;
> -	     offset = fdt_next_subnode(fdt, offset)) {
> +	fdt_for_each_subnode(offset, fdt, 0) {
>  		test_node(fdt, offset);
>  		count++;
>  	}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]