Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46395 )
Change subject: soc/mediatek/mt8192: add rtc MT6359P driver ......................................................................
Patch Set 51:
(29 comments)
https://review.coreboot.org/c/coreboot/+/46395/51//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46395/51//COMMIT_MSG@8 PS51, Line 8: Since it's not trivial to me, could you mention how the code is "refactored" (by moving between files)? For example, why the code mt8173 and mt8183 needs to be changed?
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... File src/soc/mediatek/common/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 103: 0 Does 0 mean success or failure here?
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... File src/soc/mediatek/common/rtc_osc_init.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 4: #include <soc/rtc.h> rtc.h should come before rtc_common.h
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 9: diff1, diff2 diff_left, diff_right
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 10: = 0 No need to initialize this and "middle".
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 20: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 34: Remove extra blank line.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 44: if (val > RTC_FQMTR_LOW_BASE) : diff1 = val - RTC_FQMTR_LOW_BASE; : else : diff1 = RTC_FQMTR_LOW_BASE - val; This can be simplified to
diff1 = ABS(val - RTC_FQMTR_LOW_BASE)
Same for diff2.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 56: : One space after ":"
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 69: C Please be consistent with the case (C or c).
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/in... File src/soc/mediatek/mt8173/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/in... PS51, Line 111: fail failed
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/in... PS51, Line 122: fail failed
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... File src/soc/mediatek/mt8173/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... PS51, Line 87: W w
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... PS51, Line 87: . No period for consistency with comments above.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... PS51, Line 87: i I
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/in... PS51, Line 219: fail failed
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/in... PS51, Line 230: fail failed
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/rt... File src/soc/mediatek/mt8183/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/rt... PS51, Line 228: /* in recovery mode, We need 20ms delay for register setting. */ Same.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... PS51, Line 14: E Could you add a "," for all the enums in this file?
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... PS51, Line 89: to low low
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... PS51, Line 89: lower leakage current hw design change What does that mean?
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... File src/soc/mediatek/mt8192/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 21: (unsigned int) Do we need the cast? Same below.
Even if we do, the type should be u32.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 46: /* Unlock for reload */ Move to its own line.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 67: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 136: out !! out!
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 220: u8 If this is boolean, please use native type such as int.
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 241: W w
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 241: i I
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 319: 0x%x %#x
Same for all occurrences in this file.