Hi Kieran,
On Tue, Jan 07, 2020 at 09:40:35AM +0000, Kieran Bingham wrote:
> On 31/12/2019 16:13, 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;
> > }
> >
> > +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);
>
> Are all 'known' devices on a bus (all the ones declared in DT etc)
> marked as 'busy' or taken by the time this call is made? (edit, I don't
> think they are)
>
> Perhaps this is a constructed corner case, but I'm just trying to follow
> it through:
>
> I.e. if say the adv748x had in DT defined aliases at 0x08, 0x09,
> 0x0A..., but not yet probed (thus no device is listening at these
> addresses) ... and then a max9286 came along and asked for 'any' spare
> address with this call, would it be given 0x08 first?
>
> If so (which I think is what the case would be currently, until I'm
> pointed otherwise) do we need to mark all addresses on the bus as
> reserved against this some how?
>
> I'm not sure how that would occur, as it would be up to the adv748x in
> that instance to parse it's extended register list to identify the extra
> aliases it will create, *and* that would only happen if the device
> driver was enabled in the first place.
>
> So this seems a bit 'racy' in a different context; not the i2c_lock_bus,
> but rather the probe order of devices on the bus could affect the
> allocations.
>
> Perhaps that is unavoidable though...
But it's a real problem... Could the I2C core parse all the addresses on
the bus before probing drivers ?
> > + if (ret == -ENODEV) {
> > + alias = i2c_new_dummy_device(adap, addr);
> > + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr);
> > + break;
> > + }
> > + }
> > +
> > + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> > + return alias;
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_new_alias_device);
> > +
> > int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr)
> > {
> > return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index f834687989f7..583ca2aec022 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -441,6 +441,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info);
> > struct i2c_client *
> > i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info);
> >
> > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap);
> > +
> > /* If you don't know the exact address of an I2C device, use this variant
> > * instead, which can probe for device presence in a list of possible
> > * addresses. The "probe" callback function is optional. If it is provided,
--
Regards,
Laurent Pinchart
