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

[Simon Glass]

Hi Masahiro,

On 4 December 2014 at 00:24, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> 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.
>

I have a pretty cast-iron rule that I don't want to change behaviour
when boards  move to driver model. We can always decide to change
things later but I am trying to avoid the situation where people hit
problems when switching to driver model. If we have one such problem
for each uclass, it could be an insurmountable problem for some
people.

I don't see a problem with talking to a device which doesn't respond
to probe, or in fact isn't even there. It should produce an -EREMOTEIO
error. Provided that it does that, things are fine. While it might be
considered to have no point, that is the user's decisions. There could
be problems with probing, problems with the bus, etc. U-Boot is used
for bring-up of new hardware.

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

I see this as new behaviour. I would be happy to enable it as an
option, presumably at the bus level, with a new flag (although not in
this series).

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

It does exist in driver model, just not on the bus (yet). The driver
model probe() method has been called, and if someone connected a
device to the I2C pins then it would start working. There is the
option to use 'i2c probe' to get the behaviour you are looking for. I
can't understand why you would not use that in this case.

Regards,
Simon