JG Poxu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: add efuse calibration in auxadc ......................................................................
Patch Set 6:
(7 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@33 PS5, Line 33: ~((uint32_t)1)
need MTK's confirm why this is ~1 instead of -1 or how it was selected.
I am going to change to ~0xffff
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@40 PS5, Line 40: int
uint32_t (and the rest)
Not necessary, because the int type is completely sufficient The value of cali_reg only needs 20 bits lower
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@48 PS5, Line 48: ((cali_reg & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT)
remove one level of quote just like above: […]
I will fix it
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@49 PS5, Line 49: cali_ge_a - 512
Hi Po, […]
yes
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@74 PS5, Line 74: rawvalue
I'd prefer calling this raw_value
I will fix it
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();
will it make more sense if we just put this if-check inside mt_auxadc_update_cali? […]
Since auxadc_get_voltage is called frequently, using if-check here can reduce the number of mt_auxadc_update_cali() calls, which can reduce the running time.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
uint64_t ?
Because rawvalue * 1500000 operation results will exceed int32_t