Hi Peter,
On 5/3/24 7:30 AM, Peter Rosin wrote:
Hi!
2024-05-02 at 17:01, Farouk Bouabid wrote:
Hi Peter,
On 29.04.24 17:46, Peter Rosin wrote:
Hi!
2024-04-26 at 18:49, Farouk Bouabid wrote:
Allow the mux to have the same address as a child device. This is useful
when the mux can only use an i2c-address that is used by a child device
because no other addresses are free to use. eg. the mux can only use
address 0x18 which is used by amc6821 connected to the mux.
Signed-off-by: Farouk Bouabid <farouk.bouabid@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/i2c/i2c-mux.c | 10 +++++++++-
include/linux/i2c-mux.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 57ff09f18c37..f5357dff8cc5 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -331,7 +331,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
priv->adap.owner = THIS_MODULE;
priv->adap.algo = &priv->algo;
priv->adap.algo_data = priv;
- priv->adap.dev.parent = &parent->dev;
priv->adap.retries = parent->retries;
priv->adap.timeout = parent->timeout;
priv->adap.quirks = parent->quirks;
@@ -348,6 +347,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
else
priv->adap.class = class;
+ /*
+ * When creating the adapter, the node devices are checked for i2c address
+ * match with other devices on the parent adapter, among which is the mux itself.
+ * If a match is found the node device is not probed successfully.
+ * Allow the mux to have the same address as a child device by skipping this check.
+ */
+ if (!(muxc->share_addr_with_children))
+ priv->adap.dev.parent = &parent->dev;
This is a dirty hack that will not generally do the right thing.
The adapter device parent is not there solely for the purpose of
detecting address clashes, so the above has other implications
that are not desirable.
Therefore, NACK on this approach. It simply needs to be more involved.
Sorry.
Cheers,
Peter
Another way to approach this is by implementing this flag as a quirk for the added adapter:
(tested but not cleaned up)
Yes, good idea, this is much more targeted and generally feels a lot
better.
"""
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ff5c486a1dbb..6a0237f750db 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -821,9 +821,21 @@ static int i2c_check_mux_children(struct device *dev, void *addrp)
static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
{
struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
+ bool skip_check = false;
int result = 0;
- if (parent)
+ if (adapter->quirks) {
+ if (adapter->quirks->flags & I2C_AQ_SHARE_ADDR) {
+ struct i2c_client *client = of_find_i2c_device_by_node(adapter->dev.of_node->parent);
+
+ if (client) {
+ skip_check = client->addr == addr;
+ put_device(&client->dev);
+ }
+ }
+ }
+
+ if (parent && !skip_check)
result = i2c_check_mux_parents(parent, addr);
if (!result)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 57ff09f18c37..e87cb0e43725 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -334,7 +334,26 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
priv->adap.dev.parent = &parent->dev;
priv->adap.retries = parent->retries;
priv->adap.timeout = parent->timeout;
- priv->adap.quirks = parent->quirks;
+ /*
+ * When creating the adapter, the node devices are checked for i2c address
+ * match with other devices on the parent adapter, among which is the mux itself.
+ * If a match is found the node device is not probed successfully.
+ * Allow the mux to have the same address as a child device by skipping this check.
+ */
+ if (!muxc->share_addr_with_children)
+ priv->adap.quirks = parent->quirks;
+ else {
+ struct i2c_adapter_quirks *quirks = kzalloc(sizeof(*quirks), GFP_KERNEL);
This leaks, dev_kzalloc?
Quick questions about this though.
priv is allocated with kzalloc and not devm_kzalloc and is then manually
kfree'd either as part of the error path or in i2c_del_mux_adapters. Is
there a reason for this? Shouldn't we migrate this to devm allocation as
well?
Similarly, I was wondering if we couldn't add a devm_add_action_or_reset
for i2c_del_mux_adapters in i2c_add_mux_adapter? Is there something that
prevents us from doing this?
Cheers,
Quentin
