Re: [PATCH v12 1/7] i2c: core: add callback to change bus frequency

[Peter Rosin]

Hi!

On 2026-07-19 16:59, Marcus Folkesson wrote:
> All devices on the same I2C bus share the same clock line and the bus
> frequency has therefor be chosen so that all attached devices are able

Perhaps one of:

has therefore got to be
must therefore be

> to tolarate that clock rate. IOW, the bus speed must be set for the

tolerate

> slowest attached device.
> 
> With I2C multiplexers/switches on the other hand, it would be possible
> to have different "domains" that runs with different speeds.

that run

> Prepare for such a feature by provide an optional callback function to

providing

> change bus frequency.
> 
> As a side effect, several bus drivers keep the bus speed in a
> private structure and can now have this value stored in a uniform way
> instead.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
> ---
>   include/linux/i2c.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 20fd41b51d5c..b3b0d16b4ddd 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -729,6 +729,9 @@ struct i2c_adapter_quirks {
>   /*
>    * i2c_adapter is the structure used to identify a physical i2c bus along
>    * with the access algorithms necessary to access it.
> + *
> + * @set_clk_freq: Set clock frequency for the adapter. Returns the actual set frequency.
> + *      This function is optional.
>    */
>   struct i2c_adapter {
>   	struct module *owner;
> @@ -742,6 +745,8 @@ struct i2c_adapter {
>   	struct rt_mutex mux_lock;
>   
>   	int timeout;			/* in jiffies */
> +	u32 clock_Hz;			/* bus clock speed */
> +	int (*set_clk_freq)(struct i2c_adapter *adap, u32 clock_Hz); /* Optional */

To expand on the comment from v9 by Wolfram [1], I too think it
would be good if this function pointer did not live directly in
i2c_adapter but instead in some other struct so that the actual
pointer could live in read-only memory. I.e. if i2c_algorithm
does not fit for some reason, a new function pointer struct might
be called for?

Also, returning int instead of unsigned hints that the function
might return a negative error code, but the new code in 3/7 does
not handle errors. So, either change to unsigned int, or update
the support code in 3/7 to handle errors. I think the latter is
the better approach.

Cheers,
Peter

[1] https://lore.kernel.org/all/ahX44_rzaRlTLSLU@shikoro/

>   	int retries;
>   	struct device dev;		/* the adapter device */
>   	unsigned long locked_flags;	/* owned by the I2C core */
>