Hi Masahiro,
On 4 December 2014 at 00:24, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
On Wed, 3 Dec 2014 19:32:18 -0700 Simon Glass sjg@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@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.
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@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
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