[U-Boot-Users] LIBFDT: first version of fdt_find_compatible_node

[Wolfgang Grandegger]

Jerry Van Baren wrote:
> Wolfgang Grandegger wrote:
>> Hello,
>>
>> attached you can find a patch implementing fdt_find_compatible_node():
> 
> Yeeee-ha! :-)
> 
>> /*
>>  * Find a node based on its device type and one of the tokens in
>>  * its its "compatible" property. On success, the offset of that
>>  * node is returned or an error code:
>>  *
>>  *   startoffset  - the node to start searching from or 0, the node
>>  *                  you pass will not be searched, only the next one
>>  *                  will; typically, you pass 0 to start the search
>>  *                  and then what the previous call returned.
>>  *   type         - the device type string to match against
>>  *   compat       - the string to match to one of the tokens
>>  *                  in the "compatible" list.
>>  */
>>
>> It should be used as shown below:
>>
>>     offset = 0;
>>     do {
>>         offset = fdt_find_compatible_node(fdt, offset, "type", "comp");
>>     } while (offset >= 0);
>>
>> This first hack also implements a cached version as alternative, 
>> because tag re-scanning might take quite long. In principle, the cache 
>> could also be used for other functions, like fdt_path_offset(), and 
>> could be invalidated in case the FDT gets updated.
>>
>> What do you think?
>>
>> Thanks.
>>
>> Wolfgang.
> 
> Looks good.  In the real patch, I would like to see the cache addition 
> split from the fdt_node_is_compatible() fdt_find_compatible_node() and 
> addition.

Yes, OK, the patch needs some cleanup and improvement.

[...]
>> +#endif  /* CONFIG_OF_LIBFDT_TABLE > 0 */
>> +
>> +/*
>> + * Check if the specified node is compatible by comparing the
>> + * tokens in its "compatible" property with the specified string:
>> + *
>> + *   nodeoffset - starting place of the node
>> + *   compat     - the string to match to one of the tokens
>> + *                in the "compatible" list.
>> + */
>> +int fdt_node_is_compatible(const void *fdt, int nodeoffset,
>> +               const char *compat)
>> +{
>> +    const char* cp;
>> +    int cplen, l;
>> +
>> +    cp = fdt_getprop(fdt, nodeoffset, "compatible", &cplen);
>> +    if (cp == NULL)
>> +        return 0;
>> +    while (cplen > 0) {
>> +        if (strncmp(cp, compat, strlen(compat)) == 0)
>> +            return 1;
>> +        l = strlen(cp) + 1;
>> +        cp += l;
>> +        cplen -= l;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> I see this came directly from arch/powerpc/kernel/prom.c, but using "l" 
> for a variable is evil.  For a minute, I was wondering how the compiler 
> was compiling "1" (one) as a lvalue.  I would prefer it to be "len" or 
> something more descriptive.

The code looks strange indeed. Will use "len" instead of "1", aheee "l".

[...]

> For the above version of fdt_find_compatible_node(), as well as the one 
> that fills the cache table (snipped), I'm thinking it would be better to 
> use the function I added:
> 
> uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset, char 
> **namep)
> 
> I added this to step through the nodes and properties for dumping the 
> tree.  Rather than having Yet Another (slightly modified) Copy of the 
> loop & switch, you should be able to use fdt_next_tag() to step through 
> the nodes and properties, doing the
>   if (streq(fdt_string(fdt, namestroff), "device_type"))
> on the **namep parameter on every call to find the device_type 
> properties.  Does this make sense?

Yes, my concern was the overhead due to looking up the location of each 
node name and property. But for fast scanning, the cached version is 
much better anyhow, I think.

What is your opinion on the cached version? Do we need it? Should we 
keep both?

Wolfgang.