| Message ID | 20240426-dev-mule-i2c-mux-v1-1-045a482f6ffb@theobroma-systems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Mule I2C multiplexer support | expand |
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 > + > /* > * 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; > >
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(-)