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+2 do some preparational refactoring. After patch 3, 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 4+5 are again some preparational refactoring. After patch 6, 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: dummy@13 { reg = <0x13>, <0x14>; }; After patch 7 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 (7): i2c: add sanity check for parameter of i2c_verify_client() 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 | 1 - Documentation/devicetree/bindings/i2c/i2c.txt | 4 +- drivers/i2c/i2c-core-base.c | 29 +++++-- drivers/i2c/i2c-core-of.c | 86 +++++++++++-------- drivers/i2c/i2c-core.h | 3 + drivers/i3c/master.c | 2 +- include/linux/i2c.h | 6 +- 7 files changed, 83 insertions(+), 48 deletions(-) -- 2.20.1 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
We export this function, so we should check the paramter to make it NULL-compatible. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-base.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index cefad0881942..8f46d1bb8c62 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -517,7 +517,7 @@ EXPORT_SYMBOL_GPL(i2c_client_type); */ struct i2c_client *i2c_verify_client(struct device *dev) { - return (dev->type == &i2c_client_type) + return (dev && dev->type == &i2c_client_type) ? to_i2c_client(dev) : NULL; } -- 2.20.1 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/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> --- 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 8f46d1bb8c62..8df2fa10c48a 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
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> --- Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 1 - 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(+), 7 deletions(-) diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt index 6b25a80ae8d3..2762effdd270 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>; }; }; 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 8df2fa10c48a..4000a4384306 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
'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> --- 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
- 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 Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-of.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c index 710704cd583e..74b9f3fbb5ef 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\n", node); 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) @@ -100,9 +102,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap) client = of_i2c_register_device(adap, node); if (IS_ERR(client)) { - dev_err(&adap->dev, - "Failed to create I2C device for %pOF\n", - node); + pr_err("failed to create I2C device for %pOF\n", node); of_node_clear_flag(node, OF_POPULATED); } } @@ -243,8 +243,7 @@ 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); + pr_err("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
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 | 68 +++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c index 74b9f3fbb5ef..316db0c3b3c8 100644 --- a/drivers/i2c/i2c-core-of.c +++ b/drivers/i2c/i2c-core-of.c @@ -21,20 +21,12 @@ #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 +43,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 +72,33 @@ 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; + struct property *prop; + const __be32 *cur; + u32 reg; pr_debug("register %pOF\n", node); - ret = of_i2c_get_board_info(node, &info); - if (ret) - return ERR_PTR(ret); + of_property_for_each_u32(node, "reg", prop, cur, reg) { + 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\n", + reg, node); + if (first_reg) + return client; + } - client = i2c_new_client_device(adap, &info); - if (IS_ERR(client)) - pr_err("failure registering %pOF\n", node); + 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
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 | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 4000a4384306..ba325f8107a3 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -975,6 +975,8 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client, u16 default_addr) { struct device_node *np = client->dev.of_node; + struct device *reserved_dev, *adapter_dev = &client->adapter->dev; + struct i2c_client *reserved_client; u32 addr = default_addr; int i; @@ -984,7 +986,21 @@ 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); + reserved_client = i2c_verify_client(reserved_dev); + + if (reserved_client) { + if (reserved_client->dev.of_node != 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
Hi Wolfram, On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > We export this function, so we should check the paramter to make it parameter > NULL-compatible. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> And then the check in i2c_acpi_find_client_by_adev() can be removed. BTW, can the i2c_verify_client() check in that function actually fail? If yes, it should call put_device(dev) on failure, like of_find_i2c_device_by_node() does. 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
On Thu, Feb 20, 2020 at 6:27 PM Wolfram Sang <wsa+renesas@sang-engineering.com> 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> 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Wolfram, On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> 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> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> but one question below. > --- 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>; > }; > }; There's a second instance to remove 18 lines below. > --- 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)); Could this cause a regression, e.g. if people already have such dummy nodes in their DTS, and use sysfs new_device from userspace to instantiate the device later? 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
On Fri, Feb 21, 2020 at 10:45 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang > <wsa+renesas@sang-engineering.com> 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> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> FTR, depending on the extra dummy removed. > but one question below. > > > --- 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>; > > }; > > }; > > There's a second instance to remove 18 lines below. 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
On Thu, Feb 20, 2020 at 6:26 PM 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> 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Wolfram, Thanks for your patch! On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > - don't prefix the device if %pOF is provided. That information is > enough. While that information is sufficient to identify the device, using a mix of dev_*() and pr_*("... %pOF...") makes it harder to grep for relevant information in the kernel log. Hence I'm not convinced this is an improvement. > - 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 > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Nevertheless: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Wolfram, On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> 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: Geert Uytterhoeven <geert+renesas@glider.be> One comment below. > --- a/drivers/i2c/i2c-core-of.c > +++ b/drivers/i2c/i2c-core-of.c > @@ -21,20 +21,12 @@ > > #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 +43,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); Perhaps the time is ripe to start considering #address-cells, instead of assuming 1, here ... > + 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 +72,33 @@ 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; > + struct property *prop; > + const __be32 *cur; > + u32 reg; > > pr_debug("register %pOF\n", node); > > - ret = of_i2c_get_board_info(node, &info); > - if (ret) > - return ERR_PTR(ret); > + of_property_for_each_u32(node, "reg", prop, cur, reg) { ... and especially here, if this code can ever be reused for i3c, which uses 3. > + 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\n", > + reg, node); > + if (first_reg) > + return client; > + } > > - client = i2c_new_client_device(adap, &info); > - if (IS_ERR(client)) > - pr_err("failure registering %pOF\n", node); > + if (first_reg) { > + first_client = client; > + first_reg = false; > + } > + } > > - return client; > + return first_client; > } > > void of_i2c_register_devices(struct i2c_adapter *adap) 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Wolfram, On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> 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> Thanks for your patch! > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -975,6 +975,8 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client, > u16 default_addr) > { > struct device_node *np = client->dev.of_node; > + struct device *reserved_dev, *adapter_dev = &client->adapter->dev; > + struct i2c_client *reserved_client; > u32 addr = default_addr; > int i; > > @@ -984,7 +986,21 @@ 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); > + reserved_client = i2c_verify_client(reserved_dev); > + > + if (reserved_client) { > + if (reserved_client->dev.of_node != np || > + strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0) > + return ERR_PTR(-EBUSY); Missing put_device(reserved_dev). > + > + strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name)); > + return reserved_client; > + } else put_device(reserved_dev) (perhaps i2c_verify_client() checking dev was not such a great idea, as callers need to act on dev && !verified anyway?) > + > return i2c_new_dummy_device(client->adapter, addr); > } > EXPORT_SYMBOL_GPL(i2c_new_ancillary_device); 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Wolfram, On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > 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+2 do some preparational refactoring. After patch 3, 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 4+5 are again some > preparational refactoring. After patch 6, 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: > > dummy@13 { Hence should that be "reserved@13"? > reg = <0x13>, <0x14>; > }; 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi, On 21/02/20 10:45, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang > <wsa+renesas@sang-engineering.com> 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> Cc:ing Alexandre who raised the need for a described-but-disabled I2C node. > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > but one question below. > >> --- 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>; >> }; >> }; > > There's a second instance to remove 18 lines below. > >> --- 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)); > > Could this cause a regression, e.g. if people already have such dummy > nodes in their DTS, and use sysfs new_device from userspace to > instantiate the device later? Such a DTS would be illegal because "compatible" has been a required property so far. Thus one could leave such people out in the cold because they went on an unsupported path. Not super nice anyway. However I'd like to view the issue from the DT point of view. DT describes the hardware, and it is possible (and even desirable) that the firmware provides the DTB independently from the OS, and the kernel consumes it. It this scenario, firmware could and should describe all I2C slaves with proper "compatible" property, and there is no way to remove it, in a clean way at least. But the kernel currently ignores nodes that have no matching driver, right? So in this case the kernel knows that that address is used, but ignores this information and considers the address as available. Seen in this perspective, we should have a "compatible" for all nodes: it is just describing the hardware and could be out of the kernel control. But instead of discarding all nodes without a matching driver, the i2c-core-of code should mark them as "reserved". Does it sound correct? Clearly this does not fit the case reported by Alexandre: a device having a driver which is known to be badly buggy, so we don't want to instantiate it. But again, this should not affect DT as it is not describing the HW, but only an implementation detail. Probably disabling or blacklisting the driver would be a better option there? My apologies to Wolfram, I appreciate a lot the effort you are doing, but before reviewing this patch I have never realized what I tried to explain above. -- Luca _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi, On 20/02/20 18:24, 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: Luca Ceresoli <luca@lucaceresoli.net> -- Luca _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
On Thu, 20 Feb 2020 18:23:59 +0100, 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> > --- > Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 1 - > 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(+), 7 deletions(-) > Acked-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi, On 20/02/20 18:24, 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
Hi, On 21/02/20 11:13, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang > <wsa+renesas@sang-engineering.com> 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> > > Thanks for your patch! > >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -975,6 +975,8 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client, >> u16 default_addr) >> { >> struct device_node *np = client->dev.of_node; >> + struct device *reserved_dev, *adapter_dev = &client->adapter->dev; >> + struct i2c_client *reserved_client; >> u32 addr = default_addr; >> int i; >> >> @@ -984,7 +986,21 @@ 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); >> + reserved_client = i2c_verify_client(reserved_dev); >> + >> + if (reserved_client) { >> + if (reserved_client->dev.of_node != np || >> + strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0) >> + return ERR_PTR(-EBUSY); > > Missing put_device(reserved_dev). > >> + >> + strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name)); Any strong reason for not giving the device a more informative name? Reading "dummy" in several /sys/bus/i2c/devices/?-????/name files is not helping. Using the 'name' string that is passed to i2c_new_ancillary_device() would be way better, perhaps prefixed by dev->name. But this opens the question of why not doing it in i2c_new_dummy_device() as well, which currently receives no "name" parameter. Of course this is not strictly related to this patch and can be done in a later step. About the patch itself, except for the issues pointed out by Geert the approach looks generally good to me. -- Luca _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
[-- Attachment #1.1: Type: text/plain, Size: 2892 bytes --] Hi Luca, > But the kernel currently ignores nodes that have no matching driver, > right? So in this case the kernel knows that that address is used, but > ignores this information and considers the address as available. I'd rather call it "unbound" than available. See later. > Seen in this perspective, we should have a "compatible" for all nodes: > it is just describing the hardware and could be out of the kernel > control. But instead of discarding all nodes without a matching driver, And what compatible value would you use if you know there is something sitting there and don't know what? This is what this series aims to address because we thought a compatible name like "reserved" would not be a good idea. > the i2c-core-of code should mark them as "reserved". > > Does it sound correct? With this patch series, this is quite what happens for ancillary addresses. They get their own dummy device automatically now, are marked as reserved and can only be obtained by the driver which bound to the main address (of_node of ancillary addr == of_node of main addr). For the main address, I think things are a bit different. They already have their struct device. The only thing we gain from reserving them (= binding to the dummy driver) is that they are kinda blocked for userspace access. The "protection" is kinda low, though. There are already ways to communicate with bound addresses from userspace. In kernel space we still need to probe this address until a driver is bound to it, I don't see what a "reserved" state gains us here. If we are talking about the pool of available addresses, we are all good because we operate on existing struct device and don't care if they are bound or not. Or? What would be kinda nice, though, is when i2cdetect could show reserved addresses (unbound but having a struct device) as "RR" or so. However, I currently can't see a way to do it without breaking compability. > Clearly this does not fit the case reported by Alexandre: a device > having a driver which is known to be badly buggy, so we don't want to > instantiate it. But again, this should not affect DT as it is not > describing the HW, but only an implementation detail. Probably disabling > or blacklisting the driver would be a better option there? "Fixing the driver" is the first thing coming to my mind ;) But yeah, blacklisting would be another good solution. With only the information above, DT is not the right place to fix a broken driver. > My apologies to Wolfram, I appreciate a lot the effort you are doing, > but before reviewing this patch I have never realized what I tried to > explain above. All good, Luca! Talking over code usually brings in viewpoints which have been missed so far. This is expected. Actually, I am very happy to have this discussion! All the best, Wolfram [-- 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
[-- Attachment #1.1: Type: text/plain, Size: 363 bytes --] > > +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); > > Perhaps the time is ripe to start considering #address-cells, instead > of assuming 1, here ... I will check both instances. Thanks, Geert! [-- 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
[-- Attachment #1.1: Type: text/plain, Size: 183 bytes --] > (perhaps i2c_verify_client() checking dev was not such a great idea, as > callers need to act on dev && !verified anyway?) Can be argued. I will have a second thought 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
[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --] > >> + strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name)); > > Any strong reason for not giving the device a more informative name? Yes, sadly... > Reading "dummy" in several /sys/bus/i2c/devices/?-????/name files is not > helping. Using the 'name' string that is passed to > i2c_new_ancillary_device() would be way better, perhaps prefixed by > dev->name. But this opens the question of why not doing it in ... I never liked the plain "dummy" name as well. However, because 'name' is what we need to bind to a driver we can't have a more descriptive or run-time generated name at that place. > i2c_new_dummy_device() as well, which currently receives no "name" > parameter. I thought about it but discarded the idea because then you still have no connection to the driver which created the dummy device. My favourite idea so far is to advertise i2c_new_ancillary_device() instead of i2c_new_dummy_device(), because there we already have access to the client structure. With that, we could add another link in sysfs to the main address and vice-versa. > Of course this is not strictly related to this patch and can be done in > a later step. Exactly. [-- 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
On 12/03/2020 12:19:51+0100, Wolfram Sang wrote: > > Clearly this does not fit the case reported by Alexandre: a device > > having a driver which is known to be badly buggy, so we don't want to > > instantiate it. But again, this should not affect DT as it is not > > describing the HW, but only an implementation detail. Probably disabling > > or blacklisting the driver would be a better option there? > > "Fixing the driver" is the first thing coming to my mind ;) But yeah, > blacklisting would be another good solution. With only the information > above, DT is not the right place to fix a broken driver. > To be clear, the driver is working properly but the HW isn't. It is a PMIC and we need to avoid linux talking to it so the PMIC doesn't end up killing the bus. We end up with a node properly described in the device tree but with status = "disabled". The relevance to the discussion was that you know what is there and you want to avoid using its address. See the pmic node on i2c1 in arch/arm/boot/dts/at91-sama5d3_xplained.dts for what I'm referring to. > > My apologies to Wolfram, I appreciate a lot the effort you are doing, > > but before reviewing this patch I have never realized what I tried to > > explain above. > > All good, Luca! Talking over code usually brings in viewpoints which > have been missed so far. This is expected. Actually, I am very happy to > have this discussion! > > All the best, > > Wolfram > -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
[-- Attachment #1.1: Type: text/plain, Size: 2287 bytes --] > > @@ -984,7 +986,21 @@ 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); > > + reserved_client = i2c_verify_client(reserved_dev); > > + > > + if (reserved_client) { > > + if (reserved_client->dev.of_node != np || > > + strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0) > > + return ERR_PTR(-EBUSY); > > Missing put_device(reserved_dev). Actually, I think the code could even be like this: struct i2c_client *reserved_client = NULL; ... 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); We put the device early - as soon we don't access the struct anymore. I think we don't need the refcnt any further because what we are doing here is to hand over the initial refcnt from the core to the requesting driver. We turn the device from "reserved" (internally managed) to "dummy" (managed by the driver). So, I think the code is okay regarding the struct device. I will have a second look when it comes to concurrency problems regarding the struct i2c_client, though. > (perhaps i2c_verify_client() checking dev was not such a great idea, as > callers need to act on dev && !verified anyway?) Yeah, since I refactored the ACPI code as well, patch 1 from this series can probably go. Thanks again for your review, Geert! [-- 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
[-- Attachment #1.1: Type: text/plain, Size: 1614 bytes --] > > +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); > > Perhaps the time is ripe to start considering #address-cells, instead > of assuming 1, here ... I think here it is okay because we really want the first entry of the first tuple. > > + of_property_for_each_u32(node, "reg", prop, cur, reg) { > > ... and especially here, if this code can ever be reused for i3c, which uses 3. But here I agree. I reimplemented the code to handle it, and it worked with '#address-cells = <2>;' as expected. Here is the diff to this patch: @@ -16,6 +16,7 @@ #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> @@ -75,13 +76,14 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap, struct i2c_client *client, *first_client = ERR_PTR(-ENOENT); struct i2c_board_info info; bool first_reg = true; - struct property *prop; - const __be32 *cur; - u32 reg; + unsigned int i = 0; + const __be32 *prop; + u16 reg; pr_debug("register %pOF\n", node); - of_property_for_each_u32(node, "reg", prop, cur, reg) { + 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); Thanks, Geert! I will send out the new version in a few minutes. [-- 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
Hi Wolfram, On 12/03/20 12:19, Wolfram Sang wrote: > Hi Luca, > >> But the kernel currently ignores nodes that have no matching driver, >> right? So in this case the kernel knows that that address is used, but >> ignores this information and considers the address as available. > > I'd rather call it "unbound" than available. See later. > >> Seen in this perspective, we should have a "compatible" for all nodes: >> it is just describing the hardware and could be out of the kernel >> control. But instead of discarding all nodes without a matching driver, > > And what compatible value would you use if you know there is something > sitting there and don't know what? This is what this series aims to > address because we thought a compatible name like "reserved" would not > be a good idea. The scenario I have in mind is when DT has a proper compatible string, but the kernel has no driver for that chip. Could be not implemented or simply not compiled. There are 3 cases generally: 1. compatible string present, kernel has a matching driver 2. compatible string present, kernel has no matching driver 3. compatible string not present Case 1 is obvious. Case 3 is currently ignored, with your patch the address will be reserved. Case 2 is currently ignored, but we have all the information to reserve the address just like in case 2, but there's no plan to reserve it. Why not? (not necessarily in this series, I'm just trying to understand if the idea is correct) -- Luca _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c