Hi Wolfram,
thanks for having started working on this!
On 01/01/20 17:55, Laurent Pinchart wrote:
> Hi Wolfram,
>
> Thank you for the patch.
>
> On Tue, Dec 31, 2019 at 05:13:58PM +0100, Wolfram Sang wrote:
>> Some devices are able to reprogram their I2C address at runtime. This
>> can prevent address collisions when one is able to activate and
>> reprogram these devices one by one. For that to work, they need to be
>> assigned an unused address. This new functions allows drivers to request
>> for such an address. It assumes all non-occupied addresses are free. It
>> will then send a message to such a free address to make sure there is
>> really nothing listening there.
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++
>> include/linux/i2c.h | 2 ++
>> 2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 51bd953ddfb2..5a010e7e698f 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
>> return err;
>> }
>>
>
> Missing kerneldoc, but you already know about this.
>
>> +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap)
>> +{
>> + struct i2c_client *alias = ERR_PTR(-EBUSY);
>> + int ret;
>> + u16 addr;
>> +
>> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
>> +
>> + for (addr = 0x08; addr < 0x78; addr++) {
>> + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe);
>> + if (ret == -ENODEV) {
>> + alias = i2c_new_dummy_device(adap, addr);
>> + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr);
>> + break;
>> + }
>> + }
>
> This looks quite inefficient, especially if the beginning of the range
> is populated with devices. Furthermore, I think there's a high risk of
> false negatives, as acquiring a free address and reprogramming the
> client to make use of it are separate operations.
Right. Applying the alias could raise other errors, thus one would need
i2c_new_alias_device() to keep the alias locked until programming it has
either failed or has been successfully programmed.
> Another call to
> i2c_new_alias_device() could occur in-between. There's also the issue
> that I2C hasn't been designed for scanning, so some devices may not
> appreciate this.
>
> What happened to the idea of reporting busy address ranges in the
> firmware (DT, ACPI, ...) ?
Indeed that's how I remember it as well, and I'm a bit suspicious about
sending out probe messages that might have side effects (even if the
false negative issue mentioned by Laurent were solved). You know, I've
been taught to "expect the worse" :) so I'd like to better understand
what are the strong reasons in favor of probing, as well as the
potential side effects.
--
Luca
