On Fri, 2009-06-05 at 21:02 -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 05 Jun 2009 17:52:12 -0400
> Andy Walls <awalls@xxxxxxxxx> escreveu:
>
> > Hi,
> >
> > This patch:
> >
> > 1. adds explicit support for the FQ1216LME MK3
> >
> > 2. points the tveeprom module to the FQ1216LME MK3 entry for EEPROMs
> > claiming FQ1216LME MK3 and MK5.
> >
> > 3. refactors some code in simple_post_tune() because
> >
> > a. I needed to set the Auxillary Byte, as did TUNER_LG_TDVS_H06XF, so I
> > could set the TUA6030 TOP to external AGC per the datasheet.
> >
> > b. I wanted to do fast tuning while managing PLL phase noise, like the
> > TUNER_MICROTUNE_4042FI5 was already doing.
> >
> >
> > Hermann & Dmitri,
> >
> > I think what is done for setting the charge pump current for the
> > TUNER_MICROTUNE_4042FI5 & FQ1216LME_MK3 in this patch is better than
> > fixing the control byte to a constant value of 0xce.
>
> The idea seems good, and it is probably interesting to do it also with other
> tuners.
>
> On a quick view to see the code, however, one part of the code popped up on my eyes:
>
> > +static int simple_wait_pll_lock(struct dvb_frontend *fe, unsigned int timeout,
> > + unsigned long interval)
> > +{
> > + unsigned long expire;
> > + int locked = 0;
> > +
> > + for (expire = jiffies + msecs_to_jiffies(timeout);
> > + !time_after(jiffies, expire);
> > + udelay(interval)) {
> > +
> > + if (tuner_islocked(tuner_read_status(fe))) {
> > + locked = 1;
> > + break;
> > + }
> > + }
> > + return locked;
> > +}
>
>
> It is better to use a do {} while construct in situations like above, to make
> the loop easier to read.
Ok. That's easy to change.
> Yet, I don't like the idea of using udelay to wait for up a long interval like
> on the above code - you can delay it up to max(unsigned int) with the above code.
>
> Holding CPU for a long period of time is really a very bad idea. Instead, you
> should use, for example, a waitqueue for it, like:
First: I agree busy waiting is simplistic and stupid - things could be
done better.
However I essentially only reimplemented the loop that
TUNER_MICROTUNE_4042FI5 used: it busy waited for 1 ms in intervals of 10
usecs.
Aside from assuming some sort of exploit, I'm not sure how the loop
could wind up busy waiting for max(unsigned int) msecs as the two calls
to it were:
simple_wait_pll_lock(fe, 3, 128);
simple_wait_pll_lock(fe, 1, 10);
depending on the tuner type. I also though time_after() was supposed to
handle jiffies wrapping around.
> static int simple_wait_pll_lock(struct dvb_frontend *fe, unsigned int timeout)
> {
> DEFINE_WAIT(wait);
> wait_event_timeout(wait, tuner_islocked(tuner_read_status(fe)), timeout);
> return (tuner_islocked(tuner_read_status(fe)));
> }
>
> This is cleaner, and, as wait_event_timeout() will call schedule(), the CPU will
> be released to deal with other tasks or to go to low power consumption level,
> saving some power (especially important on notebooks) and causing penalty on
> machine's performance
Well, there's not a real event for which to wait. It's really just
going to be a poll of the FL bit at an interval smaller than the minimum
wait. Implementing something that schedule()s between the polls
shouldn't be that hard.
Although, I'll defer doing something more complex right now though,
until I test it with a tuner I have. My real goal was to get the
FQ1216LME properly supported.
Over the air analog TV is disappearing for me in 7 days. I may not care
about analog tuning perfomance in a week. ;)
Regards,
Andy
> Cheers,
> Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
