Hi Stephen, On Fri, Jan 13, 2012 at 1:41 PM, Stephen Warren <swar...@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 <swar...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot