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: 2702 bytes --]

Hi Peter,

On Wed, Feb 18, 2026 at 08:02:54AM +0100, Marcus Folkesson wrote:
> 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.

I've reworked this during the weekend and think I've got some my
answers. I will send out a new version later today.

Thanks!

Best regards,
Marcus Folkesson

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