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: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
