Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35725 )
Change subject: device/software_i2c: Add function to recover bus ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c File src/device/software_i2c.c:
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 368: wait(bus);
The software controller connected to this pulls both lines low (the Aspeed AST). […]
Well... you're kinda using the API wrong. The thing intializing software_i2c() (i.e. the thing setting software_i2c[n] to a non-NULL value) is supposed to initialize both lines to high. Then the rest of the software I2C code makes sure they're always both high at the end of every externally visible function. If you follow that convention, this should never be needed here.
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 371: By this time SCL is high
what's here to do?
Since you added a wait_for_scl() this should be handled now.
https://review.coreboot.org/c/coreboot/+/35725/3/src/include/device/i2c_simp... File src/include/device/i2c_simple.h:
https://review.coreboot.org/c/coreboot/+/35725/3/src/include/device/i2c_simp... PS3, Line 48: int software_i2c_recover_bus(unsigned int bus); nit: move up to other software_i2c functions?