Re: [PATCH 2/4] libfdt: Add iterator over properties

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

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

On Fri, May 27, 2016 at 10:28:47AM +0200, Maxime Ripard wrote:
> Implement a macro based on fdt_first_property_offset and
> fdt_next_property_offset that provides a convenience to iterate over all
> the properties of a given node.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Looks, ok but a couple of nits:

1) Needs a testcase.

2) I prefer the convention that in for_each_XX style macros, the first
argument should be the "loop counter" (i.e. the property offset in
this case).

3) Please make the offset parameter called 'offset' or
'property_offset', and change the macro name to
'fdt_for_each_property_offset'.  I know this is pedantic, but the
naming convention is designed to repeatedly remind the user that
offsets are offsets and so *cannot* be used as persistent handles.

> ---
>  libfdt/libfdt.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 331f32dd04b5..6589e08db7ab 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -456,6 +456,30 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset);
>  int fdt_next_property_offset(const void *fdt, int offset);
>  
>  /**
> + * fdt_for_each_property - iterate over all properties of a node
> + * @fdt:	FDT blob (const void *)
> + * @node:	node offset (int)
> + * @property:	property offset (int)
> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_property(fdt, node, property) {
> + *		...
> + *		use property
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and property is used as
> + * iterator in the loop. It should therefore be a locally allocated
> + * variable. The node variable on the other hand is never modified, so
> + * it can be constant or even a literal.
> + */
> +#define fdt_for_each_property(fdt, node, property)		\
> +	for (property = fdt_first_property_offset(fdt, node);	\
> +	     property >= 0;					\
> +	     property = fdt_next_property_offset(fdt, property))
> +
> +/**
>   * fdt_get_property_by_offset - retrieve the property at a given offset
>   * @fdt: pointer to the device tree blob
>   * @offset: offset of the property to retrieve

-- 
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 --]