Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33433 )
Change subject: mainboard/facebook/fbg1701: Configure TC358860 eDP to MIPI controller ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... File src/mainboard/facebook/fbg1701/mainboard.h:
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... PS4, Line 25: } edp_data_t; There is some dislike about typedeffing structs, specially when they are for one-time use only.
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... PS4, Line 27: static const edp_data_t mainboard_TC348860_InitTable[] = { Hmm.. allocation in .h file, doesn't this trigger defined-but-unused errors for files other than mainboard.c now?
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... File src/mainboard/facebook/fbg1701/mainboard.c:
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... PS4, Line 25: void mainboard_configure_edp_bridge(void) Not sure why this was not placed in mainboard/ramstage.c instead?
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... PS4, Line 27: edp_data_t *edptable; Target is a const array.
https://review.coreboot.org/#/c/33433/4/src/mainboard/facebook/fbg1701/mainb... PS4, Line 41: smbus_i2c_block_write(edptable->address, edptable->offset, The call should work fine with const arguments, also check return value for errors, in case you need to do retry.