Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@9 PS10, Line 9: SSPM is a Secure System Power Manager for : power control in secure domain. Please use the full text width.
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@11 PS10, Line 11: is load is to load
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@11 PS10, Line 11: SSPM firmware Is that already in the 3rdparty repository?
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@35 PS10, Line 35: die("SSPM file not found."); As the name is in a variable, please print the name in the error message too.
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@38 PS10, Line 38: are is
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@38 PS10, Line 38: /* Memory barrier to ensure that all fw code are loaded : * before we release the reset pin. : */ Please follow the coding style.
/* … … */
[1]: https://doc.coreboot.org/coding_style.html?highlight=coding#commenting