[flashrom] [PATCH] Add support for PicoTAP (by GOEPEL) FT2232 SPI programmer

[Uwe Hermann]

On Sat, Oct 15, 2011 at 03:00:45AM +0900, Samir Ibradžić wrote:
> Hello,
> 
> I would like to submit a patch, that enables support for another
> FT2232 device, PicoTAP by GOEPEL electronic GmbH.

Great, thanks a lot!

You forgot the "Signed-off-by" line though, please check

http://flashrom.org/Development_Guidelines
and thus also
http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

and resend the (updated) patch. Thanks!

 
> Btw, I managed to run PicoTAP in 10MHz, 15MHz and 30Mhz modes (by
> forcing DIVIDE_BY), against SST25VF016B SPI flash, read/write/erase
> all worked fine (write seems somewhat slow). For the sake of more
> testing, is there any way 20Mhz can be set in FT2232?

Hm, dunno, need to check datasheets.

 
> I am also thinking about implementing passing frequency divider as
> an option, for example:
> 
> # flashrom -p ft2232_spi:type=picotap,divider=2
> 
> DIVIDE_BY constant would be used as default. I think this parameter
> would be useful, but would like to hear more opinions.

Sounds useful, yes. I don't know if there are reasons not to make this a
user-visible option, I'd wait for more feedback here (this would be
another patch anyway).


> Index: ft2232_spi.c
> ===================================================================
> --- ft2232_spi.c	(revision 1450)
> +++ ft2232_spi.c	(working copy)
> @@ -43,6 +43,9 @@
>  #define OLIMEX_ARM_OCD_H_PID	0x002B
>  #define OLIMEX_ARM_TINY_H_PID	0x002A
> 
> +#define GOEPEL_VID		0x096C
> +#define PICOTAP_PID		0x1449
> +
>  const struct usbdev_status devs_ft2232spi[] = {
>  	{FTDI_VID, FTDI_FT2232H_PID, OK, "FTDI", "FT2232H"},
>  	{FTDI_VID, FTDI_FT4232H_PID, OK, "FTDI", "FT4232H"},
> @@ -53,6 +56,7 @@
>  	{OLIMEX_VID, OLIMEX_ARM_TINY_PID, OK, "Olimex", "ARM-USB-TINY"},
>  	{OLIMEX_VID, OLIMEX_ARM_OCD_H_PID, NT, "Olimex", "ARM-USB-OCD-H"},
>  	{OLIMEX_VID, OLIMEX_ARM_TINY_H_PID, NT, "Olimex", "ARM-USB-TINY-H"},
> +	{GOEPEL_VID, PICOTAP_PID, OK, "GOEPEL electronic GmbH", "PicoTAP"},

Please sort the entries by vendor ID, then by device ID (we added
comment for that sorting a few minutes ago in the repository :)

Also use just "GOEPEL" instead of "GOEPEL electronic GmbH" here, please.
We don't include legalese such as "GmbH", "Inc.", etc. in the vendor names.


> @@ -66,7 +70,7 @@
>   * In either case, the divisor is a simple integer clock divider.
>   * If clock_5x is set, this divisor divides 30MHz, else it divides 6MHz.
>   */
> -#define DIVIDE_BY 3  /* e.g. '3' will give either 10MHz or 2MHz SPI
> clock. */
> +#define DIVIDE_BY 1  /* e.g. '3' will give either 10MHz or 2MHz SPI
> clock. */

This should probably not be in the initial patch, but rather in another
DIVIDE_BY related patch (see above).


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org