On Sat, 15 Jul 2006, Andrew de Quincey wrote:
> A suggested change to the pll structure is here, comments follow:
>
> struct dvb_pll_desc {
[deleted]
> u8 config;
> u8 cb;
> u8 byte5;
> } entries[12];
> };
In the Infineon PLL datasheets, the non-divisor bytes of the PLL control
message are named:
"Control byte" or "CB"
"Bandswitch byte" or "BB"
"Auxiliary byte" or "AB"
In the struct, config corresponds to CB, and cb corresponds to BB. ie. cb
isn't cb, it's bb. It's very confusing. I suggest renaming these fields
to cb and bb, or to control and band, or something like that.
I checked the latest dvb-pll.c file from your refactor branch. For every
single PLL, the offset and stepsize parameters are the same for all bands.
How about moving them outside of the entries structure, to save space and
clutter.
> The setbw() method is changed to override(), and takes the complete set of
> params. setbw was originally added to support DVBT tuners which had different
> bandwidth dependant settings. However it also now needs to support DVBS
> tuners with symbolrate dependant settings. From a cleanness point of view,
> I'd rather just get the complete set of tuning params and let the tuner
> specific code pull out what it needs.
Take a look at this hunk from that part of the patch:
-static void tda665x_bw(u8 *buf, u32 freq, int bandwidth)
-{
- if (bandwidth == BANDWIDTH_8_MHZ)
+static void tda665x_override(fe_type_t fe_type, struct dvb_frontend_parameters *params, u8 *buf)
+{
+ if (fe_type != FE_OFDM)
+ return;
+ if (params->u.ofdm.bandwidth == BANDWIDTH_7_MHZ)
buf[3] |= 0x08;
}
First, it was changed from turning on at 8 MHz to turning on at 7 MHz. Is
that a bug in the original code, or a mistake in the refactor patch?
Instead of if(fe_type != FE_OFDM) return, would BUG_ON(fe_type != FE_OFDM)
be better? The original function was never meant to be called for
non-DVB-T tuners.
These override functions are pretty much the same. How about just one
override function that all the PLLs can point to? Or instead of the
override function, have a one bit flag that turns on the override code
(which is just one statement anyway).
For td1316_override(), can't the frequency stuff just go inside the pll
struct entries? That is how all the other PLLs control band switching.
> Do we actually need to specify a max bound of 12 on the number of entries as
> above? I mean it _appears_ to compile if you just leave it as "entries[]".
> However I appreciate there may be some compiler issues which prevent doing
> this that I am unaware of... let me know.
This is ok and will save lots of wasted space. The only drawbacks are you
can not use sizeof and get the correct result (it will not include entries
at all). You also can not have an array of struct dvb_pll_desc.
Speaking of the size of entires[], the entry_count is wrong for
dvb_pll_philips_su1278 and dvb_pll_philips_td1316_dvbc.
> Also note that I have (temporarily) removed frequency correction from the
> revised PLL definitions (i.e. I removed the (PLLREF/2) from DIV = (FREQ +
> (PLLREF/2))/PLLREF). This will be done centrally in dvb-pll.c in the future.
In your latest patch, you still have the PLLREF/2 in the offset for
dvb_pll_alps_tdbe2.
The special PLL_INIT and PLL_SLEEP frequencies scream kludge. Is that
really the best way to do it? How about just having the init/sleep
function send the magic byte sequence it needs, without using the calc_regs
function?
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
