Attention is currently required from: Ravi kumar. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39614 )
Change subject: sc7180: Add display dsi interface programming ......................................................................
Patch Set 40:
(2 comments)
File src/soc/qualcomm/sc7180/display/dsi.c:
https://review.coreboot.org/c/coreboot/+/39614/comment/db5e7f8a_c76135d5 PS40, Line 105: hbp = edid->mode.hbl - edid->mode.hso; Hi Vinod,
Just looking at this and trying to understand it again for the MIPI panels, I got confused by this line: according to the EDID spec, HBL should be the whole blanking interval (front porch + sync pulse + back porch), HSO is the front porch, and HSPW is the syncing interval. So shouldn't that mean that the back porch should be
hbp = edid->mode.hbl - edid->mode.hso - edid->mode.hspw
? What you're doing here looks like you still have both the back porch and syncing interval added together and just calling it "back porch". Is that correct? (Maybe the rest of your code still works okay with this, I'm not really sure what your video_mode_active_h register needs to contain -- is it supposed to contain the whole interval except the front porch, or the whole interval except both front porch and sync pulse? Your implementation currently seems to do the former. But even if that's correct, I think the variable names here should be updated or code otherwise rewritten to clearly show that that's the intention, because right now it just looks like a bug.)
https://review.coreboot.org/c/coreboot/+/39614/comment/907a1087_91f6a814 PS40, Line 122: hbp - 1)); This too is a bit confusing because you'd really just need to write
(edid->mode.ha + edid->mode.hbl)
... what you wrote is equivalent but more complicated to follow. Also, there's that -1 where I'm not sure where that comes from... are you sure that's supposed to be there?