From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Szyprowski Subject: Re: [PATCH v7 2/4] drivers: of: add function to scan fdt nodes given by path Date: Fri, 30 Aug 2013 12:42:23 +0200 Message-ID: <5220770F.9080806@samsung.com> References: <1377527959-5080-1-git-send-email-m.szyprowski@samsung.com> <1377527959-5080-3-git-send-email-m.szyprowski@samsung.com> <20130829214007.CDB813E1222@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <20130829214007.CDB813E1222@localhost> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Grant Likely Cc: Mark Rutland , devicetree@vger.kernel.org, Laura Abbott , Pawel Moll , Arnd Bergmann , Stephen Warren , Tomasz Figa , Tomasz Figa , Michal Nazarewicz , linaro-mm-sig@lists.linaro.org, Marc , Kyungmin Park , Sylwester Nawrocki , Kumar Gala , Olof Johansson , Nishanth Peethambaran , Sascha Hauer , linux-arm-kernel@lists.infradead.org, Ian Campbell List-Id: devicetree@vger.kernel.org Hello, On 8/29/2013 11:40 PM, Grant Likely wrote: > On Mon, 26 Aug 2013 16:39:17 +0200, Marek Szyprowski 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 > > Acked-by: Michal Nazarewicz > > Acked-by: Tomasz Figa > > Reviewed-by: Rob Herring > > Some nits below, but otherwise: > > Acked-by: Grant Likely 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