Clarification on i2c-pca-platform driver timeout

Clarification on i2c-pca-platform driver timeout

[Jean Delvare]

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?

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

In the i2c-pca-platform driver, I then see:

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.

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?

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.

Thanks,
-- 
Jean Delvare

Re: Clarification on i2c-pca-platform driver timeout

[Wolfram Sang]

[-- 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 --]

Re: Clarification on i2c-pca-platform driver timeout

[Jean Delvare]

On Mon, 23 Feb 2009 15:50:27 +0100, Wolfram Sang wrote:
> 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.

OK, makes more sense...

> > 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);
> }

Ah, thanks for pointing me to this piece of code... which I have
forgotten in my previous patch. Poor code, as even if timeout is
expressed in units of 10 ms, msleep(10) can sleep more then 10 ms... up
to 20 ms at HZ=100. This should be converted to time_after().

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

No, it's not that urgent. Most of the change set is for 2.6.30 anyway,
as there is no bug that needs immediate fixing, more of a general
cleanup. I'll back out the i2c-pca-isa part for now.

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

If you can come up with a working patch in the next few days, please
do. If not, I'll take care of it. In the meantime I'm reviewing your
patch adding PCA9665 support.

-- 
Jean Delvare

Re: Clarification on i2c-pca-platform driver timeout

[Wolfram Sang]

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

> If you can come up with a working patch in the next few days, please
> do. If not, I'll take care of it. In the meantime I'm reviewing your
> patch adding PCA9665 support.

You don't need to take care of it. I have the patches done and compile
tested, just need to setup the real hardware for final tests...

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

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