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. -- To view, visit https://review.coreboot.org/23338 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ffcd9a2db4395816f0e8b6ce6c3b0d8e930c9e6 Gerrit-Change-Number: 23338 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak@v3.sk> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 18:34:36 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
participants (1)
-
Nico Huber (Code Review)