[U-Boot] [PATCH v2] OMAP3: add CM-T35 board

[Wolfgang Denk]

Dear Mike Rapoport,

In message <4CFF26B0.1080900 at compulab.co.il> you wrote:
> 
> This patch adds support for CM-T35 board
> 
> Signed-off-by: Mike Rapoport <mike at compulab.co.il>
> ---
...
> diff --git a/MAKEALL b/MAKEALL
> index c54c6e8..e0fe12f 100755
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -412,6 +412,7 @@ LIST_ARM11="			\
>  LIST_ARMV7="		\
>  	am3517_evm		\
>  	ca9x4_ct_vxp		\
> +	cm_t35			\
>  	devkit8000		\
>  	igep0020		\
>  	igep0030		\

Boards don't get added to MAKEALL any more. Please verify that your
board automatically gets picked up from boards.cfg


> +/*
> + * Routine: misc_init_r
> + * Description: Init ethernet (done here so udelay works)
> + */
> +int misc_init_r(void)
> +{
> +#ifdef CONFIG_DRIVER_OMAP34XX_I2C
> +	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> +#endif
> +
> +	dieid_num_r();
> +
> +	return 0;
> +}

Comment and code don't appear to match.

> +/*
> + * Routine: set_muxconf_regs
> + * Description: Setting up the configuration Mux registers specific to the
> + *		hardware. Many pins need to be moved from protect to primary
> + *		mode.
> + */
> +void set_muxconf_regs(void)
> +{
> +	MUX_CM_T35();
> +}

Why don't you write this code here directly? Adding another macro
level here just obfuscates the code.  Or what is your rationale for
moving this into a header file ?

> +#ifdef CONFIG_GENERIC_MMC
> +int board_mmc_init(bd_t *bis)
> +{
> +	omap_mmc_init(0);
> +	return 0;

Error handling?


> +static int handle_mac_address(void)
> +{
> +	unsigned char enetaddr[6];
> +	int rc;
> +
> +	memset(enetaddr, 0, 6);
> +	rc = eth_getenv_enetaddr("ethaddr", enetaddr);

What exactly is the memset() good for?

> diff --git a/board/cm_t35/config.mk b/board/cm_t35/config.mk
> new file mode 100644
> index 0000000..e81e283
> --- /dev/null
> +++ b/board/cm_t35/config.mk
...
> +CONFIG_SYS_TEXT_BASE = 0x80008000

Please move to board config file and get rid of config.mk

> --- /dev/null
> +++ b/include/configs/cm_t35.h
...
> +/*
> + * select serial console configuration
> + */
> +#define CONFIG_CONS_INDEX		3
> +#define CONFIG_SYS_NS16550_COM3		OMAP34XX_UART3
> +#define CONFIG_SERIAL3			3	/* UART3 on Beagle Rev 2 */

Beagle Rev 2?

> +#undef CONFIG_CMD_IMI		/* iminfo			*/

What is the exact reson for disabling iminfo ?


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
You see but you do not observe.
Sir Arthur Conan Doyle, in "The Memoirs of Sherlock Holmes"