Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 3:
(19 comments)
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG@7 PS3, Line 7: sc7180: Add support for sn65dsi86 bridge. Please remove the dot/period at the end of the commit message.
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG@10 PS3, Line 10: Please mention the datasheet name and revision.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 13: #define bridge_debug(x...) do {if (0) printk(BIOS_DEBUG, x); } while (0) Please add a separate debug macro. Please look how other drivers do it.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 15: #define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x&0xFF00)>>8) Please add spaces around the operator and use lowercase hex letters.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 47: * by the bridge in Mbps unit. Fits on the line above?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 59: Disable disable
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 97: int unsigned int
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 141: int unsigned int
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 219: int size_t or unsigned_int?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 224: int unsigned int?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 248: printk(BIOS_ERR, "ERROR: valid dp rate not found"); Maybe:
No valid dp rate found.
What is the consequence for the user?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 291: printk(BIOS_ERR, "ERROR: PLL lock failure\n"); … after 500 ms.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 310: printk(BIOS_ERR, "ERROR: Link training failed");
… after 500 ms timeout.
What is the consequence for the user? Display might not work?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 329: version versions
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 332: to compatible to be
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 327: /* : * support bridge HPD function : * some hardware version do not support bridge hdp, : * we use 360ms to try to get the hpd single now, : * if we can not get bridge hpd single, it will delay 360ms, : * also meet the bridge power timing request, to compatible : * all of the hardware version : */ Please re-flow for 96 characters.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 337: 360 360 ms is very long for coreboot.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 385: /* Disable vstream */ Why should it be disabled?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 393: /* color bar disabled */
Disable color bar
What is a color bar?