Change in flashrom[master]: digilent_spi: add a driver for the iCEblink40 development board
Nico Huber has posted comments on this change. ( https://review.coreboot.org/23338 )
Change subject: digilent_spi: add a driver for the iCEblink40 development board ......................................................................
Patch Set 2:
(15 comments)
Code looks quite nice :) haven't looked at the protocol yet, though.
https://review.coreboot.org/#/c/23338/2/digilent_spi.c File digilent_spi.c:
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@14 PS2, Line 14: version 2 of the License is this restriction intentional? I've just noticed, most of flashrom is licensed v2+
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@32 PS2, Line 32: #if CONFIG_DIGILENT_SPI == 1 This is unnecessary, the Makefile already takes care of it.
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@58 PS2, Line 58: 0 nit, spurious 0?
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@103 PS2, Line 103: 0 please always set a reasonable timeout
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@120 PS2, Line 120: return res_len; Rather `tx_len`? how about an error message for short replies?
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@184 PS2, Line 184: real_speed =(res[5] << 24) |(res[4] << 16) |(res[3] << 8) | res[2]; Missing spaces after = and | operators.
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@241 PS2, 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)
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@249 PS2, 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().
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@253 PS2, Line 253: &buf[writecnt] `buf + writecnt` would be much clearer to me. But that could be just me. ;)
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@264 PS2, Line 264: if (ret != 0) Error message, please.
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@269 PS2, Line 269: if (ret != 0) Same here.
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@303 PS2, Line 303: What about the libusb handle?
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@356 PS2, Line 356: spiclock Other programmers call it `spispeed`.
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@362 PS2, Line 362: return -1; `goto close_handle;` ?
https://review.coreboot.org/#/c/23338/2/digilent_spi.c@374 PS2, 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.
-
Nico Huber (Code Review)