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:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 13: #define HALF_DIVIDER(div2x) (div2x ? (div2x - 1) : 0)
I'm unclear what this macro is doing, it might be better to do this operation explicitly in mdss_clo […]
Done
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 109: struct mdss_clock_config mdss_extclk_config[] = {
Please do not bind information with string comparisons. Just do something like this: […]
Done
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ;
While we're starting to use these functions for non-boot-critical clocks, it would be a good idea to […]
We would like to take such cleanups in the next project as it does not break any functionality.
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 38: u32 cbcr;
Please fit what you're doing into the existing framework rather than rebuilding a copy next to it. […]
Done
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 235: struct mdss_clock_config {
Please remove this too, I don't know how it got in here.
Done