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:
signature.asc
Description: Digital signature
