[U-Boot] [PATCH v3 02/19] dm: i2c: Make i2c_get_chip_for_busnum() fail if the chip is not detected

[Simon Glass]

Hi Jean-Jacques,

On 22 October 2018 at 08:12, Jean-Jacques Hiblot <jjhiblot at 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 <jjhiblot at 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.

If the chip is not present that is generally an error, or at least it
is assumed to be by the callers.

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.

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

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.

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?

>         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