Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);
i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }
AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);
For your chip, this would be called with `offset_bytes == 2`.