[PATCH] ft2232_spi.c: use ftdi.h named constants instead of magic numbers

[Antony Pavlov]

Signed-off-by: Antony Pavlov 
---
 ft2232_spi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ft2232_spi.c b/ft2232_spi.c
index 7e7e2b1..4921caa 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -350,7 +350,7 @@ int ft2232_spi_init(void)

if (clock_5x) {
    	msg_pdbg("Disable divide-by-5 front stage\n");
-		buf[0] = 0x8a;		/* Disable divide-by-5. */
+		buf[0] = DIS_DIV_5;
    	if (send_buf(ftdic, buf, 1)) {
    		ret = -5;
    		goto ftdi_err;
@@ -361,7 +361,7 @@ int ft2232_spi_init(void)
    }

msg_pdbg("Set clock divisor\n");
-	buf[0] = 0x86;		/* command "set divisor" */
+	buf[0] = TCK_DIVISOR;
    buf[1] = (divisor / 2 - 1) & 0xff;
    buf[2] = ((divisor / 2 - 1) >> 8) & 0xff;
    if (send_buf(ftdic, buf, 3)) {
@@ -374,7 +374,7 @@ int ft2232_spi_init(void)

/* Disconnect TDI/DO to TDO/DI for loopback. */
    msg_pdbg("No loopback of TDI/DO TDO/DI\n");
-	buf[0] = 0x85;
+	buf[0] = LOOPBACK_END;
    if (send_buf(ftdic, buf, 1)) {
    	ret = -7;
    	goto ftdi_err;
@@ -453,7 +453,7 @@ static int ft2232_spi_send_command(struct flashctx *flash,
     * read command, then do the fetch of the results.
     */
    if (readcnt) {
-		buf[i++] = 0x20;
+		buf[i++] = MPSSE_DO_READ;
    	buf[i++] = (readcnt - 1) & 0xff;
    	buf[i++] = ((readcnt - 1) >> 8) & 0xff;
    	ret = send_buf(ftdic, buf, i);

[PATCH] ft2232_spi.c: use ftdi.h named constants instead of magic numbers

[Stefan Tauner]

Hello Antony,

thanks for your patch. It looks good in general, but there are still
some magic constants in the code:
- the initial values for cs_bits and pindir
- 0x11 to enable writes

I guess those can be obtained from the header as well?

[PATCH] ft2232_spi.c: use ftdi.h named constants instead of magic numbers

[Stefan Tauner]

On Sat, 24 Jan 2015 16:24:36 +0100
Stefan Tauner  wrote:
> Hello Antony,
>
> thanks for your patch. It looks good in general, but there are still
> some magic constants in the code:
> the initial values for cs_bits and pindir

No, because they are actually simple bit masks and there are no defines
for the meaning of the bits. But we could add our own defines for the
bit offsets, e.g. "#define TMS_CS_OFF (3)" and or them together in
other #defines for the few combinations we actually need for the
initial values... I don't think that's worth it though. But I have
added various comments explaining this.
> 0x11 to enable writes

The 0x11 seems to be equivalent of "MPSSE_DO_WRITE | MPSSE_WRITE_NEG",
right? If anyone can confirm this, I'll commit that resulting patch.

[PATCH] ft2232_spi.c: use ftdi.h named constants instead of magic numbers

[Boris Baykov]

> On Sat, 24 Jan 2015 16:24:36 +0100
> Stefan Tauner  wrote:
> > 0x11 to enable writes
> The 0x11 seems to be equivalent of "MPSSE_DO_WRITE | MPSSE_WRITE_NEG",
> right? If anyone can confirm this, I'll commit that resulting patch.

Yes, this is right. I've just checked FTDI AN114 document about this.
The MPSSE_DO_WRITE | MPSSE_WRITE_NEG is correct for the 0x11 number.

Also, today I've added ft2232_spi_send_multicommand function for ft2232
programmer to group multi-commands into one ftdi request and encreased
ftdi write buffer. Now I have the only 3-7 ms delay for each 256b write
instead of 2 delays before: 1st for WREN and 2nd for PROGRAM command.
The result I've got is 18 min for write + verify of 32 MB flash.

Boris

[PATCH] ft2232_spi.c: use ftdi.h named constants instead of magic numbers

[Antony Pavlov]

On Sat, 24 Jan 2015 20:03:23 +0100
Stefan Tauner  wrote:
> On Sat, 24 Jan 2015 16:24:36 +0100
> Stefan Tauner  wrote:
> > Hello Antony,
> >
> > thanks for your patch. It looks good in general, but there are still
> > some magic constants in the code:
> > the initial values for cs_bits and pindir
>
> No, because they are actually simple bit masks and there are no defines
> for the meaning of the bits. But we could add our own defines for the
> bit offsets, e.g. "#define TMS_CS_OFF (3)" and or them together in
> other #defines for the few combinations we actually need for the
> initial values... I don't think that's worth it though. But I have
> added various comments explaining this.
> > 0x11 to enable writes
>
> The 0x11 seems to be equivalent of "MPSSE_DO_WRITE | MPSSE_WRITE_NEG",
> right? If anyone can confirm this, I'll commit that resulting patch.

I have just examined libmpsse.

Please see int SetMode() in libmpsse/src/mpsse.c
(see https://code.google.com/p/libmpsse/source/browse/trunk/src/mpsse.c#298),
for SPI mode 0 and SPI mode 3:

mpsse->tx |= MPSSE_WRITE_NEG;
    ...
    mpsse->txrx |= MPSSE_WRITE_NEG;

next, the mpsse->tx* value is passed to build_block_buffer()
https://code.google.com/p/libmpsse/source/browse/trunk/src/mpsse.c#762
e.g.

buf = build_block_buffer(mpsse, mpsse->tx, (unsigned char *) (data + n), txsize, &buf_size);

Here is part of build_block_buffer(struct mpsse_context *mpsse, uint8_t cmd, ...
(see https://code.google.com/p/libmpsse/source/browse/trunk/src/support.c#154)

/* Copy in the command for this block */
    buf[i++] = cmd;
    buf[i++] = (rsize & 0xFF);
    if(!(cmd & MPSSE_BITMODE))
    {
            buf[i++] = ((rsize >> 8) & 0xFF);
    }

/* On a write, copy the data to transmit after the command */
    if(cmd == mpsse->tx || cmd == mpsse->txrx)
    {
            memcpy(buf+i, data+k, dsize);

/* i == offset into buf */
            i += dsize;
            /* k == offset into data */
            k += dsize;
    }

So I suppose that you are right and the 0x11 seems to be equivalent of "MPSSE_DO_WRITE | MPSSE_WRITE_NEG".

-- 
Best regards,
  Antony Pavlov

[PATCH] ft2232_spi.c: use ftdi.h named constants instead of magic numbers

[Stefan Tauner]

On Sun, 25 Jan 2015 02:56:40 +0400
Antony Pavlov  wrote:
> So I suppose that you are right and the 0x11 seems to be equivalent of "MPSSE_DO_WRITE | MPSSE_WRITE_NEG".

Ok, thanks (to Boris too)!
I have committed the respective change in r1872.