Attention is currently required from: Bao Zheng, Marshall Dawson, Matt DeVillier.
Felix Held has posted comments on this change by Bao Zheng. ( https://review.coreboot.org/c/coreboot/+/84233?usp=email )
Change subject: amdfwtool: Add combo new layout for new family ......................................................................
Patch Set 6:
(5 comments)
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/84233/comment/b0075987_a9799bff?usp... : PS6, Line 1594: check_new_combo_layout i'd rename this function to needs_new_combo_layout to clarify what it does from the name
https://review.coreboot.org/c/coreboot/+/84233/comment/7f28770a_47641150?usp... : PS6, Line 1596: if (needs_ish(soc_id)) : return true; : : return false; this can be simplified to
return needs_ish(soc_id);
https://review.coreboot.org/c/coreboot/+/84233/comment/5c26ffd1_e7da1eae?usp... : PS6, Line 1700: ctx.pspdir = NULL; maybe add a comment why this one is moved outside of the do while loop
https://review.coreboot.org/c/coreboot/+/84233/comment/fb1d4812_eaed5145?usp... : PS6, Line 1758: (combo_index == 0 && cb_config.combo_new_rab) || : !cb_config.combo_new_rab not 100% sure if this makes things easier or more difficult to read, but this could be simplified to
!cb_config.combo_new_rab || combo_index == 0
File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/84233/comment/d979f74b_36663491?usp... : PS6, Line 726: static bool needs_ish(enum platform platform_type) i'd keep this function in this file and just remove the static, so that it can be used from the other file