[U-Boot] [PATCH V2 11/11] Add support for Freescale's mx35pdk board.

[Stefano Babic]

On 01/20/2011 10:41 AM, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 
> In message <1295513194-16158-12-git-send-email-sbabic at denx.de> you wrote:
>> The patch adds suupport for the Freescale's mx35pdk board
>> (known as well as mx35_3stack).
> 
> Checkpatch says:
> 
> 	[U-Boot] [PATCH V2 11/11] Add support for Freescale's mx35pdk board.
> 	total: 0 errors, 1 warnings, 1331 lines checked
> 
> (Prototype for smc911x_initialize() is in netdev.h).

Thanks, I have not search with attention. I fix it.

> Please use TAB for indentation.

Thanks - in this case checkpatch reports nothing, and I suppose
everything is ok.

> 
> 
>> +int checkboard(void)
>> +{
>> +	struct ccm_regs *ccm =
>> +		(struct ccm_regs *)IMX_CCM_BASE;
>> +
>> +	puts("Board: MX35 PDK ");
>> +
>> +	/*
>> +	 * Be sure that I2C is initialized to check
>> +	 * the board revision
>> +	 */
>> +	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> 
> Error checking?

I wil ladd it

> 
>> +	/* Print board revision */
>> +	puts(board_detect() ? "2.0" : "1.0");
>> + 
>> +	/* Print CPU revision */
>> +	puts(" i.MX35 ");
> 
> I mentioned this before:  If you make board_detect() return 1 or 2, 
> you can combine the calls to puts() for example like that:

I was unsure, it seemed to me easier to understand as it is implemented
now, becaues board_detect() is used to detect if the PMIC is installed
or not. It returns 0 or 1, and tell if the test for the PMIC was
successful.

Probably the former version of the board has no pmic at all or was not
connected to I2C. So in another part of code board_detect is used in
boolean form:

if (board_detect()) {

I thought that to combine the result makes some confusion. Probably it
is clearer to use get_board_rev() instead of board_detect() and to
extract the revision number from the returned u32:

 	printf("Board: MX35 PDK %d.0 i.MX35 ", (get_board_rev() >> 8) & 0xFF);

> Eventually similar improvment could be done here. [Just a hint - feel
> free to post-pone into a later patch if this affects other boards as
> well.]

Agree, to change this I would prefer a patch for all i.MX boards at the
same time. They use at the moment the same approach I use now.

best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================