Taniya Das 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 26:
(7 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.
The implementation is updated as per the latest comments from Julius.
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?
The implementation is updated as per the latest comments from Julius.
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 374: int
Please use CB_SUCCESS and friends.
We would like to keep such cleanups for the next project as we have passed CS.
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.
Done
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 424: 1
Please add enums for this.
Implementation is updated as per latest comments from Julius.
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.
Done
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 35: ((offsetof(type, mem)) + base)
This fits into one line.
Implementation updated as per latest comments from Julius.