Re: [PATCH v11 2/7] i2c: mux: add idle_state property to i2c_mux_core
From: Peter Rosin <peda@lysator.liu.se>
Date: 2026-07-14 11:29:33
Also in:
linux-i2c, lkml
Hi Marcus, Sorry for the very late feedback. On 2026-07-13 09:19, Marcus Folkesson wrote:
quoted hunk ↗ jump to hunk
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@gmail.com> --- 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.
quoted hunk ↗ jump to hunk
return muxc; }diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h index 1784ac7afb11..624fadbaa703 100644 --- a/include/linux/i2c-mux.h +++ b/include/linux/i2c-mux.h@@ -13,6 +13,7 @@ #ifdef __KERNEL__ +#include <dt-bindings/mux/mux.h>
I think it is bad to bring in this include here. That include belongs to the mux subsystem. Yes, it is included by others as well, but please don't expand on that.
quoted hunk ↗ jump to hunk
#include <linux/bitops.h> struct i2c_mux_core {@@ -22,6 +23,17 @@ struct i2c_mux_core { unsigned int arbitrator:1; unsigned int gate:1; + /* + * The mux controller state to use when inactive.
A "mux controller" is a concept in the mux subsystem, and there is no equivalent for I2C muxes. Cheers, Peter
quoted hunk ↗ jump to hunk
+ * 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. + */ + int idle_state; + void *priv; int (*select)(struct i2c_mux_core *, u32 chan_id);@@ -38,6 +50,20 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent, int (*select)(struct i2c_mux_core *, u32), int (*deselect)(struct i2c_mux_core *, u32)); +/* + * Mux drivers may only change idle_state, and may only do so + * between allocation and registration of the mux controller. + */ +static inline void i2c_mux_set_idle_state(struct i2c_mux_core *muxc, int state) +{ + muxc->idle_state = state; +} + +static inline int i2c_mux_idle_state(struct i2c_mux_core *muxc) +{ + return muxc->idle_state; +} + /* flags for i2c_mux_alloc */ #define I2C_MUX_LOCKED BIT(0) #define I2C_MUX_ARBITRATOR BIT(1)