Weiyi Lu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45402 )
Change subject: soc/mediatek: Add function to get clock frequency of MT8192 ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/common/inc... File src/soc/mediatek/common/include/soc/pll_common.h:
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/common/inc... PS2, Line 65: N
Please add a trailing "," for consistency.
Done
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/common/inc... PS2, Line 67: unsigned int
u32
Done
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/mt8192/pll... File src/soc/mediatek/mt8192/pll.c:
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/mt8192/pll... PS2, Line 488: return 0;
Do we want to die() in this case?
Done
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/mt8192/pll... PS2, Line 497: setbits32(&mtk_topckgen->clk26cali_0, CLK26CALI_0_TRIGGER);
Could also we use the SET32_BITFIELDS API for this?
Done
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/mt8192/pll... PS2, Line 503: else
Also add {} for the else block.
Done
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/mt8192/pll... PS2, Line 504: output = 0;
If this is an error case, perhaps at least print an error message here?
Done
https://review.coreboot.org/c/coreboot/+/45402/2/src/soc/mediatek/mt8192/pll... PS2, Line 514: (
No need for the parentheses.
Done