[U-Boot-Users] fdt_find_compatible_node() and friends

[Wolfgang Grandegger]

Jerry Van Baren wrote:
> Wolfgang Grandegger wrote:
>> Hi Jerry,
>>
>> Jerry Van Baren wrote:
>> [...]
>>> Hi Wolfgang G,
>>>
>>> I've applied your patches to my local (working) repository and will 
>>> push the changes tonight (my tonight, your tomorrow ;-).  I created a 
>>> subroutine out of three snippets of code in cmd_fdt.c which your 
>>> fdt_find_node_by_path() change  fixed up so I had to fix one line in 
>>> the new subroutine by hand.  Not bad at all considering the changes I 
>>> made in that file.
>>
>> Thanks. In the meantime I observed, that
>>
>>    fdt_find_node_by_path(fdt, 0, "/");
>>
>> returns an error. Is that by purpose?
> [snip]
>> Wolfgang.
> 
> Hmm, interesting observation.  The character '/' is a figment of our 
> imagination, offset 0 in the tree is the root node.  The character '/' 
> is the path separator and doesn't actually exist anywhere in the fdt - 
> when parsing paths, the stuff between the slashes is searched for and 
> the slashes themselves are skipped over.
> 
> What you asked in the above call is a node with no name under the root 
> node, i.e. "//" in human-speak.  That wasn't found, of course.  On the 
> other hand, it is a pretty obvious "mistake" (I was guilty of making the 
> same mistake when I first tried to use fdt_path_offset()).

And it is frequently used in the kernel as the following command reveals:

$ find . -name '*.c' | xargs grep find_node_by_path | grep '"/"'

> 
> It seems like I was forever doing a conditional:
> 
> 59     if (strcmp(pathp, "/") == 0) {
> 60             nodeoffset = 0;
> 61     } else {
> 62             nodeoffset = fdt_path_offset (fdt, pathp);
> 63             if (nodeoffset < 0) {
> 
> <http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=blob;f=common/cmd_fdt.c;h=bf0aef70cedea6ec4a2a446af54c9e1467617a96;hb=HEAD#l55> 

I actually wanted to get the string for the property "model" and yes, 
fdt_getprop(fdt, 0, "model", NULL) does work.

> 
> Trivia: Your patch we are discussing changed exactly this 
> fdt_path_offset() into fdt_find_node_by_path() - this is the one place 
> your patch didn't apply, because I refactored the three conditionals 
> into a single wrapper subroutine.
> 
> At the risk of being accused of codling our users, I would propose we 
> add the equivalent "/" detection code (above) to 
> fdt_find_node_by_path(), (I will do that tonight unless you beat me to 
> it).  It seems silly to have the caller replicate or wrap the 
> conditional since it is going to be such a common idiom/mistake.

Thanks... and no hurry.

Wolfgang.