Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35147 )
Change subject: mediatek/mt8183: Allow modifying vdram2 voltage ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35147/1/src/soc/mediatek/mt8183/mt6... File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/c/coreboot/+/35147/1/src/soc/mediatek/mt8183/mt6... PS1, Line 913: } if (vdram2_uv < 530000) {
trailing statements should be on next line (or did you mean 'else if'?)
Done
https://review.coreboot.org/c/coreboot/+/35147/1/src/soc/mediatek/mt8183/mt6... PS1, Line 931: while ((cali_trim < 15) && (vdram2_votrim[cali_trim] != cali_offset_mv)) ++cali_trim;
trailing statements should be on next line
Done
https://review.coreboot.org/c/coreboot/+/35147/1/src/soc/mediatek/mt8183/mt6... PS1, Line 931: while ((cali_trim < 15) && (vdram2_votrim[cali_trim] != cali_offset_mv)) ++cali_trim;
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/35147/4/src/soc/mediatek/mt8183/mt6... File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/c/coreboot/+/35147/4/src/soc/mediatek/mt8183/mt6... PS4, Line 910: if (vdram2_uv > 680000) { : assert(0); : return; : } else if (vdram2_uv < 530000) { : assert(0); : return; : } Why not just assert(vdram2_uv >= 530000 && vdram2_uv <= 680000);