JG Poxu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32800 )
Change subject: mt8183: adding efuse calibration in auxadc ......................................................................
Patch Set 9:
(6 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)
I am going to change to ~0xffff
As long as it is not in the range of -512~512, it is ok.
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@35 PS5, Line 35: int
uint32_t, if the init itself was uint32_t type .
Cali_oe & cali_ge may be negative
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@36 PS5, Line 36: int
uint32_t
Cali_oe & cali_ge may be negative
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 will fix it
https://review.coreboot.org/#/c/32800/5/src/soc/mediatek/mt8183/auxadc.c@84 PS5, Line 84: int64_t
Yes I know it can't be int32_t. […]
Int64_t is completely enough， The maximum value of raw_valude is only 4095. There is no case of strong data loss. Do you think it should be changed to uint64_t?
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c File src/soc/mediatek/mt8183/auxadc.c:
https://review.coreboot.org/#/c/32800/7/src/soc/mediatek/mt8183/auxadc.c@26 PS7, Line 26: : #define ADC_GE_A_SHIFT 10 : #define ADC_GE_A_MASK (0x3ff << ADC_GE_A_SHIFT) : #define ADC_OE_A_SHIFT 0 : #define ADC_OE_A_MASK (0x3ff << ADC_OE_A_SHIFT) : #define ADC_CALI_EN_A_SHIFT 20 : #define ADC_CALI_EN_A_MASK (0x1 << ADC_CALI_EN_A_SHIFT) : #define AUXADC_CALI_INIT -0xffff
Please use tabs or spaces consistently for alignment.
I will fix it