Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45399 )
Change subject: soc/mediatek/mt8192: add pmic MT6359P driver ......................................................................
Patch Set 1:
(12 comments)
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/mt6359p.h:
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/inc... PS1, Line 52: buck_uV buck_uv
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/inc... PS1, Line 53: unsigned int u32 to be consistent with the type of buck_uv?
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... File src/soc/mediatek/mt8192/mt6359p.c:
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 21: static static const
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 182: static static const
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 293: if (pmif_arb == NULL) : pmif_arb = get_pmif_controller(PMIF_SPI, 0); Since pmif_arb should have been initialized in mt6359p_init(), there seems to be no need for this. Same in mt6359p_write().
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 330: 1 0x1 or 0x01 for consistency?
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 345: size_t int
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 353: size_t int
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 424: assert(0); Use die()?
die("ERROR: Unknown buck_id %u", buck_id);
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 438: pmif_arb What if this is still NULL (returned from get_pmif_controller())? Should we call die() within get_pmif_controller() if the passed mstid is unsupported?
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 441: 0x%x %#x
https://review.coreboot.org/c/coreboot/+/45399/1/src/soc/mediatek/mt8192/mt6... PS1, Line 441: 0x%x %#x