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 56:
(10 comments)
https://review.coreboot.org/c/coreboot/+/46395/56//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46395/56//COMMIT_MSG@10 PS56, Line 10: Refactor 8173 and 8183 code, extract common API for common useage. Refactor mt8173 and mt8183 code by extracting common API.
Please put as many words in a line as possible (without exceeding 72 characters).
https://review.coreboot.org/c/coreboot/+/46395/56//COMMIT_MSG@11 PS56, Line 11: Refactor rtc_read and rtc_write interface,8173 and 8183 access RTC via pmic_wrap : while 8192 via pmif Move rtc_read and rtc_write to each SoC folder, because mt8173 and mt8183 access rtc via pmic wrapper, while mt8192 accesses it via pmif.
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
0 here means failure. […]
Ack. It seems like all rtc functions (except rtc_init()) are using 1 as the success return value. This is quite confusing and incompatible with the convention.
It'd be nice to upload *another* cleanup patch to fix this. See b/176307061.
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 44: if (val > RTC_FQMTR_LOW_BASE) : diff1 = val - RTC_FQMTR_LOW_BASE; : else : diff1 = RTC_FQMTR_LOW_BASE - val;
Ack
Not done.
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8173/rt... File src/soc/mediatek/mt8173/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8173/rt... PS56, Line 20: /* export 32K clock RTC_32K2V8 */ Since this is irrelevant, let's keep it unchanged.
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8173/rt... PS56, Line 78: /* use SW to detect 32K mode instead of HW */ Since this is irrelevant, let's keep it unchanged.
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8192/rt... File src/soc/mediatek/mt8192/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8192/rt... PS56, Line 67: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8192/rt... PS56, Line 192: %x Maybe %#x?
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8192/rt... PS56, Line 280: err: Add a blank line before this.
https://review.coreboot.org/c/coreboot/+/46395/56/src/soc/mediatek/mt8192/rt... PS56, Line 281: fail failed