Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
(2 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)
As long as it is not in the range of -512~512, it is ok.
well there's some problem here. ~((uint32_t)1) = -2.
If the calibration values may be negative, especially there's no where explaining there's a range of -512~512, I think it is better to have another variable to control init value instead of trying to determine an "unused value". i,e.,
static int has_calibrated, cali_oe, cali_ge;
And remove the AUXADC_CALI_INIT.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Int64_t is completely enough， […]
Hmmm. I think there are few things that can be improved.
1. If raw_value should 0~4096, I think there can be an assert to clarify.
2. After raw_value -= cali_oe, can it be negative? If yes, then raw_value should be int32_t, or even int64_t.