[U-Boot] [PATCH 6/6] ARM:OMAP3:SDP3430: initial support

[Nishanth Menon]

Peter Tyser said the following on 09/19/2009 09:34 AM:
thanks for your review
> Hi Nishanth,
>
> On Fri, 2009-09-18 at 21:21 -0500, Nishanth Menon wrote:
>   
>> From: David Brownell <david-b at pacbell.net>
>>
>> Start of SDP3430 support in "mainline"
>> u-boot mainline code
>>
>> Original Patch written by David Brownell
>>     
>
> I don't think the above comments are necessary.  David will be credited
> with authorship already, and the subject line and text below make it
> clear what this patch does.
>  
>   
Ack..
>>  create mode 100644 board/ti/sdp3430/sdp.h
>>  create mode 100644 board/ti/sdp3430/u-boot.lds
>>  create mode 100644 include/configs/omap3_sdp.h
>>     
>
> The board config header file should be renamed to sdp.h from
> omap3_sdp.h.  There was a recent thread discussing this convention "ARM
> Pull Request" around Sept 6.
>   
sdp3430 - there are many software development platforms ->
omap2420 based, omap2430 based etc..
Thanks for pointing this chain out.. a specific link describing
the thought will help and prevent me misunderstanding the
intention here.
>   
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e9db278..adc8a63 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -619,6 +619,7 @@ Guennadi Liakhovetski <g.liakhovetski at gmx.de>
>>  Nishanth Menon <nm at ti.com>
>>  
>>  	omap3_zoom1	ARM CORTEX-A8 (OMAP3xx SoC)
>> +	omap3_sdp	ARM CORTEX-A8 (OMAP3xx SoC)
>>     
>
> You may as well keep the boards ordered alphabetically (and remove the
> omap_ prefix from sdp).
>   
ack to alphabetical sort.
>> +/******************************************************************************
>> + * Routine: board_init
>> + * Description: Early hardware init.
>> + *****************************************************************************/
>> +int board_init(void)
>> +{
>> +	DECLARE_GLOBAL_DATA_PTR;
>> +
>>     
>
>
> I'd use the preferred multi-line comment style:
> /*
>  *
>  */
>
> There are lots of other non-preferred multi-line comments throughout the
> patch as well.
>   
ack.
> I personally don't think its necessary to put "Routine: <name>" stuff
> for each function either.  It doesn't add any benefit, adds cruft to
> grep output, and might get out of sync with the real function name at
> some point.  If it were me, I would get rid of "Description: " text too.
> Its pretty obvious that the text "Early hardware init" is a description
> of the function.
>   
not to all.. I dont like it either, I would rather go doxygen style..
will convert.

Regards,
Nishanth Menon