Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 14:
(3 comments)
Hmm... well... I'm still somewhat confused. Your patch changed behavior now. Previously you were reading the current value and subtracting 6, now you're just overwriting the value. I thought you were doing that because you didn't know what the existing calibration value was (e.g. it might be different per board and initialized from fuses or something), so you could only adjust the existing one.
But if you are sure that setting it directly is okay then this approach is fine.
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c@73... PS14, Line 736: unsigned int vsim2_mv, unsigned int cali_mv If these voltages really just get added like that, does it make sense to have two separate parameters? Why not just have one millivolts parameter and then do something like
assert(millivolts % 10 == 0); int cali_mv = millivolts % 100; switch (millivolts - cali_mv) { ... }
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c@76... PS14, Line 762: return; This should die() or assert()
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c@76... PS14, Line 768: if (cali_mv > 100) You should assert() that the value is in range instead