[U-Boot] [PATCH v2 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[Simon Glass]

Hi Stephen,

On Fri, Jan 13, 2012 at 1:41 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 01/11/2012 04:08 PM, Simon Glass wrote:
>> Stephen Warren pointed out that we should use nodes whether or not they
>> have an alias in the /aliases section. The aliases section specifies the
>> order so far as it can, but is not essential. Operating without alisses
>> is useful when the enumerated order of nodes does not matter (admittedly
>> rare in U-Boot).
>>
>> This is considerably more complex, and it is important to keep this
>> complexity out of driver code. This patch creates a function
>> fdtdec_find_aliases() which returns an ordered list of node offsets
>> for a particular compatible ID, taking account of alias nodes.
>
> Overall, this looks basically OK. Just a few minor comments below, but
> once those are fixed,
>
> Acked-by: Stephen Warren <swarren at nvidia.com>
>
>> diff --git a/include/fdtdec.h b/include/fdtdec.h
>
>> +/**
>> + * Find the nodes for a peripheral and return a list of them in the correct
> ...
>> + * This function checks node properties and will not return node which are
>
> s/node/nodes/
>
>> + * marked disabled (status = "disabled").
>> + *
>> + * @param blob               FDT blob to use
>> + * @param name               Root name of alias to search for
>> + * @param id         Compatible ID to look for
>> + * @param node               Place to put list of found nodes
>
> s/node/node_list/
>
>> + * @param maxcount   Maximum number of nodes to find
>> + * @return number of nodes found on success, FTD_ERR_... on error
>> + */
>> +int fdtdec_find_aliases_for_id(const void *blob, const char *name,
>> +                     enum fdt_compat_id id, int *node_list, int maxcount);
>> +
>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>
>> @@ -35,6 +35,11 @@ DECLARE_GLOBAL_DATA_PTR;
>>  static const char * const compat_names[COMPAT_COUNT] = {
>>  };
>>
>> +const char *fdtdec_get_compatible(enum fdt_compat_id id)
>> +{
>> +     return compat_names[id];
>
> Range-check?

These and other changes made - but see one comment below.

>
>> +/* TODO: Can we tighten this code up a little? */
>> +int fdtdec_find_aliases_for_id(const void *blob, const char *name,
>> +                     enum fdt_compat_id id, int *node_list, int maxcount)
>> +{
>> +     int name_len = strlen(name);
>> +     int nodes[maxcount];
>> +     int num_found = 0;
>> +     int offset, node;
>> +     int alias_node;
>> +     int count;
>> +     int i, j;
>> +
>> +     /* find the alias node if present */
>> +     alias_node = fdt_path_offset(blob, "/aliases");
>> +
>> +     /*
>> +      * start with nothing, and we can assume that the root node can't
>> +      * match
>> +      */
>> +     memset(nodes, '\0', sizeof(nodes));
>> +
>> +     /* First find all the compatible nodes */
>> +     node = 0;
>> +     for (node = count = 0; node >= 0 && count < maxcount;) {
>
> node = 0 is duplicated on those two lines.
>
>> +             node = fdtdec_next_compatible(blob, node, id);
>> +             if (node >= 0)
>> +                     nodes[count++] = node;
>> +     }
>> +     if (node >= 0)
>> +             debug("%s: warning: maxcount exceeded with alias '%s'\n",
>> +                    __func__, name);
>> +
>> +     /* Now find all the aliases */
>> +     memset(node_list, '\0', sizeof(*node_list) * maxcount);
>> +
>> +     for (offset = fdt_first_property_offset(blob, alias_node);
>> +                     offset > 0;
>> +                     offset = fdt_next_property_offset(blob, offset)) {
>> +             const struct fdt_property *prop;
>> +             const char *path;
>> +             int number;
>> +             int found;
>> +
>> +             node = 0;
>> +             prop = fdt_get_property_by_offset(blob, offset, NULL);
>> +             path = fdt_string(blob, fdt32_to_cpu(prop->nameoff));
>> +             if (prop->len && 0 == strncmp(path, name, name_len))
>> +                     node = fdt_path_offset(blob, prop->data);
>> +             if (node <= 0)
>> +                     continue;
>> +
>> +             /* Get the alias number */
>> +             number = simple_strtoul(path + name_len, NULL, 10);
>> +             if (number < 0 || number >= maxcount) {
>> +                     debug("%s: warning: alias '%s' is out of range\n",
>> +                            __func__, path);
>> +                     continue;
>> +             }
>> +
>> +             /* Make sure the node we found is actually in our list! */
>> +             found = -1;
>> +             for (j = 0; j < count; j++)
>> +                     if (nodes[j] == node) {
>> +                             found = j;
>> +                             break;
>> +                     }
>> +
>> +             if (found == -1) {
>> +                     debug("%s: warning: alias '%s' points to a node "
>> +                             "'%s' that is missing or is not compatible "
>> +                             " with '%s'\n", __func__, path,
>> +                             fdt_get_name(blob, node, NULL),
>> +                            compat_names[id]);
>> +                     continue;
>> +             }
>> +
>> +             /*
>> +              * Add this node to our list in the right place, and mark
>> +              * it as done.
>> +              */
>> +             if (fdtdec_get_is_enabled(blob, node)) {
>
> Do you want to warn here if node_list[number] is already set; if the
> user creates multiple aliases with the same ID?

I deal with this in a later series which supports multiple compatible
IDs - for now I will add an assert().

>
>> +                     node_list[number] = node;
>> +                     if (number >= num_found)
>> +                             num_found = number + 1;
>> +             }
>> +             nodes[j] = 0;
>> +     }
>> +
>> +     /* Add any nodes not mentioned by an alias */
>> +     for (i = j = 0; i < maxcount; i++) {
>> +             if (!node_list[i]) {
>> +                     for (; j < maxcount; j++)
>> +                             if (nodes[j] &&
>> +                                     fdtdec_get_is_enabled(blob, nodes[j]))
>> +                                     break;
>> +
>> +                     /* Have we run out of nodes to add? */
>> +                     if (j == maxcount)
>> +                             continue;
>
> break?

Yes, they are equivalent so I have changed it.

>
>> +
>> +                     node_list[i] = nodes[j++];
>> +                     if (i >= num_found)
>> +                             num_found = i + 1;
>> +             }
>> +     }
>> +
>> +     return num_found;
>> +}
>> +
>

Regards,
Simon

> --
> nvpublic