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