[U-Boot] [PATCH 3/3] XPedite5370 board support

[Wolfgang Denk <wd@denx.de>]

Dear Peter Tyser,

In message <1227046406.3065.49.camel@localhost.localdomain> you wrote:
>
> This information is very useful to a customer and doesn't add much as
> far as output.  No newlines at least.  The output also prints
> information which is configurable.  I think giving the user feedback
> about how they have the board configured is useful.

You really need this level of detail only for board bring up and
debugging. After that, this is a constand and never changes, right? Si
it is normally enough to print the RAM size (and eventually some speed
characteristics).

> We have the standard Freescale DDR printf's turned into debug as that is
> much, much more verbose than the output above.

Indeed :-(

> > > +	flash_sel = !((pca953x_get_val(CONFIG_SYS_I2C_PCA953X_ADDR0) &
> > > +			  CONFIG_SYS_PCA953X_C0_FLASH_PASS_CS));
> > > +	printf("FLASH: Executed from FLASH%d\n", flash_sel ? 2 : 1);
> > 
> > Please be less noisy! s/printf/debug/
> 
> This information is also very useful to a customer.

Is it, really? well...

> > > +#define	CONFIG_EXTRA_ENV_SETTINGS				\
> > > + "autoload=yes\0"						\
...
> > > + "prog_fdt2="CONFIG_PROG_FDT2"\0"				\
> > > + "bootcmd_net=run set_bootargs; "CONFIG_BOOT_OS_NET"\0"		\
> > > + "bootcmd_flash1=run set_bootargs; bootm "CONFIG_OS1_FLASH_ADDR_STR" - "CONFIG_FDT1_FLASH_ADDR_STR"\0" \
> > > + "bootcmd_flash2=run set_bootargs; bootm "CONFIG_OS2_FLASH_ADDR_STR" - "CONFIG_FDT2_FLASH_ADDR_STR"\0" \
> > > + "bootcmd=run bootcmd_flash1\0"
> > > +#endif	/* __CONFIG_H */
> > 
> > Alignment not by TAB, lines way too long.
> 
> What are you referring to as far as "Alignment not by TAB"?

You indent the lines by a single space, but they should be indented by
a TAB.

> Everyone's config file breaks the 80 column rule, is this really
> necessary to change?  I think splitting it up makes an already confusing
> define even more confusing.

At least the ';' is a good place to split. And you might check if  it
really  makes  sense  to  use  variable names with 25 characters like
"CONFIG_OS2_FLASH_ADDR_STR" (no, it doesn't).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"In Christianity neither morality nor religion come into contact with
reality at any point."                          - Friedrich Nietzsche