Julius Werner has posted comments on this change. ( https://review.coreboot.org/19477 )
Change subject: rockchip/rk3399: Add MIPI driver ......................................................................
Patch Set 4:
(9 comments)
https://review.coreboot.org/#/c/19477/4/src/soc/rockchip/rk3399/include/soc/... File src/soc/rockchip/rk3399/include/soc/mipi.h:
Line 225: #define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y)) Oh, I only just noticed that you're redefining these here. Please don't do that... instead, use the existing implementations from <stdlib.h> and <timer.h> (this changes the naming from MSEC_PER_SEC to MSECS_PER_SEC, etc.).
https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/mipi.c File src/soc/rockchip/rk3399/mipi.c:
Line 68: return dptdin_map[i].testdin;
if max_mbps = 100； when we use >= will return 0x10,but we want 0x20
Yeah... right. I don't know what I was thinking there. Sorry.
Line 406:
rk3288 and rk3399 have the same mipi ip，it will be more convenient if we wa
That doesn't require you to have this here, though. You can just put a
static struct rk_mipi_regs *mipi_regs = (void *)MIPI_BASE;
at the top of this file and use that... it will still pick the right address for 3288 and 3399 because MIPI_BASE comes from the right <soc/addressmap.h> header.
(That reminds me, btw... if this code is already compatible with 3288, just put it in rockchip/common/mipi.c instead.)
https://review.coreboot.org/#/c/19477/4/src/soc/rockchip/rk3399/mipi.c File src/soc/rockchip/rk3399/mipi.c:
PS4, Line 182: target_bps target_bps should also be unsigned long, just in case. Also, I think it would be clearer to use u32 and u64 instead of unsigned int and unsigned long so there can be no confusion about how long the types are.
PS4, Line 183: 1500000000 1500 * MHz
PS4, Line 206: 5000000 5 * MHz and 40 * MHz
PS4, Line 216: lane_mbps This should also be lane_bps instead and all subsequent calculations with it should be in bits/Hz if possible.
Line 405: int ret; Remove this and just do
if (function_call(...) < 0) return;
directly.
Line 436: mdelay(500); I feel like I must have asked this somewhere already... where do the 500ms come from and are they really necessary? We don't usually care about display init that much because we skip it on normal boot, but 500ms is a *really* long time...
-
Julius Werner (Code Review)