[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:

(2 comments)

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

Line 429: 	mdelay(12);
...and, actually, when I'm looking at the kernel patch mentioned below more closely it does:

 1. regulator enabling [already done for us by the time we reach this function]
 2. several msleep(12) [presumably waiting for the regulators, we might be able to skip that here]
 3. EXIT_SLEEP_MODE
 4. msleep(120)
 5. SET_DISPLAY_ON
 [and that's it, I'm not sure where it does the VID_MODE... probably later]

So, are you sure that you really got the delays in the right place here? Should the msleep(120) be between EXIT_SLEEP_MODE and SET_DISPLAY_ON instead, maybe? Looks to me like either the kernel code or this patch is wrong.


Line 432: 	mdelay(120);
> Done
You said "Done" but you didn't really answer my question... that's really the more important part. We need to know what this number represents to be able to implement it in the right way.

In https://patchwork.kernel.org/patch/9642191/ it seems that this is called "T6"... is that a number from the panel datasheet? (I don't have one unfortunately...)

If this *is* panel dependent, then the number must be in devicetree.cb like all the other panel values. You'd need to add a new field for this to chip.h, then read that in display.c and pass it into this function as an additional parameter. (If, on the other hand, it is not panel dependent and would be the same for every panel on this SoC, it's okay to hardcode it.)


-- 
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