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

[David Vrabel <david.vrabel@citrix.com>]

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@citrix.com>
>>
>> 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@citrix.com>
>> ---
>>  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.

>> +    {
>> +        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