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

[Oliver Endriss <o.endriss@gmx.de>]

On Monday 14 July 2003 14:33, Johannes Stezenbach wrote:
> Oliver Endriss wrote:
> > you might consider this patch:
>
> [snip]
>
> 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..

> (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). 
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)
- if there is some interrupt load in the system you can easily miss 
  this transition. Anyway, it doesn't matter.

So I decided to read the status register max. 5 times without any 
delay. If the busy bit is not set, we may assumed that the I2C transfer
has been completed and we missed the transitions of the busy flag.
No need for any delays (IMHO).

> 	while (!timeout) {
>   		status = i2c_status_check(saa);
> 		if ((status & SAA7146_I2C_ERR) || !(status & SAA7146_I2C_BUSY))
> 			break;
> 		cond_resched();
> 		udelay(100);
> 	}

Same as the old code, too.
Well, I prefer busy-reading over busy-waiting unless there is a
reason not to do it (?)
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.)

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

Oliver


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