Code looks quite nice :) haven't looked at the protocol yet, though.

View Change

15 comments:

• File digilent_spi.c:

  • Patch Set #2, Line 14: version 2 of the License

    is this restriction intentional? I've just noticed, most of flashrom
    is licensed v2+

  • Patch Set #2, Line 32: #if CONFIG_DIGILENT_SPI == 1

    This is unnecessary, the Makefile already takes care of it.

  • Patch Set #2, Line 58: 0

    nit, spurious 0?

  • Patch Set #2, Line 103: 0

    please always set a reasonable timeout

  • Patch Set #2, Line 120: return res_len;

    Rather `tx_len`? how about an error message for short replies?

  • Patch Set #2, Line 184: real_speed =(res[5] << 24) |(res[4] << 16) |(res[3] << 8) | res[2];

    Missing spaces after = and | operators.

  • Patch Set #2, Line 241: static int digilent_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr)

    please break this line (112 chars limit for flashrom)

  • Patch Set #2, Line 249: buf = alloca(len);

    I don't know how portable it is (and it's not used yet in
    flashrom). I'd prefer malloc().

  • Patch Set #2, Line 253: &buf[writecnt]

    `buf + writecnt` would be much clearer to me. But that could
    be just me. ;)

  • Patch Set #2, Line 264: if (ret != 0)

    Error message, please.

  • Patch Set #2, Line 269: if (ret != 0)

    Same here.

  • Patch Set #2, Line 303:

    What about the libusb handle?

  • Patch Set #2, Line 356: spiclock

    Other programmers call it `spispeed`.

  • Patch Set #2, Line 362: return -1;

    `goto close_handle;` ?

  • Patch Set #2, Line 374:

    	if (reset_board) {
    		gpio_open();
    		gpio_set_dir(1);
    		gpio_set_value(0);
    	}
    
    	spi_open();
    	spi_set_speed(speed_hz);
    	spi_set_mode(0x00);
    

    Check return values, please.

To view, visit change 23338. To unsubscribe, or for help writing mail filters, visit settings.