On Tuesday 18 July 2006 03:55, Trent Piepho wrote:
> 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.
OK, I've read the docs over now ... first stab at a revised structure follows:
struct dvb_pll_desc {
char *name;
fe_type_t type;
u32 min;
u32 max;
u32 offset;
u32 stepsize;
void (*override)(fe_type_t fe_type, struct dvb_frontend_parameters
*params, u8 *buf);
u8 entry_count;
struct {
u32 limit;
u8 cfg_size;
u8 cfg[4];
} entries[];
};
Where cfg_size is the number of PLL config bytes to send for that entry. I
think this should allow us to specify pretty much any sort of programming
combination for these devices.
And you're right, I have no idea why the offset/stepsize were duplicated in
the entries... Actually I'm sure I remember asking that myself when the
dvb-pll structure was originally added a year or so ago by Gerd :)
> > 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?
Most likely; my automated test code doesn't deal with bandwidth, and I've not
yet done a detailed manual check over the code to ensure the override()
functions are correct.
> 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.
I would take it further than that, as I've done in the structure above: embed
a frontend type value into the PLL structs and do a check in the
dvb_pll_attach() function.
> 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).
Can't say yet; I need to complete the entire first pass of pll conversion in
order to know what is needed. If possible, yes, I would like those override
functions to go.
> 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.
Indeed; I'm not sure where that came from. The pll definitions I myself have
done all do as you suggest.
> > 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.
Cool. I don't see the array/sizeof issue as a big problem.
> Speaking of the size of entires[], the entry_count is wrong for
> dvb_pll_philips_su1278 and dvb_pll_philips_td1316_dvbc.
Doh, this is what comes of adding the INIT idea in _after_ you've already
tested some of the PLL entries.
> > 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.
I found out its not possible to do it centrally. Some of the PLL definitions
have had this PLLREF/2 added into their frequency offset already. When I did
the centralised thing, it caused them to stop tuning properly :(
As I don't have information on all the boards I don't know which have the
offset and which don't, so I had to just go back to doing it per-PLL.
> 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?
Yeah, would be cleaner. I assume you're ok with the reuse of the entries field
to store the SLEEP/INIT information though?
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
