[U-Boot] [PATCH] V2 NAND_SPL support for phycore imx31to Scott Wood

[Scott Wood]

On Wed, Jan 28, 2009 at 07:43:54AM +0300, Maxim Artamonov wrote:
> In message on Friday 12 December 2008 04:55:49 Scott Wood wrote:
> > On Fri, Dec 05, 2008 at 02:13:51PM +0300, Maxim Artamonov wrote:
> > > +#ifdef CONFIG_NAND_SPL
> > > +/* somewhat macro to reduce bin size for CONFIG_NAND_SPL*/
> > > +.macro FILLREGS begreg, val, count, step
> > 
> > Why not use this macro always?
> Why not? But, I think this 'll lose clearity of code. Let's discuss this.

You want unnecessary ifdefs to *increase* clarity?

> > > +#define NFC_BUF_SIZE            (*((volatile u16 *)(NFC_BASE_ADDR + 0xE00)))
> > > +#define NFC_BUF_ADDR            (*((volatile u16 *)(NFC_BASE_ADDR + 0xE04)))
> >> ...
> > > +#define NFC_CONFIG1             (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1A)))
> > > +#define NFC_CONFIG2             (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1C)))
> > 
> > Use I/O accessors, 
> What you about, tell me please?

Use readw()/writew().

> > and move the NFC register layout to a 
> > non-arch-specific header file (preferably as a struct), as it's shared
> > with other chips.
> 
> Which chips? 

There was a separate driver for the same NAND controller on 5xxx PowerPC
posted.

> This is a system's integration task, but I wrote only for imx-31
> phycore.

That's what I'm complaining about.  Please do not have tunnel vision such
that you only see your own chip.

> > > +	if (mx31_nand_check_ecc ())
> > > +		while (1){
> > > +			break;};	/*!!!!!*/
> > 
> > What is this?
> Ok, let's it be infinitive while-cycle.

Except it isn't, due to the break.

In any case, when there's something unusual enough to produce a comment
of /*!!!!!*/, it would be much more helpful to replace that comment with
*words* that describe what's going through your head. :-)

-Scott