Nico Huber has posted comments on this change. ( https://review.coreboot.org/28087 )
Change subject: Add initial J-Link SPI programmer ......................................................................
Patch Set 1:
(17 comments)
https://review.coreboot.org/#/c/28087/1/Makefile File Makefile:
https://review.coreboot.org/#/c/28087/1/Makefile@646 PS1, Line 646: # Enable J-Link for now. please update the comment
https://review.coreboot.org/#/c/28087/1/flashrom.8.tmpl File flashrom.8.tmpl:
PS1: nice documentation
https://review.coreboot.org/#/c/28087/1/flashrom.8.tmpl@1133 PS1, Line 1133: 4: GND On the first look, I thought the listing has the same order as the physical pins. Let's see how it'd look like if it did:
+-------+ | 1 2 | 1: VTref 2: | 3 4 | 3: TRST 4: GND | 5 6 | 5: TDI 6: GND +-+ 7 8 | 7: 8: GND | 9 10 | 9: TCK 10: GND | 11 12 | 11: 12: GND +-+ 13 14 | 13: TDO 14: | 15 16 | 15: RESET 16: | 17 18 | 17: 18: | 19 20 | 19: PWR_5V 20: +-------+
IMO, more convenient, your call.
https://review.coreboot.org/#/c/28087/1/flashrom.8.tmpl@1157 PS1, Line 1157: syntax where \fBfrequency\fP is the SPI clock frequency in Hz. For all other programmers that take arbitrary numbers for `spispeed` it's in kHz. Please use that here, too. (There is always talk about unifying the code to parse `spispeed` throughout flashrom, patches are welcome. :))
https://review.coreboot.org/#/c/28087/1/jlink_spi.c File jlink_spi.c:
PS1: 112 columns line limit, please remove unnecessary line breaks.
good coding overall :)
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@8 PS1, Line 8: * the Free Software Foundation; version 2 of the License. My usual question here: Is the limitation to v2 intended (or just copy pasted from one of the problematic files)?
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@14 PS1, Line 14: * : * You should have received a copy of the GNU General Public License : * along with this program; if not, write to the Free Software : * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Please remove.
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@20 PS1, Line 20: Please start with /* on an otherwise empty line (like in your other comments).
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@21 PS1, Line 21: * See https://www.segger.com/ for more info. Usually, URLs are discouraged in the code, but I guess this one is somewhat stable?
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@39 PS1, Line 39: UINT16_MAX / 8 This is limited by the *software* interface, right? Please mention that in the commant, if so.
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@71 PS1, Line 71: jaylink_strerror(ret)); We exercise a 112 column limit for flashrom, no need to break the line.
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@78 PS1, Line 78: jaylink_jtag_clear_trst Not sure if the distinction which function was actually called would be helpful for debugging (you'd know which one it was by the state of `reset_cs`). OTOH, it would save some lines if you'd move the return check out of the outer if block (e.g. with "Asserting CS failed: %s.\n").
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@95 PS1, Line 95: clear set
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@103 PS1, Line 103: clear set
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@130 PS1, Line 130: free(buffer); Do not free it here or set `buffer = NULL;`. Otherwise we'd double free on shutdown, I guess.
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@134 PS1, Line 134: buffer = tmp; `buffer_size = length;` might be a good idea here oO
https://review.coreboot.org/#/c/28087/1/jlink_spi.c@378 PS1, Line 378: JAYLINK_DEV_EXT_CAPS_SIZE `sizeof(caps)` preferred
-
Nico Huber (Code Review)