Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33907 )
Change subject: mediatek/mt8183: Calibrate RTC eosc clock ......................................................................
Patch Set 3:
(12 comments)
https://review.coreboot.org/#/c/33907/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33907/2//COMMIT_MSG@7 PS2, Line 7: Calibration
Imperative mood: Calibrate
Done
https://review.coreboot.org/#/c/33907/2//COMMIT_MSG@9 PS2, Line 9: Calibration
Calibrate
Done
https://review.coreboot.org/#/c/33907/2//COMMIT_MSG@9 PS2, Line 9: go
goes
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c File src/soc/mediatek/mt8183/rtc.c:
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@89 PS2, Line 89: if (val != 0) {
if (val) {
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@94 PS2, Line 94: (val & RTC_XOSCCALI_MASK));
the coding style is still expecting max length of 80 characters. Please change to […]
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@98 PS2, Line 98: 10
can we have some meaningful enum for the 10 and 11 here (for name of the field for shifting)?
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@102 PS2, Line 102: rtc_read(PMIC_RG_FQMTR_RST, &fqmtr_rst); : rtc_write(PMIC_RG_FQMTR_RST, fqmtr_rst | PMIC_FQMTR_RST);
there's a lot of similar patterns in this file. […]
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@107 PS2, Line 107: PMIC_FQMTR_CON0_BUSY & fqmtr_busy
for consistency, I think you usually put (var & const) so we should do same thing here: […]
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@112 PS2, Line 112: fix
do you mean fixed clock?
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@112 PS2, Line 112: freq. meter
frequency meter
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@204 PS2, Line 204: /* set eosc clock */
Is this comment needed?
Done
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@212 PS2, Line 212: /* eosc32 calibration for powerdown clock */
Calibrate eosc32 for powerdown clock
Done