Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46878 )
Change subject: soc/mediatek/mt8192: add clkbuf and srclken_rc MT6359P driver ......................................................................
Patch Set 50:
(8 comments)
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/cl... File src/soc/mediatek/mt8192/clkbuf.c:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/cl... PS50, Line 21: = NULL No need for this.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/cl... PS50, Line 43: ( No parentheses.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/cl... PS50, Line 60: u32 pmic_cw00 = 0, pmic_cw09 = 0, pmic_cw12 = 0, pmic_cw13 = 0, : pmic_cw15 = 0, pmic_cw19 = 0, top_spi_con1 = 0, : ldo_vrfck_op_en = 0, ldo_vbbck_op_en = 0, ldo_vrfck_en = 0, : ldo_vbbck_en = 0; : u32 vrfck_hv_en = 0; No need for initialization.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/cl... PS50, Line 138: #ifndef MTK_SRCLKEN_RC_SUPPORT Is this always defined for mt8192? If we really need an if-else statement, I'd prefer using Kconfig.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/pmif.h:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... PS50, Line 200: start external External
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/srclken_rc.h:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... PS50, Line 155: SRCLKEN_FPM_MASK_B_MSK This is not used anywhere.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... PS50, Line 230: RC_SUBSYS_SET Where's the usage?
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/rt... File src/soc/mediatek/mt8192/rtc.c:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/rt... PS50, Line 339: mt6359_dcxo_disable_unused Also remove it from rtc.h.