Hi Simon,
On 03/11/2018 07:07, Simon Glass wrote:
Hi Jean-Jacques,
On 22 October 2018 at 08:12, Jean-Jacques Hiblot <jjhib...@ti.com> wrote:
i2c_get_chip_for_busnum() really should check the presence of the chip on
the bus. Most of the users of this function assume that this is done.
Signed-off-by: Jean-Jacques Hiblot <jjhib...@ti.com>
---
Changes in v3:
- removed commit introducing dm_i2c_probe_device(). Instead probe the
presence of the chip on the I2C bus in i2c_get_chip_for_busnum().
Changes in v2: None
drivers/i2c/i2c-uclass.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
index c5a3c4e..975318e 100644
--- a/drivers/i2c/i2c-uclass.c
+++ b/drivers/i2c/i2c-uclass.c
@@ -347,6 +347,17 @@ int i2c_get_chip_for_busnum(int busnum, int chip_addr,
uint offset_len,
debug("Cannot find I2C bus %d\n", busnum);
return ret;
}
+
+ /* detect the presence of the chip on the bus */
+ ret = i2c_probe_chip(bus, chip_addr, 0);
+ debug("%s: bus='%s', address %02x, ret=%d\n", __func__, bus->name,
+ chip_addr, ret);
+ if (ret) {
+ debug("Cannot detect I2C chip %02x on bus %d\n", chip_addr,
+ busnum);
+ return ret;
+ }
+
I really don't like to keep going on about this, but I think this is
still not quite right.
That is the development model of many open source projects that made
their success. So I don't mind.
If the chip is not present that is generally an error, or at least it
is assumed to be by the callers.
agreed. That is the purpose of the modification
i2c_get_chip() is called just above the code you add. This normally
probes an existing device at that i2c address, which presumably checks
that it is present. However if there is no device at the given
address, it calls device_bind():
debug("%s: Searching bus '%s' for address %02x: ", __func__,
bus->name, chip_addr);
for (device_find_first_child(bus, &dev); dev;
device_find_next_child(&dev)) {
struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
int ret;
if (chip->chip_addr == chip_addr) {
ret = device_probe(dev);
debug("found, ret=%d\n", ret);
f (ret)
return ret;
*devp = dev;
return 0;
}
}
debug("not found\n");
return i2c_bind_driver(bus, chip_addr, offset_len, devp);
So I think your new code below should only be used in the case where
device_bind() was called.
I don't think it hurts to probe the presence of the chip after the
device_probe() is called.
And I am not sure that the all drivers detect the chip in their probe()
functions.
What is the case where you:
a) Don't put the I2C device in the device tree
b) Expect it to be bound
c) Want to know if it is not present
This is exactly the use case I'm trying to address. There are several
possible cases:
- Some platforms haven't gone full DM yet and don't put the I2C device
in the DT.
- Other just don't have a DM I2C driver for the chip and only send a few
commands to initialize it. Adding a real I2C-driver may be overkill.
- Other platforms rely on the detection of external devices to further
identify the platform with no need to actually use the external chip.
Looking at it closely, i2c_get_chip() is not consistent. It probes the
device it is is already bound, but if not, it binds a device and then
fails to probe it. I think it should call device_prove() after binding
it.
Calling device_probe() would work only if a DM I2C driver exists for the
chip and if a description exists in the DT. So I don't think it would
work for the cases I mentioned above.
Would that be good enough?
Otherwise, perhaps we just need a function like:
i2c_probe_device(struct udevice *dev)
which calls i2c_probe_chip() on the appropriate address?
That works, it was what was done in the first version. But I think this
version is better as it doesn't introduce a new function and make
i2c_get_chip_for_busnum() behave as people expect it to behave: fail if
the chip is not there. The cost is a systematic and sometimes
unnecessary I2C probe sequence. That penalty is small.
There is another solution: add a probe() function to the
i2c_generic_chip_drv driver that fails if it doesn't detect the chip. It
would do the job.
ret = i2c_get_chip(bus, chip_addr, offset_len, devp);
if (ret) {
debug("Cannot find I2C chip %02x on bus %d\n", chip_addr,
--
2.7.4
Again I'm sorry for all the back and forth. I think it would help to
have more information in the commit message about the circumstances in
which this change is needed.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
