Hello,
On 8/29/2013 11:40 PM, Grant Likely wrote:
On Mon, 26 Aug 2013 16:39:17 +0200, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> Acked-by: Tomasz Figa <t.figa@xxxxxxxxxxx>
> Reviewed-by: Rob Herring <rob.herring@xxxxxxxxxxx>
Some nits below, but otherwise:
Acked-by: Grant Likely <grant.likely@xxxxxxxxxxxx>
Thanks!
I'm happy to take this through the DT tree, or have you take it via the
CMA tree.
I have already put the whole patchset in linux-next via my
dma-mapping/cma tree,
so I would like to keep it there to avoid further confusion.
I only wonder how to handle the patches to get them merged to v3.12-rc1.
After your review there will be some changes to the binding, its
documentation
and the code itself. Would you mind if I send pull request with the current
version and then provide incremental patches to fix the issues you have
reported? Or should we delay this patchset till v3.13-rc1?
> ---
> 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
>
Best regards
--
Marek Szyprowski
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
