Vinod Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39614 )
Change subject: sc7180: Add display dsi interface programming. ......................................................................
Patch Set 26:
(9 comments)
https://review.coreboot.org/c/coreboot/+/39614/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39614/24//COMMIT_MSG@7 PS24, Line 7: sc7180: Add display dsi interface programming.
Please remove the period/dot at the end of the commit message summary.
Done
https://review.coreboot.org/c/coreboot/+/39614/23/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi.c:
https://review.coreboot.org/c/coreboot/+/39614/23/src/soc/qualcomm/sc7180/di... PS23, Line 5: nclude <delay.h>
please, is it used ?
Done
https://review.coreboot.org/c/coreboot/+/39614/23/src/soc/qualcomm/sc7180/di... PS23, Line 6: #include <edid.h>
missing <types. […]
Done
https://review.coreboot.org/c/coreboot/+/39614/23/src/soc/qualcomm/sc7180/di... PS23, Line 83: void mdss_dsi_video_mode_config(struct edid *edid, : uint32_t bpp)
in one line ?
Done
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.
Done
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.
Done
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.
Done
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 setb […]
Done
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.
Done