From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Sat, 10 Apr 2010 00:51:06 +0200
Subject: [U-Boot] [PATCH 3/7] added Mercury EP2500 board support It uses
	the mcf5282 processor with real time clock and EEPROM.
In-Reply-To: <op.vatgy2wjqigx4y@cyprus.local>
References: <op.vatgy2wjqigx4y@cyprus.local>
Message-ID: <20100409225106.A5E021E173@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear "David Wu",

In message <op.vatgy2wjqigx4y@cyprus.local> you wrote:
> Signed-off-by: David Wu <davidwu@arcturusnetworks.com>
> ---
>   board/Mercury/ep2500/Makefile   |   44 ++++++
>   board/Mercury/ep2500/config.mk  |   23 +++
>   board/Mercury/ep2500/ep2500.c   |  191 +++++++++++++++++++++++++
>   board/Mercury/ep2500/u-boot.lds |  140 ++++++++++++++++++
>   include/configs/EP2500.h        |  297  
> +++++++++++++++++++++++++++++++++++++++
>   5 files changed, 695 insertions(+), 0 deletions(-)
>   create mode 100644 board/Mercury/ep2500/Makefile
>   create mode 100644 board/Mercury/ep2500/config.mk
>   create mode 100644 board/Mercury/ep2500/ep2500.c
>   create mode 100644 board/Mercury/ep2500/u-boot.lds
>   create mode 100644 include/configs/EP2500.h

Your Subject (= commit message tile) is WAY too long.

Don't try to press everything in a single line.

Entries to MAINTAINERS and MAKEALL are missing.

> +void board_get_enetaddr(uchar *enet)
> +{
> +	int i;
> +	unsigned char buff[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> +
> +	/* Read MAC address (6 bytes) in EEPROM */
> +	if (i2c_read
> +	    (CONFIG_SYS_I2C_EEPROM_ADDR, MAC_ADDRESS_OFFSET_IN_EEPROM,
> +	     CONFIG_SYS_I2C_EEPROM_ADDR_LEN, buff, 6) != 0) {
> +		puts("Error reading the EEPROM.\n");
> +	}
> +
> +	/*
> +	 * When buff returns all 0xFF the EEPROM has not
> +	 * been programed with a valid MAC. In this case
> +	 * we set enet to all 0x00 as 0xFF is not valid
> +	 * for this usage model.
> +	 */
> +	if (buff[0] == 0xff && buff[1] == 0xff && buff[2] == 0xff &&
> +	    buff[3] == 0xff && buff[4] == 0xff && buff[5] == 0xff) {

We have standard functions for such checks. Please use them.

> +		for (i = 0; i < 6; i++)
> +			enet[i] = 0;
> +	} else {
> +		for (i = 0; i < 6; i++)
> +			enet[i] = buff[i];
> +	}

Hm... the whole code makes no sense to me, as neiter all-ones nor
all-zeros are legal MAC addresses.


> +int misc_init_r(void)
> +{
> +#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
> +	uchar enetaddr[6];
> +
> +	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
> +		board_get_enetaddr(enetaddr);
> +		eth_setenv_enetaddr("ethaddr", enetaddr);

Please see recent discussion about this topic.


> +void i2c_init_board(void)
> +{
> +	struct fsl_i2c *dev;
> +
> +	dev = (struct fsl_i2c *)(CONFIG_SYS_IMMR + CONFIG_SYS_I2C_OFFSET);
> +
> +	if (readb(&dev->sr) & I2C_SR_MBB) {
> +		writeb(0, &dev->cr);
> +		writeb(0xa0, &dev->cr);
> +		readb(&dev->dr);
> +		writeb(0x0, &dev->sr);
> +		writeb(0, &dev->cr);
> +		writeb(I2C_CR_MEN, &dev->cr);

Please add comments what you are doing. Replace 0x0A by a symbolic
name. Use a consistent style (i. e. wither use "0x0" or "0", but
don't mix both styles.

> +#ifdef CONFIG_MCFFEC
> +#	define CONFIG_ETHADDR	00:00:00:00:00:00
> +#	define CONFIG_IPADDR	192.168.1.2
> +#	define CONFIG_NETMASK	255.255.255.0
> +#	define CONFIG_SERVERIP	192.168.1.1
> +#	define CONFIG_GATEWAYIP	192.168.1.1
> +#	define CONFIG_OVERWRITE_ETHADDR_ONCE
> +/*#define CONFIG_ENV_OVERWRITE */

Please remove this, it will not be accepted.


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
Calm down, it's *__only* ones and zeroes.