Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39614 )
Change subject: sc7180: Add display dsi interface programming. ......................................................................
Patch Set 26:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi.c:
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/di... PS26, Line 11: void mdss_dsi_host_init(int num_of_lanes); See other patch about no prototypes in .c files.
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/di... PS26, Line 19: DMA_STREAM1 Please use #define or 'enum' for constants like this.
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/di... PS26, Line 128: 0x9 Please define named constants for all magic numbers that explain what they mean.
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/di... PS26, Line 163: setbits32(&dsi0->clk_ctrl, DSI_ESCCLK_ON); Do you need to set all these bits with a separate write cycle? If not, please just use a single setbits32() and OR the bits together.
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/mipi_dsi.h:
https://review.coreboot.org/c/coreboot/+/39614/26/src/soc/qualcomm/sc7180/in... PS26, Line 3: _PLATFORM_MSM_SHARED_MIPI_DSI_H_ Please match (part of) the actual file path.