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

[Masahiro Yamada]

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();
}




Best Regards
Masahiro Yamada