[U-Boot] [RFC][PATCH 2b/3] New i386 board (includes code relocation)

[Wolfgang Denk]

Dear Graeme Russ,


Note: all comments made to part 'a' of this patch apply here, too.

In message <48E0D7CB.6040706 at gmail.com> you wrote:
> Split to meet mailing list size limit

This should not be part of the git commit message, so please move
below the '---' line.

> Initial addition of eNET files - builds clean but will not run until
> additional i386 code changes are made
> 
> Signed-off-by: Graeme Russ <graeme.russ at gmail.com>
> 
> --

These should be 3 (three) dashes:

---

And any comments that are not intended for the commit message must go
*below* that line..

> diff --git a/board/eNET/fpga.c b/board/eNET/fpga.c
> new file mode 100644
> index 0000000..a3e4677
> --- /dev/null
> +++ b/board/eNET/fpga.c
> @@ -0,0 +1,149 @@
> +/*
> + * (C) Copyright 2002
> + * Wolfgang Grandegger, DENX Software Engineering, wg at denx.de.

Is this correct and complete?

> +#include "fpga.h"
> +
> +static u8 fpga_init(void);
> +static u8 fpga_write(void *buf, size_t bsize);
> +static u8 fpga_finalise(void);
> +static void fpga_close(void);
> +
> +
> +int fpga_load(int devnum, void *buf, size_t bsize )
> +{
> +	u8 ret = FPGA_SUCCESS;

Do you really have to copy that code?

> diff --git a/board/eNET/hardware.h b/board/eNET/hardware.h
> new file mode 100644
> index 0000000..eab612c
> --- /dev/null
> +++ b/board/eNET/hardware.h
> @@ -0,0 +1,13 @@
> +/*
> + * hardware.h
> + *
> + *  Created on: 17/09/2008
> + *      Author: graeme
> + */
> +
> +#ifndef HARDWARE_H_
> +#define HARDWARE_H_
> +
> +#include "hardware_defs.h"
> +
> +#endif /* HARDWARE_H_ */

Um... A header file that just includes a single othe rheader file does
not make sense to me here.

> diff --git a/include/asm-i386/ic/sc520_defs.h b/include/asm-i386/ic/sc520_defs.h
> new file mode 100644
> index 0000000..c8f6311
> --- /dev/null
> +++ b/include/asm-i386/ic/sc520_defs.h
> @@ -0,0 +1,1489 @@
> +/*
> + * sc520_defs.h
> + *
> + *  Created on: 17/09/2008
> + *      Author: graeme
> + */

Do we really need all these 1500 lines of definitions?

Really?

...
> +/* General-Purpose CMOS RAM Mask */
> +
> +#define RTC_CMOS_REG_X		0xFF			/* CMOS RAM Location */
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +#endif /* _ASM_IC_SC520_DEFS_H_ */

Too many empty lines - max. 2 allowed. Here, not more tna 1 should be
used.

This applies to some other files as well.


Please clean up, rebase against current code, and resubmit.

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
"It follows that any commander in chief who undertakes to carry out a
plan which he considers defective is at fault; he must put forth  his
reasons,  insist  of  the  plan being changed, and finally tender his
resignation rather than be the instrument of his army's downfall."
- Napoleon, "Military Maxims and Thought"