Re: Clarification on i2c-pca-platform driver timeout

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

Hello Jean,

On Mon, Feb 23, 2009 at 03:23:08PM +0100, Jean Delvare wrote:
> Hi Wolfram,
> 
> I would like you to clarify the situation when it comes to the value of
> the timeout field of struct i2c_pca9564_pf_platform_data:
> 
> struct i2c_pca9564_pf_platform_data {
> 	(...)
> 	int timeout;		/* timeout = this value * 10us */
> };
> 
> The only user, board-sh7785lcr.c, sets this value to 100, resulting in
> a 1 ms timeout. This seems really short. Is this really intended?

This is a typo. It should say 10ms as a unit, resulting in 1s total.

> Why is the timeout value defined in such a strange unit?

I didn't change this in i2c-algo-pca.c back then, look at the beginning
of pca_xfer:

while ((state = pca_status(adap)) != 0xf8 && timeout--) {
        msleep(10);
}

So, timeout acts as a loop counter in waiting for a free bus, which is
why your change in "Adapter timeout is in jiffies" alone won't do for
pca-based drivers. There seems to be a bigger rework needed for handling
the timeout :( I wanted to look into it this evening, but it seems to
be a bit urgent?

> static int __devinit i2c_pca_pf_probe(struct platform_device *pdev)
> {
> 	struct i2c_pca_pf_data *i2c;
> 	(...)
> 	struct i2c_pca9564_pf_platform_data *platform_data =
> 				pdev->dev.platform_data;
> 	(...)
> 	i2c->adap.timeout = platform_data->timeout;
> 
> The problem is that i2c->adap.timeout is supposed to be expressed in
> jiffies, not units of 10 us. So there is a conversion missing.

Yup, see above.

> Lastly, you define a timeout value but never use it. Shouldn't you use
> wait_event_interruptible_timeout() instead of
> wait_event_interruptible() in i2c_pca_pf_waitforcompletion?

This is probably one part of the complete solution.

> An upcoming patch will add code which handles a timeout at i2c-core
> level, so it matters to get all i2c bus drivers right first.

Sounds reasonable...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]