[coreboot] New interface for I2C in coreboot

[Werner Zeh]

Hi coreboot community.

Although I am working on coreboot for now more than one year mostly behind the scenes I am fairly new on the contributor side.  Yesterday, I have started contribution and one of my first value that I would like to give back to the community is a driver for I2C controllers that starts to get integrated into x86-chips from intel (Baytrail has one as well as X1000 Quark). Please be aware that this is not a driver for the well-known SMBus controller.

I have written the driver a few weeks ago and at this time there was a pretty simple and therefore really generic interface for I2C buses in coreboot. The interface is described in src/include/device/i2c.h and was consist of two simple functions: i2c_read() and i2c_write().

With this interface the user of the driver (which in most of the cases will be another driver for let’s say an EEPROM) can simply call for instance the i2c_read()-function and provide as arguments the i2c-bus, the chip-address of the slave (EEPROM here), the address inside the slave, a length and a buffer where to store the driver. With these parameters, the I2C driver is able to read the desired data in one call and store them into the buffer provided. Similar procedure is for the write function. You can see that one logical transfer can be directly mapped to one function call of the driver (i2c_read or i2c_write).

In mid-December 2014, the interface was changed massive by this two commits: 7947 and 7751. I have had a look at the new interface and what should I say: It is not only completely different (what in general would be OK for me) but it is in my point of view a step back. 

Let’s see what is happened here. The functions i2c_read() and i2c_write() where replaced by i2c_readb(), i2c_writeb(), i2c_read_raw() and i2c_write_raw(). The first two functions can read a single byte from a slave on a given register address (EEPROM address in my example here). The second two are meant to read a chunk of data from the slave. Both of the functions actually are a wrapper to call the real i2c driver interface called i2c_transfer().

But wait a moment…with the last two functions there is no way to tell the driver _where_ to start reading inside the slave. Let us have a closer look to how this interface can be used to transfer a chunk of data from a given address inside the slave. To do this, let us consider we want to read 10 bytes starting at offset 20.

1.	Store the offset we need (in this case 20) into a buffer
2.	Call i2c_write_raw(bus, chip_adr, &buffer, 1);
3.	Call i2c_read_raw(bus, chip_adr, &buffer,10);

Here are my personal problems with this kind of interface:
1.	Where we earlier had one simple call to i2c_read(), we now have to fill one variable with the length we need and call two different functions (i2c_write_raw and i2c_read_raw).
2.	Though the read transfer is one logical access to the slave, we have split it into two (without any benefit I can see now)
3.	We have added two wrapper layers where we earlier had none!

Unfortunately, the first two mentioned functions of the interface cannot be used for transferring more than byte because the length field is missing. In contrast, the two functions that have a length field do not have a slave address field. This is frustrating!

If one have a hardware implemented I2C-controller in mind, this interface is really not that intuitive and simple. This is because we have to perform a function switch in the middle of a logical bus action. And: why should one perform i2c_write operation in order to read the data? This is not logical to me. I know that I2C bus works this way, but it is the responsibility of the driver to hide this from the user. And with this interface, this “hiding” is simply impossible because now the user have to do it!

I do not see any benefits of this interface and I really cannot see the need for the introduced change in the interface. Of course it is technical possible to read and write data over I2C with this interface…but to be honest: I would not be proud of this solution!

Of course we can add an address field to the i2c_read_raw and i2c_write_raw functions. But at the end we will end up with a complex interface for a very simple bus. And we will have several wrapper layers compared to the interface we had before. There is even no need of _one_ wrapper layer for this bus!

I hope I was able to point out what my concern is about and want to discuss this with the community. I want to hear more opinions on this interface and at the end want to know whether it is worse to change my I2C driver to be used with this interface. The last point I will do if one can show me the _real_ benefit of the new interface.

Thank you
Werner