Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
Fork in Southbridge Intel?
Well I have no access to #547871.
In the doc I referred to, see Figure 16, this is our do_i2c_eeprom_read() implementation. There the diagram clearly says "Send DATA1 register" to refer to SMBHSTDAT1. From what I can tell, "Command Code" in that paper always refers to SMBHSTCMD.
The weirdness here is code currently fills both SMBHSTCMD and SMBHSTDAT1 registers with the same value. I guess I am not happy about this until I hear it works by writing SMBHSTDAT1 only (and not SMBHSTCMD) and after this possible discrepancy is documented within the code and not only the commit message.
Looking at i2c-i801 kernel module, SMBHSTDAT1 is not involved for I2C block writes there either.
Yes, I'd fork it inside common/smbus.c. There's probably going to be some conditionals around CMD/DAT1 use if we expand this to older silicons.