[linux-dvb] Re: Busy waiting in i2c_busy_rise_and_fall (was: Re: Re: full featured card without signal and required video memory investigation)

[Johannes Stezenbach <js@convergence.de>]

Oliver Endriss wrote:
> On Monday 14 July 2003 14:33, Johannes Stezenbach wrote:
> >
> > What not make explicit what Robert said (pseudo-code):
> 
> Well, Robert wrote:
> | Hmm, I had mentioned this before. Try replacing the wait for the rise with
> | an empty read of IICSTA. In my experience, only the first read of IICSTA
> | will sometimes not indicate the correct busy states, but the second one is
> | always correct.
> 
> Robert did not say that the first read returns garbage, but that the busy
> flag has not (yet) been set..

Well, if one starts an I2C transaction and then reads IICSTA, one would
expect that BUSY is set. If not, the read returned stale data ("garbage").

What I get from Roberts description it's not a timing problem (it's not
sufficient to udelay(1), or do a dummy read from some other register),
but one should explicitely do one dummy read from IICSTA, discard the
value, and then do a second read to get valid data.

> > (Note that the comment for i2c_busy_rise_and_fall() says "if we are
> > debugging it checks if the busy flags rises and falls correctly").
> >
> > 	(void) i2c_status_check(saa); /* 1st read returns garbage */
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Not really required because the loop catches this case
> (loop until busy == true or timeout)
> 
> > 	while (!timeout) {
> > 		/* I don't know if this loop is necessary at all, or if
> > 		 * SAA7146_I2C_BUSY is guaranteed to be set after the
> > 		 * first dummy read (unless SAA7146_I2C_ERR)
> > 		 */
> >   		status = i2c_status_check(saa);
> > 		if (status & SAA7146_I2C_BUSY)
> > 			break;
> > 		cond_resched();
> > 		udelay(100); /* don't know if it's any good to busy read I2C_STATUS*/ 
> >       }
> 
> Basically your code is the same as in the old implementation
> (mdelay replaced by udelay). 

No, because of the explcit dummy read from IICSTA this loop should never
catch on. Unless what Robert claims is wrong. And what my comment means
is "if Robert is right this loop is unnecessary".

> IMHO it doesn't make much sense to *wait* for a /busy -> busy transition,
> because
> - busy should be true after the second read (according to Robert)

Exactly what I said.

This also means that the function is now named wrong, and the comment
above the function is also wrong.

> - if there is some interrupt load in the system you can easily miss 
>   this transition. Anyway, it doesn't matter.

right

> Well, I prefer busy-reading over busy-waiting unless there is a
> reason not to do it (?)

I don't know. It occupies the PCI bus some.

> Finally I moved the cond_resched() before i2c_status_check() because
> the busy flag is normally still asserted when we enter the loop.
> (Just give the scheduler a chance to do something useful.)

IMHO that doesn't matter.

> > > -static u32 SAA7146_I2C_BBR = SAA7146_I2C_BUS_BIT_RATE_3200;
> > > +static u32 SAA7146_I2C_BBR = SAA7146_I2C_BUS_BIT_RATE_120; /* 275
> > > kHz */
> >
> > I guess Michael Hunold has had a reason to do that. May his analog TV
> > cards have problems with that bus clock.
> 
> Maybe. Michael, could please tell us about that?

I asked him and it's that low because the original Windows drivers from
Siemens used that. Just bump it up, we'll find out if it causes
problems.


Johannes


-- 
Info:
To unsubscribe send a mail to ecartis@linuxtv.org with "unsubscribe linux-dvb" as subject.