Change in coreboot[master]: drivers/sn65dsi86: Switch EDID reading to use "indirect mode"

[Douglas Anderson (Code Review)]

Attention is currently required from: Vinod Polimera, Julius Werner.
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52959 )

Change subject: drivers/sn65dsi86: Switch EDID reading to use "indirect mode"
......................................................................

Patch Set 5:

(1 comment)

File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:

https://review.coreboot.org/c/coreboot/+/52959/comment/f4341904_3f7758fd 
PS5, Line 253: if (i2c_writeb(bus, chip, SN_AUX_CMD_REG, AUX_CMD_SEND | (cmd << 4)) ||
             : 		    !wait_ms(100, !i2c_readb(bus, chip, SN_AUX_CMD_REG, &buf) &&
             : 				  !(buf & AUX_CMD_SEND)) ||
             : 		    i2c_readb(bus, chip, SN_AUX_CMD_STATUS_REG, &buf) ||
             : 		    (buf & (NAT_I2C_FAIL | AUX_SHORT | AUX_DFER | AUX_RPLY_TOUT))) {
             : 			printk(BIOS_ERR, "ERROR: aux command send failed\n");
             : 			return CB_ERR;
             : 		}
If it were me, I wouldn't have written separate commands. The way this is written makes me think too hard, and no good has ever come of thinking too hard. Splitting it up into separate calls would also potentially allow you to give better error message (like saying _which_ of the error bits was set).

I won't insist on it, but it'd be nice.

One other note: the way you have things setup right now if you ever get a single error then you'll fail forevermore. If you want to avoid that, you need to manually "ack" the error bits at the beginning of the function.

It also might not hurt to have a comment saying that, in theory, we think SHORT might come up with some panels and need to be handled but we haven't seen a panel like that yet. If anyone ever does see SHORT come up in the future it would maybe be a little quicker for them to realize that the problem was...