[RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device

[RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device

[Wolfram Sang]

Finally, here is my prototype to request an unused I2C address at
runtime. This is needed by serial/deserializer pairs like GMSL or
FPD-Link. This is the first outcome of various discussions how to
support the above devices.

It is only for obtaining the alias. I started sketching actual use cases
for GMSL and FPD-Link, but they should not be part of this series. I am
eager to get feedback on this, if this really matches what we discussed
and if it is useful for downstream users as-is.

I don't have GMSL hardware myself, so the last patch is a simple
testcase which can be copied to any I2C driver you probe during boot.

More testing is still needed, especially regression testing for the
refactored functions in patch 1. kdocs are missing, too. I'd like some
feedback on the approach first.

While I think some kind of caching is useful, I am not super-happy with
the actual implementation in patch 4. The use of another I2C_CLIENT_*
flag just for the caching looks a bit too much to me. Still, I wanted to
keep it simple for the users of this mechanism and not enforce a
'return-the-alias' function call.

Anyhow, I hope this series is useful to you and will spawn fruitful
discussion.

Thanks for all your cooperation and working together in 2019. I am
looking forward to continue that path in 2020.

Have a great new year, everyone,

   Wolfram


Wolfram Sang (5):
  i2c: core: refactor scanning for a client
  i2c: core: add new variant to check for a client
  i2c: core: add function to request an alias
  i2c: core: add simple caching to the 'alias' scanning
  simple test case for the I2C alias functionality

 drivers/i2c/i2c-core-base.c | 109 +++++++++++++++++++++++++-----------
 include/linux/i2c.h         |   4 ++
 sound/soc/codecs/ak4642.c   |  20 +++++++
 3 files changed, 101 insertions(+), 32 deletions(-)

-- 
2.20.1

[RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Wolfram Sang]

There is a pattern to check for existence of a client which is copied in
i2c_detect_address() and i2c_new_scanned_device():

1) check if address is valid
2) check if address is already registered
3) send a message and check the reponse

Because this pattern will be needed a third time soon, refactor it into
its own function.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 57 ++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index a1eb28a3cc54..20a726dc78db 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2108,29 +2108,39 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
 	return err >= 0;
 }
 
-static int i2c_detect_address(struct i2c_client *temp_client,
-			      struct i2c_driver *driver)
+static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
+			    int (*probe)(struct i2c_adapter *adap, unsigned short addr))
 {
-	struct i2c_board_info info;
-	struct i2c_adapter *adapter = temp_client->adapter;
-	int addr = temp_client->addr;
 	int err;
 
 	/* Make sure the address is valid */
 	err = i2c_check_7bit_addr_validity_strict(addr);
-	if (err) {
-		dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
-			 addr);
+	if (WARN(err, "Invalid probe address 0x%02x\n", addr))
 		return err;
-	}
 
 	/* Skip if already in use (7 bit, no need to encode flags) */
-	if (i2c_check_addr_busy(adapter, addr))
-		return 0;
+	if (i2c_check_addr_busy(adap, addr))
+		return -EBUSY;
 
 	/* Make sure there is something at this address */
-	if (!i2c_default_probe(adapter, addr))
-		return 0;
+	if (!probe(adap, addr))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int i2c_detect_address(struct i2c_client *temp_client,
+			      struct i2c_driver *driver)
+{
+	struct i2c_board_info info;
+	struct i2c_adapter *adapter = temp_client->adapter;
+	int addr = temp_client->addr;
+	int err;
+
+	/* Only report broken addresses, busy addresses are no error */
+	err = i2c_scan_for_client(adapter, addr, i2c_default_probe);
+	if (err < 0)
+		return err == -EINVAL ? -EINVAL : 0;
 
 	/* Finally call the custom detection function */
 	memset(&info, 0, sizeof(struct i2c_board_info));
@@ -2232,26 +2242,9 @@ i2c_new_scanned_device(struct i2c_adapter *adap,
 	if (!probe)
 		probe = i2c_default_probe;
 
-	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
-		/* Check address validity */
-		if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) {
-			dev_warn(&adap->dev, "Invalid 7-bit address 0x%02x\n",
-				 addr_list[i]);
-			continue;
-		}
-
-		/* Check address availability (7 bit, no need to encode flags) */
-		if (i2c_check_addr_busy(adap, addr_list[i])) {
-			dev_dbg(&adap->dev,
-				"Address 0x%02x already in use, not probing\n",
-				addr_list[i]);
-			continue;
-		}
-
-		/* Test address responsiveness */
-		if (probe(adap, addr_list[i]))
+	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++)
+		if (i2c_scan_for_client(adap, addr_list[i], probe) == 0)
 			break;
-	}
 
 	if (addr_list[i] == I2C_CLIENT_END) {
 		dev_dbg(&adap->dev, "Probing failed, no device found\n");
-- 
2.20.1

[RFC PATCH 2/5] i2c: core: add new variant to check for a client

[Wolfram Sang]

For the new 'alias' feature, we need to scan for devices while holding
the lock. We focus on read_byte transactions for now to keep things
simple. Requesting an alias will be rare, so there is not much overhead.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 20a726dc78db..51bd953ddfb2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2108,6 +2108,23 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
 	return err >= 0;
 }
 
+static int i2c_unlocked_read_byte_probe(struct i2c_adapter *adap, unsigned short addr)
+{
+	union i2c_smbus_data dummy;
+	int err;
+
+	if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
+		err = __i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+				     I2C_SMBUS_BYTE, &dummy);
+	} else {
+		dev_warn(&adap->dev, "No suitable probing method supported for address 0x%02X\n",
+			 addr);
+		err = -EOPNOTSUPP;
+	}
+
+	return err >= 0;
+}
+
 static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
 			    int (*probe)(struct i2c_adapter *adap, unsigned short addr))
 {
-- 
2.20.1

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

[Wolfram Sang]

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@sang-engineering.com>
---
 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);
+		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,
-- 
2.20.1

[RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning

[Wolfram Sang]

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)
 
-- 
2.20.1

[RFC PATCH 5/5] simple test case for the I2C alias functionality

[Wolfram Sang]

Not for upstream!

Not-Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  2 +-
 sound/soc/codecs/ak4642.c   | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 0cc4a5c49a15..97d3e9f8dfa7 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1307,7 +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 */
+	adap->alias_idx = 0x3e;	/* first valid I2C address */
 	rt_mutex_init(&adap->bus_lock);
 	rt_mutex_init(&adap->mux_lock);
 	mutex_init(&adap->userspace_clients_lock);
diff --git a/sound/soc/codecs/ak4642.c b/sound/soc/codecs/ak4642.c
index 353237025514..d34476b80b41 100644
--- a/sound/soc/codecs/ak4642.c
+++ b/sound/soc/codecs/ak4642.c
@@ -639,6 +639,25 @@ static int ak4642_i2c_probe(struct i2c_client *i2c,
 	struct regmap *regmap;
 	struct ak4642_priv *priv;
 	struct clk *mcko = NULL;
+struct i2c_client *test, *test2, *test3;
+
+test = i2c_new_alias_device(i2c->adapter);
+printk(KERN_INFO "****** wsa: %08x %08x\n", test->addr, i2c->adapter->alias_idx);
+
+test2 = i2c_new_alias_device(i2c->adapter);
+printk(KERN_INFO "****** wsa: %08x %08x\n", test2->addr, i2c->adapter->alias_idx);
+
+//i2c_unregister_device(test2);
+//printk(KERN_INFO "****** wsa: %08x %08x\n", test2->addr, i2c->adapter->alias_idx);
+
+i2c_unregister_device(test);
+printk(KERN_INFO "****** wsa: %08x %08x\n", test->addr, i2c->adapter->alias_idx);
+
+test = i2c_new_alias_device(i2c->adapter);
+printk(KERN_INFO "****** wsa: %08x %08x\n", test->addr, i2c->adapter->alias_idx);
+
+test3 = i2c_new_alias_device(i2c->adapter);
+printk(KERN_INFO "****** wsa: %08x %08x\n", test3->addr, i2c->adapter->alias_idx);
 
 	if (np) {
 		const struct of_device_id *of_id;
@@ -659,6 +678,7 @@ static int ak4642_i2c_probe(struct i2c_client *i2c,
 		return -EINVAL;
 	}
 
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-- 
2.20.1

Re: [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device

[Wolfram Sang]

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


> Anyhow, I hope this series is useful to you and will spawn fruitful
> discussion.

A branch is here (it depends on i2c/for-next):

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c_alias_device

and buildbot is happy, too.


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

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Tue, Dec 31, 2019 at 05:13:56PM +0100, Wolfram Sang wrote:
> There is a pattern to check for existence of a client which is copied in
> i2c_detect_address() and i2c_new_scanned_device():
> 
> 1) check if address is valid
> 2) check if address is already registered
> 3) send a message and check the reponse
> 
> Because this pattern will be needed a third time soon, refactor it into
> its own function.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 57 ++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index a1eb28a3cc54..20a726dc78db 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2108,29 +2108,39 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
>  	return err >= 0;
>  }
>  
> -static int i2c_detect_address(struct i2c_client *temp_client,
> -			      struct i2c_driver *driver)
> +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
> +			    int (*probe)(struct i2c_adapter *adap, unsigned short addr))
>  {
> -	struct i2c_board_info info;
> -	struct i2c_adapter *adapter = temp_client->adapter;
> -	int addr = temp_client->addr;
>  	int err;
>  
>  	/* Make sure the address is valid */
>  	err = i2c_check_7bit_addr_validity_strict(addr);
> -	if (err) {
> -		dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
> -			 addr);
> +	if (WARN(err, "Invalid probe address 0x%02x\n", addr))

Does this deserve a full backtrace ? If so could you mention it in the
commit message ?

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		return err;
> -	}
>  
>  	/* Skip if already in use (7 bit, no need to encode flags) */
> -	if (i2c_check_addr_busy(adapter, addr))
> -		return 0;
> +	if (i2c_check_addr_busy(adap, addr))
> +		return -EBUSY;
>  
>  	/* Make sure there is something at this address */
> -	if (!i2c_default_probe(adapter, addr))
> -		return 0;
> +	if (!probe(adap, addr))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int i2c_detect_address(struct i2c_client *temp_client,
> +			      struct i2c_driver *driver)
> +{
> +	struct i2c_board_info info;
> +	struct i2c_adapter *adapter = temp_client->adapter;
> +	int addr = temp_client->addr;
> +	int err;
> +
> +	/* Only report broken addresses, busy addresses are no error */
> +	err = i2c_scan_for_client(adapter, addr, i2c_default_probe);
> +	if (err < 0)
> +		return err == -EINVAL ? -EINVAL : 0;
>  
>  	/* Finally call the custom detection function */
>  	memset(&info, 0, sizeof(struct i2c_board_info));
> @@ -2232,26 +2242,9 @@ i2c_new_scanned_device(struct i2c_adapter *adap,
>  	if (!probe)
>  		probe = i2c_default_probe;
>  
> -	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
> -		/* Check address validity */
> -		if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) {
> -			dev_warn(&adap->dev, "Invalid 7-bit address 0x%02x\n",
> -				 addr_list[i]);
> -			continue;
> -		}
> -
> -		/* Check address availability (7 bit, no need to encode flags) */
> -		if (i2c_check_addr_busy(adap, addr_list[i])) {
> -			dev_dbg(&adap->dev,
> -				"Address 0x%02x already in use, not probing\n",
> -				addr_list[i]);
> -			continue;
> -		}
> -
> -		/* Test address responsiveness */
> -		if (probe(adap, addr_list[i]))
> +	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++)
> +		if (i2c_scan_for_client(adap, addr_list[i], probe) == 0)
>  			break;
> -	}
>  
>  	if (addr_list[i] == I2C_CLIENT_END) {
>  		dev_dbg(&adap->dev, "Probing failed, no device found\n");

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/5] i2c: core: add new variant to check for a client

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Tue, Dec 31, 2019 at 05:13:57PM +0100, Wolfram Sang wrote:
> For the new 'alias' feature, we need to scan for devices while holding
> the lock. We focus on read_byte transactions for now to keep things
> simple. Requesting an alias will be rare, so there is not much overhead.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 20a726dc78db..51bd953ddfb2 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2108,6 +2108,23 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
>  	return err >= 0;
>  }
>  
> +static int i2c_unlocked_read_byte_probe(struct i2c_adapter *adap, unsigned short addr)
> +{
> +	union i2c_smbus_data dummy;
> +	int err;
> +
> +	if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
> +		err = __i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +				     I2C_SMBUS_BYTE, &dummy);
> +	} else {
> +		dev_warn(&adap->dev, "No suitable probing method supported for address 0x%02X\n",

This should be wrapped after the first argument.

s/02X/02x/

As this function is static and unused in this patch, this may introduce
a new warning when bisection. I see little reason to keep this patch
standalone, you can squash it with 3/5. It otherwise looks good to me.

> +			 addr);
> +		err = -EOPNOTSUPP;
> +	}
> +
> +	return err >= 0;
> +}
> +
>  static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
>  			    int (*probe)(struct i2c_adapter *adap, unsigned short addr))
>  {

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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@sang-engineering.com>
> ---
>  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. 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, ...) ?

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

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

[Luca Ceresoli]

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@sang-engineering.com>
>> ---
>>  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

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

[Wolfram Sang]

[-- 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 --]

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

[Wolfram Sang]

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

Hi Luca,

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

Please see my reply to Laurent, I don't think it is racy. But please
elaborate if you think I am wrong.

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

As I said to Laurent, too, I think the risk that a bus is not fully
described is higher than a device which does not respond to a read_byte.
In both cases, we would wrongly use an address in use.

Also, all the best for you in 2020!

   Wolfram


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

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

[Luca Ceresoli]

Hi Wolfram,

On 02/01/20 22:13, Wolfram Sang wrote:
> Hi Luca,
> 
>>> 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.
> 
> Please see my reply to Laurent, I don't think it is racy. But please
> elaborate if you think I am wrong.

Uhm, you are right here, it's not racy. Sorry, I had read the code
quickly and didn't notice the i2c_new_dummy_device() call.

So this means if i2c_new_alias_device() succeeds but the caller later
fails while applying the alias, then it has to call
i2c_unregister_device() to free the alias. Correct?

>>> 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.
> 
> As I said to Laurent, too, I think the risk that a bus is not fully
> described is higher than a device which does not respond to a read_byte.
> In both cases, we would wrongly use an address in use.

OK, I'm still uncomfortable with sending unexpected transactions to the
dark outer space, but this is more a feeling than based on facts, and
you know more than me, so I guess I can live with that.

> Also, all the best for you in 2020!

Thanks. Best wishes to you too for the new year!

-- 
Luca

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

[Laurent Pinchart]

On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote:
> Hi Wolfram,
> 
> On 02/01/20 22:13, Wolfram Sang wrote:
> > Hi Luca,
> > 
> >>> 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.
> > 
> > Please see my reply to Laurent, I don't think it is racy. But please
> > elaborate if you think I am wrong.
> 
> Uhm, you are right here, it's not racy. Sorry, I had read the code
> quickly and didn't notice the i2c_new_dummy_device() call.
> 
> So this means if i2c_new_alias_device() succeeds but the caller later
> fails while applying the alias, then it has to call
> i2c_unregister_device() to free the alias. Correct?

I was wrong as well, sorry about that.

> >>> 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.
> > 
> > As I said to Laurent, too, I think the risk that a bus is not fully
> > described is higher than a device which does not respond to a read_byte.
> > In both cases, we would wrongly use an address in use.

I don't fully agree with this, I think we shouldn't impose a penalty on
every user because some device trees don't fully describe the hardware.
I think we should, at the very least, skip the probe and rely on DT if
DT explicitly states that all used addresses are listed. We discussed a
property to report addresses used by devices not described in DT, if
that property is listed I would prefer trusting DT.

> OK, I'm still uncomfortable with sending unexpected transactions to the
> dark outer space, but this is more a feeling than based on facts, and
> you know more than me, so I guess I can live with that.
> 
> > Also, all the best for you in 2020!
> 
> Thanks. Best wishes to you too for the new year!

Likewise. Let's start with a simple wish of getting this issue resolved
:-)


-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Kieran Bingham]

Hi Wolfram,

On 31/12/2019 16:13, Wolfram Sang wrote:
> There is a pattern to check for existence of a client which is copied in
> i2c_detect_address() and i2c_new_scanned_device():
> 
> 1) check if address is valid
> 2) check if address is already registered
> 3) send a message and check the reponse

s/reponse/response/
   (My email client highlights spelling issues, sorry :-D)


> Because this pattern will be needed a third time soon, refactor it into
> its own function.

This looks reasonable to me, I see Laurent has a concern over the use of
a WARN to present a backtrace, but I think in this instance it will be
useful as it will facilitate identifying what code path provided the
incorrect address.

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 | 57 ++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index a1eb28a3cc54..20a726dc78db 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2108,29 +2108,39 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
>  	return err >= 0;
>  }
>  
> -static int i2c_detect_address(struct i2c_client *temp_client,
> -			      struct i2c_driver *driver)
> +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
> +			    int (*probe)(struct i2c_adapter *adap, unsigned short addr))
>  {
> -	struct i2c_board_info info;
> -	struct i2c_adapter *adapter = temp_client->adapter;
> -	int addr = temp_client->addr;
>  	int err;
>  
>  	/* Make sure the address is valid */
>  	err = i2c_check_7bit_addr_validity_strict(addr);
> -	if (err) {
> -		dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
> -			 addr);
> +	if (WARN(err, "Invalid probe address 0x%02x\n", addr))
>  		return err;
> -	}
>  
>  	/* Skip if already in use (7 bit, no need to encode flags) */
> -	if (i2c_check_addr_busy(adapter, addr))
> -		return 0;
> +	if (i2c_check_addr_busy(adap, addr))
> +		return -EBUSY;
>  
>  	/* Make sure there is something at this address */
> -	if (!i2c_default_probe(adapter, addr))
> -		return 0;
> +	if (!probe(adap, addr))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int i2c_detect_address(struct i2c_client *temp_client,
> +			      struct i2c_driver *driver)
> +{
> +	struct i2c_board_info info;
> +	struct i2c_adapter *adapter = temp_client->adapter;
> +	int addr = temp_client->addr;
> +	int err;
> +
> +	/* Only report broken addresses, busy addresses are no error */
> +	err = i2c_scan_for_client(adapter, addr, i2c_default_probe);
> +	if (err < 0)
> +		return err == -EINVAL ? -EINVAL : 0;
>  
>  	/* Finally call the custom detection function */
>  	memset(&info, 0, sizeof(struct i2c_board_info));
> @@ -2232,26 +2242,9 @@ i2c_new_scanned_device(struct i2c_adapter *adap,
>  	if (!probe)
>  		probe = i2c_default_probe;
>  
> -	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
> -		/* Check address validity */
> -		if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) {
> -			dev_warn(&adap->dev, "Invalid 7-bit address 0x%02x\n",
> -				 addr_list[i]);
> -			continue;
> -		}
> -
> -		/* Check address availability (7 bit, no need to encode flags) */
> -		if (i2c_check_addr_busy(adap, addr_list[i])) {
> -			dev_dbg(&adap->dev,
> -				"Address 0x%02x already in use, not probing\n",
> -				addr_list[i]);
> -			continue;
> -		}
> -
> -		/* Test address responsiveness */
> -		if (probe(adap, addr_list[i]))
> +	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++)
> +		if (i2c_scan_for_client(adap, addr_list[i], probe) == 0)
>  			break;
> -	}
>  
>  	if (addr_list[i] == I2C_CLIENT_END) {
>  		dev_dbg(&adap->dev, "Probing failed, no device found\n");
> 

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

[Kieran Bingham]

Hi Wolfram,

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@sang-engineering.com>
> ---
>  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...

--
Kieran


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

Re: [RFC PATCH 2/5] i2c: core: add new variant to check for a client

[Kieran Bingham]

Hi Wolfram,

On 31/12/2019 16:13, Wolfram Sang wrote:
> For the new 'alias' feature, we need to scan for devices while holding
> the lock. We focus on read_byte transactions for now to keep things
> simple. Requesting an alias will be rare, so there is not much overhead.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 20a726dc78db..51bd953ddfb2 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2108,6 +2108,23 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr)
>  	return err >= 0;
>  }
>  
> +static int i2c_unlocked_read_byte_probe(struct i2c_adapter *adap, unsigned short addr)
> +{
> +	union i2c_smbus_data dummy;
> +	int err;
> +
> +	if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_BYTE)) {
> +		err = __i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +				     I2C_SMBUS_BYTE, &dummy);
> +	} else {
> +		dev_warn(&adap->dev, "No suitable probing method supported for address 0x%02X\n",

Laurent has already pointed out the format issue here, and that this
function might be squashed into 3/5 anyway,

But otherwise it looks good to me, so if you keep it separate:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> +			 addr);
> +		err = -EOPNOTSUPP;
> +	}
> +
> +	return err >= 0;
> +}
> +
>  static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
>  			    int (*probe)(struct i2c_adapter *adap, unsigned short addr))
>  {
> 

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Geert Uytterhoeven]

Hi Kieran,

On Tue, Jan 7, 2020 at 10:26 AM Kieran Bingham <kieran@ksquared.org.uk> wrote:
> This looks reasonable to me, I see Laurent has a concern over the use of
> a WARN to present a backtrace, but I think in this instance it will be
> useful as it will facilitate identifying what code path provided the
> incorrect address.

Quoting GregKH:
| We really do not want WARN_ON() anywhere, as that causes systems with
| panic-on-warn to reboot.

https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Kieran Bingham]

Hi Geert,

On 07/01/2020 09:53, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Tue, Jan 7, 2020 at 10:26 AM Kieran Bingham <kieran@ksquared.org.uk> wrote:
>> This looks reasonable to me, I see Laurent has a concern over the use of
>> a WARN to present a backtrace, but I think in this instance it will be
>> useful as it will facilitate identifying what code path provided the
>> incorrect address.
> 
> Quoting GregKH:
> | We really do not want WARN_ON() anywhere, as that causes systems with
> | panic-on-warn to reboot.

Ugh , why would anyone panic-on-warn :) Panic on Panic! (or at least at
the disco...)

> 
> https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/
> 

Well there we go then ...

Perhaps:
	s/WARN/dev_err/

(it was previously dev_warn())

--
Kieran


> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

-- 
Regards
--
Kieran

Re: [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning

[Kieran Bingham]

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)
>  
> 

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Wolfram Sang]

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


> Quoting GregKH:
> | We really do not want WARN_ON() anywhere, as that causes systems with
> | panic-on-warn to reboot.
> 
> https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/

Huh? This renders WARN completely useless, or? If somebody wants panic
on warn, this person should get it?

If we don't add a stack trace, we only know that *someone* registered an
invalid address. Finding out who can be annoying. Kieran spotted this
correctly.


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

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jan 7, 2020 at 11:26 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > Quoting GregKH:
> > | We really do not want WARN_ON() anywhere, as that causes systems with
> > | panic-on-warn to reboot.
> >
> > https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/
>
> Huh? This renders WARN completely useless, or? If somebody wants panic
> on warn, this person should get it?

I also have my doubts...

> If we don't add a stack trace, we only know that *someone* registered an
> invalid address. Finding out who can be annoying. Kieran spotted this
> correctly.

What other information will you have in the backtrace, that you don't have
available inside the function?
Would printing the i2c_driver name be sufficient?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Wolfram Sang]

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


> > > https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/
> >
> > Huh? This renders WARN completely useless, or? If somebody wants panic
> > on warn, this person should get it?
> 
> I also have my doubts...

Good to know :)

> What other information will you have in the backtrace, that you don't have
> available inside the function?
> Would printing the i2c_driver name be sufficient?

Yes, but we don't have the client struct, only the adapter and the
address:

+static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
+                           int (*probe)(struct i2c_adapter *adap, unsigned short addr))

And even if we had the client struct, it would be empty because for most
cases scanning happens before binding to the driver. We don't even have
the name/type in case of i2c_detect().


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

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Luca Ceresoli]

Hi,

On 07/01/20 12:23, Wolfram Sang wrote:
> 
>>>> https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/
>>>
>>> Huh? This renders WARN completely useless, or? If somebody wants panic
>>> on warn, this person should get it?
>>
>> I also have my doubts...
> 
> Good to know :)
> 
>> What other information will you have in the backtrace, that you don't have
>> available inside the function?
>> Would printing the i2c_driver name be sufficient?
> 
> Yes, but we don't have the client struct, only the adapter and the
> address:
> 
> +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr,
> +                           int (*probe)(struct i2c_adapter *adap, unsigned short addr))
> 
> And even if we had the client struct, it would be empty because for most
> cases scanning happens before binding to the driver. We don't even have
> the name/type in case of i2c_detect().
> 

Maybe dev_warn() +  dump_stack()?

Just my 2c,
-- 
Luca

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

[Luca Ceresoli]

Hi Laurent,

On 03/01/20 01:10, Laurent Pinchart wrote:
> On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote:
>> Hi Wolfram,
>>
>> On 02/01/20 22:13, Wolfram Sang wrote:
>>> Hi Luca,
>>>
>>>>> 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.
>>>
>>> Please see my reply to Laurent, I don't think it is racy. But please
>>> elaborate if you think I am wrong.
>>
>> Uhm, you are right here, it's not racy. Sorry, I had read the code
>> quickly and didn't notice the i2c_new_dummy_device() call.
>>
>> So this means if i2c_new_alias_device() succeeds but the caller later
>> fails while applying the alias, then it has to call
>> i2c_unregister_device() to free the alias. Correct?
> 
> I was wrong as well, sorry about that.
> 
>>>>> 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.
>>>
>>> As I said to Laurent, too, I think the risk that a bus is not fully
>>> described is higher than a device which does not respond to a read_byte.
>>> In both cases, we would wrongly use an address in use.
> 
> I don't fully agree with this, I think we shouldn't impose a penalty on
> every user because some device trees don't fully describe the hardware.
> I think we should, at the very least, skip the probe and rely on DT if
> DT explicitly states that all used addresses are listed. We discussed a
> property to report addresses used by devices not described in DT, if
> that property is listed I would prefer trusting DT.

It would be nice, but I'm not sure this is really doable. Say the DT for
board X lists all the used slave addresses. Then the kernel would assume
all the other addresses are available. But then somebody includes the DT
of board X in the DT for product Z, based on board X + add-on board Y.
Add-on board Y has 2 I2C chips, but only one is described in DT. Now the
kernel still thinks it knows all the used address, but this is wrong.

At my current pondering status, I think only two approaches are doable:
either assuming all DTs fully describe the hardware (which is still a
good goal to pursue, generally speaking) or use Wolfram's proposal. The
difference between the two is the call to i2c_unlocked_read_byte_probe().

However a hybrid approach is to speak out loud if we get a response from
an address that is not marked as busy, to invite the developers to fix
their DT. In other words:

 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;
+} else if (ret == 0) {
+        dev_err(&adap->dev,
+                "alien found at %02x, please add it to your DT!!!\n",
+                addr);
 }

Wolfram, do think this could work? Do we have all the addresses listed
in DT marked as busy early enough?

-- 
Luca

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Wolfram Sang]

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


> Maybe dev_warn() +  dump_stack()?

That would technically work, yet it feels like a bit like we now
open-code WARN because we shall not use it. I will ask gkh about his
statement...


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

Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client

[Kieran Bingham]

On 07/01/2020 16:45, Wolfram Sang wrote:
> 
>> Maybe dev_warn() +  dump_stack()?
> 
> That would technically work, yet it feels like a bit like we now
> open-code WARN because we shall not use it. I will ask gkh about his
> statement...

Indeed, that would be my expected purpose for WARN() :-(

Please let us know what GKH says!

-- 
Regards
--
Kieran

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

[Laurent Pinchart]

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@sang-engineering.com>
> > ---
> >  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

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

[Laurent Pinchart]

Hi Luca,

On Tue, Jan 07, 2020 at 04:03:29PM +0100, Luca Ceresoli wrote:
> On 03/01/20 01:10, Laurent Pinchart wrote:
> > On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote:
> >> On 02/01/20 22:13, Wolfram Sang wrote:
> >>>>> 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.
> >>>
> >>> Please see my reply to Laurent, I don't think it is racy. But please
> >>> elaborate if you think I am wrong.
> >>
> >> Uhm, you are right here, it's not racy. Sorry, I had read the code
> >> quickly and didn't notice the i2c_new_dummy_device() call.
> >>
> >> So this means if i2c_new_alias_device() succeeds but the caller later
> >> fails while applying the alias, then it has to call
> >> i2c_unregister_device() to free the alias. Correct?
> > 
> > I was wrong as well, sorry about that.
> > 
> >>>>> 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.
> >>>
> >>> As I said to Laurent, too, I think the risk that a bus is not fully
> >>> described is higher than a device which does not respond to a read_byte.
> >>> In both cases, we would wrongly use an address in use.
> > 
> > I don't fully agree with this, I think we shouldn't impose a penalty on
> > every user because some device trees don't fully describe the hardware.
> > I think we should, at the very least, skip the probe and rely on DT if
> > DT explicitly states that all used addresses are listed. We discussed a
> > property to report addresses used by devices not described in DT, if
> > that property is listed I would prefer trusting DT.
> 
> It would be nice, but I'm not sure this is really doable. Say the DT for
> board X lists all the used slave addresses. Then the kernel would assume
> all the other addresses are available. But then somebody includes the DT
> of board X in the DT for product Z, based on board X + add-on board Y.
> Add-on board Y has 2 I2C chips, but only one is described in DT. Now the
> kernel still thinks it knows all the used address, but this is wrong.

That's the fault of the system integrator though. We can't prevent
people from making incorrect DT, and we shouldn't go to great length to
still support them.

> At my current pondering status, I think only two approaches are doable:
> either assuming all DTs fully describe the hardware (which is still a
> good goal to pursue, generally speaking) or use Wolfram's proposal. The
> difference between the two is the call to i2c_unlocked_read_byte_probe().
> 
> However a hybrid approach is to speak out loud if we get a response from
> an address that is not marked as busy, to invite the developers to fix
> their DT. In other words:
> 
>  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;
> +} else if (ret == 0) {
> +        dev_err(&adap->dev,
> +                "alien found at %02x, please add it to your DT!!!\n",
> +                addr);
>  }
> 
> Wolfram, do think this could work? Do we have all the addresses listed
> in DT marked as busy early enough?

-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

Hi Laurent,

On 07/01/2020 17:11, Laurent Pinchart wrote:
> 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@sang-engineering.com>
>>> ---
>>>  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 ?

That's my point :-D

The core 'could' parse all reg entries, and conclude that any extended
entries within a device node are aliases as well, which should be
reserved, but I don't think it could know if the device is actually
going to be enabled by a driver (well, it could look it up).

I think if core-i2c parses all device tree nodes for register addresses
first, it would have to consider all addresses it came across as
potentially in use.

But it would also have to traverse any i2c-muxes too!

--
Kieran


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

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

[Wolfram Sang]

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


> > > As I said to Laurent, too, I think the risk that a bus is not fully
> > > described is higher than a device which does not respond to a read_byte.
> > > In both cases, we would wrongly use an address in use.
> 
> I don't fully agree with this, I think we shouldn't impose a penalty on
> every user because some device trees don't fully describe the hardware.

I haven't decided yet. However, my general preference is that for a
generic OS like Linux, saftey comes first, then performance. If you have
a fully described DT, then the overhead will be 1 read_byte transaction
per requested alias at probe time. We could talk about using quick_read
to half the overhead. You could even patch it away, if it is too much
for $customer.

> I think we should, at the very least, skip the probe and rely on DT if
> DT explicitly states that all used addresses are listed. We discussed a
> property to report addresses used by devices not described in DT, if
> that property is listed I would prefer trusting DT.

Yeah, we discussed this property and I have no intentions of dropping
it. I haven't though of including it into this series, but it probably
makes sense. We don't have to define much anyhow, just state what
already exists, I guess.

From Documentation/devicetree/bindings/i2c/i2c-ocores.txt:

	dummy@60 {
		compatible = "dummy";
		reg = <0x60>;
	};

I think "dummy" is generic enough to be described in i2c.txt.


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

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

[Wolfram Sang]

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


> +} else if (ret == 0) {
> +        dev_err(&adap->dev,
> +                "alien found at %02x, please add it to your DT!!!\n",
> +                addr);
>  }
> 
> Wolfram, do think this could work? Do we have all the addresses listed
> in DT marked as busy early enough?

Not sure. We could do that, but we will still have the transaction you
guys worry about.


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

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

[Wolfram Sang]

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


> > It would be nice, but I'm not sure this is really doable. Say the DT for
> > board X lists all the used slave addresses. Then the kernel would assume
> > all the other addresses are available. But then somebody includes the DT
> > of board X in the DT for product Z, based on board X + add-on board Y.
> > Add-on board Y has 2 I2C chips, but only one is described in DT. Now the
> > kernel still thinks it knows all the used address, but this is wrong.
> 
> That's the fault of the system integrator though. We can't prevent
> people from making incorrect DT, and we shouldn't go to great length to
> still support them.

Currently, there is no paradigm that all I2C busses must be fully
described. Enforcing it now all of a sudden is not too user-friendly,
or? Especially since calling read_byte once is not necessarily "great
length" in my book. If you have 8 cameras on a 400kHz bus, the 8 * 18
bits should take 360us if I am not mistaken?


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Jan 8, 2020 at 2:20 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > > As I said to Laurent, too, I think the risk that a bus is not fully
> > > > described is higher than a device which does not respond to a read_byte.
> > > > In both cases, we would wrongly use an address in use.
> >
> > I don't fully agree with this, I think we shouldn't impose a penalty on
> > every user because some device trees don't fully describe the hardware.
>
> I haven't decided yet. However, my general preference is that for a
> generic OS like Linux, saftey comes first, then performance. If you have
> a fully described DT, then the overhead will be 1 read_byte transaction
> per requested alias at probe time. We could talk about using quick_read
> to half the overhead. You could even patch it away, if it is too much
> for $customer.
>
> > I think we should, at the very least, skip the probe and rely on DT if
> > DT explicitly states that all used addresses are listed. We discussed a
> > property to report addresses used by devices not described in DT, if
> > that property is listed I would prefer trusting DT.
>
> Yeah, we discussed this property and I have no intentions of dropping
> it. I haven't though of including it into this series, but it probably
> makes sense. We don't have to define much anyhow, just state what
> already exists, I guess.
>
> From Documentation/devicetree/bindings/i2c/i2c-ocores.txt:
>
>         dummy@60 {
>                 compatible = "dummy";
>                 reg = <0x60>;
>         };
>
> I think "dummy" is generic enough to be described in i2c.txt.

Dummy-the-node or dummy-the-compatible value?
Probably dummy nodes should have no compatible value at all?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

On Wed, Jan 08, 2020 at 02:27:08PM +0100, Wolfram Sang wrote:
> 
> > > It would be nice, but I'm not sure this is really doable. Say the DT for
> > > board X lists all the used slave addresses. Then the kernel would assume
> > > all the other addresses are available. But then somebody includes the DT
> > > of board X in the DT for product Z, based on board X + add-on board Y.
> > > Add-on board Y has 2 I2C chips, but only one is described in DT. Now the
> > > kernel still thinks it knows all the used address, but this is wrong.
> > 
> > That's the fault of the system integrator though. We can't prevent
> > people from making incorrect DT, and we shouldn't go to great length to
> > still support them.
> 
> Currently, there is no paradigm that all I2C busses must be fully
> described. Enforcing it now all of a sudden is not too user-friendly,
> or?

We're only enforcing it for systems that want to make use of this new
API, so it's not breaking backward compatibility.

> Especially since calling read_byte once is not necessarily "great
> length" in my book. If you have 8 cameras on a 400kHz bus, the 8 * 18
> bits should take 360us if I am not mistaken?

That's assuming the first scanned address is free. There could also be
I2C-controller I2C muxes or gates in front of the bus. Things can
quickly get more expensive.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Wed, Jan 08, 2020 at 02:19:29PM +0100, Wolfram Sang wrote:
> 
> > > > As I said to Laurent, too, I think the risk that a bus is not fully
> > > > described is higher than a device which does not respond to a read_byte.
> > > > In both cases, we would wrongly use an address in use.
> > 
> > I don't fully agree with this, I think we shouldn't impose a penalty on
> > every user because some device trees don't fully describe the hardware.
> 
> I haven't decided yet. However, my general preference is that for a
> generic OS like Linux, saftey comes first, then performance. If you have
> a fully described DT, then the overhead will be 1 read_byte transaction
> per requested alias at probe time. We could talk about using quick_read
> to half the overhead. You could even patch it away, if it is too much
> for $customer.
> 
> > I think we should, at the very least, skip the probe and rely on DT if
> > DT explicitly states that all used addresses are listed. We discussed a
> > property to report addresses used by devices not described in DT, if
> > that property is listed I would prefer trusting DT.
> 
> Yeah, we discussed this property and I have no intentions of dropping
> it. I haven't though of including it into this series, but it probably
> makes sense. We don't have to define much anyhow, just state what
> already exists, I guess.
> 
> From Documentation/devicetree/bindings/i2c/i2c-ocores.txt:
> 
> 	dummy@60 {
> 		compatible = "dummy";
> 		reg = <0x60>;
> 	};
> 
> I think "dummy" is generic enough to be described in i2c.txt.

We may want a compatible value that guarantees noone will ever match it
:-) I was imagining a single property at the bus level with multiple
ranges instead, but dummy nodes could be OK too.


-- 
Regards,

Laurent Pinchart

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

[Wolfram Sang]

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


> >> 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?

You have a point here. Ancillary addresses are not blocked until the
driver probes, this is true. I wonder now if we should handle multiple
addresses in i2c-core-of.c somehow, too? It does block the first <reg>
entry, but not all.


> The core 'could' parse all reg entries, and conclude that any extended
> entries within a device node are aliases as well, which should be
> reserved, but I don't think it could know if the device is actually
> going to be enabled by a driver (well, it could look it up).

We could argue that if it is described in DT, it should be blocked in
any case, or?

> But it would also have to traverse any i2c-muxes too!

I probably need a second thought about muxes as well.


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

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

[Laurent Pinchart]

On Wed, Jan 08, 2020 at 02:35:33PM +0100, Wolfram Sang wrote:
> 
> > >> 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?
> 
> You have a point here. Ancillary addresses are not blocked until the
> driver probes, this is true. I wonder now if we should handle multiple
> addresses in i2c-core-of.c somehow, too? It does block the first <reg>
> entry, but not all.
> 
> > The core 'could' parse all reg entries, and conclude that any extended
> > entries within a device node are aliases as well, which should be
> > reserved, but I don't think it could know if the device is actually
> > going to be enabled by a driver (well, it could look it up).
> 
> We could argue that if it is described in DT, it should be blocked in
> any case, or?

That seems fair to me.

> > But it would also have to traverse any i2c-muxes too!
> 
> I probably need a second thought about muxes as well.

-- 
Regards,

Laurent Pinchart

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

[Wolfram Sang]

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


> > Currently, there is no paradigm that all I2C busses must be fully
> > described. Enforcing it now all of a sudden is not too user-friendly,
> > or?
> 
> We're only enforcing it for systems that want to make use of this new
> API, so it's not breaking backward compatibility.

Well, even new systems might need to update old DTSIs which they
include.

> > Especially since calling read_byte once is not necessarily "great
> > length" in my book. If you have 8 cameras on a 400kHz bus, the 8 * 18
> > bits should take 360us if I am not mistaken?
> 
> That's assuming the first scanned address is free. There could also be
> I2C-controller I2C muxes or gates in front of the bus. Things can
> quickly get more expensive.

Not on a fully described bus, or? The first address will always be free.


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

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

[Peter Rosin]

On 2020-01-01 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@sang-engineering.com>
>> ---
>>  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. 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, ...) ?

Another argument against probing (perhaps weak, but still) is that the
probed address might already be programmed in (one of) the reprogrammable
chip(s) making it respond to the probe and thus preventing the use of a
perfectly good alias address.

In /extreme/ situations this /might/ prevent finding a needed alias at
all.

Cheers,
Peter

Re: [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning

[Peter Rosin]

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