Attention is currently required from: Thomas Heijligen, Jean THOMAS.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 )
Change subject: dirtyjtag: Add DirtyJTAG programmer ......................................................................
Patch Set 4:
(21 comments)
Patchset:
PS4: Looks like a lot, but actually just some minor issues :) Looks good to me else. Thanks for contributing!
File dirtyjtag_spi.c:
https://review.coreboot.org/c/flashrom/+/67878/comment/9bef9d82_cb1905c7 PS4, Line 34: DJTAGUNK It seems this is unused?
What does UNK mean? Unknown? If so, I would write that out.
https://review.coreboot.org/c/flashrom/+/67878/comment/05073c0d_d4896e32 PS4, Line 39: 0xC0CA Please use lower case
https://review.coreboot.org/c/flashrom/+/67878/comment/5251a3f7_63b700cf PS4, Line 47: dirytjtag_command_identifier typo: dir*ty*jtag
https://review.coreboot.org/c/flashrom/+/67878/comment/7e1ca032_b7993262 PS4, Line 57: dirytjtag_signal_identifier typo: dir*ty*jtag
https://review.coreboot.org/c/flashrom/+/67878/comment/678b77b2_0fefabed PS4, Line 66: char* data `char *data`
https://review.coreboot.org/c/flashrom/+/67878/comment/32da85ef_593223f5 PS4, Line 87: char* data `char *data`
https://review.coreboot.org/c/flashrom/+/67878/comment/a05769b0_221f2eb7 PS4, Line 125: static int dirtyjtag_djtag1_spi_send_command(struct dirtyjtag_spi_data *context, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) This is also pretty long. Same as with line 174.
https://review.coreboot.org/c/flashrom/+/67878/comment/bc3a430e_2baa6234 PS4, Line 128: writecnt+readcnt spaces
https://review.coreboot.org/c/flashrom/+/67878/comment/d0f30199_bc5e7460 PS4, Line 129: size_t num_xfer = (len+max_xfer_size-1)/max_xfer_size; // ceil(len/max_xfer_size) spaces
https://review.coreboot.org/c/flashrom/+/67878/comment/993d73f3_1e5dcf3a PS4, Line 131: char *tx_buf = malloc(max_xfer_size*num_xfer); spaces
https://review.coreboot.org/c/flashrom/+/67878/comment/982455fd_be9956be PS4, Line 136: char *rx_buf = malloc(max_xfer_size*num_xfer); spaces
https://review.coreboot.org/c/flashrom/+/67878/comment/c08ef3a9_999bc155 PS4, Line 146: len%max_xfer_size add spaces before and after %
https://review.coreboot.org/c/flashrom/+/67878/comment/b3b09043_a682afe8 PS4, Line 147: len%max_xfer_size same here
https://review.coreboot.org/c/flashrom/+/67878/comment/71c3c872_37d30806 PS4, Line 149: txn_size = 30; How about setting this value during declaration in line 145? So it would be the "default" and if applicable it gets overridden. This way we can remove the else-block then.
https://review.coreboot.org/c/flashrom/+/67878/comment/5e6efbce_6cb7d232 PS4, Line 152: tx_buf+30*i same here
https://review.coreboot.org/c/flashrom/+/67878/comment/17d90845_5d3ae69c PS4, Line 155: rx_buf+i*30 same here
https://review.coreboot.org/c/flashrom/+/67878/comment/84c2e349_13165043 PS4, Line 174: static int dirtyjtag_spi_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) That line is pretty long. Please move each argument into a separate line or group them somehow. For example `writecnt` and `readcnt` could go into one line. Same with `writearr` and `readarr`.
https://review.coreboot.org/c/flashrom/+/67878/comment/b6fa893c_9d5865f6 PS4, Line 248: tmp, strerror(errno)); nit: I would use one line here. It's not that long.
https://review.coreboot.org/c/flashrom/+/67878/comment/faaf1c1a_8f2d907f PS4, Line 289: 0xFF lower case
https://review.coreboot.org/c/flashrom/+/67878/comment/a05ec946_7576f67c PS4, Line 290: 0xFF make this lower case
-
Felix Singer (Code Review)