Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 15:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 353: uint32_t mdss_clk_type) It looks like this should fit into 96 characters.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 369: reg_addr = (void *)mdss_extclk_config[i].cbcr; Is it common that this typo is not 0 or 1? Maybe add an else branch with a debug message?
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 374: int Please use CB_SUCCESS and friends.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 410: CLK_CTL_RCG_MND_BMSK); This should fit into 96 characters.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 424: 1 Please add enums for this.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/addressmap.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 16: #define DISP_CC_BASE 0x0AF00000 Please align using tabs.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 34: #define mdss_reg_ptr(type, mem, base) \ Macros should be uppercase.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/in... PS15, Line 35: ((offsetof(type, mem)) + base) This fits into one line.