| Message ID | 20240426-dev-mule-i2c-mux-v1-1-045a482f6ffb@theobroma-systems.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Mule I2C multiplexer support | expand |
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@theobroma-systems.com> >> --- >> 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) """ 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); + if (!quirks) + return -ENOMEM; + + if (parent->quirks) + *quirks = *(parent->quirks); // @fixme memcpy + + quirks->flags |= I2C_AQ_SHARE_ADDR; + priv->adap.quirks = quirks; + } + if (muxc->mux_locked) priv->adap.lock_ops = &i2c_mux_lock_ops; else diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h index 98ef73b7c8fd..17ac68bf1703 100644 --- a/include/linux/i2c-mux.h +++ b/include/linux/i2c-mux.h @@ -21,6 +21,7 @@ struct i2c_mux_core { unsigned int mux_locked:1; unsigned int arbitrator:1; unsigned int gate:1; + unsigned int share_addr_with_children:1; void *priv; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 5e6cd43a6dbd..2ebac9e672ef 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -711,6 +711,8 @@ struct i2c_adapter_quirks { #define I2C_AQ_NO_ZERO_LEN (I2C_AQ_NO_ZERO_LEN_READ | I2C_AQ_NO_ZERO_LEN_WRITE) /* adapter cannot do repeated START */ #define I2C_AQ_NO_REP_START BIT(7) +/* @fixme document and find proper name */ +#define I2C_AQ_SHARE_ADDR BIT(8) /* * i2c_adapter is the structure used to identify a physical i2c bus along """ This works, however this only supports device-tree because of of_find_i2c_device_by_node. If we want to support acpi then we can either: 1. Get the Mule i2c device address from fwnode_get_next_parent_dev but this is static since v6.9-rcx. 2. Pass the Mule i2c device address as a new member of struct i2c_adapter_quirks. I would go for 2. Do you suggest something else? Best regards Farouk
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@theobroma-systems.com> >>> --- >>> 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? > + if (!quirks) > + return -ENOMEM; > + > + if (parent->quirks) > + *quirks = *(parent->quirks); // @fixme memcpy > + > + quirks->flags |= I2C_AQ_SHARE_ADDR; > + priv->adap.quirks = quirks; > + } > + > if (muxc->mux_locked) > priv->adap.lock_ops = &i2c_mux_lock_ops; > else > diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h > index 98ef73b7c8fd..17ac68bf1703 100644 > --- a/include/linux/i2c-mux.h > +++ b/include/linux/i2c-mux.h > @@ -21,6 +21,7 @@ struct i2c_mux_core { > unsigned int mux_locked:1; > unsigned int arbitrator:1; > unsigned int gate:1; > + unsigned int share_addr_with_children:1; > > void *priv; > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 5e6cd43a6dbd..2ebac9e672ef 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -711,6 +711,8 @@ struct i2c_adapter_quirks { > #define I2C_AQ_NO_ZERO_LEN (I2C_AQ_NO_ZERO_LEN_READ | I2C_AQ_NO_ZERO_LEN_WRITE) > /* adapter cannot do repeated START */ > #define I2C_AQ_NO_REP_START BIT(7) > +/* @fixme document and find proper name */ > +#define I2C_AQ_SHARE_ADDR BIT(8) > > /* > * i2c_adapter is the structure used to identify a physical i2c bus along > > """ > > This works, however this only supports device-tree because of of_find_i2c_device_by_node. If we want to support acpi then we can either: > > > 1. Get the Mule i2c device address from fwnode_get_next_parent_dev but this is static since v6.9-rcx. > > 2. Pass the Mule i2c device address as a new member of struct i2c_adapter_quirks. > > > I would go for 2. Do you suggest something else? Yes, 2 is definitely neater. Thanks! Cheers, Peter
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@theobroma-systems.com> >>>> --- >>>> 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
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; + /* * Try to populate the mux adapter's of_node, expands to * nothing if !CONFIG_OF. diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h index 98ef73b7c8fd..17ac68bf1703 100644 --- a/include/linux/i2c-mux.h +++ b/include/linux/i2c-mux.h @@ -21,6 +21,7 @@ struct i2c_mux_core { unsigned int mux_locked:1; unsigned int arbitrator:1; unsigned int gate:1; + unsigned int share_addr_with_children:1; void *priv;
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@theobroma-systems.com> --- drivers/i2c/i2c-mux.c | 10 +++++++++- include/linux/i2c-mux.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-)