Re: qe: add ability to upload QE firmware

[Timur Tabi <timur@freescale.com>]

Anton Vorontsov wrote:
>> +	if (firmware->length == (length + sizeof(u32))) {
>> +		/* Length is valid, and there's a CRC */
>> +		crc = be32_to_cpu(*((__be32 *) ((void *) firmware + length)));
> 
> Spaces are not needed after "(__be32 *)" and "(void *)". The whole
> construction isn't easy to parse, thus spaces are only distracting.

I have to disagree.  I added the spaces to make it easier to read.

>> +	/* If there's only one microcode, then we assume it's common for all
>> +	   RISCs, so we set the CERCR.CIR bit to share the IRAM with all RISCs.
>> +	   This should be safe even on SOCs with only one RISC.
>> +
>> +	   If there are multiple 'microcode' structures, but each one points
>> +	   to the same microcode binary (ignoring offsets), then we also assume
>> +	   that we want share RAM.
>> +	 */
> 
> Comment style is unorthodox.

Should I prefix each line with a "*"?

>> +		code = (void *) firmware + be32_to_cpu(ucode->code_offset);
> 
> space after (void *).

Really?  This:

	code = (void *)firmware + be32_to_cpu(ucode->code_offset);

is harder to read.  Without the space, it looks like *)firmware is one word.

I shall apply the other changes.  Thanks.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale