Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31591 )
Change subject: mediatek/mt8183: Add DSI driver ......................................................................
Patch Set 25:
(12 comments)
https://review.coreboot.org/c/coreboot/+/31591/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31591/1//COMMIT_MSG@7 PS1, Line 7: mediaek
mediatek
Ack
https://review.coreboot.org/c/coreboot/+/31591/1//COMMIT_MSG@8 PS1, Line 8:
Where is the data sheet (or the name, revision), and is that driver written from scratch or taken fr […]
Ack
https://review.coreboot.org/c/coreboot/+/31591/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31591/20//COMMIT_MSG@8 PS20, Line 8:
Can you please add a datasheet name/revision.
Ack
https://review.coreboot.org/c/coreboot/+/31591/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31591/25//COMMIT_MSG@16 PS25, Line 16: mesaages
messages
Ack
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... File src/soc/mediatek/mt8183/dsi.c:
PS25:
Is all this still gonna get deduplicated with MT8173 code?
yes and the work to refactor is in https://review.coreboot.org/c/coreboot/+/34562
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... PS25, Line 81: >
Consider changing to ">=" for consistency
This needs MTK to confirm.
@jitao/yungnian?
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... PS25, Line 93: lanes);
Can be moved to the previous line
Done
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... PS25, Line 295: printk(BIOS_ERR, "HFP less than d-phy, FPS will under 60Hz\n");
Exceeds 80 columns
Done
https://review.coreboot.org/c/coreboot/+/31591/25/src/soc/mediatek/mt8183/ds... PS25, Line 302: printk(BIOS_ERR, "HFP less than d-phy, FPS will under 60Hz\n");
Exceeds 80 columns
Done
https://review.coreboot.org/c/coreboot/+/31591/24/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dsi.h:
https://review.coreboot.org/c/coreboot/+/31591/24/src/soc/mediatek/mt8183/in... PS24, Line 105: #define END_OF_TABLE 1
We should make this one 0, so that if someone accidentally just uses an empty struct initializer to […]
Done
https://review.coreboot.org/c/coreboot/+/31591/24/src/soc/mediatek/mt8183/in... PS24, Line 106: #define INIT_GENENIC_CMD 2
typo
Done
https://review.coreboot.org/c/coreboot/+/31591/24/src/soc/mediatek/mt8183/in... PS24, Line 112: u8 data[64];
Considering how many of these you throw in for some of those panels, we should really try to optimiz […]
Let us change that to 7 first.