Attention is currently required from: Raul Rangel, Martin Roth, Zheng Bao, Karthik Ramasubramanian, Felix Held. Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57612 )
Change subject: [WIP]amdfwtool: Add an optional colume of level ......................................................................
Patch Set 6:
(7 comments)
File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/57612/comment/655ee7bc_492d5cdc PS2, Line 47: filename
Level
Done
https://review.coreboot.org/c/coreboot/+/57612/comment/dfcdb461_e8c6b133 PS2, Line 78: case '\0':
Why this one has to be a separate case? Can it be coupled with default?
Done
https://review.coreboot.org/c/coreboot/+/57612/comment/9d793582_9b6dec72 PS2, Line 421: oneline[match[1].rm_eo] = '\0'; : oneline[match[2].rm_eo] = '\0'; : oneline[match[3].rm_eo] = '\0';
Please add a comment on each line regarding what those matches are.
Done
https://review.coreboot.org/c/coreboot/+/57612/comment/59a6f57a_f241c0fb PS2, Line 425: } else
Nit: Enclose the else block with braces for consistency.
Done
https://review.coreboot.org/c/coreboot/+/57612/comment/16dc02a3_d3968f00 PS2, Line 503:
Please add a comment saying if the optional level field is present, then extract the level char.
Done
https://review.coreboot.org/c/coreboot/+/57612/comment/9336ec3c_9e16ed96 PS2, Line 512: path_filename, ch_lvl, cb_config) == 0) {
line over 96 characters […]
Done
File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/57612/comment/623cd237_f04482d2 PS6, Line 94: default: \
Possible switch case/default not preceded by break or fallthrough comment
dont understand?
-
Bao Zheng (Code Review)