Hi Masahiro,
On 3 December 2014 at 19:01, Masahiro Yamada <yamad...@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@54006800 @ 9fb49108, 1409312768
> |- serial@54006900 @ 9fb49158, 1409313024
> |- serial@54006a00 @ 9fb491a8, 1409313280
> |- serial@54006b00 @ 9fb491f8, 1409313536
> |- * i2c@58400000 @ 9fb49268, 0
> ||- * generic_50 @ 9fb51f00
> |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave
> address 0x49 !!!
> |- i2c@58480000 @ 9fb492b8, 1
> |- i2c@58500000 @ 9fb49308, 2
> `- i2c@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
