Hi Simon, Heiko,
On Thu, 20 Nov 2014 14:04:52 +0000 Simon Glass <[email protected]> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
