[U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C

[Masahiro Yamada]

Hi Simon, Heiko,



On Thu, 20 Nov 2014 14:04:52 +0000
Simon Glass <sjg at 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