Re: [RFC PATCH 3/5] i2c: core: add function to request an alias

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

Hi Laurent,

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

Well, we have to start somewhere? And actually, the range from 0x08
onwards is the least used I encountered so far. What would you suggest?

> 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. Another call to
> i2c_new_alias_device() could occur in-between.

This is why the whole function is protected using i2c_lock_bus. No other
device can scan simultaneously. And once we have the dummy device
registered, it is blocked like any other registered device. As we
register the dummy device with the lock being held, I don't see how the
above race can happen.

> There's also the issue
> that I2C hasn't been designed for scanning, so some devices may not
> appreciate this.

This is inevitable. What GMSL and FPD-Link do is very non-I2Cish. I
don't see a "perfect" solution. We could skip this transaction and rely
only on that all devices are pre-registered. Yet, I think the
requirement that all busses *must* be fully described is more dangerous.
I'd rather risk that some rare device doesn't like the transaction. I
think a byte_read is the best we can do. Much better than a quick
command, for sure.

> What happened to the idea of reporting busy address ranges in the
> firmware (DT, ACPI, ...) ?

Fully in place. All pre-registered devices won't be considered because
i2c_scan_for_client() uses i2c_check_addr_busy(). Did you think the new
helper only relies on the transfer sent out? This is not the case, the
transfer is only the final safety measure for so far unclaimed
addresses.

All the best for 2020,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]