[U-Boot] [PATCH] OMAP3: Add Corscience Tricorder board

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Thomas,

Several comments in addition to Tom's comments:

On 12/15/11 12:16, Thomas Weber wrote:
> Tricorder is a board which is very similar to the Devkit8000. It
> is designed as a base platform for further medical devices.
> 
> www.corscience.de/en/medical-engineering/products/multiparameter/mp10-board.html
> 
> Signed-off-by: Thomas Weber <weber@corscience.de>
> ---
>  MAINTAINERS                            |    1 +
>  board/corscience/tricorder/Makefile    |   46 ++++
>  board/corscience/tricorder/tricorder.c |  114 ++++++++++
>  board/corscience/tricorder/tricorder.h |  375 ++++++++++++++++++++++++++++++++
>  boards.cfg                             |    1 +
>  include/configs/tricorder.h            |  325 +++++++++++++++++++++++++++
>  6 files changed, 862 insertions(+), 0 deletions(-)
>  create mode 100644 board/corscience/tricorder/Makefile
>  create mode 100644 board/corscience/tricorder/tricorder.c
>  create mode 100644 board/corscience/tricorder/tricorder.h
>  create mode 100644 include/configs/tricorder.h

[...]

> diff --git a/board/corscience/tricorder/tricorder.c b/board/corscience/tricorder/tricorder.c
> new file mode 100644
> index 0000000..3426f2e
> --- /dev/null
> +++ b/board/corscience/tricorder/tricorder.c
> @@ -0,0 +1,114 @@
> +/*
> + * (C) Copyright 2004-2008
> + * Texas Instruments, <www.ti.com>
> + *
> + * Author :
> + *	Sunil Kumar <sunilsaini05@gmail.com>
> + *	Shashi Ranjan <shashiranjanmca05@gmail.com>
> + *
> + * (C) Copyright 2011
> + * Corscience GmbH & Co. KG, <www.corscience.de>
> + * Thomas Weber <weber@corscience.de>

Something is wrong here with authorship...
IMO, you should be the author,
and people above listed in derived from ... or something.

> + *
> + * Derived from Devkit8000 code by
> + * Frederik Kriewitz <frederik@kriewitz.eu>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA

I would also, remove the postal address, as it may change
(and did in the past), but it is your call.

> + */
> +#include <common.h>
> +#include <twl4030.h>
> +#include <asm/io.h>
> +#include <asm/arch/mmc_host_def.h>
> +#include <asm/arch/mux.h>
> +#include <asm/arch/sys_proto.h>
> +#include <asm/arch/mem.h>
> +#include <asm/mach-types.h>
> +#include "tricorder.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * Routine: board_init
> + * Description: Early hardware init.
> + */
> +int board_init(void)
> +{
> +	gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
> +	/* board id for Linux */
> +	gd->bd->bi_arch_number = MACH_TYPE_TRICORDER;

This should be done in board config file (see the README).
Also, after moving this to config file, I think, the include
for mach-types.h can be removed.

> +	/* boot param addr */
> +	gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100);
> +
> +	return 0;
> +}

[...]

> diff --git a/board/corscience/tricorder/tricorder.h b/board/corscience/tricorder/tricorder.h
> new file mode 100644
> index 0000000..c875dfa
> --- /dev/null
> +++ b/board/corscience/tricorder/tricorder.h

[...]

> +#define MUX_TRICORDER() \
> + /* SDRC */\

Please, use tabs for indentation and please fix globally.

[...]
> +	/* */\

This is really a nice comment, but...
That is one of the reasons I like the whole thing done in a
function or an array rather than in a multi-line macro...

[...]

> diff --git a/include/configs/tricorder.h b/include/configs/tricorder.h
> new file mode 100644
> index 0000000..6a66bee
> --- /dev/null
> +++ b/include/configs/tricorder.h

[...]

> +/* I2C */
> +#define CONFIG_HARD_I2C
> +#define CONFIG_SYS_I2C_SPEED		100000
> +#define CONFIG_SYS_I2C_SLAVE		1
> +#define CONFIG_SYS_I2C_BUS		0
> +#define CONFIG_SYS_I2C_BUS_SELECT	1
> +#define CONFIG_DRIVER_OMAP34XX_I2C	1

Some of these should not have a value, right?

> +#define CONFIG_ENV_IS_IN_NAND		1
> +#define SMNAND_ENV_OFFSET		0x260000 /* environment starts here */
> +
> +#define CONFIG_ENV_OFFSET		SMNAND_ENV_OFFSET

Can't the above be inlined?

[...]


-- 
Regards,
Igor.