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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

Hi Marcus,

finally I found some time... thank you for your patience!

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'?

> +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?

[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?

Happy hacking,

   Wolfram