Messages in this thread | | | Date | Fri, 3 May 2024 18:20:10 +0200 | Subject | Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes | From | Quentin Schulz <> |
| |
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
| |