Message-ID: <0186ebba-958b-8076-3706-1edc75b6c6d3@axentia.se>
Date: Tue, 23 Sep 2025 17:10:16 +0200
From: Peter Rosin <peda@...ntia.se>
To: Marcus Folkesson <marcus.folkesson@...il.com>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Michael Hennerich <michael.hennerich@...log.com>,
 Bartosz Golaszewski <brgl@...ev.pl>, Andi Shyti <andi.shyti@...nel.org>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH RFC 0/7] I2C Mux per channel bus speed

Hi!

2025-09-22 at 08:20, Marcus Folkesson wrote:
> This is an RFC on how to implement a feature to have different bus
> speeds on different channels with an I2C multiplexer/switch.
> 
> The benefit with this approach is that you may group devices after
> the fastest bus speed they can handle.
> A real-world example is that you could have e.g. a display running @400kHz
> and a smart battery running @100kHz using the same I2C controller.
> 
> There are many corner cases where this may cause a problem for some
> hardware topologies. I've tried to describe those I could think of
> in the documentation, see Patch #7.
> 
> E.g. one risk is that if the mux driver does not disconnect channels
> when Idle, this may cause a higher frequency to "leak" through to
> devices that are supposed to run at lower bus speed.
> This is not only a "problem" for changing bus speed but could also be
> an issue for potential address conflicts.
> 
> The implementation is split up into several patches:
> 
> Patch #1 Introduce a callback for the i2c controller to set bus speed
> Patch #2 Introduce idle state to the mux core.
> Patch #3 Introduce functionality to adjust bus speed depending on mux
>          channel.
> Patch #4 Set idle state for an example mux driver
> Patch #5 Cleanup i2c-davinci driver a bit to prepare it for set_clk_freq
> Parch #6 Implement set_clk_freq for the i2c-davinci driver
> Parch #7 Update documentation with this feature
It seems excessive to add idle_state to struct i2c_mux_core for the sole
purpose of providing a warning in case the idle state runs on lower speed.
Especially so since the default idle behavior is so dependent on the mux.

E.g. the idle state is completely opaque to the driver of the pinctrl mux.
It simply has no way of knowing what the idle pinctrl state actually means,
and can therefore not report back a valid idle state to the i2c-mux core.

The general purpose mux is also problematic. There is currently no API
for the gpmux to dig out the idle state from the mux subsystem. That
can be fixed, of course, but the mux susbsystem might also grow a way
to change the idle state at runtime. Or some other consumer of the "mux
control" used by the I2C gpmux might set it to a new state without the
I2C gpmux having a chance to prevent it (or even know about it).

You can have a gpio mux that only muxes SDA while SCL is always forwarded
to all children. That might not be healthy for devices not expecting
overly high frequencies on the SCL pin. It's probably safe, but who knows?

The above are examples that make the warning inexact.

I'd prefer to just kill this idle state hand-holding from the code and
rely on documentation of the rules instead. Whoever sets this up must
understand I2C anyway; there are plenty of foot guns, so avoiding this
particular one (in a half-baked way) is no big help, methinks.

This has the added benefit of not muddying the waters for the idle state
defines owned by the mux subsystem.

Cheers,
Peter

Powered by blists - more mailing lists