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

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

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

Hi Peter!

On Tue, Feb 17, 2026 at 10:37:39AM +0100, Peter Rosin wrote:
> Hi!
> 
> 2026-02-16 at 19:50, Marcus Folkesson wrote:
> > Hi Peter!
> > 
> > On Mon, Feb 16, 2026 at 05:40:37PM +0100, Peter Rosin wrote:
> >> Hi!
> >>
> >>> +static struct i2c_mux_core *i2c_mux_first_mux_locked(struct i2c_adapter *adap)
> >>> +{
> >>> +	struct i2c_adapter *parent;
> >>> +
> >>> +	while ((parent = i2c_parent_is_i2c_adapter(adap)) != NULL) {
> >>> +		struct i2c_mux_priv *priv = adap->algo_data;
> >>
> >> This assumption does not hold, making the cast pretty wild indeed. There
> >> are other i2c_adapters with a parent besides muxes. See e.g. i2c_atr.c
> > 
> > I see. Hrm, not sure how to decide if it is a mux or not. The best I
> > could come up with is to look at the i2c_adapter.lock_ops. E.g.
> > 
> > 
> > 	while ((parent = i2c_parent_is_i2c_adapter(adap)) != NULL) {
> > 		/*
> > 		 * Check if this adapter is a mux channel by verifying its
> > 		 * lock_ops. Only mux channels use these specific lock operations.
> > 		 */
> > 		if (adap->lock_ops == &i2c_mux_lock_ops ||
> > 		    adap->lock_ops == &i2c_parent_lock_ops) {
> > 			struct i2c_mux_priv *priv = adap->algo_data;
> > 
> > 			if (priv->muxc->mux_locked)
> > 				return priv->muxc;
> > 		}
> > 		adap = parent;
> > 	}
> > 
> > Or do you have a better idea?
> 
> That looks fragile. My recommendation would be to avoid trying to
> guess how a potentially diverse adapter tree should be handled
> locally in the mux code. To me, it would feel better to introduce
> locking/recursion in i2c_adapter_set_clk_freq() for muxes (and
> address translators), i.e. take inspiration from i2c_transfer()
> and i2c_smbus_xfer().

That would be a more robust solution indeed.

> 
> I guess an unlocked __i2c_adapter_set_clk_freq() is needed.
> 

Rethinking the whole locking approach;
If I follow the same locking logic as in i2c_transfer/__i2c_transfer, do I
really need do take any more locks than the root adapter with
I2C_LOCK_ROOT_ADAPTER?
As the frequency can only be lowered, an intermediate i2c message will
not mess anything up?

If so, do you think  i2c_root_adapter() should be moved to i2c-core-base.c?
Then i2c_adapter_set(_root?)_clk_freq() could lookup the root adapter
and take the lock there.


> Cheers,
> Peter

Best regards,
Marcus Folkesson

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