From mboxrd@z Thu Jan  1 00:00:00 1970
From: Igor Grinberg <grinberg@compulab.co.il>
Date: Wed, 30 Nov 2011 10:21:44 +0200
Subject: [U-Boot] [PATCH 13/13] mcx: support for HTKW mcx board
In-Reply-To: <1322498261-20645-14-git-send-email-yanok@emcraft.com>
References: <1322498261-20645-1-git-send-email-yanok@emcraft.com>
	<1322498261-20645-14-git-send-email-yanok@emcraft.com>
Message-ID: <4ED5E798.2020806@compulab.co.il>
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

Several comments in addition to Tom's comments below.

On 11/28/11 18:37, Ilya Yanok wrote:
> This patch adds support for the HTKW mcx AM3517-based board.
> Serial, Ethernet, NAND, MMC, RTC, EHCI USB host and both
> NAND and MMC SPLs are supported.
> 
> Requires updated mach-types file.
> 
> Signed-off-by: Ilya Yanok <yanok@emcraft.com>
> ---
>  MAINTAINERS             |    4 +
>  board/htkw/mcx/Makefile |   38 +++++
>  board/htkw/mcx/mcx.c    |   88 ++++++++++
>  board/htkw/mcx/mcx.h    |  408 +++++++++++++++++++++++++++++++++++++++++++++++
>  boards.cfg              |    1 +
>  include/configs/mcx.h   |  378 +++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 917 insertions(+), 0 deletions(-)
>  create mode 100644 board/htkw/mcx/Makefile
>  create mode 100644 board/htkw/mcx/mcx.c
>  create mode 100644 board/htkw/mcx/mcx.h
>  create mode 100644 include/configs/mcx.h

[...]

> diff --git a/board/htkw/mcx/mcx.c b/board/htkw/mcx/mcx.c
> new file mode 100644
> index 0000000..7ee23b0
> --- /dev/null
> +++ b/board/htkw/mcx/mcx.c

[...]

> +
> +/*
> + * Routine: misc_init_r
> + * Description: Init i2c, ethernet, etc... (done here so udelay works)

Please, fix the comment.

> + */
> +int misc_init_r(void)
> +{
> +	dieid_num_r();
> +
> +	return 0;
> +}

[...]

> +#if defined(CONFIG_GENERIC_MMC) && defined(CONFIG_OMAP_HSMMC) && \
> +	!defined(CONFIG_SPL_BUILD)
> +int board_mmc_init(bd_t *bis)
> +{
> +	omap_mmc_init(0);
> +	return 0;

return omap_mmc_init(0);

I would also define this function ("weak") in some common place,
so we will not need to copy/paste it all over the boards, but
of course, it is not a blocker and I don't request this from you.
Only if you want to... :-)

> +}
> +#endif
> +
> +#ifdef CONFIG_USB_EHCI_OMAP
> +#define USB_HOST_PWR_EN		132
> +int board_usb_init(void)
> +{
> +	gpio_request(USB_HOST_PWR_EN, "USB_HOST_PWR_EN");
> +	gpio_direction_output(USB_HOST_PWR_EN, 1);

probably, you should check here for failure and
return the right value? may be printf something?

> +
> +	return 0;
> +}
> +#endif

[...]


-- 
Regards,
Igor.