Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31478 )
Change subject: mediatek/mt8183: Add display driver ......................................................................
Patch Set 3:
(12 comments)
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c File src/soc/mediatek/mt8183/ddp.c:
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@32 PS3, Line 32: blank links in this function are not needed.
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@84 PS3, Line 84: /* Config width */ probably no need since it's pretty trivial; or a comment like "Configure with and height to reg size_con_0".
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@87 PS3, Line 87: /* Config height */ probably no need since it's pretty trivial
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@168 PS3, Line 168: /* Setup RDMA0 */ I think this is very trivial. Instead maybe you can explain why only 0 is needed in comments?
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@172 PS3, Line 172: /* Setup Color */ no need since the function name below clearly explains.
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@175 PS3, Line 175: /* Setup CCORR */ no need since the function name below clearly explains. Probably better if the comment explains what is CCORR (what it stands for).
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@178 PS3, Line 178: /* Setup AAL */ No need unless if you want to add what AAL stands for in the comment.
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@181 PS3, Line 181: /* Setup GAMMA */ no need
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@184 PS3, Line 184: /* Setup DITHER */ no need
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@187 PS3, Line 187: /* Setup main path connection */ no need
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@190 PS3, Line 190: /* Setup main path mutex */ no need
https://review.coreboot.org/#/c/31478/3/src/soc/mediatek/mt8183/ddp.c@204 PS3, Line 204: 0x380 any comments for what this magic number is? maybe define it in address map or regs offset?