Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31591 )
Change subject: mediatek/mt8183: add dsi driver for mt8183 ......................................................................
Patch Set 14:
(12 comments)
https://review.coreboot.org/#/c/31591/14/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/#/c/31591/14/3rdparty/blobs@a1 PS14, Line 1: we should not delete 3rdparty/blob library. please revert this.
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c File src/soc/mediatek/mt8183/dsi.c:
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@23 PS14, Line 23: static any reason to declare dsi_write32 here, instead of simply using write32?
if really needed, I think this can be 'static inline'.
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@48 PS14, Line 48: int the printf said data_rate is unsigned int.
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@65 PS14, Line 65: (u64)(edid->mode.pixel_clock * 1000 * bpp) / lanes; we should cast to larger types in the beginning, i.e:
(u64)edid->mode.pixel_clock * 1000 * bpp / lanes;
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@103 PS14, Line 103: (u64) u64 should be converted earlier, whenever the value will exceed 32 bits.
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@110 PS14, Line 110: 30 Add a comment for what we're waiting for?
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@115 PS14, Line 115: 40 any comment for why 40?
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@298 PS14, Line 298: "HFP less than d-phy, FPS will under 60Hz\n"); exceed col 80
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@305 PS14, Line 305: "HFP less than d-phy, FPS will under 60Hz\n"); exceed col 80
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@377 PS14, Line 377: (void *)DSI_BASE + 0x200 + i (void *)(DSI_BASE + 0x200 + i)
In fact you should make it clear what's that 200 for. maybe something like
uintptr_t dsi_something = DSI_BASE + 0x200;
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/dsi.c@395 PS14, Line 395: 4 add a comment for why 4
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/include/soc... File src/soc/mediatek/mt8183/include/soc/dsi.h:
https://review.coreboot.org/#/c/31591/14/src/soc/mediatek/mt8183/include/soc... PS14, Line 476: MTK_DSI_HOST_IS_READ I'm not seeing a strong reason to put this as macro here, bcz it's only called one time.
Maybe move this back into c source, or change to an inline function.