On Sat, Jun 25, 2011 at 02:04:32AM +0800, Shawn Guo wrote:

The patch adds function of_alias_scan to populate a global lookup
table with the properties of 'aliases' node and function
of_alias_get_id for drivers to find alias id from the lookup table.

Signed-off-by: Shawn Guo <redacted>
Cc: Grant Likely <redacted>

Hey Shawn,

Comments below, but I've gone ahead and fixed them because I'm keen to
get this patch merged.  I just wanted to let you know what I changed.
I'll post the modified version so you can test it.

Nit: kerneldoc format is "/**" to start the comment, and function
names should have '()' on it.

+ * @np:		Pointer to the given device_node
+ * @stem:	Alias stem of the given device_node
+ *
+ * The function travels the lookup table to find an available id for the
+ * given device_node with given alias stem.  It returns the id found.
+ */
+static int of_alias_find_available_id(struct device_node *np, const char *stem)
+{
+	struct alias_prop *app;
+	int id = 0;
+
+	while (1) {
+		bool used = false;
+		list_for_each_entry(app, &aliases_lookup, link) {
+			if (!strcmp(app->alias_stem, stem) &&
+			    app->alias_id == id) {
+				used = true;
+				break;
+			}
+		}
+
+		if (used)
+			id++;
+		else
+			return id;
+	}
+}

I've simplified this somewhat to do the list_for_each_entry() only
once, and to bump up the id to +1 the largest entry:

	list_for_each_entry(app, &aliases_lookup, link)
		if ((strcmp(app->stem, stem) == 0) && id <= app->id)
			id = app->id + 1;

.... actually, after looking further, I've rolled this code into the
of_alias_get_id() function.

+
+/**
+ * of_alias_lookup_add - Add alias into lookup table
+ * @alias:	Alias name
+ * @id:		Alias id
+ * @stem:	Alias stem name
+ * @np:		Pointer to device_node
+ *
+ * The function adds one alias into the lookup table and populate it
+ * with the info passed in.  It returns 0 in sucess, or error code.
+ */
+static int of_alias_lookup_add(char *alias, int id, const char *stem,
+			       struct device_node *np)
+{
+	struct alias_prop *app;
+	size_t sz = sizeof(*app) + strlen(stem) + 1;
+
+	app = kzalloc(sz, GFP_KERNEL);
+	if (!app) {
+		/*
+		 * It may fail due to being called by boot code,
+		 * so try alloc_bootmem before return error.
+		 */
+		app = alloc_bootmem(sz);
+		if (!app)
+			return -ENOMEM;
+	}

I'm not too fond of this since we *know* when this is going to be
called from early boot.  So, instead, I've changed the code to use the
already existing dt_alloc functions as an argument to of_alias_lookup_add().

Also, alloc_bootmem() doesn't work on all platforms.
early_init_dt_alloc_memory_arch() needs to be used instead.

+
+	app->np = np;
+	app->alias = alias;
+	app->alias_id = id;
+	strcpy(app->alias_stem, stem);
+	list_add_tail(&app->link, &aliases_lookup);
+
+	return 0;
+}
+
+/**
+ * of_alias_decode - Decode alias stem and id from alias name
+ * @alias:	Alias name to be decoded
+ * @stem:	Pointer used to return stem name
+ *
+ * The function decodes alias stem name and alias id from the given
+ * alias name.  It returns stem name via parameter 'stem', and stem id
+ * via the return value.  If no id is found in alias name, it returns 0
+ * as the default.
+ */
+static int of_alias_decode(char *alias, char *stem)
+{
+	int len = strlen(alias);
+	char *end = alias + len;
+
+	while (isdigit(*--end))
+		len--;

Need to protect against len < 0

+
+	strncpy(stem, alias, len);
+	stem[len] = '\0';
+
+	return strlen(++end) ? simple_strtoul(end, NULL, 10) : 0;
+}
+
+/**
+ * of_alias_scan - Scan all properties of 'aliases' node
+ *
+ * The function scans all the properties of 'aliases' node and populate
+ * the the global lookup table with the properties.  It returns the
+ * number of alias_prop found, or error code in error case.
+ */
+int of_alias_scan(void)

Changed to '__init void of_aliases_scan(void)'.  There is no point in
returning an error code since it is never checked, and there is no
point in bailing on an error just in case it is a problem with only
one alias.

+{
+	struct property *pp;
+	int ret, num = 0;
+
+	if (!of_aliases)
+		return -ENODEV;
+
+	for_each_property(pp, of_aliases->properties) {
+		char stem[32];
+		int id = of_alias_decode(pp->name, stem);
+
+		/* Skip those we do not want to proceed */
+		if (!strcmp(pp->name, "name") ||
+		    !strcmp(pp->name, "phandle") ||
+		    !strcmp(pp->name, "linux,phandle"))
+			continue;
+
+		ret = of_alias_lookup_add(pp->name, id, stem,
+					  of_find_node_by_path(pp->value));
+		if (ret < 0)
+			return ret;
+		else
+			num++;
+	}
+
+	return num;
+}
+
+/**
+ * of_alias_get_id - Get alias id for the given device_node
+ * @np:		Pointer to the given device_node
+ * @stem:	Alias stem of the given device_node
+ *
+ * The function travels the lookup table to get alias id for the given
+ * device_node and alias stem.  It returns the alias id if find it.
+ * If not, dynamically creates one in the lookup table and returns it,
+ * or returns error code if fail to create.
+ */
+int of_alias_get_id(struct device_node *np, const char *stem)
+{
+	struct alias_prop *app;
+	int id = -1;
+
+	list_for_each_entry(app, &aliases_lookup, link) {
+		if (np == app->np) {
+			id = app->alias_id;
+			break;
+		}
+	}
+
+	if (id == -1) {
+		id = of_alias_find_available_id(np, stem);
+		if (of_alias_lookup_add(NULL, id, stem, np) < 0)
+			return -ENODEV;
+	}
+
+	return id;
+}

This whole function needs to be protected by a mutex.

I don't think the /aliases at 0 variant is necessary.  I've dropped it.

g.