Received-SPF: pass (google.com: domain of linux-kernel+bounces-171028-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <34968505-f8b1-4c6f-9d9c-5938edcdba68@cherry.de>
Date: Tue, 7 May 2024 11:48:41 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/7] i2c: mux: add the ability to share mux core
 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>, Heiko Stuebner <heiko@sntech.de>,
 Quentin Schulz <quentin.schulz@theobroma-systems.com>
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: <20240506-dev-mule-i2c-mux-v2-0-a91c954f65d7@cherry.de>
 <20240506-dev-mule-i2c-mux-v2-1-a91c954f65d7@cherry.de>
 <9b12dc9f-054b-fba5-e23d-48d0fe1a00e2@axentia.se>
Content-Language: en-US
From: Quentin Schulz <quentin.schulz@cherry.de>
In-Reply-To: <9b12dc9f-054b-fba5-e23d-48d0fe1a00e2@axentia.se>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?Znk2Vjd4cmlDcVEycEVnSlVrMUVEM1BqekU4WHFicW80U3F5OWN0T2ZFU0l3?=
 =?utf-8?B?K2pqdnZaTGJ1M00rNGVod0sreG56b0FKSzdZVzBsNllXWGJ2dW5CT0ZrZDdy?=
 =?utf-8?B?aTUzTVY2MkJuMUxVQnNiTmprZE9yN1FtZnhJMWR3MnpoK29ua01sdjhqYnNU?=
 =?utf-8?B?SlVyMnZNNWZiNlozbXRIOFZzSkJRZ1p1dVBwSURTUEZQWFBCVXlUZ2JJaW4y?=
 =?utf-8?B?a01NTGxxRHVtakpRL2I5TWxQOWkzYlZwL04vSk1KejEzelpEVXBlaWdXRFVo?=
 =?utf-8?B?Zzkvb1J2cWZ1ZzNONHBJVmZUdlFZbHhpL1RJTkJncmI3VWUzSmJKeDVWWlNl?=
 =?utf-8?B?Uk5HdHZya3lBeXpoSTVGaDBrMjl2T2dObzBjNk44VmorZ01WUG5oYWVnMWNV?=
 =?utf-8?B?RG5iemhhTjVuMnRhSTNDd1BpUFpmR0NjQzVXbnk1cEljK1FYdnEzM1FuRjFS?=
 =?utf-8?B?aUdCeWxjNytzWVZKM2hvczFFbUhjVy9SYWVBZWxNU3krWmRDcS9qVVpBRUpX?=
 =?utf-8?B?S28weFE2TVJvYS9oOGVRL3UzeU1rb2hIVkdwMEtSYnhzc2dERGZHdm9idEtn?=
 =?utf-8?B?dlN4OC9BL3FPb2FlNGhCK0RKRzl2MWY1WHJFV0x3UVlIcXkwRndqVkI4cEhE?=
 =?utf-8?B?aE5JU0djNkQxYXk5Sm5zVExuQlVvSkdzY00zWXg2cGlHOUVpSm5PUVdRQ2Vh?=
 =?utf-8?B?UE5uUXJDY3VMVGpPbTlQbUpCWW5aUEFkcEU3cDRxeis3R002aUhVcVNKL2hG?=
 =?utf-8?B?QXZUakF3Rmp1bFdCOXdjQXd3QnhDMDJaUEZiSzU1a0xlV01JMFFsREk5c01t?=
 =?utf-8?B?RDFvZklxS1ZkeTkvZlU3MEFTNWRyVm1TM2MrYWZ6QmhRbVJmb1JEb21mVU1I?=
 =?utf-8?B?U3F0eHdNR0svV00vRm1CaGhaejArc245OVBwb1F3SVN6UkFQZjloUSs5b0xP?=
 =?utf-8?B?eWRIVlkrQnMyTnJMMU1nekJSb21ock1zdGIrd3dvWXY0MGQ2Vm4zd3FISHRX?=
 =?utf-8?B?aFk1Y3FCay83b2hHR3B6UktHQlp0VGRXejM1ajU3RmZJRDJ4T2wrcEd6TnFJ?=
 =?utf-8?B?czFMMWw5UGhSZmF0eFRGS2Q4OGJUZlgrMzUvZnlRR01CT0hYVWYvSjFERWN0?=
 =?utf-8?B?em1pbjNudFIyOXRkdmZ3eHM5ZTBsRHo5ZUtvR2pkQmtLSnpEb2wwK2hYSzlM?=
 =?utf-8?B?RE5Sbk95aHNOYy9JaWlTQnpmVmhBc0M0SUlUU3cxWnB3OGVjSXJtVXNYc2po?=
 =?utf-8?B?TTE4YnpXOTZHelliVHE4b1VyTGF1QVpWMi9Cb0lraXNiRTd5M1hGNTc3WjlE?=
 =?utf-8?B?WnZrSXJEa3pPRml0VDExeEczb3ZLdGZ5OXE1Q3duSHRuMFdwbTVaMDdZUWlt?=
 =?utf-8?B?eHNpY2pDUGZRRzNtZ2FIUTYvakVvVWdsZXpZS3ZpMkowMEgzZzAzb0NGMmdj?=
 =?utf-8?B?V0xvdjZSYVFmR3hnSmFvcmxZeWZzdnV4YkNkT2NZWEZyZFliSWRjaFNPamUr?=
 =?utf-8?B?VGFrajQvRnpLUlB1TFFFK1ZPWUlSZlJmYWM5Z205NGVaS0VUM3o3WkRlQ2o1?=
 =?utf-8?B?Ymk5UEFiOTA4UXZ2d0owZGdNdHphYk5oOHkrZUozOWxOWVUxQnR5SXI2QjRF?=
 =?utf-8?B?bzlOR0NHNFh5eitYenRNWW81NVBYRmVmWGhucmtEMWU3cTNVMEtRM2dmNFhC?=
 =?utf-8?B?QVA1cWQyeGxOZEdxWVU2eE5GSXgwdXZOM2syYnJ4b0tRYytKWGJEbGpuVnNa?=
 =?utf-8?B?TlRCMjJ2OHZwbUtoSDJhVm1oR0lUOWpTdmliUFpMbXBISlZXWXJOTTBsZ1Jw?=
 =?utf-8?B?M0ZVODdRdlZIbUxydkw5MEZvTURqcEVxbUhPcWJ2VGxsbHNCSUNDb1VyWkdG?=
 =?utf-8?B?V0Z5akUzRmJld3RjVUVRc090am5GN2JVYUxVbHVWVE1wZVFpenhGWjdrVDdr?=
 =?utf-8?B?T050VVFlM2tCQmVtMWJta013MDFQWWNUK3VjeWpPaHh5Nzh1cTlEM1JhcFp0?=
 =?utf-8?B?UGhiWDQ0eXM3MkpJRzEycThMOVQ0cmxZQ2t1bnlpclpQUllqQTVnYWtyeGVN?=
 =?utf-8?B?b1pwSjlKRG04d3U3Y3I0OUlvemk4Q2lHd3RQRWoyK29lYWJzakhCeGw1ZUpT?=
 =?utf-8?B?VzlBVExwMmE3T0Z3VnlMQkpmcHJXZnV1R0VzSHd1OGtSaFhSVkM1OVJCME50?=
 =?utf-8?B?c0E9PQ==?=
X-OriginatorOrg: cherry.de
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f0a89f7-7b0b-405d-8c4a-08dc6e7ae239
X-MS-Exchange-CrossTenant-AuthSource: DB7PR04MB4842.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 09:48:44.1825
 (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: eiMTtDb2M33nHVKbg4v5s/igDa7dDANMS7ah/hWduIhvL8OlN9whmdZdKeH6SCDLf0Ye0c+2jhJ+Mq2oxGdUxVs5q4TIFmp2QVBDgcZzt0Y=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB9554

Hi Peter,

On 5/6/24 11:26 PM, Peter Rosin wrote:
> Hi!
> 
> Regarding the subject (and elsewhere) I think of "mux core" as roughly
> the code in the i2c-mux.c file. So, for me, the "mux core" does not have
> an address, it is a mux "driver instance" or "device" that sits on the
> I2C address that you need to share.
> 

I'm the one who suggested mux core here (privately) :)

The issue is that in my head a mux device is the i2c adapter/bus (from 
i2c-mux.yaml dt binding example):

"""
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;

         i2c-mux@70 {
             compatible = "nxp,pca9548";
             reg = <0x70>;
             #address-cells = <1>;
             #size-cells = <0>;

             i2c@3 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 reg = <3>;

                 gpio@20 {
                     compatible = "nxp,pca9555";
                     gpio-controller;
                     #gpio-cells = <2>;
                     reg = <0x20>;
                 };
             };
             i2c@4 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 reg = <4>;

                 gpio@20 {
                     compatible = "nxp,pca9555";
                     gpio-controller;
                     #gpio-cells = <2>;
                     reg = <0x20>;
                 };
             };
         };
     };
"""

"mux core" here would refer to i2c-mux@70, "mux device"/"mux" i2c@3 or 
i2c@4. E.g. when I'm saying "in mux 3", I'm talking about i2c@3 here.

For me a driver instance is a device, so "mux driver instance" would be 
a "mux device". Ah... naming is hard. Anyway, up to you, I just wanted 
to make sure we're talking about the same thing and there's no confusion 
here.

[...]
> I also wonder if that second condition (...->type == &i2c_client_type) should
> be a WARN_ON_ONCE? I don't see how the flag can be set sanely on an adapter
> that is not itself an I2C client. Can it?
> 

Agreed, good suggestion here... Though... 
https://lwn.net/Articles/969923/ it seems new additions of WARN_ON are 
now discouraged? Not looking to start a discussion here about whether 
WARN_ON is good or bad, merely pointing at this if it was missed somehow.

>> +
>> +		if (!quirks)
>> +			return -ENOMEM;
>> +
>> +		if (parent->quirks)
>> +			memcpy(quirks, parent->quirks, sizeof(*quirks));
>> +
>> +		quirks->flags |= I2C_AQ_SKIP_ADDR_CHECK;
>> +		quirks->skip_addr_in_parent = client->addr;
>> +		priv->adap.quirks = quirks;
> 
> The I2C_AQ_SKIP_ADDR_CHECK flag should probably not be propagated?
> 

Oh... you mean if we have a mux on an i2c adapter of a mux and the 
adapters handled by the parent mux have SKIP_ADDR set and we don't want 
the adapters handled by the leaf mux to have this flag as well? Is that 
what you meant?

Cheers,
Quentin