Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device
@ 2019-12-31 16:13 Wolfram Sang
  2019-12-31 16:13 ` [RFC PATCH 1/5] i2c: core: refactor scanning for a client Wolfram Sang
                   ` (5 more replies)
  0 siblings, 6 replies; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:13 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Kieran Bingham, Jacopo Mondi,
	Laurent Pinchart, Vladimir Zapolskiy, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
@ 2019-12-31 16:13 ` Wolfram Sang
  2020-01-01 16:45   ` Laurent Pinchart
  2020-01-07  9:26   ` Kieran Bingham
  2019-12-31 16:13 ` [RFC PATCH 2/5] i2c: core: add new variant to check " Wolfram Sang
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:13 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Kieran Bingham, Jacopo Mondi,
	Laurent Pinchart, Vladimir Zapolskiy, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC PATCH 2/5] i2c: core: add new variant to check for a client
  2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
  2019-12-31 16:13 ` [RFC PATCH 1/5] i2c: core: refactor scanning for a client Wolfram Sang
@ 2019-12-31 16:13 ` " Wolfram Sang
  2020-01-01 16:49   ` Laurent Pinchart
  2020-01-07  9:42   ` Kieran Bingham
  2019-12-31 16:13 ` [RFC PATCH 3/5] i2c: core: add function to request an alias Wolfram Sang
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:13 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Kieran Bingham, Jacopo Mondi,
	Laurent Pinchart, Vladimir Zapolskiy, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC PATCH 3/5] i2c: core: add function to request an alias
  2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
  2019-12-31 16:13 ` [RFC PATCH 1/5] i2c: core: refactor scanning for a client Wolfram Sang
  2019-12-31 16:13 ` [RFC PATCH 2/5] i2c: core: add new variant to check " Wolfram Sang
@ 2019-12-31 16:13 ` Wolfram Sang
  2020-01-01 16:55   ` Laurent Pinchart
  2020-01-07  9:40   ` Kieran Bingham
  2019-12-31 16:13 ` [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning Wolfram Sang
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:13 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Kieran Bingham, Jacopo Mondi,
	Laurent Pinchart, Vladimir Zapolskiy, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning
  2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
                   ` (2 preceding siblings ...)
  2019-12-31 16:13 ` [RFC PATCH 3/5] i2c: core: add function to request an alias Wolfram Sang
@ 2019-12-31 16:13 ` Wolfram Sang
  2020-01-07  9:59   ` Kieran Bingham
  2019-12-31 16:14 ` [RFC PATCH 5/5] simple test case for the I2C alias functionality Wolfram Sang
  2019-12-31 16:27 ` [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
  5 siblings, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:13 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Kieran Bingham, Jacopo Mondi,
	Laurent Pinchart, Vladimir Zapolskiy, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [RFC PATCH 5/5] simple test case for the I2C alias functionality
  2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
                   ` (3 preceding siblings ...)
  2019-12-31 16:13 ` [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning Wolfram Sang
@ 2019-12-31 16:14 ` Wolfram Sang
  2019-12-31 16:27 ` [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
  5 siblings, 0 replies; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:14 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Kieran Bingham, Jacopo Mondi,
	Laurent Pinchart, Vladimir Zapolskiy, 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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device
  2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
                   ` (4 preceding siblings ...)
  2019-12-31 16:14 ` [RFC PATCH 5/5] simple test case for the I2C alias functionality Wolfram Sang
@ 2019-12-31 16:27 ` Wolfram Sang
  5 siblings, 0 replies; 40+ messages in thread
From: Wolfram Sang @ 2019-12-31 16:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Luca Ceresoli, Kieran Bingham,
	Jacopo Mondi, Laurent Pinchart, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2019-12-31 16:13 ` [RFC PATCH 1/5] i2c: core: refactor scanning for a client Wolfram Sang
@ 2020-01-01 16:45   ` Laurent Pinchart
  2020-01-07  9:26   ` Kieran Bingham
  1 sibling, 0 replies; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-01 16:45 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Luca Ceresoli, Kieran Bingham,
	Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/5] i2c: core: add new variant to check for a client
  2019-12-31 16:13 ` [RFC PATCH 2/5] i2c: core: add new variant to check " Wolfram Sang
@ 2020-01-01 16:49   ` Laurent Pinchart
  2020-01-07  9:42   ` Kieran Bingham
  1 sibling, 0 replies; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-01 16:49 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Luca Ceresoli, Kieran Bingham,
	Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2019-12-31 16:13 ` [RFC PATCH 3/5] i2c: core: add function to request an alias Wolfram Sang
@ 2020-01-01 16:55   ` Laurent Pinchart
  2020-01-02 18:58     ` Luca Ceresoli
  2020-01-02 21:03     ` Wolfram Sang
  2020-01-07  9:40   ` Kieran Bingham
  1 sibling, 2 replies; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-01 16:55 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Luca Ceresoli, Kieran Bingham,
	Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-01 16:55   ` Laurent Pinchart
@ 2020-01-02 18:58     ` Luca Ceresoli
  2020-01-02 21:13       ` Wolfram Sang
  2020-01-02 21:03     ` Wolfram Sang
  1 sibling, 1 reply; 40+ messages in thread
From: Luca Ceresoli @ 2020-01-02 18:58 UTC (permalink / raw)
  To: Laurent Pinchart, Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Kieran Bingham, Jacopo Mondi,
	Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-01 16:55   ` Laurent Pinchart
  2020-01-02 18:58     ` Luca Ceresoli
@ 2020-01-02 21:03     ` Wolfram Sang
  1 sibling, 0 replies; 40+ messages in thread
From: Wolfram Sang @ 2020-01-02 21:03 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Luca Ceresoli,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-02 18:58     ` Luca Ceresoli
@ 2020-01-02 21:13       ` Wolfram Sang
  2020-01-02 22:27         ` Luca Ceresoli
  0 siblings, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2020-01-02 21:13 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Laurent Pinchart, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-02 21:13       ` Wolfram Sang
@ 2020-01-02 22:27         ` Luca Ceresoli
  2020-01-03  0:10           ` Laurent Pinchart
  0 siblings, 1 reply; 40+ messages in thread
From: Luca Ceresoli @ 2020-01-02 22:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Laurent Pinchart, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-02 22:27         ` Luca Ceresoli
@ 2020-01-03  0:10           ` Laurent Pinchart
  2020-01-07 15:03             ` Luca Ceresoli
  2020-01-08 13:19             ` Wolfram Sang
  0 siblings, 2 replies; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-03  0:10 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Wolfram Sang, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2019-12-31 16:13 ` [RFC PATCH 1/5] i2c: core: refactor scanning for a client Wolfram Sang
  2020-01-01 16:45   ` Laurent Pinchart
@ 2020-01-07  9:26   ` Kieran Bingham
  2020-01-07  9:53     ` Geert Uytterhoeven
  1 sibling, 1 reply; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07  9:26 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2019-12-31 16:13 ` [RFC PATCH 3/5] i2c: core: add function to request an alias Wolfram Sang
  2020-01-01 16:55   ` Laurent Pinchart
@ 2020-01-07  9:40   ` Kieran Bingham
  2020-01-07 17:11     ` Laurent Pinchart
  1 sibling, 1 reply; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07  9:40 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 2/5] i2c: core: add new variant to check for a client
  2019-12-31 16:13 ` [RFC PATCH 2/5] i2c: core: add new variant to check " Wolfram Sang
  2020-01-01 16:49   ` Laurent Pinchart
@ 2020-01-07  9:42   ` Kieran Bingham
  1 sibling, 0 replies; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07  9:42 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07  9:26   ` Kieran Bingham
@ 2020-01-07  9:53     ` Geert Uytterhoeven
  2020-01-07  9:58       ` Kieran Bingham
  2020-01-07 10:25       ` Wolfram Sang
  0 siblings, 2 replies; 40+ messages in thread
From: Geert Uytterhoeven @ 2020-01-07  9:53 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Wolfram Sang, Linux I2C, Linux-Renesas, Luca Ceresoli,
	Jacopo Mondi, Laurent Pinchart, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07  9:53     ` Geert Uytterhoeven
@ 2020-01-07  9:58       ` Kieran Bingham
  2020-01-07 10:25       ` Wolfram Sang
  1 sibling, 0 replies; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07  9:58 UTC (permalink / raw)
  To: Geert Uytterhoeven, Kieran Bingham
  Cc: Wolfram Sang, Linux I2C, Linux-Renesas, Luca Ceresoli,
	Jacopo Mondi, Laurent Pinchart, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning
  2019-12-31 16:13 ` [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning Wolfram Sang
@ 2020-01-07  9:59   ` Kieran Bingham
  0 siblings, 0 replies; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07  9:59 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07  9:53     ` Geert Uytterhoeven
  2020-01-07  9:58       ` Kieran Bingham
@ 2020-01-07 10:25       ` Wolfram Sang
  2020-01-07 10:36         ` Geert Uytterhoeven
  1 sibling, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2020-01-07 10:25 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kieran Bingham, Wolfram Sang, Linux I2C, Linux-Renesas,
	Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07 10:25       ` Wolfram Sang
@ 2020-01-07 10:36         ` Geert Uytterhoeven
  2020-01-07 11:23           ` Wolfram Sang
  0 siblings, 1 reply; 40+ messages in thread
From: Geert Uytterhoeven @ 2020-01-07 10:36 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Kieran Bingham, Wolfram Sang, Linux I2C, Linux-Renesas,
	Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07 10:36         ` Geert Uytterhoeven
@ 2020-01-07 11:23           ` Wolfram Sang
  2020-01-07 15:03             ` Luca Ceresoli
  0 siblings, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2020-01-07 11:23 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kieran Bingham, Wolfram Sang, Linux I2C, Linux-Renesas,
	Luca Ceresoli, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07 11:23           ` Wolfram Sang
@ 2020-01-07 15:03             ` Luca Ceresoli
  2020-01-07 16:45               ` Wolfram Sang
  0 siblings, 1 reply; 40+ messages in thread
From: Luca Ceresoli @ 2020-01-07 15:03 UTC (permalink / raw)
  To: Wolfram Sang, Geert Uytterhoeven
  Cc: Kieran Bingham, Wolfram Sang, Linux I2C, Linux-Renesas,
	Jacopo Mondi, Laurent Pinchart, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-03  0:10           ` Laurent Pinchart
@ 2020-01-07 15:03             ` Luca Ceresoli
  2020-01-07 17:13               ` Laurent Pinchart
  2020-01-08 13:22               ` Wolfram Sang
  2020-01-08 13:19             ` Wolfram Sang
  1 sibling, 2 replies; 40+ messages in thread
From: Luca Ceresoli @ 2020-01-07 15:03 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Wolfram Sang, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07 15:03             ` Luca Ceresoli
@ 2020-01-07 16:45               ` Wolfram Sang
  2020-01-07 16:52                 ` Kieran Bingham
  0 siblings, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2020-01-07 16:45 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Geert Uytterhoeven, Kieran Bingham, Wolfram Sang, Linux I2C,
	Linux-Renesas, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 1/5] i2c: core: refactor scanning for a client
  2020-01-07 16:45               ` Wolfram Sang
@ 2020-01-07 16:52                 ` Kieran Bingham
  0 siblings, 0 replies; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07 16:52 UTC (permalink / raw)
  To: Wolfram Sang, Luca Ceresoli
  Cc: Geert Uytterhoeven, Kieran Bingham, Wolfram Sang, Linux I2C,
	Linux-Renesas, Jacopo Mondi, Laurent Pinchart,
	Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-07  9:40   ` Kieran Bingham
@ 2020-01-07 17:11     ` Laurent Pinchart
  2020-01-07 17:14       ` Kieran Bingham
  0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-07 17:11 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Luca Ceresoli,
	Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-07 15:03             ` Luca Ceresoli
@ 2020-01-07 17:13               ` Laurent Pinchart
  2020-01-08 13:27                 ` Wolfram Sang
  2020-01-08 13:22               ` Wolfram Sang
  1 sibling, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-07 17:13 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Wolfram Sang, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-07 17:11     ` Laurent Pinchart
@ 2020-01-07 17:14       ` Kieran Bingham
  2020-01-08 13:35         ` Wolfram Sang
  0 siblings, 1 reply; 40+ messages in thread
From: Kieran Bingham @ 2020-01-07 17:14 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Luca Ceresoli,
	Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-03  0:10           ` Laurent Pinchart
  2020-01-07 15:03             ` Luca Ceresoli
@ 2020-01-08 13:19             ` Wolfram Sang
  2020-01-08 13:29               ` Geert Uytterhoeven
  2020-01-08 13:34               ` Laurent Pinchart
  1 sibling, 2 replies; 40+ messages in thread
From: Wolfram Sang @ 2020-01-08 13:19 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Luca Ceresoli, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-07 15:03             ` Luca Ceresoli
  2020-01-07 17:13               ` Laurent Pinchart
@ 2020-01-08 13:22               ` Wolfram Sang
  1 sibling, 0 replies; 40+ messages in thread
From: Wolfram Sang @ 2020-01-08 13:22 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Laurent Pinchart, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-07 17:13               ` Laurent Pinchart
@ 2020-01-08 13:27                 ` Wolfram Sang
  2020-01-08 13:31                   ` Laurent Pinchart
  0 siblings, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2020-01-08 13:27 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Luca Ceresoli, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-08 13:19             ` Wolfram Sang
@ 2020-01-08 13:29               ` Geert Uytterhoeven
  2020-01-08 13:34               ` Laurent Pinchart
  1 sibling, 0 replies; 40+ messages in thread
From: Geert Uytterhoeven @ 2020-01-08 13:29 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Laurent Pinchart, Luca Ceresoli, Wolfram Sang, Linux I2C,
	Linux-Renesas, Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-08 13:27                 ` Wolfram Sang
@ 2020-01-08 13:31                   ` Laurent Pinchart
  2020-01-08 13:38                     ` Wolfram Sang
  0 siblings, 1 reply; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-08 13:31 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Luca Ceresoli, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-08 13:19             ` Wolfram Sang
  2020-01-08 13:29               ` Geert Uytterhoeven
@ 2020-01-08 13:34               ` Laurent Pinchart
  1 sibling, 0 replies; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-08 13:34 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Luca Ceresoli, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-07 17:14       ` Kieran Bingham
@ 2020-01-08 13:35         ` Wolfram Sang
  2020-01-08 13:36           ` Laurent Pinchart
  0 siblings, 1 reply; 40+ messages in thread
From: Wolfram Sang @ 2020-01-08 13:35 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Laurent Pinchart, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Luca Ceresoli, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-08 13:35         ` Wolfram Sang
@ 2020-01-08 13:36           ` Laurent Pinchart
  0 siblings, 0 replies; 40+ messages in thread
From: Laurent Pinchart @ 2020-01-08 13:36 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Kieran Bingham, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Luca Ceresoli, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [RFC PATCH 3/5] i2c: core: add function to request an alias
  2020-01-08 13:31                   ` Laurent Pinchart
@ 2020-01-08 13:38                     ` Wolfram Sang
  0 siblings, 0 replies; 40+ messages in thread
From: Wolfram Sang @ 2020-01-08 13:38 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Luca Ceresoli, Wolfram Sang, linux-i2c, linux-renesas-soc,
	Kieran Bingham, Jacopo Mondi, Vladimir Zapolskiy

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, back to index

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-31 16:13 [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang
2019-12-31 16:13 ` [RFC PATCH 1/5] i2c: core: refactor scanning for a client Wolfram Sang
2020-01-01 16:45   ` Laurent Pinchart
2020-01-07  9:26   ` Kieran Bingham
2020-01-07  9:53     ` Geert Uytterhoeven
2020-01-07  9:58       ` Kieran Bingham
2020-01-07 10:25       ` Wolfram Sang
2020-01-07 10:36         ` Geert Uytterhoeven
2020-01-07 11:23           ` Wolfram Sang
2020-01-07 15:03             ` Luca Ceresoli
2020-01-07 16:45               ` Wolfram Sang
2020-01-07 16:52                 ` Kieran Bingham
2019-12-31 16:13 ` [RFC PATCH 2/5] i2c: core: add new variant to check " Wolfram Sang
2020-01-01 16:49   ` Laurent Pinchart
2020-01-07  9:42   ` Kieran Bingham
2019-12-31 16:13 ` [RFC PATCH 3/5] i2c: core: add function to request an alias Wolfram Sang
2020-01-01 16:55   ` Laurent Pinchart
2020-01-02 18:58     ` Luca Ceresoli
2020-01-02 21:13       ` Wolfram Sang
2020-01-02 22:27         ` Luca Ceresoli
2020-01-03  0:10           ` Laurent Pinchart
2020-01-07 15:03             ` Luca Ceresoli
2020-01-07 17:13               ` Laurent Pinchart
2020-01-08 13:27                 ` Wolfram Sang
2020-01-08 13:31                   ` Laurent Pinchart
2020-01-08 13:38                     ` Wolfram Sang
2020-01-08 13:22               ` Wolfram Sang
2020-01-08 13:19             ` Wolfram Sang
2020-01-08 13:29               ` Geert Uytterhoeven
2020-01-08 13:34               ` Laurent Pinchart
2020-01-02 21:03     ` Wolfram Sang
2020-01-07  9:40   ` Kieran Bingham
2020-01-07 17:11     ` Laurent Pinchart
2020-01-07 17:14       ` Kieran Bingham
2020-01-08 13:35         ` Wolfram Sang
2020-01-08 13:36           ` Laurent Pinchart
2019-12-31 16:13 ` [RFC PATCH 4/5] i2c: core: add simple caching to the 'alias' scanning Wolfram Sang
2020-01-07  9:59   ` Kieran Bingham
2019-12-31 16:14 ` [RFC PATCH 5/5] simple test case for the I2C alias functionality Wolfram Sang
2019-12-31 16:27 ` [RFC PATCH 0/5] i2c: implement mechanism to retrieve an alias device Wolfram Sang

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org
	public-inbox-index linux-renesas-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git