Yuchen Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46395 )
Change subject: soc/mediatek/mt8192: add rtc MT6359P driver ......................................................................
Patch Set 47:
(10 comments)
https://review.coreboot.org/c/coreboot/+/46395/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46395/4//COMMIT_MSG@9 PS4, Line 9: Add rtc MT6359P driver.
Please elaborate and add the datasheet name an revision.
Done
https://review.coreboot.org/c/coreboot/+/46395/4//COMMIT_MSG@10 PS4, Line 10:
Why can’t more common code be used?
Done
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... File src/soc/mediatek/mt8192/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 31: pmif_arb->read_cmd
Why isn’t data of type `void`?
fixed in ps46
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 51: dcxo clock
What is dcxo?
DCXO is a modue in PMIC. The 32K clock of MT6359 RTC comes from DCXO 26M by default.
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 307: rtc_write(RTC_BBPU, (bbpu | RTC_BBPU_KEY | RTC_BBPU_RESET_ALARM | RTC_BBPU_RESET_SPAR) & (~RTC_BBPU_SPAR_SW));
line over 96 characters
done
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 350: //rtc_osc_init();
Why?
Done
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 353: mdelay(20);
Please add a comment why 20 ms delay is needed.
Done. In recovery mode, We need 20ms delay for register setting.
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 376: */
Please use one of the recommended styles [1]. […]
Done
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 387: bbpu
What is that?
BBPU means 'baseband power up' which is a control register of RTC.
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 443: //rtc_write_field(PMIC_RG_DCXO_CW12, 0x0, 0x1, 0);
Why is this commented out?
Done