Hi Simon, Heiko,
On Thu, 20 Nov 2014 14:04:52 +0000 Simon Glass sjg@chromium.org wrote:
Let's assume we want to write some data to a EEPROM chip connected to i2c bus.
We generally send
[byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0 [byte 3] WData1 ...
From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data.
I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written.
return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,len);In this implementation, the offset_address is treated with "addr" and the *real* data is handled with "buffer".
It seems odd from the perspective of I2C protocol, I think.
Likewise, when we read data from a EEPROM chip connected to i2c bus,
We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1
[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.
In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.
We could certainly change this. I'm not sure that I have a strong opinion either way.
I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data).
Yes, the code of the write (for the address) in the .read() handler would the same as the .write handler. I thought it would not be a good idea to duplicate the write transaction code in every driver.
If we can accept this change, we only need to implement "write -> restart -> read" code in i2c_read() in i2c-uclass.c. Each driver would be much simpler.
That is the point of my suggestion.
}debug("not found\n");return i2c_bind_driver(bus, chip_addr, devp);+}
If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?
The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?
Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property
I could not understand at all what this code wants to do.
This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it.
As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only.
Sorry, I could not parse you here.
I2C is not a hot-plugged bus. I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions?
If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device.
With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected.
Do you mean you implemented the same (similar) behavior as the legacy I2C framework?
Because the clock-frequency is set to <400000> in the sandbox DTS, writing to I2C fails unless we change the I2C speed.
Is this intentional?
Personally, I like everything to work on the mail line.
This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops.
I see.
Best Regards Masahiro Yamada