lkml.org 
[lkml]   [2024]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes
From
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

\
 
 \ /
  Last update: 2024-05-27 18:15    [W:0.070 / U:1.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site