Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses
@ 2020-03-18 15:00 Wolfram Sang
  2020-03-18 15:00 ` [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name Wolfram Sang
                   ` (7 more replies)
  0 siblings, 8 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jacopo Mondi, Niklas Söderlund, Wolfram Sang, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham,
	Laurent Pinchart, Luca Ceresoli, linux-i3c

First the changes since V1 for those familiar with this series:

* old patch 1 dropped, not needed anymore after some reimplementation
  here
* added tags from last revision (except for patches 5+6 because there
  were changes in code)
* patch 5 has a better of-iterator which respects now that addresses
  might have different #address-cells and such
* patch 6 now puts the device it obtained
* one more "dummy" removed fromt the binding docs

TODO: make sure there are no concurrency issues in patch 6 when handling
the struct i2c_client.

Many thanks to Geert and Luca for the review and discussions!

And here the cover-letter for V1:

One outcome of my dynamic address assignment RFC series[1] was that we
need a way to describe an I2C bus in DT fully. This includes unknown
devices and devices requiring multiple addresses. This series implements
that.

Patches 1 does some preparational refactoring. After patch 2, we can
have child nodes with an address, but no compatible. Those addresses
will be marked busy now. They are handled by the dummy driver as well,
but named "reserved" instead of dummy. Patches 3+4 are again some
preparational refactoring. After patch 5, all addresses in a 'reg' array
are now blocked by the I2C core, also using the dummy driver but named
"reserved". So, we can have something like this:

	reserved@13 {
	       reg = <0x13>, <0x14>;
	};

After patch 6 then, i2c_new_ancillary_device is spiced up to look for
such a reserved address and return it as a good-old "dummy" device.
Sanity checks include that only a sibling from the same DT node can
request such an ancillary address. Stealing addresses from other drivers
is not possible anymore. This is something I envisioned for some time
now and I am quite happy with the implementation and how things work.

There is only one thing giving me some headache now. There is a danger
of a regression maybe. If someone has multiple 'reg' entries in the DT
but never used i2c_new_ancillary_device but i2c_new_dummy_device, then
things will break now because i2c_new_dummy_device has not enough
information to convert a "reserved" device to a "dummy" one. It will
just see the address as busy. However, all binding documentations I
found which use 'reg' as an array correctly use
i2c_new_ancillary_device. On the other hand, my search strategy for
finding such bindings and DTs do not feel perfect to me. Maybe there are
also some more corner cases in this area, so this series is still RFC.

And some more documentation is needed. Before that, though, the generic
I2C binding docs need some overhaul, too.

All tested on a Renesas Lager board (R-Car H2). A git branch can be
found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c_alias_device_v2

The I3C list is on CC not only because there is 1-line change in their
subsystem, but maybe also because they need to be aware of these changes
for their I2C fallback? I don't really know, let me know if you are not
interested.

Looking forward to comments!

Happy hacking,

   Wolfram

[1] https://www.spinics.net/lists/linux-i2c/msg43291.html

Wolfram Sang (6):
  i2c: use DEFINE for the dummy driver name
  i2c: allow DT nodes without 'compatible'
  i2c: of: remove superfluous parameter from exported function
  i2c: of: error message unification
  i2c: of: mark a whole array of regs as reserved
  i2c: core: hand over reserved devices when requesting ancillary
    addresses

 .../devicetree/bindings/i2c/i2c-ocores.txt    |  2 -
 Documentation/devicetree/bindings/i2c/i2c.txt |  4 +-
 drivers/i2c/i2c-core-base.c                   | 33 +++++--
 drivers/i2c/i2c-core-of.c                     | 89 +++++++++++--------
 drivers/i2c/i2c-core.h                        |  3 +
 drivers/i3c/master.c                          |  2 +-
 include/linux/i2c.h                           |  6 +-
 7 files changed, 88 insertions(+), 51 deletions(-)

-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
@ 2020-03-18 15:00 ` Wolfram Sang
  2020-04-15  8:09   ` Kieran Bingham
  2020-03-18 15:00 ` [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible' Wolfram Sang
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jacopo Mondi, Niklas Söderlund, Geert Uytterhoeven,
	Wolfram Sang, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Luca Ceresoli, linux-i3c

We use it in multiple places, so make sure it is consistent whenever we
need to change it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/i2c/i2c-core-base.c | 8 ++++----
 drivers/i2c/i2c-core.h      | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cefad0881942..3d7b8a00a7d9 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -853,7 +853,7 @@ EXPORT_SYMBOL_GPL(i2c_unregister_device);
 
 
 static const struct i2c_device_id dummy_id[] = {
-	{ "dummy", 0 },
+	{ I2C_DUMMY_DRV_NAME, 0 },
 	{ },
 };
 
@@ -869,7 +869,7 @@ static int dummy_remove(struct i2c_client *client)
 }
 
 static struct i2c_driver dummy_driver = {
-	.driver.name	= "dummy",
+	.driver.name	= I2C_DUMMY_DRV_NAME,
 	.probe		= dummy_probe,
 	.remove		= dummy_remove,
 	.id_table	= dummy_id,
@@ -896,7 +896,7 @@ static struct i2c_driver dummy_driver = {
 struct i2c_client *i2c_new_dummy_device(struct i2c_adapter *adapter, u16 address)
 {
 	struct i2c_board_info info = {
-		I2C_BOARD_INFO("dummy", address),
+		I2C_BOARD_INFO(I2C_DUMMY_DRV_NAME, address),
 	};
 
 	return i2c_new_client_device(adapter, &info);
@@ -1487,7 +1487,7 @@ static void i2c_do_del_adapter(struct i2c_driver *driver,
 static int __unregister_client(struct device *dev, void *dummy)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
-	if (client && strcmp(client->name, "dummy"))
+	if (client && strcmp(client->name, I2C_DUMMY_DRV_NAME))
 		i2c_unregister_device(client);
 	return 0;
 }
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 517d98be68d2..fb89fabf84d3 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -22,6 +22,8 @@ int i2c_check_7bit_addr_validity_strict(unsigned short addr);
 int i2c_dev_irq_from_resources(const struct resource *resources,
 			       unsigned int num_resources);
 
+#define I2C_DUMMY_DRV_NAME "dummy"
+
 /*
  * We only allow atomic transfers for very late communication, e.g. to send
  * the powerdown command to a PMIC. Atomic transfers are a corner case and not
-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
  2020-03-18 15:00 ` [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name Wolfram Sang
@ 2020-03-18 15:00 ` Wolfram Sang
  2020-04-10 13:49   ` Luca Ceresoli
  2020-04-15  8:48   ` Kieran Bingham
  2020-03-18 15:00 ` [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function Wolfram Sang
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Rob Herring, Jacopo Mondi, Niklas Söderlund,
	Geert Uytterhoeven, Wolfram Sang, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham,
	Laurent Pinchart, Luca Ceresoli, linux-i3c

Sometimes, we have unknown devices in a system and still want to block
their address. For that, we allow DT nodes with only a 'reg' property.
These devices will be bound to the "dummy" driver but with the name
"reserved". That way, we can distinguish them and even hand them over to
the "dummy" driver later when they are really requested using
i2c_new_ancillary_device().

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 2 --
 Documentation/devicetree/bindings/i2c/i2c.txt        | 4 +++-
 drivers/i2c/i2c-core-base.c                          | 1 +
 drivers/i2c/i2c-core-of.c                            | 8 +++-----
 drivers/i2c/i2c-core.h                               | 1 +
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
index 6b25a80ae8d3..fc8ea27934b3 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
@@ -50,7 +50,6 @@ Examples:
 		reg-io-width = <1>;	/* 8 bit read/write */
 
 		dummy@60 {
-			compatible = "dummy";
 			reg = <0x60>;
 		};
 	};
@@ -68,7 +67,6 @@ or
 		reg-io-width = <1>;	/* 8 bit read/write */
 
 		dummy@60 {
-			compatible = "dummy";
 			reg = <0x60>;
 		};
 	};
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 9a53df4243c6..989b315e09dc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -21,7 +21,9 @@ flags can be attached to the address. I2C_TEN_BIT_ADDRESS is used to mark a 10
 bit address. It is needed to avoid the ambiguity between e.g. a 7 bit address
 of 0x50 and a 10 bit address of 0x050 which, in theory, can be on the same bus.
 Another flag is I2C_OWN_SLAVE_ADDRESS to mark addresses on which we listen to
-be devices ourselves.
+be devices ourselves. The 'reg' property of a child is required. The
+'compatible' property is not. Empty 'compatible' child entries can be used to
+describe unknown devices or addresses which shall be blocked for other reasons.
 
 Optional properties
 -------------------
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 3d7b8a00a7d9..84464e439df5 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -854,6 +854,7 @@ EXPORT_SYMBOL_GPL(i2c_unregister_device);
 
 static const struct i2c_device_id dummy_id[] = {
 	{ I2C_DUMMY_DRV_NAME, 0 },
+	{ I2C_RESERVED_DRV_NAME, 0 },
 	{ },
 };
 
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 6787c1f71483..d8d111ad6c85 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -27,17 +27,15 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 
 	memset(info, 0, sizeof(*info));
 
-	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0) {
-		dev_err(dev, "of_i2c: modalias failure on %pOF\n", node);
-		return -EINVAL;
-	}
-
 	ret = of_property_read_u32(node, "reg", &addr);
 	if (ret) {
 		dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
 		return ret;
 	}
 
+	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
+		strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));
+
 	if (addr & I2C_TEN_BIT_ADDRESS) {
 		addr &= ~I2C_TEN_BIT_ADDRESS;
 		info->flags |= I2C_CLIENT_TEN;
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index fb89fabf84d3..77b3a925ed95 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -23,6 +23,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
 			       unsigned int num_resources);
 
 #define I2C_DUMMY_DRV_NAME "dummy"
+#define I2C_RESERVED_DRV_NAME "reserved"
 
 /*
  * We only allow atomic transfers for very late communication, e.g. to send
-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
  2020-03-18 15:00 ` [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name Wolfram Sang
  2020-03-18 15:00 ` [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible' Wolfram Sang
@ 2020-03-18 15:00 ` Wolfram Sang
  2020-03-19 12:41   ` Boris Brezillon
  2020-04-15  8:13   ` Kieran Bingham
  2020-03-18 15:00 ` [RFC PATCH v2 4/6] i2c: of: error message unification Wolfram Sang
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jacopo Mondi, Niklas Söderlund, Geert Uytterhoeven,
	Wolfram Sang, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Luca Ceresoli, linux-i3c

'dev' is only used for printing an error message. However, that
information is not needed because '%pOF' fully describes the location of
the error. Drop the 'dev' and remove the superfluous parameter.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/i2c/i2c-core-of.c | 7 +++----
 drivers/i3c/master.c      | 2 +-
 include/linux/i2c.h       | 6 ++----
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index d8d111ad6c85..710704cd583e 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -19,8 +19,7 @@
 
 #include "i2c-core.h"
 
-int of_i2c_get_board_info(struct device *dev, struct device_node *node,
-			  struct i2c_board_info *info)
+int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
 {
 	u32 addr;
 	int ret;
@@ -29,7 +28,7 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 
 	ret = of_property_read_u32(node, "reg", &addr);
 	if (ret) {
-		dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
+		pr_err("of_i2c: invalid reg on %pOF\n", node);
 		return ret;
 	}
 
@@ -69,7 +68,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 
 	dev_dbg(&adap->dev, "of_i2c: register %pOF\n", node);
 
-	ret = of_i2c_get_board_info(&adap->dev, node, &info);
+	ret = of_i2c_get_board_info(node, &info);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 7f8f896fa0c3..cc0549a9fc64 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1943,7 +1943,7 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
 	if (!boardinfo)
 		return -ENOMEM;
 
-	ret = of_i2c_get_board_info(dev, node, &boardinfo->base);
+	ret = of_i2c_get_board_info(node, &boardinfo->base);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f834687989f7..d84aaf0d83d5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -942,8 +942,7 @@ const struct of_device_id
 *i2c_of_match_device(const struct of_device_id *matches,
 		     struct i2c_client *client);
 
-int of_i2c_get_board_info(struct device *dev, struct device_node *node,
-			  struct i2c_board_info *info);
+int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info);
 
 #else
 
@@ -969,8 +968,7 @@ static inline const struct of_device_id
 	return NULL;
 }
 
-static inline int of_i2c_get_board_info(struct device *dev,
-					struct device_node *node,
+static inline int of_i2c_get_board_info(struct device_node *node,
 					struct i2c_board_info *info)
 {
 	return -ENOTSUPP;
-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* [RFC PATCH v2 4/6] i2c: of: error message unification
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
                   ` (2 preceding siblings ...)
  2020-03-18 15:00 ` [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function Wolfram Sang
@ 2020-03-18 15:00 ` Wolfram Sang
  2020-04-10 17:02   ` Luca Ceresoli
  2020-04-15  8:17   ` Kieran Bingham
  2020-03-18 15:00 ` [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved Wolfram Sang
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jacopo Mondi, Niklas Söderlund, Geert Uytterhoeven,
	Wolfram Sang, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Luca Ceresoli, linux-i3c

- don't prefix the device if %pOF is provided. That information is
  enough.
- move the prefix to pr_fmt
- change prefix from "of_i2c" to "i2c_of" because the code was moved
  out of the of-domain long ago
- drop error string for callers of of_i2c_register_device because it
  already reports enough (thanks to Tang Bin for the report!)

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/i2c/i2c-core-of.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 710704cd583e..f2d09ea0d336 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -8,6 +8,8 @@
  * Copyright (C) 2013, 2018 Wolfram Sang <wsa@the-dreams.de>
  */
 
+#define pr_fmt(fmt) "i2c_of: " fmt
+
 #include <dt-bindings/i2c/i2c.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -28,7 +30,7 @@ int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
 
 	ret = of_property_read_u32(node, "reg", &addr);
 	if (ret) {
-		pr_err("of_i2c: invalid reg on %pOF\n", node);
+		pr_err("invalid reg on %pOF\n", node);
 		return ret;
 	}
 
@@ -66,7 +68,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 	struct i2c_board_info info;
 	int ret;
 
-	dev_dbg(&adap->dev, "of_i2c: register %pOF\n", node);
+	pr_debug("register %pOF\n", node);
 
 	ret = of_i2c_get_board_info(node, &info);
 	if (ret)
@@ -74,7 +76,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 
 	client = i2c_new_client_device(adap, &info);
 	if (IS_ERR(client))
-		dev_err(&adap->dev, "of_i2c: Failure registering %pOF\n", node);
+		pr_err("failure registering %pOF (%ld)\n", node, PTR_ERR(client));
 
 	return client;
 }
@@ -88,7 +90,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 	if (!adap->dev.of_node)
 		return;
 
-	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
+	dev_dbg(&adap->dev, "walking child nodes\n");
 
 	bus = of_get_child_by_name(adap->dev.of_node, "i2c-bus");
 	if (!bus)
@@ -99,12 +101,8 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 			continue;
 
 		client = of_i2c_register_device(adap, node);
-		if (IS_ERR(client)) {
-			dev_err(&adap->dev,
-				 "Failed to create I2C device for %pOF\n",
-				 node);
+		if (IS_ERR(client))
 			of_node_clear_flag(node, OF_POPULATED);
-		}
 	}
 
 	of_node_put(bus);
@@ -243,8 +241,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 
 		client = of_i2c_register_device(adap, rd->dn);
 		if (IS_ERR(client)) {
-			dev_err(&adap->dev, "failed to create client for '%pOF'\n",
-				 rd->dn);
 			put_device(&adap->dev);
 			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(PTR_ERR(client));
-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
                   ` (3 preceding siblings ...)
  2020-03-18 15:00 ` [RFC PATCH v2 4/6] i2c: of: error message unification Wolfram Sang
@ 2020-03-18 15:00 ` Wolfram Sang
  2020-04-10 17:05   ` Luca Ceresoli
                     ` (2 more replies)
  2020-03-18 15:00 ` [RFC PATCH v2 6/6] i2c: core: hand over reserved devices when requesting ancillary addresses Wolfram Sang
                   ` (2 subsequent siblings)
  7 siblings, 3 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jacopo Mondi, Niklas Söderlund, Wolfram Sang, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham,
	Laurent Pinchart, Luca Ceresoli, linux-i3c

Back then, 'reg' properties in I2C DT bindings only contained one
address and this address was assigned a device and, thus, blocked.
Meanwhile, chips using multiple addresses are common and the 'reg'
property can be an array described by 'reg-names'. This code enhances
I2C DT parsing, so it will reserve all addresses described in an array.
They will be bound to the 'dummy' driver as 'reserved' iff the first
address can be assigned successfully. If that is not the case, the array
is not further considered. If one later address of the array can not be
assigned, it will be reported but we don't bail out. The driver has to
decide if that address is critical or not.

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

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index f2d09ea0d336..67eb2cd305cf 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -16,25 +16,18 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/sysfs.h>
 
 #include "i2c-core.h"
 
-int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
+static void of_i2c_decode_board_info(struct device_node *node, u32 addr,
+				     bool first_addr, struct i2c_board_info *info)
 {
-	u32 addr;
-	int ret;
-
 	memset(info, 0, sizeof(*info));
 
-	ret = of_property_read_u32(node, "reg", &addr);
-	if (ret) {
-		pr_err("invalid reg on %pOF\n", node);
-		return ret;
-	}
-
-	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
+	if (!first_addr || of_modalias_node(node, info->type, sizeof(info->type)) < 0)
 		strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));
 
 	if (addr & I2C_TEN_BIT_ADDRESS) {
@@ -51,11 +44,27 @@ int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
 	info->of_node = node;
 	info->fwnode = of_fwnode_handle(node);
 
-	if (of_property_read_bool(node, "host-notify"))
-		info->flags |= I2C_CLIENT_HOST_NOTIFY;
+	if (first_addr) {
+		if (of_property_read_bool(node, "host-notify"))
+			info->flags |= I2C_CLIENT_HOST_NOTIFY;
+
+		if (of_get_property(node, "wakeup-source", NULL))
+			info->flags |= I2C_CLIENT_WAKE;
+	}
+}
+
+int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
+{
+	u32 addr;
+	int ret;
+
+	ret = of_property_read_u32(node, "reg", &addr);
+	if (ret) {
+		pr_err("invalid reg on %pOF\n", node);
+		return ret;
+	}
 
-	if (of_get_property(node, "wakeup-source", NULL))
-		info->flags |= I2C_CLIENT_WAKE;
+	of_i2c_decode_board_info(node, addr, true, info);
 
 	return 0;
 }
@@ -64,21 +73,34 @@ EXPORT_SYMBOL_GPL(of_i2c_get_board_info);
 static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 						 struct device_node *node)
 {
-	struct i2c_client *client;
+	struct i2c_client *client, *first_client = ERR_PTR(-ENOENT);
 	struct i2c_board_info info;
-	int ret;
+	bool first_reg = true;
+	unsigned int i = 0;
+	const __be32 *prop;
+	u16 reg;
 
 	pr_debug("register %pOF\n", node);
 
-	ret = of_i2c_get_board_info(node, &info);
-	if (ret)
-		return ERR_PTR(ret);
+	while ((prop = of_get_address(node, i++, NULL, NULL))) {
+		reg = of_read_number(prop, 1);
+		of_i2c_decode_board_info(node, reg, first_reg, &info);
+
+		client = i2c_new_client_device(adap, &info);
+		if (IS_ERR(client)) {
+			pr_err("failure registering addr 0x%02x for %pOF (%ld)\n",
+				reg, node, PTR_ERR(client));
+			if (first_reg)
+				return client;
+		}
 
-	client = i2c_new_client_device(adap, &info);
-	if (IS_ERR(client))
-		pr_err("failure registering %pOF (%ld)\n", node, PTR_ERR(client));
+		if (first_reg) {
+			first_client = client;
+			first_reg = false;
+		}
+	}
 
-	return client;
+	return first_client;
 }
 
 void of_i2c_register_devices(struct i2c_adapter *adap)
-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* [RFC PATCH v2 6/6] i2c: core: hand over reserved devices when requesting ancillary addresses
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
                   ` (4 preceding siblings ...)
  2020-03-18 15:00 ` [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved Wolfram Sang
@ 2020-03-18 15:00 ` Wolfram Sang
  2020-04-15 10:07   ` Luca Ceresoli
  2020-03-28  3:50 ` [RFC PATCH v2 0/6] i2c: of: reserve unknown and " Wolfram Sang
  2020-04-15  8:27 ` Wolfram Sang
  7 siblings, 1 reply; 29+ messages in thread
From: Wolfram Sang @ 2020-03-18 15:00 UTC (permalink / raw)
  To: linux-i2c
  Cc: Jacopo Mondi, Niklas Söderlund, Wolfram Sang, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham,
	Laurent Pinchart, Luca Ceresoli, linux-i3c

With i2c_new_ancillary_address, we can check if the intended driver is
requesting a reserved address. Update the function to do these checks.
If the check passes, the "reserved" device will become a regular "dummy"
device.

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

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 84464e439df5..81fb320de28d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -974,7 +974,9 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
 						const char *name,
 						u16 default_addr)
 {
-	struct device_node *np = client->dev.of_node;
+	struct device_node *reserved_np, *np = client->dev.of_node;
+	struct device *reserved_dev, *adapter_dev = &client->adapter->dev;
+	struct i2c_client *reserved_client = NULL;
 	u32 addr = default_addr;
 	int i;
 
@@ -984,7 +986,25 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
 			of_property_read_u32_index(np, "reg", i, &addr);
 	}
 
-	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
+	dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);
+
+	/* No need to scan muxes, siblings must sit on the same adapter */
+	reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
+	if (reserved_dev) {
+		reserved_np = reserved_dev->of_node;
+		reserved_client = i2c_verify_client(reserved_dev);
+		put_device(reserved_dev);
+	}
+
+	if (reserved_client) {
+		if (reserved_np != np ||
+		    strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
+			return ERR_PTR(-EBUSY);
+
+		strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));
+		return reserved_client;
+	}
+
 	return i2c_new_dummy_device(client->adapter, addr);
 }
 EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);
-- 
2.20.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function
  2020-03-18 15:00 ` [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function Wolfram Sang
@ 2020-03-19 12:41   ` Boris Brezillon
  2020-04-15  8:13   ` Kieran Bingham
  1 sibling, 0 replies; 29+ messages in thread
From: Boris Brezillon @ 2020-03-19 12:41 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jacopo Mondi, Luca Ceresoli, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham, linux-i2c,
	Niklas Söderlund, linux-i3c, Laurent Pinchart

On Wed, 18 Mar 2020 16:00:56 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> 'dev' is only used for printing an error message. However, that
> information is not needed because '%pOF' fully describes the location of
> the error. Drop the 'dev' and remove the superfluous parameter.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/i2c/i2c-core-of.c | 7 +++----
>  drivers/i3c/master.c      | 2 +-
>  include/linux/i2c.h       | 6 ++----
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index d8d111ad6c85..710704cd583e 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -19,8 +19,7 @@
>  
>  #include "i2c-core.h"
>  
> -int of_i2c_get_board_info(struct device *dev, struct device_node *node,
> -			  struct i2c_board_info *info)
> +int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
>  {
>  	u32 addr;
>  	int ret;
> @@ -29,7 +28,7 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
>  
>  	ret = of_property_read_u32(node, "reg", &addr);
>  	if (ret) {
> -		dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
> +		pr_err("of_i2c: invalid reg on %pOF\n", node);
>  		return ret;
>  	}
>  
> @@ -69,7 +68,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>  
>  	dev_dbg(&adap->dev, "of_i2c: register %pOF\n", node);
>  
> -	ret = of_i2c_get_board_info(&adap->dev, node, &info);
> +	ret = of_i2c_get_board_info(node, &info);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 7f8f896fa0c3..cc0549a9fc64 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1943,7 +1943,7 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
>  	if (!boardinfo)
>  		return -ENOMEM;
>  
> -	ret = of_i2c_get_board_info(dev, node, &boardinfo->base);
> +	ret = of_i2c_get_board_info(node, &boardinfo->base);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index f834687989f7..d84aaf0d83d5 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -942,8 +942,7 @@ const struct of_device_id
>  *i2c_of_match_device(const struct of_device_id *matches,
>  		     struct i2c_client *client);
>  
> -int of_i2c_get_board_info(struct device *dev, struct device_node *node,
> -			  struct i2c_board_info *info);
> +int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info);
>  
>  #else
>  
> @@ -969,8 +968,7 @@ static inline const struct of_device_id
>  	return NULL;
>  }
>  
> -static inline int of_i2c_get_board_info(struct device *dev,
> -					struct device_node *node,
> +static inline int of_i2c_get_board_info(struct device_node *node,
>  					struct i2c_board_info *info)
>  {
>  	return -ENOTSUPP;


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
                   ` (5 preceding siblings ...)
  2020-03-18 15:00 ` [RFC PATCH v2 6/6] i2c: core: hand over reserved devices when requesting ancillary addresses Wolfram Sang
@ 2020-03-28  3:50 ` Wolfram Sang
  2020-04-15  8:27 ` Wolfram Sang
  7 siblings, 0 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-03-28  3:50 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jacopo Mondi, Niklas Söderlund, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham, linux-i2c,
	Luca Ceresoli, linux-i3c, Laurent Pinchart

[-- Attachment #1.1: Type: text/plain, Size: 998 bytes --]


> There is only one thing giving me some headache now. There is a danger
> of a regression maybe. If someone has multiple 'reg' entries in the DT
> but never used i2c_new_ancillary_device but i2c_new_dummy_device, then
> things will break now because i2c_new_dummy_device has not enough
> information to convert a "reserved" device to a "dummy" one. It will
> just see the address as busy. However, all binding documentations I
> found which use 'reg' as an array correctly use
> i2c_new_ancillary_device. On the other hand, my search strategy for
> finding such bindings and DTs do not feel perfect to me. Maybe there are
> also some more corner cases in this area, so this series is still RFC.

So, I used another search strategy: I checked every
i2c_new_dummy_device() caller in the kernel tree and made sure they
don't get the address to use from DT. I can confirm this is not the
case. That gives me enough trust to say the above issue is a non-issue.

Still open for comments, of course...


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

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-03-18 15:00 ` [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible' Wolfram Sang
@ 2020-04-10 13:49   ` Luca Ceresoli
  2020-04-15  7:59     ` Wolfram Sang
  2020-04-15  8:48   ` Kieran Bingham
  1 sibling, 1 reply; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-10 13:49 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Rob Herring, Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi,

On 18/03/20 16:00, Wolfram Sang wrote:
> Sometimes, we have unknown devices in a system and still want to block
> their address. For that, we allow DT nodes with only a 'reg' property.
> These devices will be bound to the "dummy" driver but with the name
> "reserved". That way, we can distinguish them and even hand them over to
> the "dummy" driver later when they are really requested using
> i2c_new_ancillary_device().
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

As I said in the reply to v1, I think we should reserve addresses also
when there is a compatible string but no matching driver, but this is
another story and can be handled separately.

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 4/6] i2c: of: error message unification
  2020-03-18 15:00 ` [RFC PATCH v2 4/6] i2c: of: error message unification Wolfram Sang
@ 2020-04-10 17:02   ` Luca Ceresoli
  2020-04-15  8:17   ` Kieran Bingham
  1 sibling, 0 replies; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-10 17:02 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi,

On 18/03/20 16:00, Wolfram Sang wrote:
> - don't prefix the device if %pOF is provided. That information is
>   enough.
> - move the prefix to pr_fmt
> - change prefix from "of_i2c" to "i2c_of" because the code was moved
>   out of the of-domain long ago
> - drop error string for callers of of_i2c_register_device because it
>   already reports enough (thanks to Tang Bin for the report!)
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved
  2020-03-18 15:00 ` [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved Wolfram Sang
@ 2020-04-10 17:05   ` Luca Ceresoli
  2020-04-13  9:55   ` Luca Ceresoli
  2020-04-15 10:07   ` Luca Ceresoli
  2 siblings, 0 replies; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-10 17:05 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, linux-i3c

On 18/03/20 16:00, Wolfram Sang wrote:
> Back then, 'reg' properties in I2C DT bindings only contained one
> address and this address was assigned a device and, thus, blocked.
> Meanwhile, chips using multiple addresses are common and the 'reg'
> property can be an array described by 'reg-names'. This code enhances
> I2C DT parsing, so it will reserve all addresses described in an array.
> They will be bound to the 'dummy' driver as 'reserved' iff the first
> address can be assigned successfully. If that is not the case, the array
> is not further considered. If one later address of the array can not be
> assigned, it will be reported but we don't bail out. The driver has to
> decide if that address is critical or not.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved
  2020-03-18 15:00 ` [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved Wolfram Sang
  2020-04-10 17:05   ` Luca Ceresoli
@ 2020-04-13  9:55   ` Luca Ceresoli
  2020-04-15  8:10     ` Wolfram Sang
  2020-04-15 10:07   ` Luca Ceresoli
  2 siblings, 1 reply; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-13  9:55 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, linux-i3c

Hi,

On 18/03/20 16:00, Wolfram Sang wrote:
> Back then, 'reg' properties in I2C DT bindings only contained one
> address and this address was assigned a device and, thus, blocked.
> Meanwhile, chips using multiple addresses are common and the 'reg'
> property can be an array described by 'reg-names'. This code enhances
> I2C DT parsing, so it will reserve all addresses described in an array.
> They will be bound to the 'dummy' driver as 'reserved' iff the first
> address can be assigned successfully. If that is not the case, the array
> is not further considered. If one later address of the array can not be
> assigned, it will be reported but we don't bail out. The driver has to
> decide if that address is critical or not.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-of.c | 70 +++++++++++++++++++++++++--------------
>  1 file changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index f2d09ea0d336..67eb2cd305cf 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -16,25 +16,18 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_device.h>
>  #include <linux/sysfs.h>
>  
>  #include "i2c-core.h"
>  
> -int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
> +static void of_i2c_decode_board_info(struct device_node *node, u32 addr,
> +				     bool first_addr, struct i2c_board_info *info)

While I confirm the patch looks generally OK, let me add the name of
this function is not quite self-explaining. The difference between "get"
and "decode" has nothing to do with the different actions these
functions do, i.e. the new function gets (or: decodes) info about  a
single address that is passed, the old "get" function gets the info for
the first address.

I'd suggest the new function be named of_i2c_get_board_info_one_addr or
similar. Not super nice, a bit long, but self-explanatory.

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-04-10 13:49   ` Luca Ceresoli
@ 2020-04-15  7:59     ` Wolfram Sang
  2020-04-15  8:07       ` Kieran Bingham
  2020-04-16 14:53       ` Luca Ceresoli
  0 siblings, 2 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-04-15  7:59 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Rob Herring, Jacopo Mondi, Geert Uytterhoeven, Kieran Bingham,
	linux-kernel, Vladimir Zapolskiy, linux-renesas-soc,
	Wolfram Sang, linux-i2c, Niklas Söderlund, linux-i3c,
	Laurent Pinchart

[-- Attachment #1.1: Type: text/plain, Size: 554 bytes --]


> As I said in the reply to v1, I think we should reserve addresses also
> when there is a compatible string but no matching driver, but this is
> another story and can be handled separately.

Unless I misunderstand you, I think they do already. Note that
only 'i2cdetect' shows a device as busy *IFF* there is a driver bound to
it. The internal 'i2c_check_addr_busy' does not care about a driver
being bound. You can check this by trying to use
i2c_new_ancillary_device() with an address which is already described in
DT but which driver is disabled.


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

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-04-15  7:59     ` Wolfram Sang
@ 2020-04-15  8:07       ` Kieran Bingham
  2020-04-15  8:16         ` Wolfram Sang
  2020-04-16 14:53       ` Luca Ceresoli
  1 sibling, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:07 UTC (permalink / raw)
  To: Wolfram Sang, Luca Ceresoli
  Cc: Rob Herring, Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Wolfram Sang, linux-i2c,
	Niklas Söderlund, linux-i3c, Laurent Pinchart

Hi Wolfram,

On 15/04/2020 08:59, Wolfram Sang wrote:
> 
>> As I said in the reply to v1, I think we should reserve addresses also
>> when there is a compatible string but no matching driver, but this is
>> another story and can be handled separately.
> 
> Unless I misunderstand you, I think they do already. Note that
> only 'i2cdetect' shows a device as busy *IFF* there is a driver bound to
> it. The internal 'i2c_check_addr_busy' does not care about a driver
> being bound. You can check this by trying to use
> i2c_new_ancillary_device() with an address which is already described in
> DT but which driver is disabled.

Aha, is it easy enough to distinguish that difference in user-space so
that we can present a specific character to indicate this in i2cdetect?
Or is that not so easy?

--
Kieran



_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name
  2020-03-18 15:00 ` [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name Wolfram Sang
@ 2020-04-15  8:09   ` Kieran Bingham
  0 siblings, 0 replies; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:09 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Luca Ceresoli,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi Wolfram,

On 18/03/2020 15:00, Wolfram Sang wrote:
> We use it in multiple places, so make sure it is consistent whenever we
> need to change it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

> ---
>  drivers/i2c/i2c-core-base.c | 8 ++++----
>  drivers/i2c/i2c-core.h      | 2 ++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index cefad0881942..3d7b8a00a7d9 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -853,7 +853,7 @@ EXPORT_SYMBOL_GPL(i2c_unregister_device);
>  
>  
>  static const struct i2c_device_id dummy_id[] = {
> -	{ "dummy", 0 },
> +	{ I2C_DUMMY_DRV_NAME, 0 },
>  	{ },
>  };
>  
> @@ -869,7 +869,7 @@ static int dummy_remove(struct i2c_client *client)
>  }
>  
>  static struct i2c_driver dummy_driver = {
> -	.driver.name	= "dummy",
> +	.driver.name	= I2C_DUMMY_DRV_NAME,
>  	.probe		= dummy_probe,
>  	.remove		= dummy_remove,
>  	.id_table	= dummy_id,
> @@ -896,7 +896,7 @@ static struct i2c_driver dummy_driver = {
>  struct i2c_client *i2c_new_dummy_device(struct i2c_adapter *adapter, u16 address)
>  {
>  	struct i2c_board_info info = {
> -		I2C_BOARD_INFO("dummy", address),
> +		I2C_BOARD_INFO(I2C_DUMMY_DRV_NAME, address),
>  	};
>  
>  	return i2c_new_client_device(adapter, &info);
> @@ -1487,7 +1487,7 @@ static void i2c_do_del_adapter(struct i2c_driver *driver,
>  static int __unregister_client(struct device *dev, void *dummy)
>  {
>  	struct i2c_client *client = i2c_verify_client(dev);
> -	if (client && strcmp(client->name, "dummy"))
> +	if (client && strcmp(client->name, I2C_DUMMY_DRV_NAME))
>  		i2c_unregister_device(client);
>  	return 0;
>  }
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 517d98be68d2..fb89fabf84d3 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -22,6 +22,8 @@ int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>  int i2c_dev_irq_from_resources(const struct resource *resources,
>  			       unsigned int num_resources);
>  
> +#define I2C_DUMMY_DRV_NAME "dummy"
> +
>  /*
>   * We only allow atomic transfers for very late communication, e.g. to send
>   * the powerdown command to a PMIC. Atomic transfers are a corner case and not
> 


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved
  2020-04-13  9:55   ` Luca Ceresoli
@ 2020-04-15  8:10     ` Wolfram Sang
  0 siblings, 0 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-04-15  8:10 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: Jacopo Mondi, Kieran Bingham, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Wolfram Sang, linux-i2c,
	Niklas Söderlund, linux-i3c, Laurent Pinchart

[-- Attachment #1.1: Type: text/plain, Size: 1211 bytes --]


> > -int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
> > +static void of_i2c_decode_board_info(struct device_node *node, u32 addr,
> > +				     bool first_addr, struct i2c_board_info *info)
> 
> While I confirm the patch looks generally OK, let me add the name of
> this function is not quite self-explaining. The difference between "get"
> and "decode" has nothing to do with the different actions these
> functions do, i.e. the new function gets (or: decodes) info about  a
> single address that is passed, the old "get" function gets the info for
> the first address.
>
> I'd suggest the new function be named of_i2c_get_board_info_one_addr or
> similar. Not super nice, a bit long, but self-explanatory.

I view them a bit differently, I think. of_i2c_decode_board_info() is a
helper function to retrieve "some" addr. It is used by
of_i2c_get_board_info() which has the special case of getting the first
address. of_i2c_register_device() is the other user with the case of
getting each address specified. So, I wouldn't put this helper function
on the same level as the users of this helper.

Yet, no strong opinion here, I will think about it...


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

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function
  2020-03-18 15:00 ` [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function Wolfram Sang
  2020-03-19 12:41   ` Boris Brezillon
@ 2020-04-15  8:13   ` Kieran Bingham
  1 sibling, 0 replies; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:13 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Luca Ceresoli,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi Wolfram,

On 18/03/2020 15:00, Wolfram Sang wrote:
> 'dev' is only used for printing an error message. However, that
> information is not needed because '%pOF' fully describes the location of
> the error. Drop the 'dev' and remove the superfluous parameter.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

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

> ---
>  drivers/i2c/i2c-core-of.c | 7 +++----
>  drivers/i3c/master.c      | 2 +-
>  include/linux/i2c.h       | 6 ++----
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index d8d111ad6c85..710704cd583e 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -19,8 +19,7 @@
>  
>  #include "i2c-core.h"
>  
> -int of_i2c_get_board_info(struct device *dev, struct device_node *node,
> -			  struct i2c_board_info *info)
> +int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
>  {
>  	u32 addr;
>  	int ret;
> @@ -29,7 +28,7 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
>  
>  	ret = of_property_read_u32(node, "reg", &addr);
>  	if (ret) {
> -		dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
> +		pr_err("of_i2c: invalid reg on %pOF\n", node);
>  		return ret;
>  	}
>  
> @@ -69,7 +68,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>  
>  	dev_dbg(&adap->dev, "of_i2c: register %pOF\n", node);
>  
> -	ret = of_i2c_get_board_info(&adap->dev, node, &info);
> +	ret = of_i2c_get_board_info(node, &info);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 7f8f896fa0c3..cc0549a9fc64 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1943,7 +1943,7 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
>  	if (!boardinfo)
>  		return -ENOMEM;
>  
> -	ret = of_i2c_get_board_info(dev, node, &boardinfo->base);
> +	ret = of_i2c_get_board_info(node, &boardinfo->base);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index f834687989f7..d84aaf0d83d5 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -942,8 +942,7 @@ const struct of_device_id
>  *i2c_of_match_device(const struct of_device_id *matches,
>  		     struct i2c_client *client);
>  
> -int of_i2c_get_board_info(struct device *dev, struct device_node *node,
> -			  struct i2c_board_info *info);
> +int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info);
>  
>  #else
>  
> @@ -969,8 +968,7 @@ static inline const struct of_device_id
>  	return NULL;
>  }
>  
> -static inline int of_i2c_get_board_info(struct device *dev,
> -					struct device_node *node,
> +static inline int of_i2c_get_board_info(struct device_node *node,
>  					struct i2c_board_info *info)
>  {
>  	return -ENOTSUPP;
> 


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-04-15  8:07       ` Kieran Bingham
@ 2020-04-15  8:16         ` Wolfram Sang
  2020-04-15  8:38           ` Kieran Bingham
  0 siblings, 1 reply; 29+ messages in thread
From: Wolfram Sang @ 2020-04-15  8:16 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Rob Herring, Jacopo Mondi, Niklas Söderlund,
	Geert Uytterhoeven, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Wolfram Sang, linux-i2c, Luca Ceresoli,
	linux-i3c, Laurent Pinchart

[-- Attachment #1.1: Type: text/plain, Size: 763 bytes --]


> Aha, is it easy enough to distinguish that difference in user-space so
> that we can present a specific character to indicate this in i2cdetect?
> Or is that not so easy?

I thought about it shortly but have not come up with a way of doing
that. This is the code in i2cdetect:

	/* Set slave address */
	if (ioctl(file, I2C_SLAVE, i+j) < 0) {
		if (errno == EBUSY) {
			printf("UU ");
			continue;
		} else {
			fprintf(stderr, "Error: Could not set "
				"address to 0x%02x: %s\n", i+j,
				strerror(errno));
			return -1;
		}
	}

So, if we chose to use another errno to indicate 'reserved' and update
i2cdetect, all old versions of i2cdetect will have ugly error messages.
And adding another IOCTL just for printing reserved addresses neither
sounds great.


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

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 4/6] i2c: of: error message unification
  2020-03-18 15:00 ` [RFC PATCH v2 4/6] i2c: of: error message unification Wolfram Sang
  2020-04-10 17:02   ` Luca Ceresoli
@ 2020-04-15  8:17   ` Kieran Bingham
  2020-04-15  8:50     ` Kieran Bingham
  1 sibling, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:17 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Luca Ceresoli,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi Wolfram,

On 18/03/2020 15:00, Wolfram Sang wrote:
> - don't prefix the device if %pOF is provided. That information is
>   enough.
> - move the prefix to pr_fmt
> - change prefix from "of_i2c" to "i2c_of" because the code was moved
>   out of the of-domain long ago
> - drop error string for callers of of_i2c_register_device because it
>   already reports enough (thanks to Tang Bin for the report!)
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

> ---
>  drivers/i2c/i2c-core-of.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 710704cd583e..f2d09ea0d336 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -8,6 +8,8 @@
>   * Copyright (C) 2013, 2018 Wolfram Sang <wsa@the-dreams.de>
>   */
>  
> +#define pr_fmt(fmt) "i2c_of: " fmt
> +
>  #include <dt-bindings/i2c/i2c.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -28,7 +30,7 @@ int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
>  
>  	ret = of_property_read_u32(node, "reg", &addr);
>  	if (ret) {
> -		pr_err("of_i2c: invalid reg on %pOF\n", node);
> +		pr_err("invalid reg on %pOF\n", node);
>  		return ret;
>  	}
>  
> @@ -66,7 +68,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>  	struct i2c_board_info info;
>  	int ret;
>  
> -	dev_dbg(&adap->dev, "of_i2c: register %pOF\n", node);
> +	pr_debug("register %pOF\n", node);
>  
>  	ret = of_i2c_get_board_info(node, &info);
>  	if (ret)
> @@ -74,7 +76,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>  
>  	client = i2c_new_client_device(adap, &info);
>  	if (IS_ERR(client))
> -		dev_err(&adap->dev, "of_i2c: Failure registering %pOF\n", node);
> +		pr_err("failure registering %pOF (%ld)\n", node, PTR_ERR(client));
>  
>  	return client;
>  }
> @@ -88,7 +90,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>  	if (!adap->dev.of_node)
>  		return;
>  
> -	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
> +	dev_dbg(&adap->dev, "walking child nodes\n");
>  
>  	bus = of_get_child_by_name(adap->dev.of_node, "i2c-bus");
>  	if (!bus)
> @@ -99,12 +101,8 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>  			continue;
>  
>  		client = of_i2c_register_device(adap, node);
> -		if (IS_ERR(client)) {
> -			dev_err(&adap->dev,
> -				 "Failed to create I2C device for %pOF\n",
> -				 node);
> +		if (IS_ERR(client))
>  			of_node_clear_flag(node, OF_POPULATED);
> -		}
>  	}
>  
>  	of_node_put(bus);
> @@ -243,8 +241,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
>  
>  		client = of_i2c_register_device(adap, rd->dn);
>  		if (IS_ERR(client)) {
> -			dev_err(&adap->dev, "failed to create client for '%pOF'\n",
> -				 rd->dn);
>  			put_device(&adap->dev);
>  			of_node_clear_flag(rd->dn, OF_POPULATED);
>  			return notifier_from_errno(PTR_ERR(client));
> 


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses
  2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
                   ` (6 preceding siblings ...)
  2020-03-28  3:50 ` [RFC PATCH v2 0/6] i2c: of: reserve unknown and " Wolfram Sang
@ 2020-04-15  8:27 ` Wolfram Sang
  2020-04-15  8:35   ` Kieran Bingham
  7 siblings, 1 reply; 29+ messages in thread
From: Wolfram Sang @ 2020-04-15  8:27 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Jacopo Mondi, Niklas Söderlund, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Kieran Bingham, linux-i2c,
	Luca Ceresoli, linux-i3c, Laurent Pinchart

[-- Attachment #1.1: Type: text/plain, Size: 1028 bytes --]


Status update on this series:

> TODO: make sure there are no concurrency issues in patch 6 when handling
> the struct i2c_client.

This turns out to be annoying. How to make sure that we don't modify the
i2c_client while the adapter it is sitting on just gets removed. AFAICS
we need a new locking scheme just for that and I am not convinced this
is the way forward.

Also, there is still this small room for regressing when there are DTs
having multiple addresses specified in the DT and the drivers use
i2c_new_dummy_client on these addresses. I have verified that no in-tree
users of i2c_new_dummy (and friends) do work on extra addresses but
still I'd like to completely avoid this potential regression.

One solution to both problems would be to unregister the reserved device
when its address is requested. I am working on this prototype currently.
However, I am not sure yet if one issue might make this approach messy:
re-registering the reserved device when the probe of the requested
address fails.

We will see...


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

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses
  2020-04-15  8:27 ` Wolfram Sang
@ 2020-04-15  8:35   ` Kieran Bingham
  0 siblings, 0 replies; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:35 UTC (permalink / raw)
  To: Wolfram Sang, Wolfram Sang
  Cc: Jacopo Mondi, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Luca Ceresoli, linux-i2c,
	Niklas Söderlund, linux-i3c, Laurent Pinchart

On 15/04/2020 09:27, Wolfram Sang wrote:
> 
> Status update on this series:
> 
>> TODO: make sure there are no concurrency issues in patch 6 when handling
>> the struct i2c_client.
> 
> This turns out to be annoying. How to make sure that we don't modify the
> i2c_client while the adapter it is sitting on just gets removed. AFAICS
> we need a new locking scheme just for that and I am not convinced this
> is the way forward.
> 
> Also, there is still this small room for regressing when there are DTs
> having multiple addresses specified in the DT and the drivers use
> i2c_new_dummy_client on these addresses. I have verified that no in-tree
> users of i2c_new_dummy (and friends) do work on extra addresses but
> still I'd like to completely avoid this potential regression.
> 
> One solution to both problems would be to unregister the reserved device
> when its address is requested. I am working on this prototype currently.
> However, I am not sure yet if one issue might make this approach messy:
> re-registering the reserved device when the probe of the requested
> address fails.

If we 'unregister' the existing device, could we then register a new
'well named' device more appropriate to the driver, so it doesn't
continue to show up as 'reserved' in the system, but rather a more
appropriate name to the driver that registered it?

> We will see...
> 

Looking forward to it :-)

--
Kieran

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-04-15  8:16         ` Wolfram Sang
@ 2020-04-15  8:38           ` Kieran Bingham
  0 siblings, 0 replies; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:38 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Rob Herring, Jacopo Mondi, Niklas Söderlund,
	Geert Uytterhoeven, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Wolfram Sang, linux-i2c, Luca Ceresoli,
	linux-i3c, Laurent Pinchart

Hi Wolfram,

On 15/04/2020 09:16, Wolfram Sang wrote:
> 
>> Aha, is it easy enough to distinguish that difference in user-space so
>> that we can present a specific character to indicate this in i2cdetect?
>> Or is that not so easy?
> 
> I thought about it shortly but have not come up with a way of doing
> that. This is the code in i2cdetect:
> 
> 	/* Set slave address */
> 	if (ioctl(file, I2C_SLAVE, i+j) < 0) {
> 		if (errno == EBUSY) {
> 			printf("UU ");
> 			continue;
> 		} else {
> 			fprintf(stderr, "Error: Could not set "
> 				"address to 0x%02x: %s\n", i+j,
> 				strerror(errno));
> 			return -1;
> 		}
> 	}
> 
> So, if we chose to use another errno to indicate 'reserved' and update
> i2cdetect, all old versions of i2cdetect will have ugly error messages.
> And adding another IOCTL just for printing reserved addresses neither
> sounds great.

Indeed, a different errno would be about all we could do - and it would
seemingly leave old i2cdetects with complete failures, if it goes
through that non-EBUSY code path.

Ayeeeee :-S

--
Kieran


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-03-18 15:00 ` [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible' Wolfram Sang
  2020-04-10 13:49   ` Luca Ceresoli
@ 2020-04-15  8:48   ` Kieran Bingham
  2020-04-15  9:46     ` Wolfram Sang
  1 sibling, 1 reply; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:48 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Rob Herring, Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Luca Ceresoli,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi Wolfram,

On 18/03/2020 15:00, Wolfram Sang wrote:
> Sometimes, we have unknown devices in a system and still want to block
> their address. For that, we allow DT nodes with only a 'reg' property.
> These devices will be bound to the "dummy" driver but with the name
> "reserved". That way, we can distinguish them and even hand them over to
> the "dummy" driver later when they are really requested using
> i2c_new_ancillary_device().

Oh how I long to be able to give these 'identifiable names' within the
system, but that will probably mess up all the driver matching and
binding, so would be quite tricky perhaps.

But I like the ability to distinguish the two different types.

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


> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 2 --
>  Documentation/devicetree/bindings/i2c/i2c.txt        | 4 +++-
>  drivers/i2c/i2c-core-base.c                          | 1 +
>  drivers/i2c/i2c-core-of.c                            | 8 +++-----
>  drivers/i2c/i2c-core.h                               | 1 +
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> index 6b25a80ae8d3..fc8ea27934b3 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> @@ -50,7 +50,6 @@ Examples:
>  		reg-io-width = <1>;	/* 8 bit read/write */
>  
>  		dummy@60 {
> -			compatible = "dummy";
>  			reg = <0x60>;
>  		};
>  	};
> @@ -68,7 +67,6 @@ or
>  		reg-io-width = <1>;	/* 8 bit read/write */
>  
>  		dummy@60 {
> -			compatible = "dummy";
>  			reg = <0x60>;
>  		};
>  	};
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index 9a53df4243c6..989b315e09dc 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -21,7 +21,9 @@ flags can be attached to the address. I2C_TEN_BIT_ADDRESS is used to mark a 10
>  bit address. It is needed to avoid the ambiguity between e.g. a 7 bit address
>  of 0x50 and a 10 bit address of 0x050 which, in theory, can be on the same bus.
>  Another flag is I2C_OWN_SLAVE_ADDRESS to mark addresses on which we listen to
> -be devices ourselves.
> +be devices ourselves. The 'reg' property of a child is required. The
> +'compatible' property is not. Empty 'compatible' child entries can be used to
> +describe unknown devices or addresses which shall be blocked for other reasons.
>  
>  Optional properties
>  -------------------
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 3d7b8a00a7d9..84464e439df5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -854,6 +854,7 @@ EXPORT_SYMBOL_GPL(i2c_unregister_device);
>  
>  static const struct i2c_device_id dummy_id[] = {
>  	{ I2C_DUMMY_DRV_NAME, 0 },
> +	{ I2C_RESERVED_DRV_NAME, 0 },
>  	{ },
>  };
>  
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 6787c1f71483..d8d111ad6c85 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -27,17 +27,15 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
>  
>  	memset(info, 0, sizeof(*info));
>  
> -	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0) {
> -		dev_err(dev, "of_i2c: modalias failure on %pOF\n", node);
> -		return -EINVAL;
> -	}
> -
>  	ret = of_property_read_u32(node, "reg", &addr);
>  	if (ret) {
>  		dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
>  		return ret;
>  	}
>  
> +	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
> +		strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));
> +
>  	if (addr & I2C_TEN_BIT_ADDRESS) {
>  		addr &= ~I2C_TEN_BIT_ADDRESS;
>  		info->flags |= I2C_CLIENT_TEN;
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index fb89fabf84d3..77b3a925ed95 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -23,6 +23,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
>  			       unsigned int num_resources);
>  
>  #define I2C_DUMMY_DRV_NAME "dummy"
> +#define I2C_RESERVED_DRV_NAME "reserved"
>  
>  /*
>   * We only allow atomic transfers for very late communication, e.g. to send
> 


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 4/6] i2c: of: error message unification
  2020-04-15  8:17   ` Kieran Bingham
@ 2020-04-15  8:50     ` Kieran Bingham
  0 siblings, 0 replies; 29+ messages in thread
From: Kieran Bingham @ 2020-04-15  8:50 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, Geert Uytterhoeven, linux-kernel,
	Vladimir Zapolskiy, linux-renesas-soc, Luca Ceresoli,
	Laurent Pinchart, Niklas Söderlund, linux-i3c

Hi Wolfram,

On 15/04/2020 09:17, Kieran Bingham wrote:
> Hi Wolfram,
> 
> On 18/03/2020 15:00, Wolfram Sang wrote:
>> - don't prefix the device if %pOF is provided. That information is
>>   enough.
>> - move the prefix to pr_fmt
>> - change prefix from "of_i2c" to "i2c_of" because the code was moved
>>   out of the of-domain long ago
>> - drop error string for callers of of_i2c_register_device because it
>>   already reports enough (thanks to Tang Bin for the report!)
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
>> ---
>>  drivers/i2c/i2c-core-of.c | 18 +++++++-----------
>>  1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
>> index 710704cd583e..f2d09ea0d336 100644
>> --- a/drivers/i2c/i2c-core-of.c
>> +++ b/drivers/i2c/i2c-core-of.c
>> @@ -8,6 +8,8 @@
>>   * Copyright (C) 2013, 2018 Wolfram Sang <wsa@the-dreams.de>
>>   */
>>  
>> +#define pr_fmt(fmt) "i2c_of: " fmt
>> +
>>  #include <dt-bindings/i2c/i2c.h>
>>  #include <linux/device.h>
>>  #include <linux/err.h>
>> @@ -28,7 +30,7 @@ int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)


Perhaps not directly needed by this patch but,

at some point will you rename of_i2c_* to i2c_of_* ?


>>  
>>  	ret = of_property_read_u32(node, "reg", &addr);
>>  	if (ret) {
>> -		pr_err("of_i2c: invalid reg on %pOF\n", node);
>> +		pr_err("invalid reg on %pOF\n", node);
>>  		return ret;
>>  	}
>>  
>> @@ -66,7 +68,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>>  	struct i2c_board_info info;
>>  	int ret;
>>  
>> -	dev_dbg(&adap->dev, "of_i2c: register %pOF\n", node);
>> +	pr_debug("register %pOF\n", node);
>>  
>>  	ret = of_i2c_get_board_info(node, &info);
>>  	if (ret)
>> @@ -74,7 +76,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>>  
>>  	client = i2c_new_client_device(adap, &info);
>>  	if (IS_ERR(client))
>> -		dev_err(&adap->dev, "of_i2c: Failure registering %pOF\n", node);
>> +		pr_err("failure registering %pOF (%ld)\n", node, PTR_ERR(client));
>>  
>>  	return client;
>>  }
>> @@ -88,7 +90,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>>  	if (!adap->dev.of_node)
>>  		return;
>>  
>> -	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
>> +	dev_dbg(&adap->dev, "walking child nodes\n");
>>  
>>  	bus = of_get_child_by_name(adap->dev.of_node, "i2c-bus");
>>  	if (!bus)
>> @@ -99,12 +101,8 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>>  			continue;
>>  
>>  		client = of_i2c_register_device(adap, node);
>> -		if (IS_ERR(client)) {
>> -			dev_err(&adap->dev,
>> -				 "Failed to create I2C device for %pOF\n",
>> -				 node);
>> +		if (IS_ERR(client))
>>  			of_node_clear_flag(node, OF_POPULATED);
>> -		}
>>  	}
>>  
>>  	of_node_put(bus);
>> @@ -243,8 +241,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
>>  
>>  		client = of_i2c_register_device(adap, rd->dn);
>>  		if (IS_ERR(client)) {
>> -			dev_err(&adap->dev, "failed to create client for '%pOF'\n",
>> -				 rd->dn);
>>  			put_device(&adap->dev);
>>  			of_node_clear_flag(rd->dn, OF_POPULATED);
>>  			return notifier_from_errno(PTR_ERR(client));
>>
> 


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-04-15  8:48   ` Kieran Bingham
@ 2020-04-15  9:46     ` Wolfram Sang
  0 siblings, 0 replies; 29+ messages in thread
From: Wolfram Sang @ 2020-04-15  9:46 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: Rob Herring, Jacopo Mondi, Niklas Söderlund,
	Geert Uytterhoeven, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Wolfram Sang, linux-i2c, Luca Ceresoli,
	linux-i3c, Laurent Pinchart

[-- Attachment #1.1: Type: text/plain, Size: 751 bytes --]


> > Sometimes, we have unknown devices in a system and still want to block
> > their address. For that, we allow DT nodes with only a 'reg' property.
> > These devices will be bound to the "dummy" driver but with the name
> > "reserved". That way, we can distinguish them and even hand them over to
> > the "dummy" driver later when they are really requested using
> > i2c_new_ancillary_device().
> 
> Oh how I long to be able to give these 'identifiable names' within the
> system, but that will probably mess up all the driver matching and
> binding, so would be quite tricky perhaps.

I haven't found a way yet to use 'name' to give more meaningful
descriptions to dummies. My best bet so far is to use additional links
in sysfs.


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

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved
  2020-03-18 15:00 ` [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved Wolfram Sang
  2020-04-10 17:05   ` Luca Ceresoli
  2020-04-13  9:55   ` Luca Ceresoli
@ 2020-04-15 10:07   ` Luca Ceresoli
  2 siblings, 0 replies; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-15 10:07 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, linux-i3c

Hi,

 18/03/20 16:00, Wolfram Sang wrote:
> Back then, 'reg' properties in I2C DT bindings only contained one
> address and this address was assigned a device and, thus, blocked.
> Meanwhile, chips using multiple addresses are common and the 'reg'
> property can be an array described by 'reg-names'. This code enhances
> I2C DT parsing, so it will reserve all addresses described in an array.
> They will be bound to the 'dummy' driver as 'reserved' iff the first
> address can be assigned successfully. If that is not the case, the array
> is not further considered. If one later address of the array can not be
> assigned, it will be reported but we don't bail out. The driver has to
> decide if that address is critical or not.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-of.c | 70 +++++++++++++++++++++++++--------------
>  1 file changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index f2d09ea0d336..67eb2cd305cf 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -16,25 +16,18 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_device.h>
>  #include <linux/sysfs.h>
>  
>  #include "i2c-core.h"
>  
> -int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
> +static void of_i2c_decode_board_info(struct device_node *node, u32 addr,
> +				     bool first_addr, struct i2c_board_info *info)
>  {
> -	u32 addr;
> -	int ret;
> -
>  	memset(info, 0, sizeof(*info));
>  
> -	ret = of_property_read_u32(node, "reg", &addr);
> -	if (ret) {
> -		pr_err("invalid reg on %pOF\n", node);
> -		return ret;
> -	}
> -
> -	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
> +	if (!first_addr || of_modalias_node(node, info->type, sizeof(info->type)) < 0)
>  		strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));
>  
>  	if (addr & I2C_TEN_BIT_ADDRESS) {
> @@ -51,11 +44,27 @@ int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
>  	info->of_node = node;
>  	info->fwnode = of_fwnode_handle(node);
>  
> -	if (of_property_read_bool(node, "host-notify"))
> -		info->flags |= I2C_CLIENT_HOST_NOTIFY;
> +	if (first_addr) {
> +		if (of_property_read_bool(node, "host-notify"))
> +			info->flags |= I2C_CLIENT_HOST_NOTIFY;
> +
> +		if (of_get_property(node, "wakeup-source", NULL))
> +			info->flags |= I2C_CLIENT_WAKE;
> +	}
> +}
> +
> +int of_i2c_get_board_info(struct device_node *node, struct i2c_board_info *info)
> +{
> +	u32 addr;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "reg", &addr);
> +	if (ret) {
> +		pr_err("invalid reg on %pOF\n", node);
> +		return ret;
> +	}
>  
> -	if (of_get_property(node, "wakeup-source", NULL))
> -		info->flags |= I2C_CLIENT_WAKE;
> +	of_i2c_decode_board_info(node, addr, true, info);
>  
>  	return 0;
>  }
> @@ -64,21 +73,34 @@ EXPORT_SYMBOL_GPL(of_i2c_get_board_info);
>  static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
>  						 struct device_node *node)
>  {
> -	struct i2c_client *client;
> +	struct i2c_client *client, *first_client = ERR_PTR(-ENOENT);
>  	struct i2c_board_info info;
> -	int ret;
> +	bool first_reg = true;
> +	unsigned int i = 0;
> +	const __be32 *prop;
> +	u16 reg;
>  
>  	pr_debug("register %pOF\n", node);
>  
> -	ret = of_i2c_get_board_info(node, &info);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	while ((prop = of_get_address(node, i++, NULL, NULL))) {
> +		reg = of_read_number(prop, 1);
> +		of_i2c_decode_board_info(node, reg, first_reg, &info);
> +
> +		client = i2c_new_client_device(adap, &info);
> +		if (IS_ERR(client)) {
> +			pr_err("failure registering addr 0x%02x for %pOF (%ld)\n",
> +				reg, node, PTR_ERR(client));
> +			if (first_reg)
> +				return client;
> +		}

I had an opportunity to runtime test this whole series on top of my TI
DS90UB95x serdes patches and it generally works fine.

I noticed however a minor annoyance in the above while loop. During
probing, these errors are produced:

  i2c i2c-0: Failed to register i2c client reserved at 0x40 (-16)
  i2c_of: failure registering addr 0x40 for /ocp/i2c@48070000/des_0@30 (-16)

This is logged as an error, so I assumed probing had failed, instead it
succeeded. This happens because the first loop iteration (on the first
'reg') triggers the driver's probe(), which in my case calls
i2c_new_ancillary_device() to register address 0x40. The second loop
iteration finds 0x40 in DT and tries to register it as "reserved", but
it fails. By design the loop continues successfully, but the (double)
error printed is misleading.

Fixing the second error, which comes from the above loop, is easy:

 client = i2c_new_client_device(adap, &info);
 if (IS_ERR(client)) {
-	pr_err("failure registering addr 0x%02x for %pOF (%ld)\n",
-		reg, node, PTR_ERR(client));
 	if (first_reg)
+		pr_err("failure registering addr 0x%02x for %pOF (%ld)\n",
+			reg, node, PTR_ERR(client));
 		return client;
 }

The other error is produced in i2c_new_client_device() and I see no
obvious way to put an if in front of the dev_err() except checking if
client->name equals I2C_RESERVED_DRV_NAME.

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 6/6] i2c: core: hand over reserved devices when requesting ancillary addresses
  2020-03-18 15:00 ` [RFC PATCH v2 6/6] i2c: core: hand over reserved devices when requesting ancillary addresses Wolfram Sang
@ 2020-04-15 10:07   ` Luca Ceresoli
  0 siblings, 0 replies; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-15 10:07 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: Jacopo Mondi, linux-kernel, Vladimir Zapolskiy,
	linux-renesas-soc, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, linux-i3c

Hi,

On 18/03/20 16:00, Wolfram Sang wrote:
> With i2c_new_ancillary_address, we can check if the intended driver is
> requesting a reserved address. Update the function to do these checks.
> If the check passes, the "reserved" device will become a regular "dummy"
> device.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 84464e439df5..81fb320de28d 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -974,7 +974,9 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>  						const char *name,
>  						u16 default_addr)
>  {
> -	struct device_node *np = client->dev.of_node;
> +	struct device_node *reserved_np, *np = client->dev.of_node;
> +	struct device *reserved_dev, *adapter_dev = &client->adapter->dev;
> +	struct i2c_client *reserved_client = NULL;
>  	u32 addr = default_addr;
>  	int i;
>  
> @@ -984,7 +986,25 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>  			of_property_read_u32_index(np, "reg", i, &addr);
>  	}
>  
> -	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
> +	dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);

Here if we have two identical chips on the same bus, they probably will
both add an ancillary device with the same name. Then a message like:

  i2c i2c-0: ds90ub954-q1: Address for rxport0: 0x40

won't tell which ds90ub954-q1 device is using that address. I'd rather
disambiguate using something like:

  dev_info(adapter_dev, "%s: Address for %s: 0x%x\n",
           dev_name(&client->dev), name, addr);

Sure, this issue did exist before this patch, but since the line is
being promoted from dbg to info (which is OK), it's probably a good idea
to improve the content, perhaps in a separate patch.

Except for that, I tested the patch and it's working fine.

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

* Re: [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible'
  2020-04-15  7:59     ` Wolfram Sang
  2020-04-15  8:07       ` Kieran Bingham
@ 2020-04-16 14:53       ` Luca Ceresoli
  1 sibling, 0 replies; 29+ messages in thread
From: Luca Ceresoli @ 2020-04-16 14:53 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Rob Herring, Jacopo Mondi, Geert Uytterhoeven, Kieran Bingham,
	linux-kernel, Vladimir Zapolskiy, linux-renesas-soc,
	Wolfram Sang, linux-i2c, Niklas Söderlund, linux-i3c,
	Laurent Pinchart

Hi,

On 15/04/20 09:59, Wolfram Sang wrote:
> 
>> As I said in the reply to v1, I think we should reserve addresses also
>> when there is a compatible string but no matching driver, but this is
>> another story and can be handled separately.
> 
> Unless I misunderstand you, I think they do already. Note that
> only 'i2cdetect' shows a device as busy *IFF* there is a driver bound to
> it. The internal 'i2c_check_addr_busy' does not care about a driver
> being bound. You can check this by trying to use
> i2c_new_ancillary_device() with an address which is already described in
> DT but which driver is disabled.
> 

Ah, yes! I was assuming the opposite but I double checked and you're
right of course.

Sorry for the noise.

-- 
Luca

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 15:00 [RFC PATCH v2 0/6] i2c: of: reserve unknown and ancillary addresses Wolfram Sang
2020-03-18 15:00 ` [RFC PATCH v2 1/6] i2c: use DEFINE for the dummy driver name Wolfram Sang
2020-04-15  8:09   ` Kieran Bingham
2020-03-18 15:00 ` [RFC PATCH v2 2/6] i2c: allow DT nodes without 'compatible' Wolfram Sang
2020-04-10 13:49   ` Luca Ceresoli
2020-04-15  7:59     ` Wolfram Sang
2020-04-15  8:07       ` Kieran Bingham
2020-04-15  8:16         ` Wolfram Sang
2020-04-15  8:38           ` Kieran Bingham
2020-04-16 14:53       ` Luca Ceresoli
2020-04-15  8:48   ` Kieran Bingham
2020-04-15  9:46     ` Wolfram Sang
2020-03-18 15:00 ` [RFC PATCH v2 3/6] i2c: of: remove superfluous parameter from exported function Wolfram Sang
2020-03-19 12:41   ` Boris Brezillon
2020-04-15  8:13   ` Kieran Bingham
2020-03-18 15:00 ` [RFC PATCH v2 4/6] i2c: of: error message unification Wolfram Sang
2020-04-10 17:02   ` Luca Ceresoli
2020-04-15  8:17   ` Kieran Bingham
2020-04-15  8:50     ` Kieran Bingham
2020-03-18 15:00 ` [RFC PATCH v2 5/6] i2c: of: mark a whole array of regs as reserved Wolfram Sang
2020-04-10 17:05   ` Luca Ceresoli
2020-04-13  9:55   ` Luca Ceresoli
2020-04-15  8:10     ` Wolfram Sang
2020-04-15 10:07   ` Luca Ceresoli
2020-03-18 15:00 ` [RFC PATCH v2 6/6] i2c: core: hand over reserved devices when requesting ancillary addresses Wolfram Sang
2020-04-15 10:07   ` Luca Ceresoli
2020-03-28  3:50 ` [RFC PATCH v2 0/6] i2c: of: reserve unknown and " Wolfram Sang
2020-04-15  8:27 ` Wolfram Sang
2020-04-15  8:35   ` Kieran Bingham

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/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-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org
	public-inbox-index linux-i3c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


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