Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34784 )
Change subject: soc/mediatek: dsi: Refactor MIPI TX configuration ......................................................................
Patch Set 8:
(6 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 some […]
Linux kernel coding style recommends long multi-line format to have double asterisk. But I can change since coreboot seem to have different style.
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?
this is for selecting dividers, so actually there's no simple definition of max rate (when larger than some value, we will use divider=1).
let's leave this as-is until we really see some constraints in future.
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?
it's a board-specific call so I'd assume some board may return failure. but yes we can revise that when it's really needed.
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/dsi... PS7, Line 67: // MIN = 50
nit: C89 comments
Done
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 MI […]
These were recommended by MTK. I can double confirm this as follow up.
https://review.coreboot.org/c/coreboot/+/34784/7/src/soc/mediatek/mt8173/inc... PS7, Line 24: #define MTK_DSI_DATA_RATE_MIN 50
nit: Can we put a unit here (e.g. […]
Done
-
Hung-Te Lin (Code Review)