Steve Toth wrote:
> Johannes Stezenbach wrote:
> >Let's quote from your patch:
> >
> >+ timeout=0;
> >+ /* write the msb 8 bits, wait for the send to be completed */
> >+ cx24123_writereg(state, 0x22, (data>>16) & 0xff);
> >+ while ( ( cx24123_readreg(state, 0x20) & 0x40 ) == 0 )
> >+ {
> >+ /* Safety - No reason why the write should not complete,
> >and we never get here, avoid hang */
> >+ if (timeout++ >= 4) {
> >+ printk("%s: demodulator is no longer responding,
> >aborting.\n",__FUNCTION__);
> >+ return -EREMOTEIO;
> >+ }
> >+ msleep(500);
> >+ }
> >
> >"No reason why the write should not complete, and we never get here"?
> >500 msec delay?
> >
> >I haven't got the data sheet so I can only guess what the code is doing,
> >but it looks fishy. ;-)
> >What is (cx24123_readreg(state, 0x20) & 0x40) testing for?
> >
> >
> The demod uses a private 3 wire interface to communicate with the tuner.
> This is basically waiting for the tuner to go ready for the next
> available command/byte. If it doesn't, and I saw this once or twice (the
> pll seemed to die), it makes sure we never end up in a hanging situation.
>
> It may of been a bad hardware sample, or bad timing generally but I
> though the added safety didn't hurt.
>
> We can remove the timeout code if you feel the safety is unnecessary.
Seems like I am unable to communicate today :-(
Creating a potentially endless waiting loop is not the solution.
But
a) you shold update the comment from "we enever get here" to "saw this
happening a couple of times"
b) choose a reasonable timeout (where do the 4 seconds come from, why
is it worth waiting 1.5 more seconds if the initial 500msec wait
didn't fix it)
c) implement timeouts with boilerplate jiffies/time_after() code
as is preferred on lkml because it is independent from
system load etc.
How long does the private 3 wire write usually take? Doesn't
the initial check in your code fail everytime because it is
done too soon, and then your code waits needlessly for 500msecs
before checking again?
Johannes
