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

[Simon Glass]

Hi Masahiro,

On 21 November 2014 09:04, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> 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.

I'm not sure we can do that. Some drivers have to do special things to
handle the restart and it would not necessarily be a good idea to move
that logic to the uclass.

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

Yes, we need the ability to scan the bus and find which devices are present.

>
>
>
>> >
>> >
>> > 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.

Regards,
Simon