Julius Werner 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@40 PS5, Line 40: int
The problem is there's no promise int here will be int32_t. […]
I think in general it's fine for SoC-specific code to rely on stuff like type sizes which they know will always be the same for that SoC, so where using an int would otherwise make sense I think this would be okay. But anything that holds a register value should always use a fixed-width type of the corresponding size, so yes these should be uint32_t (or possibly int32_t).
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();
We'll probably only call this 4 times (board id, RAM ID, LCM ID, SKU). […]
Let's not overcomplicate things. Having this check either here or at the top of mt_auxadc_update_cali() both sounds fine. There's no need to optimize away a single function call or a single integer in memory comparison for something that will be called a handful of times per boot.