Re: [Xen-devel] [PATCH 3/8] device tree: add device_tree_for_each_node()

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-03-14 at 11:01 +0000, David Vrabel wrote:
> On 14/03/12 10:08, Ian Campbell wrote:
> > On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> >>
> >> Add device_tree_for_each_node() to iterate over all nodes in a flat
> >> device tree.  Use this in device_tree_early_init().
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> ---
> >>  xen/common/device_tree.c      |   71 
> >> ++++++++++++++++++++++++++---------------
> >>  xen/include/xen/device_tree.h |    8 +++++
> >>  2 files changed, 53 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >> index e5df748..e95ff3c 100644
> >> --- a/xen/common/device_tree.c
> >> +++ b/xen/common/device_tree.c
> >> @@ -66,7 +66,42 @@ static u32 __init prop_by_name_u32(const void *fdt, int 
> >> node, const char *prop_n
> >>      return fdt32_to_cpu(*(uint32_t*)prop->data);
> >>  }
> >>  
> >> -static void __init process_memory_node(const void *fdt, int node,
> >> +#define MAX_DEPTH 16
> >> +
> >> +/**
> >> + * device_tree_for_each_node - iterate over all device tree nodes
> >> + * @fdt: flat device tree.
> >> + * @func: function to call for each node.
> >> + * @data: data to pass to @func.
> >> + */
> >> +int device_tree_for_each_node(const void *fdt,
> >> +                              device_tree_node_func func, void *data)
> >> +{
> >> +    int node;
> >> +    int depth;
> >> +    u32 address_cells[MAX_DEPTH];
> >> +    u32 size_cells[MAX_DEPTH];
> >> +    int ret;
> >> +
> >> +    for (node = 0, depth = 0;
> >> +         node >=0 && depth >= 0;
> >> +         node = fdt_next_node(fdt, node, &depth))
> > 
> > Xen coding style has spaces both sides of the outermost "(" and ")" of a
> > for loop (similarly for if / while etc)
> 
> This is a minor difference between the Linux and Xen coding styles.
> Given people often work on both can we not aim for more consistency
> between the two?
> 
> I personally think the additional spaces reduce readability (but this is
> probably mostly because I'm more familiar with Linux style rather than
> any inherent improvement).
> 
> My recommendation would be to allow both styles of spacing withing Xen
> but make it consistent within a file.

This is for Keir to decide. At the moment the Xen coding style is what
it is.

> 
> >> +    {
> >> +        if (depth >= MAX_DEPTH)
> > 
> > Some sort of warning or error message would be useful here?
> 
> Tricky as this function is called early before printk() works and later
> (when it does).
> 
> >> +            continue;
> >> +
> >> +        address_cells[depth] = prop_by_name_u32(fdt, node, 
> >> "#address-cells");
> >> +        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> >> +
> >> +        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
> >> +                   address_cells[depth-1], size_cells[depth-1], data);
> > 
> > I suppose this function could have been written recursively and avoided
> > the arbitrary MAX_DEPTH, but actually in the hypervisor an iterative
> > version with explicit maximum stack usage seems like a reasonable idea.
> 
> This is why I used a loop.
> 
> > I should have spotted this before, coding style needs a space inside the
> > () and { should be on the next line. There's a bunch of this in both the
> > context and the new code added by this patch. Obviously the new code
> > should be fixed, I don't mind if you deal with the existing bits by a
> > cleanup sweep or by picking it up as you go along.
> 
> See comment above.
> 
> David



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel