Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 )
Change subject: soc/intel/common/block/cse: Add boot partition related APIs ......................................................................
Patch Set 69:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35402/69//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/69//COMMIT_MSG@15 PS69, Line 15: |C nit: add a space?
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 39: __packed Does this need to be packed?
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 56: __packed And this one?
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 60: cse_check_prereq_bp_cmd_info I would rename this function so that it looks like a question. For example:
cse_is_bp_cmd_info_possible
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 62: if (cse_is_hfs1_cws_normal() && (cse_is_hfs1_com_normal() || : cse_is_hfs1_com_secover_mei_msg() || cse_is_hfs1_com_soft_temp_disable())) : return true; : return false; How about splitting this out for readability? if (cse_is_hfs1_cws_normal()) { if (cse_is_hfs1_com_normal()) return true; if (cse_is_hfs1_com_secover_mei_msg()) return true; if (cse_is_hfs1_com_soft_temp_disable()) return true; } return false;
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 231: | There's some misalignment around here