Attention is currently required from: Bao Zheng, Raul Rangel. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49015 )
Change subject: amdfwtool: Use *number in config file as NVRAM entry ......................................................................
Patch Set 30:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49015/comment/8543ab60_68c01aff PS30, Line 7: Use *number in config file Why is this needed? What's the purpose?
https://review.coreboot.org/c/coreboot/+/49015/comment/a56d5e54_89ef01dd PS30, Line 10: XXXX_FW_TYPE_STRING *0xXXXXX Why not just use commas to separate them?
https://review.coreboot.org/c/coreboot/+/49015/comment/1fa5ed63_e6113aec PS30, Line 13: Add BUG= and TEST=?
File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/49015/comment/6c801859_0d464509 PS30, Line 70: else if (strcmp(fw_name, "AMD_PUBKEY_FILE") == 0) { : fw_type = AMD_FW_PSP_PUBKEY; : subprog = 0; Nit: Not related to this patch, but it seems like the simple matches like this could be put into a data structure instead of having quite so many else if statements. Maybe look at that for a future patch?
https://review.coreboot.org/c/coreboot/+/49015/comment/81e130d2_6b0c960d PS30, Line 254: if These if statements are getting deep. Maybe break them out to separate functions?
https://review.coreboot.org/c/coreboot/+/49015/comment/06d9f525_a3caaa14 PS30, Line 467: if Nit: Obviously not this patch, but 6 levels of if and while statements is pretty deep. Could this be refactored into a separate function? Again, maybe a future patch?