Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33907 )
Change subject: mediatek/mt8183: Calibration RTC eosc clock ......................................................................
Patch Set 2:
(7 comments)
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) {
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
rtc_xosc_write((....) | (....));
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)?
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.
what about creating some inline functions like:
rtc_clrsetbits(PMIC_RG_FQMTR_RST, 0, PMIC_FQMTR_RST);
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:
(fqmtr_busy & PMIC_FQMTR_CON0_BUSY)
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@112 PS2, Line 112: freq. meter frequency meter
https://review.coreboot.org/#/c/33907/2/src/soc/mediatek/mt8183/rtc.c@112 PS2, Line 112: fix do you mean fixed clock?