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

Masahiro Yamada
2014-11-20T06:05:17Z

Hi Simon,

On Wed, 19 Nov 2014 10:24:47 +0000
Simon Glass <sjg@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