Felix Held 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 1:
(7 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 353: int i2c_recover_bus(unsigned bus)
Prefer 'unsigned int' to bare use of 'unsigned'
what jenkins says
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 360: If we can set SDA when using software i2c, we can always set sda. since the check from the linux code isn't here, drop this part of the comment
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 362: Check the 'incomplete_write_byte' fault injector : * for details. this probably doesn't apply to coreboot, right?
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 371: as we need to give 9 falling-rising edges i'm not sure what you want to say exactly with this. you send 9 additional stop conditions here, right?
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 379: break; remove break after return
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 385: /* Creating STOP again, see above */ this is only true for half of the executions of the while loop contents
https://review.coreboot.org/c/coreboot/+/35725/1/src/device/software_i2c.c@3... PS1, Line 388: wait(bus); in the linux code there's a check if the sda pin is 1 when the scl variable is 1 and if that's the case the rest of the 9 additional stop conditions isn't sent to save some time. would it make sense to do this here as well? also: wouldn't for example 3 additional i2c resets be sufficient here?