From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Date: Tue, 18 Jan 2011 16:56:40 +0900
Subject: [U-Boot] [PATCH] sh: add support for sh7757lcr board
In-Reply-To: <20110117125040.GB13819@chimagu.nigauri.org>
References: <4D33C15C.9030801@renesas.com>
	<20110117125040.GB13819@chimagu.nigauri.org>
Message-ID: <4D3547B8.6020705@renesas.com>
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

Hi, Iwamatsu-san,

Thank you for your review!

2011/01/17 21:50, Nobuhiro Iwamatsu wrote:
>> +LDSCRIPT = $(OBJTREE)/board/$(BOARDDIR)/u-boot.lds
>> +#sinclude $(OBJTREE)/board/$(BOARDDIR)/config.tmp
>> +
> 
> Is this nesseary?
The "#sinclude" is not nessesary. I will remove it.
However the "LDSCRIPT" is necessary on this board because
the LSI needs the top of address when booting.

>> +CONFIG_SYS_TEXT_BASE = 0x8ef80000
> 
> Please move this to config file of board.
I will move it.

>> +static void mac_string_to_hex(unsigned char *mac, char *mac_string)
>> +{
>> +	int i;
>> +	char *s, *e;
>> +
>> +	s = mac_string;
>> +	for (i = 0; i < 6; i++) {
>> +		mac[i] = s ? simple_strtoul(s, &e, 16) : 0;
>> +		if (s)
>> +			s = (*e) ? e + 1 : e;
>> +	}
>> +}
> 
> Please use eth_parse_enetaddr.
Oh, I didn't know that function. I will replace it.

>> +static void init_ethernet_mac(void)
>> +{
>> +	char mac_string[64];
>> +	char env_string[64];
>> +	int i;
>> +	unsigned char *buf;
>> +
>> +	buf = malloc(256);
> 
> Please add NULL check.
OK, I will add it.

>> +	/* Gigabit Ethernet */
>> +	for (i = 0; i < SH7757LCR_GIGA_ETHERNET_NUM_CH; i++) {
>> +		get_sh_eth_mac(i + 2, mac_string, buf);
>> +		sprintf(env_string, "eth%daddr", i + 2);
> 
> I think that you should use i + SH7757LCR_ETHERNET_NUM_CH.
Thank you. I will modify it.

>> +int do_write_mac(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>> +{
>> +	int i, ret;
>> +	char mac_string[256];
>> +	struct spi_flash *spi;
>> +	unsigned char *buf;
>> +
>> +	if (argc != 5) {
>> +		buf = malloc(256);
> 
> please add NULL check.
I will modify it.

>> +/*
>> + * Copyright (C) 2007
>> + * Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
>> + *
>> + * Copyright (C) 2008-2009
>> + * Yoshihiro Shimoda <shimoda.yoshihiro@renesas.com>
> 
> Please update your copyright.
OK, I will update it.

Best regards,
Yoshihiro Shimoda