Re: [PATCH v5] qe: add ability to upload QE firmware

[Timur Tabi <timur@freescale.com>]

Anton Vorontsov wrote:

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

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

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

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

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

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

>> +		iprop = of_get_property(fw, "extended_modes", NULL);
> 
> Checking for size?

Ok.