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 28:
(8 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 128: 0x9
Done
I meant *all* magic numbers, not just the one I pointed at please.
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi.c:
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... PS28, Line 42: 0x3f Magic number.
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... PS28, Line 42: (0 << 16) What's a (0 << 16), anyway? If you want to make it super explicit that you're disabling a bit here, this should also be a named constant, otherwise it's pointless.
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... PS28, Line 43: 0x04 Magic number.
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... PS28, Line 44: 0 Same here.
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... PS28, Line 116: 0x0004EA60 magic number
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/di... PS28, Line 118: 0x103 magic number
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/mipi_dsi.h:
https://review.coreboot.org/c/coreboot/+/39614/28/src/soc/qualcomm/sc7180/in... PS28, Line 25: #define EOF_BLLP_PWR 0x9 Please don't put #define macros that are so short they could easily clash with other stuff into headers. Either namespace these more (e.g. DSI_VC1 or something) or (better) just #define them in the .c file instead.