Attention is currently required from: Felix Singer, Thomas Heijligen, Alexander Goncharov.
Jean THOMAS has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 )
Change subject: dirtyjtag: Add DirtyJTAG programmer ......................................................................
Patch Set 5:
(26 comments)
Patchset:
PS5: Hi Joursoir & Felix, thans for your reviews. I tried to address your coding style remarks, let me know if I missed something.
File dirtyjtag_spi.c:
https://review.coreboot.org/c/flashrom/+/67878/comment/8ad863d5_5cf9bf76 PS4, Line 34: DJTAGUNK
It seems this is unused? […]
Yes UNK stands for UNKNOWN. I won't fix this, I will remove the enum instead since this isn't used at the moment.
I'll add it back when we settle on all the protocol changes for DJTAG2.
https://review.coreboot.org/c/flashrom/+/67878/comment/8c176dde_2eaee656 PS4, Line 39: 0xC0CA
Please use lower case
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/f0e0e7f9_da669637 PS4, Line 47: dirytjtag_command_identifier
typo: dir*ty*jtag
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/2abaa0bc_b8dc8e9b PS4, Line 48: CMD_STOP = 0x00,
Please use TAB instead of two spaces.
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/b67edd36_1f834b8e PS4, Line 57: dirytjtag_signal_identifier
typo: dir*ty*jtag
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/d4a8b3c9_36d13b6f PS4, Line 58: SIG_TCK = 1 << 1,
TABs here too.
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/e4417da3_3dd15146 PS4, Line 66: char* data
`char *data`
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/ef20f8da_efd9ac38 PS4, Line 87: char* data
`char *data`
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/8723baed_ced95366 PS4, Line 120: static inline unsigned int _min(unsigned int a, unsigned int b) : { : return (a < b) ? a : b; : }
Seems to be unused?
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/02c682d7_9b8681d0 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.
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/a4aba6d3_13c49d2b PS4, Line 128: writecnt+readcnt
spaces
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/eac1a34e_a04cb603 PS4, Line 129: size_t num_xfer = (len+max_xfer_size-1)/max_xfer_size; // ceil(len/max_xfer_size)
spaces
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/4ebaec91_4acde234 PS4, Line 131: char *tx_buf = malloc(max_xfer_size*num_xfer);
spaces
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/6d9a88d6_8bb76d7d PS4, Line 133: msg_perr("%s: Failed tx_buf allocation", __func__);
Missing \n
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/968e8ebe_3cd91f06 PS4, Line 136: char *rx_buf = malloc(max_xfer_size*num_xfer);
spaces
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/05bb1699_a77fb9a7 PS4, Line 138: msg_perr("%s: Failed rx_buf allocation", __func__);
Missing \n
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/e54f0d8e_a4a31f8a PS4, Line 146: len%max_xfer_size
add spaces before and after %
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/e72cba73_543a13be PS4, Line 147: len%max_xfer_size
same here
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/9ee7a852_a707f71a PS4, Line 149: txn_size = 30;
How about setting this value during declaration in line 145? So it would be the "default" and if app […]
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/970e365c_b720cca4 PS4, Line 152: tx_buf+30*i
same here
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/3be44f64_5f81025c PS4, Line 155: rx_buf+i*30
same here
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/e8e9d571_40433bda 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. […]
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/0fa67328_5d72adb2 PS4, Line 248: tmp, strerror(errno));
nit: I would use one line here. It's not that long.
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/b1453bbf_25bc2cd9 PS4, Line 289: 0xFF
lower case
Done
https://review.coreboot.org/c/flashrom/+/67878/comment/61d2b698_d50d983f PS4, Line 290: 0xFF
make this lower case
Done
-
Jean THOMAS (Code Review)