[U-Boot] [PATCH v2] board: Add support for B&R T-Series Motherboard

[Wolfgang Denk]

Dear Hannes,

In message <1391606231-24963-1-git-send-email-oe5hpm at oevsv.at> you wrote:
> Adds support for Bernecker & Rainer Industrieelektronik GmbH T-Series
> Motherboard, using TI's AM3352 SoC.
> 
> Most of code is derived from TI's AM335x_EVM
> 
> Signed-off-by: Hannes Petermaier <oe5hpm at oevsv.at>
> Cc: trini at ti.com
> ---
> Changes for v2:
>    - added T-Series to boards.cfg (forgotten before)
> 
>  board/BuR/bur_tseries/Makefile   |   13 +
>  board/BuR/bur_tseries/board.c    |  333 +++++++++++++++++++++++++
>  board/BuR/bur_tseries/board.h    |   18 ++
>  board/BuR/bur_tseries/mux.c      |  224 +++++++++++++++++
>  board/BuR/bur_tseries/u-boot.lds |  101 ++++++++
>  boards.cfg                       |    3 +
>  include/configs/bur_tseries.h    |  512 ++++++++++++++++++++++++++++++++++++++

This is soemwhat redundant naming - you have the vendor name already
encoded in the vendor directory, so it is not necessary to repeat it
in the board directory name.

I think it would be better to have:

	board/BuR/tseries/*


> +++ b/board/BuR/bur_tseries/board.c
...
> + * 04.02.2014	hpm		very first board support for T-SERIES,
> + *				based on board/ti/am335x/board.c

Please do not add any such changelog information to the source code.
We use git to track code modifications, which is much more powerful.

> +/* --------------------------------------------------------------------------*/
> +/* -- defines for GPIO -- */
> +#define	ETHLED_ORANGE	(96+16)	/* GPIO3_16 */
> +#define	REPSWITCH	(0+20)	/* GPIO0_20 */

Should these not rather go to the board config header file?

> diff --git a/board/BuR/bur_tseries/u-boot.lds b/board/BuR/bur_tseries/u-boot.lds
> new file mode 100644
> index 0000000..76db891
> --- /dev/null
> +++ b/board/BuR/bur_tseries/u-boot.lds

Are you sure you need a board specific linker script?  Why?

> diff --git a/include/configs/bur_tseries.h b/include/configs/bur_tseries.h
> new file mode 100644
> index 0000000..a5e6dbc
> --- /dev/null
> +++ b/include/configs/bur_tseries.h
> @@ -0,0 +1,512 @@
...
> +/*#define DEBUG */
...
> +#if _HPMTEST
> +#define CONFIG_SILENT_CONSOLE
> +#define CONFIG_SILENT_U_BOOT_ONLY
> +#define CONFIG_SILENT_CONSOLE_UPDATE_ON_SET
> +#define CONFIG_SYS_DEVICE_NULLDEV
> +#endif /* _HPMTEST */

Please remove such dead code.

> +#define CONFIG_ENV_VARS_UBOOT_CONFIG	/* Strongly encouraged */

Really?  What exactly do you use these for?


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
Success is  not  the  opposite  of  failure.  Failure  is  the  vital
ingredient in the recipe for success.                  - Mario Cortes