Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31591 )
Change subject: soc/mediatek/mt8183: Add DSI driver ......................................................................
Patch Set 34:
(16 comments)
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... File src/soc/mediatek/mt8183/dsi.c:
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 23: static
any reason to declare dsi_write32 here, instead of simply using write32? […]
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 48: int
the printf said data_rate is unsigned int.
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 65: (u64)(edid->mode.pixel_clock * 1000 * bpp) / lanes;
we should cast to larger types in the beginning, i.e: […]
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 103: (u64)
u64 should be converted earlier, whenever the value will exceed 32 bits.
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 110: 30
Add a comment for what we're waiting for?
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 115: 40
any comment for why 40?
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 298: "HFP less than d-phy, FPS will under 60Hz\n");
exceed col 80
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 305: "HFP less than d-phy, FPS will under 60Hz\n");
exceed col 80
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 377: (void *)DSI_BASE + 0x200 + i
(void *)(DSI_BASE + 0x200 + i) […]
Ack
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/ds... PS14, Line 395: 4
add a comment for why 4
Ack
https://review.coreboot.org/c/coreboot/+/31591/16/src/soc/mediatek/mt8183/ds... File src/soc/mediatek/mt8183/dsi.c:
https://review.coreboot.org/c/coreboot/+/31591/16/src/soc/mediatek/mt8183/ds... PS16, Line 399: count
we may remove count?
Ack
https://review.coreboot.org/c/coreboot/+/31591/16/src/soc/mediatek/mt8183/ds... PS16, Line 413: break
return;
Ack
https://review.coreboot.org/c/coreboot/+/31591/16/src/soc/mediatek/mt8183/ds... PS16, Line 459: u32 count
oh, sorry, 8173 didn't really do table init. […]
Ack
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... File src/soc/mediatek/mt8183/dsi.c:
PS25:
yes and the work to refactor is in https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... PS25, Line 81: >
This needs MTK to confirm. […]
jitao confirmed offline this cannot be >=.
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dsi.h:
https://review.coreboot.org/c/coreboot/+/31591/14/src/soc/mediatek/mt8183/in... 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. […]
Ack