Attention is currently required from: Hung-Te Lin, Paul Menzel, Yu-Ping Wu, hsin-hsiung wang. Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )
Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting ......................................................................
Patch Set 19:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/48954/comment/ca2dd2ea_acf23824 PS6, Line 9: initial settings could be effective
Which ones?
Done
https://review.coreboot.org/c/coreboot/+/48954/comment/63d2f4a9_b55afb5c PS6, Line 9: unlock key protection
- Does that mean *disable key protection*? […]
Done
https://review.coreboot.org/c/coreboot/+/48954/comment/11d34690_1251d107 PS6, Line 11:
Please mention the datasheet name and revision.
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/48954/comment/a388f680_cc9c7bcf PS11, Line 9: We need unlock key protection so that some initial settings could be effective.
Line too long (> 72 chars).
Done
File src/soc/mediatek/mt8192/mt6359p.c:
https://review.coreboot.org/c/coreboot/+/48954/comment/040a5950_70fae6e5 PS6, Line 321: set
I would stay with *setting*.
Done
https://review.coreboot.org/c/coreboot/+/48954/comment/20a9deae_dfb889cd PS6, Line 329: }
I’d put the for loop outside, and in the body use the ternary operator: […]
Done
File src/soc/mediatek/mt8192/mt6359p.c:
https://review.coreboot.org/c/coreboot/+/48954/comment/b060c0df_b7a631c9 PS8, Line 323: if (lock) { : for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++) : mt6359p_write(key_protect_setting[i].addr, 0); : } else { : for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++) : mt6359p_write(key_protect_setting[i].addr, key_protect_setting[i].val); : }
I'd rather do […]
Done
File src/soc/mediatek/mt8192/mt6359p.c:
https://review.coreboot.org/c/coreboot/+/48954/comment/70d67f37_7de41972 PS11, Line 323: {
No curly braces.
Done