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

[Marcus Folkesson]

Hi Peter!

On Thu, Jul 23, 2026 at 01:03:53PM +0200, Peter Rosin wrote:
> > + *
> > + * @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?

My objection to move it to i2c_algorithm, was mostly based on that the
algorithm should not be aware of the current clock frequency. When I
reread the comment from Wolfram (and you), I realize that we are only
talking about the function pointer, which is fine to put into the
i2c_algorithm struct.

Sorry for the misinterpretation, I will move the pointer to
i2c_algorithm and keep clock_Hz in i2c_adapter.


> 
> 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.

Agree, thanks.
> 
> 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 */
> > 
> 


Best regards,
Marcus Folkesson

Attachment: signature.asc
Description: PGP signature