Re: [PATCH v5 2/5] i2c: mux: add support for per channel bus frequency

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2429 bytes --]

Hi Andy!

Thank you for your comments, good and observant as usual :-)

On Fri, Feb 13, 2026 at 12:21:42PM +0100, Andy Shevchenko wrote:

[...]

> > +static int i2c_mux_select_chan(struct i2c_adapter *adap, u32 chan_id)
> > +{
> > +	struct i2c_mux_priv *priv = adap->algo_data;
> > +	struct i2c_mux_core *muxc = priv->muxc;
> > +	struct i2c_adapter *parent = muxc->parent;
> > +	struct i2c_mux_core *mux_locked_ancestor = NULL;
> > +	struct i2c_adapter *root;
> > +	int ret;
> > +
> > +	if (priv->adap.clock_hz && priv->adap.clock_hz != parent->clock_hz) {
> > +		mux_locked_ancestor = i2c_mux_topmost_mux_locked(adap);
> > +		root = i2c_root_adapter(&adap->dev);
> > +
> > +		/*
> > +		 * If there's a mux-locked mux in our ancestry, lock the parent
> > +		 * of the topmost one. Mux-locked muxes don't propagate locking
> > +		 * to their parents, so we must explicitly acquire the lock above
> > +		 * the highest mux-locked ancestor to reach the root adapter.
> > +		 */
> > +		if (mux_locked_ancestor)
> > +			i2c_lock_bus(mux_locked_ancestor->parent, I2C_LOCK_ROOT_ADAPTER);
> > +
> > +		ret = i2c_adapter_set_clk_freq(root, priv->adap.clock_hz);
> > +
> > +		if (mux_locked_ancestor)
> > +			i2c_unlock_bus(mux_locked_ancestor->parent, I2C_LOCK_ROOT_ADAPTER);
> 
> > +		if (ret < 0) {
> 
> Would it (ever) have any positive returned values?
> Ditto for other similar cases.

Nope, I will change to if (ret).
> 
> > +			dev_err(&adap->dev,
> > +				"Failed to set clock frequency %dHz on root adapter %s: %d\n",
> > +				priv->adap.clock_hz, root->name, ret);
> > +
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return muxc->select(muxc, priv->chan_id);
> > +}
> 
> ...
> 
> > @@ -223,6 +317,7 @@ struct i2c_adapter *i2c_root_adapter(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(i2c_root_adapter);
> >  
> > +
> >  struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> 
> Stray and unneeded change.

I will remove it.

> 
> >  				   struct device *dev, int max_adapters,
> >  				   int sizeof_priv, u32 flags,
> 
> ...
> 
> > +		of_property_read_u32(child, "clock-frequency", &priv->adap.clock_hz);
> 
> Why OF-centric APIs? Muxes may and do appear on other systems as well.
> Okay, this function seems fully OF-centric :-(

:-/


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

Best regards,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]