Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31439 )
Change subject: drivers/intel/gma: Add DP AUX interface and I2C bus ......................................................................
Patch Set 1:
(3 comments)
There is thorough functionality in libgfxinit. If it's only about fetching the EDID, we'd just have to export one more function. Let me know if you want to give it a try.
The current implementation here does what is required to talk to Intel hardware. Though, it seems that most of the AUX transaction handling specified by Display- Port is missing.
Is this tested? with external displays?
https://review.coreboot.org/#/c/31439/1/src/drivers/intel/gma/intel_dp_aux.c File src/drivers/intel/gma/intel_dp_aux.c:
https://review.coreboot.org/#/c/31439/1/src/drivers/intel/gma/intel_dp_aux.c... PS1, Line 42: * driver is executed. What about the display engine's power wells? I don't see them being handled here. We'd have to enable PW1 for the eDP aux channel and additionally PW2 for the other channels. Is this all covered by `PanelPowerEnable`?
https://review.coreboot.org/#/c/31439/1/src/drivers/intel/gma/intel_dp_aux.c... PS1, Line 106: __weak uint32_t intel_dp_aux_ctl_soc(void) Why `weak`? What platform doesn't have to do something here? Wouldn't it be nicer to have a linking error?
https://review.coreboot.org/#/c/31439/1/src/drivers/intel/gma/intel_dp_aux.c... PS1, Line 280: + 1 I assume this is skipping the status byte? There seem to be two layers of status checking and handling missing.