[U-Boot] [PATCH 4/4] ARM: omap3: Added Teejet mt_ventoux

[Stefano Babic <sbabic@denx.de>]

On 29/12/2011 09:42, Igor Grinberg wrote:
> Hi Stefano,
> 

Hi Igor,

> Isn't Ilya is author of these?
> I would expect you (at least) to put him in Cc, no?

No, he's not, but these patches rely on Ilya's patches for AM3517. It is
sure a good idea to add him in CC

>> +distclean:	clean
>> +	rm -f $(LIB) core *.bak $(obj).depend
> 
> Please, remove these...

Sure, bad cut&paste..

>> +#include "mt_ventoux.h"
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#if defined(CONFIG_FPGA)
> 
> Probably #ifdef would be better or
> see the comment in the end of this section...

ok

>> +#define FPGA_RESET	62
>> +#define FPGA_PROG	116
>> +#define FPGA_CCLK	117
>> +#define FPGA_DIN	118
>> +#define FPGA_INIT	119
>> +#define FPGA_DONE	154
>> +
>> +/* Timing definitions for FPGA */
>> +static const u32 gpmc_fpga[] = {
>> +	FPGA_GPMC_CONFIG1,
>> +	FPGA_GPMC_CONFIG2,
>> +	FPGA_GPMC_CONFIG3,
>> +	FPGA_GPMC_CONFIG4,
>> +	FPGA_GPMC_CONFIG5,
>> +	FPGA_GPMC_CONFIG6,
>> +};
>> +
>> +static void fpga_reset(int nassert)
>> +{
>> +	if (nassert)
>> +		gpio_set_value(FPGA_RESET, 0);
>> +	else
>> +		gpio_set_value(FPGA_RESET, 1);
>> +}
> 
> Isn't this just:
> gpio_set_value(FPGA_RESET, !nassert);

Yes, and it is a cleaner way - got it, I will fix it.

> 
> Also, this function is called once, may be just inline it instead?
> 

Agree.

>> +
>> +int fpga_pgm_fn(int nassert, int nflush, int cookie)
>> +{
>> +	debug("%s:%d: FPGA PROGRAM ", __func__, __LINE__);
>> +
>> +	if (nassert) {
>> +		gpio_set_value(FPGA_PROG, 0);
>> +		debug("asserted\n");
>> +	} else {
>> +		gpio_set_value(FPGA_PROG, 1);
>> +		debug("deasserted\n");
>> +	}
>> +
>> +	return nassert;
>> +}
> 
> This function look very similar to the fpga_reset(),
> may they can be consolidated in some way?

I check it

>> +int fpga_init_fn(int cookie)
>> +{
>> +	u32 val = gpio_get_value(FPGA_INIT);
>> +
>> +	return (val == 0);
>> +}
> 
> Isn't this just:
> return !gpio_get_value(FPGA_INIT);

Right - fixed in V2

>> +int fpga_pre_config_fn(int cookie)
>> +{
>> +
> 
> needless empty line?

Thanks, I drop it

> 
> I would also recommend to put all this fpga handling code in
> a separate file and tweak the Makefile to compile it out if
> !CONFIG_FPGA instead of the #ifdefs here.
> This can also spare you the need for #ifdef around
> mt_ventoux_init_fpga(); below (if you stub it in a .h file).

After checking the hardware again there is no reason to compile without
FPGA support. The board relies on the FPGA itself and U-Boot should
never compiled without a way to load the fpga. For this reason I can
drop completely any #ifdef CONFIG_FPGA.

>> +#ifndef __CONFIG_H
>> +#define __CONFIG_H
>> +
>> +#include "tam3517-common.h"
>> +
>> +#define MACH_TYPE_AM3517_MT_VENTOUX	3832
>> +#define CONFIG_MACH_TYPE	MACH_TYPE_AM3517_MT_VENTOUX
>> +
>> +#define	CONFIG_CMD_FPGA
> 
> Can it be space instead of tab after define, like all others?

Sure, I check it in the whole file.

Thanks,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================