Re: [PATCH v6 1/2] i2c: core: Add i2c_update_timeout() helper for dynamic transfer timeouts

[Aniket RANDIVE <aniket.randive@oss.qualcomm.com>]

On 7/22/2026 10:55 AM, Mukesh Savaliya wrote:
> 
> 
> On 7/20/2026 5:11 PM, Aniket Randive wrote:
>> The transfer timeout for an I2C controller should reflect the actual
>> message length and bus frequency rather than a static 1-second value.
>> A static timeout causes unnecessary delays on error paths for short
>> messages, and may be insufficient for very long transfers.
>>
> in short write as should be relative to the bus speed and data length 
> instead of any hard coded values.

OK. will update the commit message.
Thanks,
Aniket

>> Add i2c_update_timeout() to i2c-core which computes a transfer-specific
>> timeout and stores it directly in the standard adap->timeout field.  The
>> formula accounts for 9 bits per byte (8 data + 1 ACK) at the configured
>> bus frequency.  The caller supplies a safety multiplier and a minimum
>> floor so that each driver retains full control over its timing policy
>> without those values becoming public API.
>>
>> Storing the result in adap->timeout makes it visible to all consumers of
>> that field, including the arbitration-loss retry loop in 
>> __i2c_transfer().
>>
>> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
>> ---
>>   drivers/i2c/i2c-core-base.c | 24 ++++++++++++++++++++++++
>>   include/linux/i2c.h         |  3 +++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 3ec04787a737..3280652e20d8 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/irq.h>
>>   #include <linux/jump_label.h>
>>   #include <linux/kernel.h>
>> +#include <linux/math64.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>>   #include <linux/of_device.h>
>> @@ -2000,6 +2001,29 @@ void i2c_parse_fw_timings(struct device *dev, 
>> struct i2c_timings *t, bool use_de
>>   }
>>   EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
> can i2c-core-base.c calculate within and store into adap->timeout ? 
> Current adap->timeout = HZ can be removed if we add coefficient and 
> safety factor default ?
> 
> Any other thoughts ?
> 

I think keeping the timeout calculation in the core layer is reasonable, 
but the core may not have enough information to derive a universally 
correct timeout value. Different controllers and platforms can operate 
at different bus frequencies and may require different timeout margins 
depending on hardware characteristics, clocking, latency. Because of 
this, a single default coefficient or safety factor in the core could be 
too aggressive for some platforms and unnecessarily large for others.

My preference would be for the core to perform the timeout calculation 
while allowing bus drivers to provide the required parameters (e.g. 
floor timeout and safety coefficient). This keeps the calculation 
centralized while still giving individual controllers the flexibility to 
tune the timeout based on their hardware requirements.
Using a fixed default coefficient/safety factor in the core and removing 
the existing adap->timeout = HZ fallback may risk regressions on 
platforms that require a larger timeout margin.

Thanks,
Aniket

>> +/**
>> + * i2c_update_timeout - compute and set a dynamic transfer timeout on 
>> an adapter
>> + * @adap: the i2c_adapter whose timeout field will be updated
>> + * @bus_freq_hz: I2C bus clock frequency in Hz
>> + * @len: transfer length in bytes
>> + * @safety_coeff: multiplier applied over the theoretical wire time
>> + * @min_usec: minimum timeout floor in microseconds
>> + *
>> + * Computes a transfer-specific timeout from the message length and bus
>> + * frequency, applies a safety multiplier and a minimum floor, then 
>> stores
>> + * the result in adap->timeout (in jiffies).  The caller supplies the 
>> policy
>> + * constants so they remain internal to the driver.
>> + */
>> +void i2c_update_timeout(struct i2c_adapter *adap, u32 bus_freq_hz,
>> +            size_t len, unsigned int safety_coeff,
>> +            unsigned int min_usec)
>> +{
>> +    u64 bit_usec = mul_u64_u32_div(len * 9, USEC_PER_SEC, bus_freq_hz);
>> +
>> +    adap->timeout = usecs_to_jiffies(bit_usec * safety_coeff + 
>> min_usec);
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_update_timeout);
> why is this exported ? you have alrady added in i2c.h ?

Adding in i2c.h file only provides the prototype required for 
compilation. Since the implementation present in i2c-core-base.c and is 
called from i2c Geni driver, the symbol must also be exported when the 
core and driver are built as separate modules. Otherwise modpost reports 
an undefined symbol for i2c_update_timeout(). Therefore the header 
declaration and EXPORT_SYMBOL_GPL() serve different purposes and both 
are required.

Thanks,
Aniket

>> +
>>   /* 
>> ------------------------------------------------------------------------- */
>>   int i2c_for_each_dev(void *data, int (*fn)(struct device *dev, void 
>> *data))
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 14ab4d3055af..b52974eb7e58 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -912,6 +912,9 @@ void i2c_put_adapter(struct i2c_adapter *adap);
>>   unsigned int i2c_adapter_depth(struct i2c_adapter *adapter);
>>   void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, 
>> bool use_defaults);
>> +void i2c_update_timeout(struct i2c_adapter *adap, u32 bus_freq_hz,
>> +            size_t len, unsigned int safety_coeff,
>> +            unsigned int min_usec);
>>   /* Return the functionality mask */
>>   static inline u32 i2c_get_functionality(struct i2c_adapter *adap)
>>
>