[coreboot-gerrit] Change in coreboot[master]: rockchip/rk3399: Add MIPI driver

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/19477 )

Change subject: rockchip/rk3399: Add MIPI driver
......................................................................


Patch Set 5:

(3 comments)

Thanks. Looks good to me now, essentially, but I'd still like one type change to be sure that we can't have any overflows.

https://review.coreboot.org/#/c/19477/5/src/soc/rockchip/rk3399/include/soc/mipi.h
File src/soc/rockchip/rk3399/include/soc/mipi.h:

Line 280: 	unsigned int lane_bps; /* per lane */
Better make this u64 to reduce the risk of transient overflows when we do math on it.


https://review.coreboot.org/#/c/19477/5/src/soc/rockchip/rk3399/mipi.c
File src/soc/rockchip/rk3399/mipi.c:

Line 285: 	lbcc = hcomponent * dsi->lane_bps / (8 * MSECS_PER_SEC);
This one makes me a bit uncomfortable. Changing lane_bps to u64 should guarantee that it's safe.


Line 432: 	mdelay(120);
You reduced the delay but you still didn't explain what it's for. Can you please do that (ideally add a comment for that)?

Are all these delays only specific to the SoC or are any of them also dependent on the panel?


-- 
To view, visit https://review.coreboot.org/19477
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I02475eefb187c619c614b1cd20e97074bc8d917f
Gerrit-PatchSet: 5
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: nickey.yang at rock-chips.com
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Sean Paul <seanpaul at chromium.org>
Gerrit-Reviewer: Shunqian Zheng <zhengsq at rock-chips.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Reviewer: nickey.yang at rock-chips.com
Gerrit-HasComments: Yes