Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
Not necessary, because the int type is completely sufficient […]
The problem is there's no promise int here will be int32_t. And since you'll be reading as read32(), I think the returned value should match what read32 expects, i.e., uint32_t.
Unless if the calculation below may introduce negative values.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@77 PS5, Line 77: if (cali_oe == AUXADC_CALI_INIT || cali_ge == AUXADC_CALI_INIT) : mt_auxadc_update_cali();
Since auxadc_get_voltage is called frequently, using if-check here can reduce the number of mt_auxad […]
We'll probably only call this 4 times (board id, RAM ID, LCM ID, SKU). So I really doubt if that will make much difference.
Meanwhile, if performance is really a concern, I think we should do only single integer compare instead of relying on particular values. i.e.,
static int calibrated = 0; static int cali_oe, cali_ge;
...
if (!calibrated) { mt_auxadc_update_cali(); calibrated = 1; }
This should be more efficient and clear (and get rid of the init magic value thing).
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Because rawvalue * 1500000 operation results will exceed int32_t
Yes I know it can't be int32_t. My question is if this should be int64_t or uint64_t? (given rawvalue is uint).