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

[Jean-Jacques Hiblot <jjhiblot@ti.com>]

Hi Simon,

On 03/11/2018 07:07, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On 22 October 2018 at 08:12, Jean-Jacques Hiblot <jjhiblot@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@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
>