| Message ID | 00a182c50af211e409f128630ba6ba5359da6198.1457362545.git.jglauber@cavium.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Mar 07, 2016 at 04:10:48PM +0100, Jan Glauber wrote: > From: Peter Swain <pswain@cavium.com> > > Make the i2c adapter timeout a module parameter to allow upper-level > target device drivers to retry with their own logic before their own > timeouts abort operations. > > For example, at24 eeprom driver retries for 25ms when -EAGAIN > indicates that an eeprom has gone unresponsive while committing > a newly written page (5ms on typical devices). > > Signed-off-by: Peter Swain <pswain@cavium.com> > Signed-off-by: Jan Glauber <jglauber@cavium.com> i2c-dev has IOCTLs for timeout and retries. Can't you use those?
On Sat, Mar 12, 2016 at 04:46:12PM +0100, Wolfram Sang wrote: > On Mon, Mar 07, 2016 at 04:10:48PM +0100, Jan Glauber wrote: > > From: Peter Swain <pswain@cavium.com> > > > > Make the i2c adapter timeout a module parameter to allow upper-level > > target device drivers to retry with their own logic before their own > > timeouts abort operations. > > > > For example, at24 eeprom driver retries for 25ms when -EAGAIN > > indicates that an eeprom has gone unresponsive while committing > > a newly written page (5ms on typical devices). > > > > Signed-off-by: Peter Swain <pswain@cavium.com> > > Signed-off-by: Jan Glauber <jglauber@cavium.com> > > i2c-dev has IOCTLs for timeout and retries. Can't you use those? > Yes, with these IOCTLs we don't need to add a module parameter so I'll drop that. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Agreed, this timeout modparam can go, it serves no purpose which can't be achieved with existing ioctl. It was introduced during debug to reduce spurious error count while hunting an issue finally resolved by the 80uS IFLG re-poll. But the same effect is achieved by (for example) the at24 driver's retry at 25mS timeout. The issue there is not timeout _within_ command, but failure to respect a 25mS post-write recovery time before next access to the at24, and the existing at24-level retry works well (though delaying the presentation of the next at24 transaction _within_ the at24 driver would have lead to less bus-level churn). Leaving this adapter tuning out affects nothing but the (normally invisible) adapter-level retry count, which is better addressed within the target device's code, like at24 does. Default is ioctl-tweakable, so users wanting fine tuning have a mechanism. -pete
diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c index 6041cd7..ef1720f 100644 --- a/drivers/i2c/busses/i2c-octeon.c +++ b/drivers/i2c/busses/i2c-octeon.c @@ -72,6 +72,10 @@ struct octeon_i2c { struct device *dev; }; +static int timeout = 2; +module_param(timeout, int, 0444); +MODULE_PARM_DESC(timeout, "Low-level device timeout (ms)"); + /** * octeon_i2c_write_sw - write an I2C core register * @i2c: The struct octeon_i2c @@ -425,7 +429,6 @@ static struct i2c_adapter octeon_i2c_ops = { .owner = THIS_MODULE, .name = "OCTEON adapter", .algo = &octeon_i2c_algo, - .timeout = HZ / 50, }; /* calculate and set clock divisors */ @@ -561,6 +564,8 @@ static int octeon_i2c_probe(struct platform_device *pdev) octeon_i2c_setclock(i2c); i2c->adap = octeon_i2c_ops; + i2c->adap.timeout = msecs_to_jiffies(timeout); + i2c->adap.retries = 10; i2c->adap.dev.parent = &pdev->dev; i2c->adap.dev.of_node = node; i2c_set_adapdata(&i2c->adap, i2c);