[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 at ti.com
Mon Nov 5 09:38:10 UTC 2018
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.
>
> 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
>
More information about the U-Boot
mailing list