[U-Boot] [PATCH v2] add new board nas62x0

[Marek Vasut]

Dear Luka Perkov,

> Hi Marek,
> 
> On Sun, Mar 18, 2012 at 04:15:53PM +0100, Marek Vasut wrote:
> > > + * Copyright (C) 2011 G??rald Kerma <dreagle at doukki.net>
> > 
> > Can you please fix your name here?
> 
> I think your mail agent is not displaying UTF8 characters correctly. If
> this is a problem we could change it if Gerald is ok with that. Nobody
> else had problems with this...
> 
> > > +#define IB62x0_OE_LOW		(~(0))
> > > +#define IB62x0_OE_HIGH		(~(0))
> > 
> > Fix this constant please (0xffffffff) and remove those parenthesis ...
> > btw OE_HIGH and OE_LOW have both the same value?
> 
> Are you sure? It's done this way in:
> 
> board/Marvell/dreamplug/dreamplug.h
> board/Marvell/sheevaplug/sheevaplug.h
> board/Seagate/dockstar/dockstar.h

Does that mean it's inherently correct and not just a duplicated bug?

> 
> > > +# Configure RGMII-0 interface pad voltage to 1.8V
> > > +DATA 0xFFD100e0 0x1b1b1b9b
> > 
> > Make usage of upper/lower case consistent across files in your patch
> > please (lowercase prefered).
> 
> Ok. I will send this in v4 once I get your feedback on other items.

Thanks

> 
> > > +#define CONFIG_SKIP_LOWLEVEL_INIT	/* disable board lowlevel_init 
*/
> > 
> > Are you sure you want to skip lowlevel init? It'll break cache setup etc.
> > I believe.
> 
> I will retest and send v4 once I get your feedback on other items.

Ok, what's the result? From IRC I take it you must define this ... why?
> 
> > > +# define CONFIG_SYS_PROMPT	"IB-NAS6210 # "
> > > +#elif CONFIG_BOARD_IS_IB_NAS6220
> > > +# define CONFIG_SYS_PROMPT	"IB-NAS6220 # "
> > > +#else
> > > +# error Unknown RaidSonic ICY BOX board specified
> > > +#endif
> > 
> > Please make the prompt like "=> " so we can run tests on this :)
> 
> Ok.
> 
> > > +#define CONFIG_MVSATA_IDE_USE_PORT0
> > > +# ifdef CONFIG_BOARD_IS_IB_NAS6210
> > > +#  undef CONFIG_SYS_IDE_MAXBUS
> > > +#  define CONFIG_SYS_IDE_MAXBUS		1
> > > +#  undef CONFIG_SYS_IDE_MAXDEVICE
> > > +#  define CONFIG_SYS_IDE_MAXDEVICE	1
> > > +# elif CONFIG_BOARD_IS_IB_NAS6220
> > > +#  define CONFIG_MVSATA_IDE_USE_PORT1
> > > +# endif
> > > +#define CONFIG_SYS_ATA_IDE0_OFFSET	KW_SATA_PORT0_OFFSET
> > > +# ifdef CONFIG_BOARD_IS_IB_NAS6220
> > > +#  define CONFIG_SYS_ATA_IDE1_OFFSET	KW_SATA_PORT1_OFFSET
> > > +# endif
> > > +#endif /* CONFIG_CMD_IDE */
> > 
> > please don't use this "#[space][space]define" convention.
> 
> I must disagree here. This is more readable and there are many examples
> in u-boot that use that syntax:
> 
> $ egrep '#[ ]+define' * -r | wc -l
> 12557

Again, the fact that it's used doesn't mean it's correct. It's not more readable 
actually, it's readable in the same way given you have good editor. Also, 
doesn't checkpatch.pl caugh on this stuff ?
> 
> > The patch looks good otherwise )
> 
> Cool. Thanks for reviewing.

You're welcome ;-)

> 
> Bye,
> Luka

Best regards,
Marek Vasut