Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34784 )
Change subject: soc/mediatek: dsi: Refactor MIPI TX configuration ......................................................................
Patch Set 7: Code-Review+2
(5 comments)
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/common/dsi... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/common/dsi... PS7, Line 42: /** nit: I think the double asterisk at the start of the comment is only for function docstrings or something? Anyway, I don't think it belongs here.
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/common/dsi... PS7, Line 52: if (data_rate < MTK_DSI_DATA_RATE_MIN) { nit: should there maybe be a DATA_RATE_MAX check too while we're at it?
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/dsi... File src/soc/mediatek/mt8173/dsi.c:
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/dsi... PS7, Line 23: int nit: Looks like this can't fail, so why not void?
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/dsi... PS7, Line 67: // MIN = 50 nit: C89 comments
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/inc... File src/soc/mediatek/mt8173/include/soc/dsi.h:
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/inc... PS7, Line 23: #define MTK_DSI_MIPI_RATIO_DENOMINATOR 100 Is there a good reason the ratio is different between 8173 and 8183? Sounds like that's more of a MIPI connection detail than specific to the SoC?