Received-SPF: pass (google.com: domain of linux-kernel+bounces-167892-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <8506322f-bfbf-4582-a88b-b300cad2d344@cherry.de>
Date: Fri, 3 May 2024 18:20:10 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with
 child nodes
To: Peter Rosin <peda@axentia.se>, Farouk Bouabid <farouk.bouabid@cherry.de>,
 Wolfram Sang <wsa+renesas@sang-engineering.com>,
 Andi Shyti <andi.shyti@kernel.org>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Quentin Schulz <quentin.schulz@theobroma-systems.com>,
 Heiko Stuebner <heiko@sntech.de>
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
 devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-rockchip@lists.infradead.org
References: <20240426-dev-mule-i2c-mux-v1-0-045a482f6ffb@theobroma-systems.com>
 <20240426-dev-mule-i2c-mux-v1-1-045a482f6ffb@theobroma-systems.com>
 <f7ddc503-21c6-10b8-3326-398de65bd6a8@axentia.se>
 <318b22c7-4e90-4055-a893-bb995c16e8da@cherry.de>
 <c5e375a0-8c64-f244-a5e6-bfcb3400d05a@axentia.se>
Content-Language: en-US
From: Quentin Schulz <quentin.schulz@cherry.de>
In-Reply-To: <c5e375a0-8c64-f244-a5e6-bfcb3400d05a@axentia.se>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?YWVsU1UvTUduK3M5L3J1RVJoS1QvdDVBSjlCWHIzS0hvU1dVdlZiMnRUSDFy?=
 =?utf-8?B?MmdJSEZqZ1JqZ3dNNjZBUFZGVGN4S3E2ZkZ3THc1aHpYOVBIUzBOSXFPUmhH?=
 =?utf-8?B?RnU1RDRuemdSUTdyc1doK1pWeTkxa0VsUWYvTVZyNUd0MzhsQmNXVGI1OXNh?=
 =?utf-8?B?Uk5jQU5ZT2Vsb01JZC85WlhlSGhzMGI2UkZITnhZREExY3IydkNzd2lhTmN6?=
 =?utf-8?B?WlRYUk82d1ZqcDRCUkN2VTFrTElNRVc2WTFQektlMm1OdGZGeHlQMElTZGFm?=
 =?utf-8?B?UDVNRHRJWENobUMrbThDYWhNaXdER05jQTdXN1FaUFpaRk8vS3VLRE8zMExD?=
 =?utf-8?B?bmdsRHFhSDdZcFZjbGUyV3E3OUo4UW5ZR2Z4aEhaT3dMK1FjNkF3UFQ1bXNJ?=
 =?utf-8?B?djRkWjI3OFNvM2F4QUFjdU85ME43L0FLNlE5cDhwZURkMURlaTdZTkJJZ25m?=
 =?utf-8?B?UmMyTzhVMUpsMU44M0h3ajg3YnZwakt2ZzBSS1dPMkZoZy8rdDdBeDZVM3FP?=
 =?utf-8?B?ZXpyMWtsMHdrOHJ0d1BPUERvbElYVGNQalRXbG9yWG9wU1oxeTZyMDkvRGNN?=
 =?utf-8?B?VnphQmtuU3laZnlPQTdiWDlnVzc2ZGZIcmlRZlZIMUlvWFpMZ2pybDlsZ20z?=
 =?utf-8?B?VHY4eVpJenF3anpNMGhkN3BCZ3ZvOGJWOGRWNmJoVTVlTmZtRTNOcVVCaHFP?=
 =?utf-8?B?SkszVWwydTFNRUhTYjNtWW9QZUFUd3Zmd21rVlNGQ01sTVFZNHJLR0ZHZXUr?=
 =?utf-8?B?SUljaFNQTW84WVM4dHAzcFJqaHFiR3VLd3hxOFVqbEVGdmdCODJjVk5UWHRP?=
 =?utf-8?B?dExLMlh1dzlHaVE4Smdhb3RJV2k3RS9nMmd1ODVNQjNRVmtiV1kzd3FQNkIr?=
 =?utf-8?B?SVJyYzNlSkVsUFVRelNoNDdIb2pZUmMrd0VNK1FCTWVPbnMxOWpRandDRDBT?=
 =?utf-8?B?SmlHMzlORnBURGZKTjc5NEVVeThzbDFRbnlqcmNPU0l2bStkU1owdTBJWmps?=
 =?utf-8?B?QWVTbFlNVHRaZ3VCclJvMEU3MzIvSCtZYzlYcGV2Z2hTd3FZb0V0OGJIMXBD?=
 =?utf-8?B?bWhpRk9mbXRWeElCb3hmTjE4U0VpeUwyYzBOT1E3Qnc5aEUwaDY5RVNxeFo2?=
 =?utf-8?B?Vkl2NWJuNUp6N3FuLzhHNDJaelMxVERIajB1bmxJZHFKS3dwdVZoRlh5Rkx0?=
 =?utf-8?B?bmRQUjRIR2pDY3UydEd6OC91OHd6MXpySEFwOXJRQnNJTFRUbG90c3pBK25w?=
 =?utf-8?B?cEdsRjhLOExJSUVFNkZIRTVtcVBoSmRCTDQ2VlNyMC9YVkM1SlhPQjQzOFRt?=
 =?utf-8?B?bXVET1JaYUtZU0ZWRUpLaURPczBNOTZld1RaZVNIQnB4WjZTSUk1cmpLS3Jq?=
 =?utf-8?B?WW1rbVIyS3J5WGxRNXRlN0NvNjRCTytFSk4zVUg3VVZOVXAybWpkR0lLS0hn?=
 =?utf-8?B?djRpNHZid3MvZ1VmVDZKVFVHNnorTlFJdlhEblhDZ1U4WVFGWmU1ZzJvVTNz?=
 =?utf-8?B?ZEVVYmZra0h3TUxTZFlobEw5Vjd4Zmp4M0UxbmlKV0hnMDZ3VFJ2aXpOejVU?=
 =?utf-8?B?TFFOdVptSWxGeXE2ZzVnbXdoeWFPR0dtUnV4WUtIbDJ5c3ZsQ0p6UDc2NXNk?=
 =?utf-8?B?enZ1dENwenh5MnZuVEJybWpnOUFFQUtPU3ZyZlhMNnVaQWQxVHR2TmY5OGNk?=
 =?utf-8?B?Q3AxSnA2am5pVURaR0p3dW8zNEdyKytXRzBNTU1xVFQwSmZ2YXBDTm8xdnVy?=
 =?utf-8?B?YjZPbThHbFNQNWZYbmNPU2ZNemU2TWNNdGZVWk4waHdkRkp0emxpQ3U3M1Yv?=
 =?utf-8?B?cUlsaUwzSjBWc09mdHI2QzNlUXozOWZ4UlFRQlFEYysrTmp3MVdORXA0cGpx?=
 =?utf-8?B?RkNKRjRxV0U1U0MwNUtoUzErS3pkU25PNWtWQTIwYmxPYWZkVUZlTnkreXVh?=
 =?utf-8?B?V3JzYXdLZ2Yzb2dNaFk0RzV2TEdMaGpka1BPZlBveUdUV0c0aWMzQmFWQ3ZJ?=
 =?utf-8?B?ZVBrUEZjblY4MWdDQzBjNmhTNXlTMkpGa29meEhhdVhoZGYzOG56ZVFwTGhn?=
 =?utf-8?B?d0FEL29hSnVwWGNDUXRuZGl0dXd5OGVZRnZIb0x6UWdxcS9tUlovR2RRRVNN?=
 =?utf-8?B?OGdtNXVaYVhiVTBFTWlkY0NVWVpNTVJHVGlBeTJ4Z1IrYUZTY2RtZlBESExt?=
 =?utf-8?B?dnc9PQ==?=
X-OriginatorOrg: cherry.de
X-MS-Exchange-CrossTenant-Network-Message-Id: 51e54e60-3c51-43c6-d54c-08dc6b8ce9ab
X-MS-Exchange-CrossTenant-AuthSource: DB7PR04MB4842.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 16:20:14.4548
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5e0e1b52-21b5-4e7b-83bb-514ec460677e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IYHGSo6wYq7PHivFXRUnxD1fWAY0LrgnD+nXMkjdTjR5VN6wL47C6ouhlotFv7wjzaJLkp5XNRTaDRInjrX7v+dYhNimQSxZ0clkw8qV9HQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM8PR04MB7745

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