Re: [PATCH v7 2/4] drivers: of: add function to scan fdt nodes given by path

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, 26 Aug 2013 16:39:17 +0200, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> Add a function to scan the flattened device-tree starting from the
> node given by the path. It is used to extract information (like reserved
> memory), which is required on early boot before we can unflatten the tree.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> Acked-by: Tomasz Figa <t.figa@samsung.com>
> Reviewed-by: Rob Herring <rob.herring@calxeda.com>

Some nits below, but otherwise:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

I'm happy to take this through the DT tree, or have you take it via the
CMA tree.

> ---
>  drivers/of/fdt.c       |   76 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h |    3 ++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index b10ba00..4fb06f3 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -545,6 +545,82 @@ int __init of_flat_dt_match(unsigned long node, const char *const *compat)
>  	return of_fdt_match(initial_boot_params, node, compat);
>  }
>  
> +struct fdt_scan_status {
> +	const char *name;
> +	int namelen;
> +	int depth;
> +	int found;
> +	int (*iterator)(unsigned long node, const char *uname, int depth, void *data);
> +	void *data;
> +};
> +
> +/**
> + * fdt_scan_node_by_path - iterator for of_scan_flat_dt_by_path function
> + */
> +static int __init fdt_scan_node_by_path(unsigned long node, const char *uname,
> +					int depth, void *data)

Nit: since this is an iterator, I'd like to see "iter" in the function
name.

> +{
> +	struct fdt_scan_status *st = data;
> +
> +	/*
> +	 * if scan at the requested fdt node has been completed,
> +	 * return -ENXIO to abort further scanning
> +	 */
> +	if (depth <= st->depth)
> +		return -ENXIO;
> +
> +	/* requested fdt node has been found, so call iterator function */
> +	if (st->found)
> +		return st->iterator(node, uname, depth, st->data);
> +
> +	/* check if scanning automata is entering next level of fdt nodes */
> +	if (depth == st->depth + 1 &&
> +	    strncmp(st->name, uname, st->namelen) == 0 &&
> +	    uname[st->namelen] == 0) {
> +		st->depth += 1;
> +		if (st->name[st->namelen] == 0) {
> +			st->found = 1;
> +		} else {
> +			const char *next = st->name + st->namelen + 1;
> +			st->name = next;
> +			st->namelen = strcspn(next, "/");
> +		}
> +		return 0;

The above return statement is redundant.

> +	}
> +
> +	/* scan next fdt node */
> +	return 0;
> +}
> +
> +/**
> + * of_scan_flat_dt_by_path - scan flattened tree blob and call callback on each
> + *			     child of the given path.
> + * @path: path to start searching for children
> + * @it: callback function
> + * @data: context data pointer
> + *
> + * This function is used to scan the flattened device-tree starting from the
> + * node given by path. It is used to extract information (like reserved
> + * memory), which is required on ealy boot before we can unflatten the tree.
> + */
> +int __init of_scan_flat_dt_by_path(const char *path,
> +	int (*it)(unsigned long node, const char *name, int depth, void *data),
> +	void *data)

Nit: Please match the indentation convention used by of_scan_flat_dt().
This current version is really hard to read.

> +{
> +	struct fdt_scan_status st = {path, 0, -1, 0, it, data};

This is a little fragile. If the structure gets modified, this line
will break. I know it results in more text, but please use explicit data
member assignments in initializers.

> +	int ret = 0;
> +
> +	if (initial_boot_params)

Nit:
	if (!initial_boot_params)
		return 0;

> +                ret = of_scan_flat_dt(fdt_scan_node_by_path, &st);

Nit: inconsitent indentation (tabs vs. spaces)

> +
> +	if (!st.found)
> +		return -ENOENT;
> +	else if (ret == -ENXIO)	/* scan has been completed */
> +		return 0;
> +	else
> +		return ret;

Both uses of 'else' above are redundant. The only way the execution will
pass that point is if it was the else case!

> +}
> +
>  #ifdef CONFIG_BLK_DEV_INITRD
>  /**
>   * early_init_dt_check_for_initrd - Decode initrd location from flat tree
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index ed136ad..19f26f8 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -90,6 +90,9 @@ extern void *of_get_flat_dt_prop(unsigned long node, const char *name,
>  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
>  extern int of_flat_dt_match(unsigned long node, const char *const *matches);
>  extern unsigned long of_get_flat_dt_root(void);
> +extern int of_scan_flat_dt_by_path(const char *path,
> +	int (*it)(unsigned long node, const char *name, int depth, void *data),
> +	void *data);
>  
>  extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
>  				     int depth, void *data);
> -- 
> 1.7.9.5
>