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

Nishanth Menon
2009-09-19T15:43:01Z

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@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@gmx.de>
>> Nishanth Menon <nm@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