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

[Masahiro Yamada]

Hi Simon,



On Wed, 19 Nov 2014 10:24:47 +0000
Simon Glass <sjg at chromium.org> wrote:

> >
> >
> > BTW, address_offset within the chip and data are treated in the same way in I2C bus.
> > Should we pass them separately to each driver?
> >
> > I mean, can we put the offset address and data in the buffer?
> >
> >
> 
> I'm not sure what you mean by this sorry.


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.




> 
> >
> >
> >
> >> +     }
> >> +     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?








BTW, sandbox_i2c_read() is 400KHz tolerate:


	/* For testing, don't allow reading above 400KHz */
	if (i2c->speed_hz > 400000 || alen != 1)
		return -EINVAL;



but sandbox_i2c_write() only allows 100KHz:

	/* For testing, don't allow writing above 100KHz */
	if (i2c->speed_hz > 100000 || alen != 1)
		return -EINVAL;




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 everthing to work on the mail line.



Best Regards
Masahiro Yamada