LinuxLists.cc - [RFC PATCH 0/7] i2c: of: reserve unknown and ancillary addresses

[permalink] [raw]

Subject: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

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

2020-02-20 17:26:26

[permalink] [raw]

Subject: [RFC PATCH 4/7] i2c: of: remove superfluous parameter from exported function

'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 <[email protected]>
---
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

2020-02-20 17:26:34

[permalink] [raw]

Subject: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

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

2020-02-20 17:26:41

[permalink] [raw]

Subject: [RFC PATCH 5/7] i2c: of: error message unification

- 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 <[email protected]>
---
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 <[email protected]>
*/

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

2020-02-20 17:26:46

[permalink] [raw]

Subject: [RFC PATCH 6/7] i2c: of: mark a whole array of regs as reserved

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

2020-02-20 17:27:23

[permalink] [raw]

Subject: [RFC PATCH 2/7] i2c: use DEFINE for the dummy driver name

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

Signed-off-by: Wolfram Sang <[email protected]>
---
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

2020-02-20 17:28:16

[permalink] [raw]

Subject: [RFC PATCH 1/7] i2c: add sanity check for parameter of i2c_verify_client()

We export this function, so we should check the paramter to make it
NULL-compatible.

Signed-off-by: Wolfram Sang <[email protected]>
---
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

2020-02-21 09:37:24

[permalink] [raw]

Subject: Re: [RFC PATCH 1/7] i2c: add sanity check for parameter of i2c_verify_client()

Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> wrote:
> We export this function, so we should check the paramter to make it

parameter

> NULL-compatible.
>
> Signed-off-by: Wolfram Sang <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

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

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

2020-02-21 09:39:01

[permalink] [raw]

Subject: Re: [RFC PATCH 2/7] i2c: use DEFINE for the dummy driver name

On Thu, Feb 20, 2020 at 6:27 PM Wolfram Sang
<[email protected]> wrote:
> We use it in multiple places, so make sure it is consistent whenever we
> need to change it.
>
> Signed-off-by: Wolfram Sang <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2020-02-21 09:46:43

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> 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 <[email protected]>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <[email protected]>
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 -- [email protected]

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

2020-02-21 09:49:03

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

On Fri, Feb 21, 2020 at 10:45 AM Geert Uytterhoeven
<[email protected]> wrote:
> On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
> <[email protected]> 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 <[email protected]>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>

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

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

2020-02-21 09:50:50

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] i2c: of: remove superfluous parameter from exported function

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> 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 <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2020-02-21 09:55:47

[permalink] [raw]

Subject: Re: [RFC PATCH 5/7] i2c: of: error message unification

Hi Wolfram,

Thanks for your patch!

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> 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 <[email protected]>

Nevertheless:
Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2020-02-21 10:09:55

[permalink] [raw]

Subject: Re: [RFC PATCH 6/7] i2c: of: mark a whole array of regs as reserved

Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> 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 <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

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

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

2020-02-21 10:15:30

[permalink] [raw]

Subject: Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> 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 <[email protected]>

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

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

2020-02-21 10:15:41

[permalink] [raw]

Subject: Re: [RFC PATCH 0/7] i2c: of: reserve unknown and ancillary addresses

Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<[email protected]> 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 -- [email protected]

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

2020-02-23 23:12:24

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

Hi,

On 21/02/20 10:45, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
> <[email protected]> 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 <[email protected]>

Cc:ing Alexandre who raised the need for a described-but-disabled I2C node.

> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> 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

2020-02-24 08:13:00

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] i2c: of: remove superfluous parameter from exported function

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

Reviewed-by: Luca Ceresoli <[email protected]>

--
Luca

2020-02-26 16:31:58

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

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

2020-02-28 12:12:15

[permalink] [raw]

Subject: Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

Hi,

On 21/02/20 11:13, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
> <[email protected]> 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 <[email protected]>
>
> 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

2020-02-28 12:12:29

[permalink] [raw]

Subject: Re: [RFC PATCH 6/7] i2c: of: mark a whole array of regs as reserved

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

Reviewed-by: Luca Ceresoli <[email protected]>

--
Luca

2020-03-12 11:21:28

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

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

Attachments:

(No filename) (2.82 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-12 11:21:37

[permalink] [raw]

Subject: Re: [RFC PATCH 6/7] i2c: of: mark a whole array of regs as reserved

Attachments:

(No filename) (363.00 B)
signature.asc (849.00 B)
Download all attachments

2020-03-12 11:22:55

[permalink] [raw]

Subject: Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

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

Attachments:

(No filename) (189.00 B)
signature.asc (849.00 B)
Download all attachments

2020-03-12 11:31:49

[permalink] [raw]

Subject: Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

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

Attachments:

(No filename) (1.20 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-12 11:45:13

by Alexandre Belloni

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] i2c: allow DT nodes without 'compatible'

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

2020-03-13 12:43:00

[permalink] [raw]

Subject: Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

> > @@ -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!

Attachments:

(No filename) (2.23 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-18 14:34:26

[permalink] [raw]

Subject: Re: [RFC PATCH 6/7] i2c: of: mark a whole array of regs as reserved

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

Attachments:

(No filename) (1.58 kB)
signature.asc (849.00 B)
Download all attachments

2020-04-10 14:13:24