On Sat, Dec 29, 2007 at 04:07:20PM -0600, Timur Tabi wrote:

Anton Vorontsov wrote:

quoted

quoted

+		firmware {
+			id = "Soft-UART";
+			extended_modes = <0 0>;
+			virtual_traps = <0 0 0 0 0 0 0 0>;

I believe using underscores for the property name is discouraged.

Ugh, this one change would require me to repost all my patches.  Oh 
well.  I wish someone else had noticed it before I got to version 5.

Yeah, sorry about that.

But you know it, reviewing is tedious, and only few of us are doing
reviews for the code they don't use.

I've started to use this code just now, so I reviewed it on the way.

I'll post a new version on January 7, when I get back to the office.

quoted

quoted

+A Python program that creates firmware binaries from the header files normally
+distributed by Freescale can be found on http://opensource.freescale.com.

Hm... I didn't find it there. Could you provide more specific pointer?

I'll add those files only after my patches have been applied.  The 
layout may change between now and then, and I don't want to have to 
repost the firmware.

quoted

quoted

+
+	/*
+	 * If we haven't checked yet, and a driver hasn't uploaded a firmware
+	 * yet, then check the device tree for information.
+	 */
+	do {

        ^^^^
This is very unusual method of error handling. You could stick
to gotos and lower the indentation level.

Ok, I'll change it.  I wasn't crazy about it after I wrote it, anyway.

quoted

quoted

+		qe = of_find_node_by_type(NULL, "qe");

Please, add compatible "fsl,qe" matching, so this code could
work with new device trees.

Ok, but that new design was added after I posted this patch.

quoted

quoted

+		/* Find the 'firmware' child node */
+		while ((fw = of_get_next_child(qe, fw)))
+			if (strcmp(fw->name, "firmware") == 0)
+				break;

Hmm. Maybe of_find_node_by_name? Or better by compatible.

What's wrong with looking for the firmware node inside the QE node? 
That's the only place it can be?

Ah, I see it now. Revoking my comment.

quoted

quoted

+		iprop = of_get_property(fw, "extended_modes", NULL);

Checking for size?

Ok.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2