Re: [PATCH v11 2/7] i2c: mux: add idle_state property to i2c_mux_core

[Peter Rosin]

On 2026-07-15 21:02, Marcus Folkesson wrote:
> Hi Peter,
>
> On Tue, Jul 14, 2026 at 01:29:01PM +0200, Peter Rosin wrote:
> > Hi Marcus,
> >
> > Sorry for the very late feedback.
> >
> > On 2026-07-13 09:19, Marcus Folkesson wrote:
> > > Muxes treat their channels differently when idle.
> > > Let the mux core have this information to make it available for
> > > internal use.
> > > Reuse the same state values used by CONFIG_MULTIPLEXER.
> > >
> > > Possible idle states are:
> > > - MUX_IDLE_AS_IS: Leave channels as is when idle
> > > - MUX_IDLE_DISCONNECT: Disconnect channel (set HiZ when idle)
> > > - <n>: Enable channel n when idle
> > >
> > > Default value is set to MUX_IDLE_AS_IS.
> > >
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
> > > ---
> > >    drivers/i2c/i2c-mux.c   |  1 +
> > >    include/linux/i2c-mux.h | 26 ++++++++++++++++++++++++++
> > >    2 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> > > index 681a201c239b..edf16683dc83 100644
> > > --- a/drivers/i2c/i2c-mux.c
> > > +++ b/drivers/i2c/i2c-mux.c
> > > @@ -247,6 +247,7 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> > >    	muxc->select = select;
> > >    	muxc->deselect = deselect;
> > >    	muxc->max_adapters = max_adapters;
> > > +	muxc->idle_state = MUX_IDLE_AS_IS;
> >
> > This is insufficient. AS_IS is simply not an adequate default.
> >
> > For i2c-mux-gpmux, there is currently no way to dig out what
> > the idle state is, as it is not exposed by the mux subsystem. For
> > i2c-mux-gpio, the idle state depends on both the idle-state /and/
> > the i2c-mux-idle-disconnect props. For i2c-mux-pca954x the idle
> > state can be adjusted at runtime. Etc.
> >
> > In short, idle state handling is a bit diverse, and I think this
> > adds to that mess.
> >
> > I think it will be a bit of work to come up with a scheme for the
> > I2C mux core to accurately keep track of what the idle state is.
> > One way to deal with that is to introduce a new "unknown" value
> > that can be the default for all drivers that has not yet figured
> > out how to feed the correct idle state to the core.
> >
> > And that hints at why I think reusing the mux.h bindings #include
> > is bad. The mux subsystem simply has no need for an unknown state,
> > and adding that to mux.h is therefore out of place.
>
> I see.
>
> I think I will introduce a few defines in i2c-mux.c then;
>
>
> #define I2C_MUX_IDLE_UNKNOWN	(-1)
> #define I2C_MUX_IDLE_AS_IS      (-2)
> #define I2C_MUX_IDLE_DISCONNECT (-3)

Hi!

Please keep AS_IS as -1 and DISCONNECT as -2. Using different
values will make it difficult to get rid of the use of the
defines belonging to the mux subsystem in case the actual
value has crept into some .dtb or something like that.

Also, I think the right thing to do is to put these defines
in the i2c-mux.h header so that the drivers can find them.
I assume .c was a typo?

> [...]
>
> struct i2c_mux_core {
>
>      [...]
>
> 	/*
> 	 * The mux state to use when not active.

This is not 100% accurate. The value stored here is never
actually used to set the idle state. The idle_state here is
only what the driver has declared that the idle_state is.
Perhaps word it like this instead?

	* The mux state used by the driver when idle.

Agreed, subtle difference, but...

In the future, drivers (most of them) could be changed to
use this variable to store the actual idle state. But, as
mentioned, that's not easy for i2c-mux-gpmux since the idle
state is under the control of the the mux subsystem in that
case.

Cheers,
Peter

> 	 * Possible idle states are:
> 	 *  - I2C_MUX_IDLE_UNKNOWN: Unknown idle state
> 	 *  - I2C_MUX_IDLE_AS_IS: Leave channels as is when idle
> 	 *  - I2C_MUX_IDLE_DISCONNECT: Disconnect channel (set HiZ when idle)
> 	 *  - <n>: Enable channel n (starting from 0) when idle"
> 	 *
> 	 * Default value is set to I2C_MUX_IDLE_UNKNOWN.
> 	 */
> 	int idle_state;
>
>      [...]
> };