Re: [PATCH v9 1/5] i2c: core: add callback to change bus frequency

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

Hi Wolfram!

On Tue, May 26, 2026 at 09:47:47PM +0200, Wolfram Sang wrote:
> Hi Marcus,
> 
> finally I found some time... thank you for your patience!

No worries, I'm aware of your workload and appreciate you taking the
time to review the patches.

> 
> Some comments here, but I have high level questions first to be
> discussed in patch 5. Maybe we should start there...
> 
> > +	int (*set_clk_freq)(struct i2c_adapter *adap, u32 clock_hz); /* Optional */
> 
> Shouldn't this rather go into 'struct i2c_algorithm'?

Hrm, not sure.
When looking into `struct i2c_algorithm`, it has no information about
any other state. 

Also, the `struct i2c_algorithm` is often shared between multiple
adapters, so if it had the information about the current clock
frequency, it will be shared as well, which is not what we want.

To me it feels more natural to have this callback in the adapter, but
I'm open to change it if there are good reasons to do so.

> 
> > +static inline int
> > +__i2c_adapter_set_clk_freq(struct i2c_adapter *adapter, u32 clock_hz)
> > +{
> > +	if (adapter->set_clk_freq)
> > +		return adapter->set_clk_freq(adapter, clock_hz);
> 
> So, Sashiko mentions[1] that setting 'adapter->clock_hz' below is not
> executed. This is fine in my book because the requested new rate might
> not be the actually used rate. However, I agree that it must be
> documented that the callback is required to set 'clock_hz'. Or maybe
> even better, the callback should return the newly set value and this
> function then updates the variable?

I will look into this, thanks!

> 
> [1] https://sashiko.dev/#/patchset/20260324-i2c-mux-v9-0-5292b0608243%40gmail.com
> 
> > +
> > +	/*
> > +	 * 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;
> 
> So, Sashiko is IMO correct with asking what happens if setting the new
> freq in the root adapter fails? There is no path to restore the old
> values for the children then, or did I miss it?

I will look into this as well.
Spontaneously, I would say that if the root adapter fails to set the new
frequency, the deselect chain will restore the old frequency for all
children no matters what.

> 
> Happy hacking,
> 
>    Wolfram
> 

Best regards
Marcus Folkesson