Re: [PATCH v12 3/7] i2c: mux: add support for per channel bus frequency

[Peter Rosin]

Hi!

On 2026-07-19 16:59, Marcus Folkesson wrote:
> There may be several reasons why you may need to use a certain speed
> on an I2C bus. E.g.
> 
> - When several devices are attached to the bus, the speed must be
>    selected according to the slowest device.
> 
> - Electrical conditions may limit the usable speed on the bus for
>    different reasons.
> 
> With an I2C multiplexer, it is possible to group the attached devices
> after their preferred speed by e.g. putting all "slow" devices on a
> separate channel on the multiplexer.
> 
> Consider the following topology:
> 
>                        .----------. 100kHz .--------.
>      .--------. 400kHz |          |--------| dev D1 |
>      |  root  |--+-----| I2C MUX  |        '--------'
>      '--------'  |     |          |--. 400kHz .--------.
>                  |     '----------'  '-------| dev D2 |
>                  |  .--------.               '--------'
>                  '--| dev D3 |
>                     '--------'
> 
> One requirement with this design is that a multiplexer may only use the
> same or lower bus speed as its parent.
> Otherwise, if the multiplexer would have to increase the bus frequency,
> then all siblings (D3 in this case) would run into a clock speed it may
> not support.
> 
> The bus frequency for each channel is set in the devicetree. As the
> i2c-mux bindings import the i2c-controller schema, the clock-frequency
> property is already allowed.
> If no clock-frequency property is set, the channel inherits their parent

channels inherit their parent
the channel inherits its parent
or
the channel inherits the parent

> bus speed.
> 
> The following example uses dt bindings to illustrate the topology above:
> 
>      i2c {
> 	clock-frequency = <400000>;

Indenting with a tab on this line, spaces everywhere else in the
block.

> 
>          i2c-mux {
>              i2c@0 {
>                  clock-frequency = <100000>;
> 
>                  D1 {
>                      ...
>                  };
>              };
> 
>              i2c@1 {
>                  D2 {
>                    ...
>                  };
>              };
>          };
> 
>          D3 {
>              ...
>          }
>      };
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
> ---
>   drivers/i2c/i2c-mux.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 149 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index b126ce7338c2..00de5f124132 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -36,21 +36,118 @@ struct i2c_mux_priv {
>   	u32 chan_id;
>   };
>   
> +static inline int
> +__i2c_adapter_set_clk_freq(struct i2c_adapter *adapter, u32 clock_Hz)
> +{
> +	/* If the clock frequency is already set to the requested value, do nothing. */

Long lines of text are hard to read. Please keep code and
especially text and comments below 80 columns. Sometimes it
can certainly be useful and clearer if you break that "rule",
but make that exceptional.

See "2) Breaking long lines and strings" in coding-style.

> +	if (adapter->clock_Hz == clock_Hz)
> +		return 0;
> +
> +	if (adapter->set_clk_freq) {
> +		adapter->clock_Hz = adapter->set_clk_freq(adapter, clock_Hz);
> +		if (adapter->clock_Hz != clock_Hz)
> +			dev_warn(&adapter->dev, "Could not set requested frequency. Requested %uHz, got %uHz\n",
> +				 clock_Hz, adapter->clock_Hz);
> +
> +		return 0;

Why is this not an error?

> +	}
> +
> +	/*
> +	 * If the adapter is a root adapter without .set_clk_freq() implemented, this feature is not
> +	 * supported.
> +	 */
> +	if (!i2c_parent_is_i2c_adapter(adapter))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Update the clock_Hz for non-root adapters, even if .set_clk_freq() is not implemented,
> +	 * to allow the clock frequency to be propagated to root adapters that do support it.
> +	 */
> +	adapter->clock_Hz = clock_Hz;
> +	return 0;
> +}
> +
> +static inline int
> +i2c_adapter_set_clk_freq(struct i2c_adapter *adapter, u32 clock_Hz)
> +{
> +	int ret;
> +
> +	i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
> +	ret = __i2c_adapter_set_clk_freq(adapter, clock_Hz);
> +	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
> +
> +	return ret;
> +}
> +
> +static int i2c_mux_select_chan(struct i2c_adapter *adap, u32 chan_id, u32 *oldclock)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	struct i2c_mux_core *muxc = priv->muxc;
> +	struct i2c_adapter *parent = muxc->parent;
> +	int ret;
> +
> +	if (priv->adap.clock_Hz && priv->adap.clock_Hz < parent->clock_Hz) {
> +		*oldclock = parent->clock_Hz;
> +
> +		if (muxc->mux_locked)
> +			ret = i2c_adapter_set_clk_freq(parent, priv->adap.clock_Hz);
> +		else
> +			ret = __i2c_adapter_set_clk_freq(parent, priv->adap.clock_Hz);
> +
> +		dev_dbg(&adap->dev, "Set clock frequency %uHz on %s\n",
> +			priv->adap.clock_Hz, parent->name);
> +
> +		if (ret)
> +			dev_err(&adap->dev,
> +				"Failed to set clock frequency %uHz on adapter %s: %d\n",
> +				*oldclock, parent->name, ret);

I think the whole select operation should fail and abort if
the attempt to lower the bus speed fails. Having unexpected
high speed transfers on a segment is not good.

Also, *oldclock seems to be the wrong frequency to print?

> +	}
> +
> +	return muxc->select(muxc, priv->chan_id);
> +}
> +
> +static void i2c_mux_deselect_chan(struct i2c_adapter *adap, u32 chan_id, u32 oldclock)
> +{
> +	struct i2c_mux_priv *priv = adap->algo_data;
> +	struct i2c_mux_core *muxc = priv->muxc;
> +	struct i2c_adapter *parent = muxc->parent;
> +	int ret;
> +
> +	if (muxc->deselect)
> +		muxc->deselect(muxc, priv->chan_id);
> +
> +	if (oldclock && oldclock != priv->adap.clock_Hz) {
> +		if (muxc->mux_locked)
> +			ret = i2c_adapter_set_clk_freq(parent, oldclock);
> +		else
> +			ret = __i2c_adapter_set_clk_freq(parent, oldclock);
> +
> +		dev_dbg(&adap->dev, "Restored clock frequency %uHz on %s\n",
> +			oldclock, parent->name);
> +
> +		if (ret)
> +			dev_err(&adap->dev,
> +				"Failed to set clock frequency %uHz on adapter %s: %d\n",
> +				oldclock, parent->name, ret);
> +	}
> +}
> +
>   static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
>   				 struct i2c_msg msgs[], int num)
>   {
>   	struct i2c_mux_priv *priv = adap->algo_data;
>   	struct i2c_mux_core *muxc = priv->muxc;
>   	struct i2c_adapter *parent = muxc->parent;
> +	u32 oldclock = 0;
>   	int ret;
>   
>   	/* Switch to the right mux port and perform the transfer. */
>   
> -	ret = muxc->select(muxc, priv->chan_id);
> +	ret = i2c_mux_select_chan(adap, priv->chan_id, &oldclock);
>   	if (ret >= 0)
>   		ret = __i2c_transfer(parent, msgs, num);
> -	if (muxc->deselect)
> -		muxc->deselect(muxc, priv->chan_id);
> +
> +	i2c_mux_deselect_chan(adap, priv->chan_id, oldclock);
>   
>   	return ret;
>   }
> @@ -61,15 +158,16 @@ static int i2c_mux_master_xfer(struct i2c_adapter *adap,
>   	struct i2c_mux_priv *priv = adap->algo_data;
>   	struct i2c_mux_core *muxc = priv->muxc;
>   	struct i2c_adapter *parent = muxc->parent;
> +	u32 oldclock = 0;
>   	int ret;
>   
>   	/* Switch to the right mux port and perform the transfer. */
>   
> -	ret = muxc->select(muxc, priv->chan_id);
> +	ret = i2c_mux_select_chan(adap, priv->chan_id, &oldclock);
>   	if (ret >= 0)
>   		ret = i2c_transfer(parent, msgs, num);
> -	if (muxc->deselect)
> -		muxc->deselect(muxc, priv->chan_id);
> +
> +	i2c_mux_deselect_chan(adap, priv->chan_id, oldclock);
>   
>   	return ret;
>   }
> @@ -82,16 +180,17 @@ static int __i2c_mux_smbus_xfer(struct i2c_adapter *adap,
>   	struct i2c_mux_priv *priv = adap->algo_data;
>   	struct i2c_mux_core *muxc = priv->muxc;
>   	struct i2c_adapter *parent = muxc->parent;
> +	u32 oldclock = 0;
>   	int ret;
>   
>   	/* Select the right mux port and perform the transfer. */
>   
> -	ret = muxc->select(muxc, priv->chan_id);
> +	ret = i2c_mux_select_chan(adap, priv->chan_id, &oldclock);
>   	if (ret >= 0)
>   		ret = __i2c_smbus_xfer(parent, addr, flags,
>   				       read_write, command, size, data);
> -	if (muxc->deselect)
> -		muxc->deselect(muxc, priv->chan_id);
> +
> +	i2c_mux_deselect_chan(adap, priv->chan_id, oldclock);
>   
>   	return ret;
>   }
> @@ -104,16 +203,17 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
>   	struct i2c_mux_priv *priv = adap->algo_data;
>   	struct i2c_mux_core *muxc = priv->muxc;
>   	struct i2c_adapter *parent = muxc->parent;
> +	u32 oldclock = 0;
>   	int ret;
>   
>   	/* Select the right mux port and perform the transfer. */
>   
> -	ret = muxc->select(muxc, priv->chan_id);
> +	ret = i2c_mux_select_chan(adap, priv->chan_id, &oldclock);
>   	if (ret >= 0)
>   		ret = i2c_smbus_xfer(parent, addr, flags,
>   				     read_write, command, size, data);
> -	if (muxc->deselect)
> -		muxc->deselect(muxc, priv->chan_id);
> +
> +	i2c_mux_deselect_chan(adap, priv->chan_id, oldclock);
>   
>   	return ret;
>   }
> @@ -363,6 +463,43 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>   			}
>   		}
>   
> +		of_property_read_u32(child, "clock-frequency", &priv->adap.clock_Hz);
> +
> +		/* If the mux adapter has no clock-frequency property, inherit from parent */
> +		if (!priv->adap.clock_Hz)
> +			priv->adap.clock_Hz = parent->clock_Hz;
> +
> +		if (priv->adap.clock_Hz < parent->clock_Hz)
> +			if (muxc->idle_state != I2C_MUX_IDLE_DISCONNECT ||
> +			    muxc->idle_state == chan_id) {

This logic is botched.

If idle_state isn't IDLE_DISCONNECT, then it doesn't matter if if is
or isn't equal to chan_id. If it is IDLE_DISCONNECT, then it certainly
isn't equal to chan_id.

Did you perhaps mean:

	if ((muxc->idle_state < 0 &&
	     muxc->idle_state != I2C_MUX_IDLE_DISCONNECT) ||
	    muxc->idle_state == chan_id) {

or

	if (muxc->idle_state == I2C_MUX_IDLE_UNKNOWN ||
	    muxc->idle_state == I2C_MUX_IDLE_AS_IS ||
	    muxc->idle_state == chan_id) {

> +				dev_err(muxc->dev,
> +					 "channel %u has improper idle state for this configuration\n",
> +					 chan_id);
> +				of_node_put(mux_node);
> +				ret = -EINVAL;
> +				goto err_free_priv;

You potentially leak child here...

> +			}
> +
> +		/*
> +		 * Warn if the mux adapter is not parent-locked as
> +		 * this may cause issues for some hardware topologies.
> +		 */
> +		if ((priv->adap.clock_Hz < parent->clock_Hz) && muxc->mux_locked)
> +			dev_warn(muxc->dev,
> +				 "channel %u is slower than parent on a non parent-locked mux\n",
> +				 chan_id);
> +
> +		/* We don't support mux adapters faster than their parent */
> +		if (priv->adap.clock_Hz > parent->clock_Hz) {
> +			dev_err(muxc->dev,
> +				"channel (%u) is faster (%u) than parent (%u)\n",
> +				chan_id, priv->adap.clock_Hz, parent->clock_Hz);
> +
> +			of_node_put(mux_node);
> +			ret = -EINVAL;
> +			goto err_free_priv;

...and here.

Cheers,
Peter

> +		}
> +
>   		priv->adap.dev.of_node = child;
>   		of_node_put(mux_node);
>   	}
>