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 46:
(13 comments)
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... File src/soc/mediatek/mt8183/dsi.c:
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 23: static void mipi_write32(void *a, uint32_t offset, uint32_t v)
Are these all common functions, which can be shared?
Done
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 28: static void mipi_clrsetbits_le32(void *a, uint32_t offset, uint32_t m, uint32_t v)
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 64: const struct edid *edid, struct mtk_phy_timing *phy_timing)
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 96: data_rate = (u64)(edid->mode.pixel_clock * 1000 * total_bits) / (htotal * lanes);
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 98: BIOS_ERR
Why is this an error? BIOS_INFO or NOTICE?
Done
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 133: pcw = (u64)((data_rate / 1000000) * (1 << txdiv0) * (1 << txdiv1)) << 24;
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 139: udelay(30);
Is that delay mentioned in the data sheet?
Done
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 159: static void mtk_dsi_phy_timconfig(u32 data_rate, struct mtk_phy_timing *phy_timing)
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 212: const struct edid *edid, struct mtk_phy_timing *phy_timing)
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 415: dsi DONE INT Timeout
Please rephrase to be better understandable, and also add the stop watch time.
Done
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 415: Timeout
time-out
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 416:
Please add a debug message, detailing how long it took by printing the stopwatch value.
Ack
https://review.coreboot.org/c/coreboot/+/31591/1/src/soc/mediatek/mt8183/dsi... PS1, Line 444: const struct edid *edid, struct lcm_init_table *init_cmd, u32 count)
line over 80 characters
Ack