Re: [PATCH v11 2/7] i2c: mux: add idle_state property to i2c_mux_core
Marcus Folkesson <[email protected]>
| Newsgroups | org.kernel.vger.linux-i2c,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
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 <[email protected]> > > --- > > 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) [...] struct i2c_mux_core { [...] /* * The mux state to use when not active. * 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; [...] }; Would that be a better approach? Thanks, Marcus Folkesson