On 16.06.2009 19:44, Uwe Hermann wrote:
On Tue, Jun 16, 2009 at 01:42:33PM +0200, Carl-Daniel Hailfinger wrote:
On 12.06.2009 12:22, Carl-Daniel Hailfinger wrote:
This patch adds support for a new SPI programmer, based on the FT2232H/4232H chip from FTDI.
FTDI support is autodetected during compilation.
Paul writes: There are certainly possible improvements: The code has hard-coded values for which interface of the ftdi chip to use (interface B was chosen because libftdi seems to have trouble with A right now), what clock rate use for the SPI interface (I've been running at 30Mhz, but the patch sets it to 10Mhz), and possibly others. I think this means that per-programmer options might be a good idea at some point.
Carl-Daniel writes: There is one additional FIXME comment in the code, but AFAICS that problem is not solvable with current libftdi.
Signed-off-by: Paul Fox pgf@laptop.org Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Looks ok to me, builds fine with and without libftdi installed. There are some minor comments below, but those issues can be tackled in another patch.
Thanks!
Index: flashrom-ftdi2232_spi/Makefile
--- flashrom-ftdi2232_spi/Makefile (Revision 597) +++ flashrom-ftdi2232_spi/Makefile (Arbeitskopie)
Something here should be fixed probably, some of the tests are performed multiple times it seems and some printf's are confusing:
$ make Checking for a C compiler... found. Checking for FTDI support... not found. Checking for a C compiler... found. Checking for FTDI support... not found. Checking for pciutils and zlib... found. [...] $ make (again, after a successful build) Checking for a C compiler... found. Checking for FTDI support... not found. Checking for pciutils and zlib... found. $ make distclean Checking for a C compiler... found. Checking for FTDI support... not found. rm -f flashrom *.o rm -f .dependencies .features
With libftdi installed:
$ make Checking for a C compiler... found. Checking for FTDI support... found. Checking for a C compiler... found. Checking for FTDI support... found. Checking for pciutils and zlib... found. cc -Os -Wall -Werror -DFT2232_SPI_SUPPORT=1 -c chipset_enable.c -o chipset_enable.o [...] $ make Checking for a C compiler... found. Checking for FTDI support... found. Checking for pciutils and zlib... found. $ make distclean Checking for a C compiler... found. Checking for FTDI support... found. rm -f flashrom *.o rm -f .dependencies .features
Ah yes, that's an artifact of how the detection works. Basically, the main makefile includes a helper makefile (.features) which is generated by the features target. However, since that means .features is changed after make features, make restarts to read in the new file.
I have another version of the makefile patch, but it is really ugly. It does avoid the restart, though.
@@ -22,6 +22,7 @@ CC ?= gcc STRIP = strip INSTALL = install +DIFF = diff
Please use space for indentation here as the other lines do.
Done.
Index: flashrom-ftdi2232_spi/ft2232_spi.c
--- flashrom-ftdi2232_spi/ft2232_spi.c (Revision 0) +++ flashrom-ftdi2232_spi/ft2232_spi.c (Revision 0) @@ -0,0 +1,288 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2009 Paul Fox pgf@laptop.org
- Copyright (C) 2009 Carl-Daniel Hailfinger
- based initially on example program bitbang_ft2232.c from
- the libftdi-0.16 distribution, from:
- Intra2net AG opensource@intra2net.com
Please drop this, I can't spot any non-trivial amount of code which comes/remains from that file.
Done.
- // f = ftdi_usb_open(ftdic, 0x0403, 0x6010); // FT2232
- f = ftdi_usb_open(ftdic, 0x0403, 0x6011); // FT4232
Yep, this should be coupled with a command line option later, where the user specifies the USB IDs or the like.
Yes, will be handled in a followup patch.
- if (f < 0 && f != -5) {
fprintf(stderr, "unable to open ftdi device: %d (%s)\n", f,ftdi_get_error_string(ftdic));Please start all messages to stdout or stderr with capital letter and end them with full stop for consistency, all other flashrom output does the same.
Capital letter at the beginning: OK. Full stop at the end: Sorry, these are not sentences. The Linux kernel has no full stops at the end of its messages. Most other open source projects don't have full stops at the end of their messages. Actually, I'm thinging about removing all those full stops. It would certainly help reduce size of our binaries and of all logs by a few bytes.
For stderr output we might also add "ERROR:" in front, at least for critical errors, dunno.
Hm maybe. flashrom output is not consistent in this regard.
exit(-1);- }
- if (ftdi_set_interface(ftdic, INTERFACE_B) < 0) {
fprintf(stderr, "unable to select FT2232 channel B: %s\n",ftdic->error_str);- }
- if (ftdi_usb_reset(ftdic) < 0) {
fprintf(stderr, "unable to reset ftdi device\n");- }
There's no return or exit here, are these non-critical? Same for other such checks...
No idea. Paul?
- if (ftdi_set_latency_timer(ftdic, 2) < 0) {
fprintf(stderr, "unable to set latency timer\n");- }
- if (ftdi_write_data_set_chunksize(ftdic, 512)) {
fprintf(stderr, "unable to set chunk size\n");- }
- if (ftdi_set_bitmode(ftdic, 0x00, 2) < 0) {
fprintf(stderr, "unable to set bitmode\n");- }
- printf_debug("\nft2232 chosen\n");
This is a bit unclear, please make the message more usable.
printf_debug is often unusable for everyone besides the original programmer. Maybe we could drop the message.
@@ -589,7 +605,7 @@ " -i | --image <name>: only flash image name from flash layout\n" " -L | --list-supported: print supported devices\n" " -p | --programmer <name>: specify the programmer device\n"
" (internal, dummy, nic3com, satasii, it87spi)\n"
" (internal, dummy, nic3com, satasii, it87spi, ft2232spi)\n"TODO: Document new programmer in manpage.
Done.
Thanks for the review, committed in r598.
Regards, Carl-Daniel