Re: [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Stephen Warren <swarren@wwwdotorg.org>]

On 12/02/2013 04:02 AM, Hiroshi Doyu wrote:
...
> Iterating over a property containing a list of phandles with arguments
> is a common operation for device drivers. This patch adds a new
> of_property_for_each_phandle_with_args() macro to make the iteration
> simpler.

> diff --git a/drivers/of/base.c b/drivers/of/base.c

> +const __be32 *of_phandle_iter_init(const struct device_node *np,
> +				   const char *list_name,
> +				   const __be32 **end)
> +{
> +	size_t bytes;
> +	const __be32 *cur;
> +
> +	cur = of_get_property(np, list_name, &bytes);
> +	if (bytes)
> +		*end = cur + bytes / sizeof(*cur);
> +
> +	return cur;
> +}

"bytes" doesn't seem like the correct thing to check in that if
statement. The property might exist but be zero length. Perhaps if (cur)
would be better, or just initializing *end in all cases (the value won't
be used if (!cur), so setting *end to a bogus value in that case won't
matter).

> +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
> +				   const __be32 *cur, const __be32 *end,
> +				   struct of_phandle_args *out_args)
...
> +	if (!cur)
> +		return NULL;
> +
> +	if (end - cur <= 0)
> +		return NULL;

Related, don't you want if (cur >= end) there; just compare the pointers
directly rather than explicitly calculating and testing the difference?