Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31591 )
Change subject: mediatek/mt8183: Add DSI driver ......................................................................
Patch Set 25:
(4 comments)
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?
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 end the list (as is common with many other self-terminated arrays), it will still work.
https://review.coreboot.org/c/coreboot/+/31591/24/src/soc/mediatek/mt8183/in... PS24, Line 106: #define INIT_GENENIC_CMD 2 typo
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 optimize structure size as much as possible. You're blowing (4 + 4 + 64) * 292 == 21KB on the BOE panel alone! (Granted, it's compressed, but still...)
Are you really ever anticipating that you'll need 64? Looks like the most you need for now is 6 on the Krane panel. I'd suggest for now you restrict this to
u8 cmd : 5; u8 len : 3; u8 data[7];
and see how far you get with that. (Alternatively, you could leave out the len and just have different cmd values for all different data packet lengths, like I suggested in https://review.coreboot.org/c/coreboot/+/33413/15/src/mainboard/google/kukui... .)