Re: [PATCH] Support NAND partitions >4GiB with Open Firmware

[Mitch Bradley <wmb@firmworks.com>]

David Gibson wrote:
> On Thu, Jun 26, 2008 at 05:28:42PM -1000, Mitch Bradley wrote:
>   
>> David Gibson wrote:
>>     
>>> On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:
>>>       
> [snip]
>   
>>>> +               const u_int32_t *propval;
>>>> +               u_int32_t addrcells = 0, sizecells = 0;
>>>>                int len;
>>>>
>>>> -               reg = of_get_property(pp, "reg", &len);
>>>> -               if (!reg || (len != 2 * sizeof(u32))) {
>>>> +               /*
>>>> +                * Determine the layout of a "reg" entry based on the parent
>>>> +                * node's properties, if it hasn't been done already.
>>>> +                */
>>>> +
>>>> +               if (addrcells == 0)
>>>>     
>>>>         
>>> Redundant 'if'; you've just initialized this variable to zero.
>>>       
>> The intention is that the body of the "if" should only be executed
>> once during the loop, since the parent node is the same for all
>> children.
>>     
>
> But the initialization is within the loop body as well, so this won't
> do it.  Just factor the code getting addr and size cells right out of
> the loop, instead.
>
>   

Hmmm.  Perhaps it's better to move the declaration of the variables out of
the loop instead.

Moving the of_n_*_cells() calls outside the loop requires redundant calls
to of_get_child() and of_node_put(), because of_n_*_cells() implicitly
reach up to the parent node.  That is almost certainly more expensive 
than the "if".