From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Tue, 25 Jan 2011 21:36:42 +0100
Subject: [U-Boot] [PATCH 13/13] add dvlhost (dLAN 200 AV Wireless G)
	board
In-Reply-To: <1294062338-21084-14-git-send-email-michael@schwingen.org>
References: <1294062338-21084-1-git-send-email-michael@schwingen.org>
	<1294062338-21084-14-git-send-email-michael@schwingen.org>
Message-ID: <20110125203642.C9F00B187@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 Michael Schwingen,

In message <1294062338-21084-14-git-send-email-michael@schwingen.org> you wrote:
> 
> Signed-off-by: Michael Schwingen <michael@schwingen.org>
...
> diff --git a/board/dvlhost/config.mk b/board/dvlhost/config.mk
> new file mode 100644
> index 0000000..a370337
> --- /dev/null
> +++ b/board/dvlhost/config.mk
> @@ -0,0 +1,3 @@
> +LDSCRIPT := $(SRCTREE)/board/$(BOARDDIR)/u-boot.lds
> +PLATFORM_RELFLAGS += -ffunction-sections -fdata-sections
> +PLATFORM_LDFLAGS += --gc-sections

Please drop that file, it should not be needed.

...
> +int board_init (void)
> +{
> +	gd->bd->bi_arch_number = MACH_TYPE_DVLHOST;
> +
> +	/* adress of boot parameters */
> +	gd->bd->bi_boot_params = 0x00000100;
> +
> +	/* Setup GPIOs used as output */
> +	GPIO_OUTPUT_CLEAR(CONFIG_SYS_GPIO_WDGTRIGGER);
> +	GPIO_OUTPUT_SET(CONFIG_SYS_GPIO_DLAN_PAIRING);
> +	GPIO_OUTPUT_CLEAR(CONFIG_SYS_GPIO_PCIRST);
> +
> +	/* LED latch enable and watchdog enable are tied to the same GPIO,
> +	   so we need to trigger the watchdog if we want to enable the LEDs. */

Incorrect multiline comment style, please fix globally.


> +int dram_init (void)
> +{
> +	gd->ram_size = PHYS_SDRAM_1_SIZE;
> +	return (0);

You should use get_ram_size() here.


> diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> new file mode 100644
> index 0000000..606a46f
> --- /dev/null
> +++ b/board/dvlhost/u-boot.lds

Do you really need a board specific linker script?
It looks pretty much generic to me.


> +void hw_watchdog_reset(void)
> +{
> +	*IXP425_GPIO_GPOUTR ^= (1 << (CONFIG_SYS_GPIO_WDGTRIGGER));

Please always use I/O accessors.  Please fix globally.

...
> +
> +#undef CONFIG_HW_WATCHDOG		/* HW Watchdog, board specific	*/

Please do not undef what is not defined anyway. Please fix globally.



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
By the way, ALL software projects are done by iterative  prototyping.
Some companies call their prototypes "releases", that's all.