Felix Held 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:
(3 comments)
The code itself looks good to me; I'd prefer having a less convoluted API though that abstracts a bit more the weirdness of the I2C mode of the SMBUS controller. See my comments in the code for that.
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/include/soc/s... PS9, Line 25: int smbus_i2c_block_write(u8 addr, u8 offset, const unsigned int bytes, : const u8 *buf); see my other comment on this API
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@37 PS9, Line 37: smbus_i2c_block_write(u8 addr, u8 offset, const unsigned int bytes, : const u8 *buf) I think "int smbus_i2c_block_write(u8 addr, const unsigned int bytes, const u8 *buf)" would be a nicer, more generic and possibly easier to use API. So basically also put the "offset" byte in *buf, use buf[0] instead of offset and &buf[1] instead of buf and bytes - 1 instead of bytes (and of course do some error checking for the case that bytes is less than 1). The I2C protocol doesn't define what the payload bytes (everything after the 7 or when using the address extension 14 bit I2C device address) and different devices use different amount of bytes to select an internal register address; I've also seen an I2C device that just had one 8 bit register and didn't use any internal register addressing.
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); It would probably be useful to add a bit of documentation here, since the code isn't that obvious. From what I understood, in I2C mode the first I2C payload byte is put in the SMBUS command byte, the I2C write transfer payload length excluding the first byte is put in the SMBUS length register, that doesn't get transmitted when the SMBUS controller is in I2C mode, and the remaining bytes of the I2C write transfer payload gets sent like the SMBUS payload. This is a rather convoluted way of doing an I2C write transfer, but we can't change the hardware...