Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46395 )
Change subject: soc/mediatek/mt8192: add rtc MT6359P driver ......................................................................
Patch Set 4:
(9 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.
https://review.coreboot.org/c/coreboot/+/46395/4//COMMIT_MSG@10 PS4, Line 10: Why can’t more common code be used?
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`?
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 51: dcxo clock What is dcxo?
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 350: //rtc_osc_init(); Why?
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.
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 376: */ Please use one of the recommended styles [1].
https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/46395/4/src/soc/mediatek/mt8192/rtc... PS4, Line 387: bbpu What is that?
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?