[U-Boot] [PATCH v3 01/10] dm: i2c: Add a uclass for I2C

[Simon Glass]

Hi Masahiro,

On 3 December 2014 at 19:01, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
> More comments again.
>
>
> On Mon, 24 Nov 2014 11:57:15 -0700
>> +
>> +static int i2c_probe_chip(struct udevice *bus, uint chip_addr)
>> +{
>> +     struct dm_i2c_ops *ops = i2c_get_ops(bus);
>> +     struct i2c_msg msg[1];
>> +     uint8_t ch = 0;
>> +
>> +     if (!ops->xfer)
>> +             return -ENOSYS;
>> +
>> +     msg->addr = chip_addr;
>> +     msg->flags = 0;
>> +     msg->len = 1;
>> +     msg->buf = &ch;
>> +
>> +     return ops->xfer(bus, msg, 1);
>> +}
>
> i2c_probe_chip() issues a write transaction with one length,
> but a read transaction should be used.
>
> For most of chips, the first write data is the first byte of
> the offset address, so no real data will be written into the chip.
>
> But it is possible to have offset_address_length == 0.
>
> The read transaction is always safer than the write transaction.
>
>
>
>
>
>
>
> BTW, I implemented an i2c driver for my Panasonic board base on this series,
> and I am playing around with it.
>
> I found a strange behavior.
>
>
> Assume an EEPROM chip is assigned with slave-address 0x50.
> There is no other chip on this i2c bus.
>
> If I run "i2c probe 50" command, it correctly detects the EEPROM
> chip and create a generic device node  "generic_50".
> If I run "i2c probe 49" command, it simply fails and nothing else happens.
>
> On the other hand, when I run "i2c read 49 0.2 1  84000000",
> it forcibly create a generic device node  "generic_49".
> "i2c read" command directly calls i2c_get_chip() and skips the probing step.
> This is odd.
>
> My "dm tree" output is like this:
>
> => dm tree
> ROOT 9fb49028
> - * root_driver @ 9fb49028
> `- * soc @ 9fb49098
>  |- * serial at 54006800 @ 9fb49108, 1409312768
>  |-   serial at 54006900 @ 9fb49158, 1409313024
>  |-   serial at 54006a00 @ 9fb491a8, 1409313280
>  |-   serial at 54006b00 @ 9fb491f8, 1409313536
>  |- * i2c at 58400000 @ 9fb49268, 0
>  ||- * generic_50 @ 9fb51f00
>  |`- * generic_49 @ 9fb51f70                <--- nothing exists on slave address 0x49 !!!
>  |-   i2c at 58480000 @ 9fb492b8, 1
>  |-   i2c at 58500000 @ 9fb49308, 2
>  `-   i2c at 58580000 @ 9fb49358, 3
>
> It should not create any device node for non-existing slave address.
>
>
> I think i2c_get_chip() implementation is wrong.
>
>
>
> My rough image of the correct implemenation is like this:
> The key is to split it into  i2c_create_chip() and i2c_get_chip(), I think
>
>
>
>
> int  i2c_probe ( .... )
> {
>        i2c_probe_chip();
>
>        if (failed)
>                 return;
>
>        i2c_create_chip()
> }
>
>
> int i2c_create_chip()
> {
>
>      search the suitable chip driver based on DeviceTree
>
>
>      if (found) {
>             use it
>       } else {
>             call i2c_bind_driver()  to create "generic" chip
>       }
> }
>
>
>
> int  i2c_get_chip ()
> {
>         search a child node with the given chip_addr
>
>         if (found)
>                return dev;
>
>
>         i2c_probe();
> }
>

But that would change the bahaviour - it would then become impossible
to access a chip that does not probe. The probe is a feature of the
uclass, but it does not gate use of a device. In fact with the device
tree we will typically create devices without probing them.

Regards,
Simon