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 <davi...@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.liakhovet...@gmx.de> >> Nishanth Menon <n...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot