Received-SPF: pass (google.com: domain of linux-kernel+bounces-162608-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <f7ddc503-21c6-10b8-3326-398de65bd6a8@axentia.se>
Date: Mon, 29 Apr 2024 17:46:50 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with
 child nodes
Content-Language: sv-SE, en-US
To: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>,
 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>
From: Peter Rosin <peda@axentia.se>
In-Reply-To: <20240426-dev-mule-i2c-mux-v1-1-045a482f6ffb@theobroma-systems.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RHdSSE1GZmcwM2UydDZhem5zaFZHTm5BQ0JnSFQya2xnMmJiRmRaUFVEci9y?=
 =?utf-8?B?RFpmS0k4bUxFM3lxZmo0N3pkeFlMVWFNWnFJK2xGUGdEQW4rM0k0RkgrWGNj?=
 =?utf-8?B?c090WFFyUmdIM25CNXpVU0JOM2lObnV1elF5NndtekFJMXJOYlZ6ZktpSHRm?=
 =?utf-8?B?QXRxdnRVSVFnM1Bzamo0alRqdktDVFluRkxFMEpySkgyQ3czWEhlbVIyb29C?=
 =?utf-8?B?bEUzS2lEbHZjdDlNeng3bkFFa0szckh4bkpwMnVzNTJ5VVdKa3U2NGhhQTVB?=
 =?utf-8?B?a3Fsa2NZOUlsMWJNY0pTaWpmSjJWbHZjaUpLaC9tODNZVzlEeUVBY0VKa3U2?=
 =?utf-8?B?a1Y1a21Nc2kra05QVitMbGpZWWdEdkttenVqbFFsbnRlcE5XRXRXOXRSM1Iv?=
 =?utf-8?B?OTJEZ1dqeTVwUGNxMHlDUjRJazFZQzl2aCtLWTRzNjBySi91cUdudW5YR2Z5?=
 =?utf-8?B?VWdkZG4zSDF1d1hDQmZNYVFDMXIyTm1KQlM1SU9NekJ1QlB4K0ZneWY3QXR1?=
 =?utf-8?B?cTVNQ2FvSlRyZ09nbmxzQWI3UXB4aXhRQkVaUnNnTFZwbVQrcTI1cHNXVEE5?=
 =?utf-8?B?NzRjK2Yvc05TS0ZMb1VkdVAwakFtaHg4NUluaHBJNW9UZVJLczgzck5QdVlN?=
 =?utf-8?B?bmxiUUVmU0g1WjZudUhGVm9CSy9mdW9vZVluK2orbXNrTU9nTHBzcm5PdmRH?=
 =?utf-8?B?L0VkeXlXd1MycFFuZlYyZmJuaG44MnBjZklpMldKRC9NeXAza2lySGxaQkRR?=
 =?utf-8?B?VTFrREgrUFY1bjhGL2FSQndBN2VDK1E5MDRsOGdQQVF5MXhGWklkUFFpM1B1?=
 =?utf-8?B?YXEzV3JsaWFOclFzNWNZKzh3YUJqdURPU0RiNHAzaGhabnhMaTdzSjBzcHdv?=
 =?utf-8?B?RmZsMEc3bDBFVUk3MkthQ0ltbEkxdFl0SlZaRmN2VGRvRS95RnE0VENHdUhh?=
 =?utf-8?B?dGZWMmhPREZtT3p1U0JlTnhvcDVDOUExRlh2S3FWZEJmYy9BQng5Mjlwd2Rl?=
 =?utf-8?B?QlFVTzNhUjEzaVNwVEU0Mkp4YXVXK0VSeWtRQkZnQ0t5YVRDenVnb0J2bVdw?=
 =?utf-8?B?bDF2TzJCMnBjUHpaTmlIcmpzcDI0YWhOd3FwRFE3WWQzd3BWSWxMVnRZRURX?=
 =?utf-8?B?V3VVNHp0NlN6UWpoMTRvTGdjaHpFZzhrTTgvSTI2TlpIS0xXMUxIKytpdGR6?=
 =?utf-8?B?a0p3QmZpNjZQdEl2Wmt1b2NQWlVnQWU4aysyUGFpOEthczVPTloyMzVzaUdK?=
 =?utf-8?B?c3hrTjBkMVBUT3pHVjYwd1FEejh0SkVLOWZYZW9zL2RjSnBBYVJ5VDlqWmV2?=
 =?utf-8?B?bGVGampQOXdac1ArMXowLytJN3lNRDNoQUdrcG53TmxLREVCSzUwUUdMV2s4?=
 =?utf-8?B?ZjVuYXMyZTduRTRyNUI1RGlxdXdXK25RYmFDSzlRT1JEbUhtaXZlR1pkSVdi?=
 =?utf-8?B?VEZSSVFyMkdDRWR3eWcyUjMwV1BNSGhUcnE4cXRRNnU1U3cySzNkbDE5UXBl?=
 =?utf-8?B?U01mU2hycDNLT2ZIZk5hWWJqTHovM3FVNndTK1doRmwrWVlRcjdEbkdMQkg5?=
 =?utf-8?B?eXpiVU44NHVlWDZFRHFQVCswWlppbko3ZW8rZ0pXMEdiVU5aL0dDMldWWkcy?=
 =?utf-8?B?b0xkU2tuVjlLWDNxYTZNVnBrTDRrbXpyNldCcGFYclZOM3dhV0xmc1Eyb3h0?=
 =?utf-8?B?V3U1VWlTTVk5WmhCQlowanNhMHJ4U05jL0FNSVp6OUdwK3FpRm4yTDZ3Wmtv?=
 =?utf-8?B?L3Z3TFNwWEgvTDkyZkorL3hvb1M5S200ODZwMEVpbXNsUVJUaDFOZlBNS3JU?=
 =?utf-8?B?YjE3TksrYnZycXNyMC95S2JrbTQ1cnVqeVJnenZYZ0JtU0dHUzI2alhYRUpa?=
 =?utf-8?B?cnFTQUtwbWxVcjFNTlBOMlpSZ1ltZVNMNUo2L2djenRSWWlaZVN4eUI4bHJw?=
 =?utf-8?B?ejlNMlJsRDNaenUvd24xM0ZpdkllejgyaHJlaGhsbGZ0UmFlell3clRGUzFT?=
 =?utf-8?B?N2s3UzZTc3JtYTBodTFDR2M1dENZTjF0dENJZ05maDJZMnBLYWRjUEkzTWwx?=
 =?utf-8?B?bFp1VENxdjBRTWgzdWlHSHFCaU1Pb2JETFhvaEJLZXllWHhrSmg4cWhKbkRy?=
 =?utf-8?Q?w1BJD7L39gPEexh8BQeMTkpAJ?=
X-OriginatorOrg: axentia.se
X-MS-Exchange-CrossTenant-Network-Message-Id: b9691d7f-de97-461f-8b36-08dc686396f2
X-MS-Exchange-CrossTenant-AuthSource: DU0PR02MB8500.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 15:46:52.5379
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4ee68585-03e1-4785-942a-df9c1871a234
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Ae0UUO9e3zu7b7S4bYPWSkaOWLM65vvfUXMQMiBCjimK2TnAyKr7gW0j6kP3MS9B
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR02MB10622

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;
>  
>