Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
(4 comments)
Sorry, this will not be usable after the refactoring on smbus commands. I'll implement that i2c block write command you need, and make it available on all recent intels.
Frans, can you point me to a commit where you use i2c block write? Also no need to respond to the review comments here.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 482: outb(cmd, smbus_base + SMBHSTDAT1); This does not look right, putting cmd in to two registers.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 485: outb(bytes, smbus_base + SMBHSTDAT0); Why? Isn't the block length determined by how long we run the loop and when last_byte bit is set.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 498: /* Start the command */ For i2c block command there should be a single start and single stop visible on the i2c lines. The comment suggest there is a start for each byte?
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 503: if (smbus_wait_until_active(smbus_base) < 0) This was buggy; see https://review.coreboot.org/c/coreboot/+/21119