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

[Masahiro Yamada]

Hi Simon,




On Wed, 3 Dec 2014 19:32:18 -0700
Simon Glass <sjg at chromium.org> wrote:

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

What's the problem?
If it does not probe, it means no device exists on that slave address.
Trying further access against non-exising device has no point.


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

Precisely, devices are *bound* without probing,
but should not be activated until we make sure the device really exists on the bus.



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


Here, while generic_49 does not really exist,
nevertheless it is activated (has '*' mark).
Very strange.


Best Regards
Masahiro Yamada