[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 5 November 2018 at 02:38, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
> 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 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.
>
>
> That is the development model of many open source projects that made their
> success. So I don't mind.

OK good :-)

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

It costs time and is pointless if the chip is present. On the other
hand, this function is presumably only used when there is nothing int
the DT, so perhaps people don't care.

>
> And I am not sure that the all drivers detect the chip in their probe()
> functions.

Agreed, I suspect that many don't, although I have not looked.

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

OK that all sounds fine to me. For the last case I think it makes
sense to have a function that the platform has to call.

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

OK I see. Your patch is for the other case.

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

Yes that seems somewhat equivalent.

At this point, I think we should take this patch, with a few changes:

1. Rename i2c_bind_driver() to i2c_bind_and_probe_driver(), since it
always probes, and add a function comment (should be a separate patch)

2. Update the header file comment for i2c_get_chip_for_busnum() to
describe exactly what it does

3. Expand the commit message to explain that this function is only
used in the non-DT case, and that it is probed on the bus before
binding the driver...

Regards,
Simon