| Message ID | 20191231161400.1688-5-wsa+renesas@sang-engineering.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | i2c: implement mechanism to retrieve an alias device | expand |
Hi Wolfram, On 31/12/2019 16:13, Wolfram Sang wrote: > Add some simple caching of the last used alias to skip some unneeded > scanning of the I2C bus. When freeing the device, the cache will be set > back. Seems simplistic enough and would do the job, given the current implementation... Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 15 ++++++++++++++- > include/linux/i2c.h | 2 ++ > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 5a010e7e698f..0cc4a5c49a15 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -830,6 +830,8 @@ EXPORT_SYMBOL_GPL(i2c_new_device); > */ > void i2c_unregister_device(struct i2c_client *client) > { > + struct i2c_adapter *adap = client->adapter; > + > if (IS_ERR_OR_NULL(client)) > return; > > @@ -840,6 +842,14 @@ void i2c_unregister_device(struct i2c_client *client) > > if (ACPI_COMPANION(&client->dev)) > acpi_device_clear_enumerated(ACPI_COMPANION(&client->dev)); > + > + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > + > + if (client->flags & I2C_CLIENT_ALIAS && client->addr < adap->alias_idx) Phew, I thought on the first read we were going to end up shrinking the available alias pool when we unregister devices with high addresses, (I missed the client->addr < adap->alias_idx statement first) > + adap->alias_idx = client->addr; > + > + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); > + > device_unregister(&client->dev); > } > EXPORT_SYMBOL_GPL(i2c_unregister_device); > @@ -1297,6 +1307,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > adap->lock_ops = &i2c_adapter_lock_ops; > > adap->locked_flags = 0; > + adap->alias_idx = 0x08; /* first valid I2C address */ > rt_mutex_init(&adap->bus_lock); > rt_mutex_init(&adap->mux_lock); > mutex_init(&adap->userspace_clients_lock); > @@ -2249,10 +2260,12 @@ struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) > > i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > > - for (addr = 0x08; addr < 0x78; addr++) { > + for (addr = adap->alias_idx; 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); > + alias->flags |= I2C_CLIENT_ALIAS; Potential here for a comment to clarify the alias_idx, but it's not required: /* Prevent re-scanning occupied addresses */ > + adap->alias_idx = addr + 1; > dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > break; > } > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 583ca2aec022..6427c2db5ee0 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -309,6 +309,7 @@ struct i2c_driver { > struct i2c_client { > unsigned short flags; /* div., see below */ > #define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */ > +#define I2C_CLIENT_ALIAS 0x08 /* client is an alias */ > #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */ Unrelated to this patch, but I love that the flag for CLIENT_TEN is 0x10 :-D > /* Must equal I2C_M_TEN below */ > #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */ > @@ -715,6 +716,7 @@ struct i2c_adapter { > const struct i2c_adapter_quirks *quirks; > > struct irq_domain *host_notify_domain; > + u16 alias_idx; > }; > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) > >
On 2019-12-31 17:13, Wolfram Sang wrote: > Add some simple caching of the last used alias to skip some unneeded > scanning of the I2C bus. When freeing the device, the cache will be set > back. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 15 ++++++++++++++- > include/linux/i2c.h | 2 ++ > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 5a010e7e698f..0cc4a5c49a15 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -830,6 +830,8 @@ EXPORT_SYMBOL_GPL(i2c_new_device); > */ > void i2c_unregister_device(struct i2c_client *client) > { > + struct i2c_adapter *adap = client->adapter; > + > if (IS_ERR_OR_NULL(client)) > return; > > @@ -840,6 +842,14 @@ void i2c_unregister_device(struct i2c_client *client) > > if (ACPI_COMPANION(&client->dev)) > acpi_device_clear_enumerated(ACPI_COMPANION(&client->dev)); > + > + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > + > + if (client->flags & I2C_CLIENT_ALIAS && client->addr < adap->alias_idx) > + adap->alias_idx = client->addr; > + > + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); > + > device_unregister(&client->dev); > } > EXPORT_SYMBOL_GPL(i2c_unregister_device); > @@ -1297,6 +1307,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > adap->lock_ops = &i2c_adapter_lock_ops; > > adap->locked_flags = 0; > + adap->alias_idx = 0x08; /* first valid I2C address */ > rt_mutex_init(&adap->bus_lock); > rt_mutex_init(&adap->mux_lock); > mutex_init(&adap->userspace_clients_lock); > @@ -2249,10 +2260,12 @@ struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) > > i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > > - for (addr = 0x08; addr < 0x78; addr++) { > + for (addr = adap->alias_idx; 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); > + alias->flags |= I2C_CLIENT_ALIAS; > + adap->alias_idx = addr + 1; > dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > break; > } > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 583ca2aec022..6427c2db5ee0 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -309,6 +309,7 @@ struct i2c_driver { > struct i2c_client { > unsigned short flags; /* div., see below */ > #define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */ > +#define I2C_CLIENT_ALIAS 0x08 /* client is an alias */ > #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */ > /* Must equal I2C_M_TEN below */ > #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */ > @@ -715,6 +716,7 @@ struct i2c_adapter { > const struct i2c_adapter_quirks *quirks; > > struct irq_domain *host_notify_domain; > + u16 alias_idx; > }; > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) > > If you have a situation where a driver for a device with reprogrammable I2C address is, say, built as a module and is reloaded repeatedly for some reason, and also have the situation that the device keeps responding to the last used address even after unloading the driver. Then I think you have a problem, because as I read the code the alias address will increase with every reload, and eventually hit the 0x78 roof. No? Cheers, Peter
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 5a010e7e698f..0cc4a5c49a15 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -830,6 +830,8 @@ EXPORT_SYMBOL_GPL(i2c_new_device); */ void i2c_unregister_device(struct i2c_client *client) { + struct i2c_adapter *adap = client->adapter; + if (IS_ERR_OR_NULL(client)) return; @@ -840,6 +842,14 @@ void i2c_unregister_device(struct i2c_client *client) if (ACPI_COMPANION(&client->dev)) acpi_device_clear_enumerated(ACPI_COMPANION(&client->dev)); + + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); + + if (client->flags & I2C_CLIENT_ALIAS && client->addr < adap->alias_idx) + adap->alias_idx = client->addr; + + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); + device_unregister(&client->dev); } EXPORT_SYMBOL_GPL(i2c_unregister_device); @@ -1297,6 +1307,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap) adap->lock_ops = &i2c_adapter_lock_ops; adap->locked_flags = 0; + adap->alias_idx = 0x08; /* first valid I2C address */ rt_mutex_init(&adap->bus_lock); rt_mutex_init(&adap->mux_lock); mutex_init(&adap->userspace_clients_lock); @@ -2249,10 +2260,12 @@ struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) i2c_lock_bus(adap, I2C_LOCK_SEGMENT); - for (addr = 0x08; addr < 0x78; addr++) { + for (addr = adap->alias_idx; 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); + alias->flags |= I2C_CLIENT_ALIAS; + adap->alias_idx = addr + 1; dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); break; } diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 583ca2aec022..6427c2db5ee0 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -309,6 +309,7 @@ struct i2c_driver { struct i2c_client { unsigned short flags; /* div., see below */ #define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */ +#define I2C_CLIENT_ALIAS 0x08 /* client is an alias */ #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */ /* Must equal I2C_M_TEN below */ #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */ @@ -715,6 +716,7 @@ struct i2c_adapter { const struct i2c_adapter_quirks *quirks; struct irq_domain *host_notify_domain; + u16 alias_idx; }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
Add some simple caching of the last used alias to skip some unneeded scanning of the I2C bus. When freeing the device, the cache will be set back. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-base.c | 15 ++++++++++++++- include/linux/i2c.h | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-)