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

[Stephen Warren]

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?

> +/* 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?

> +			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?

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

-- 
nvpublic