Trent Piepho wrote: > On Tue, 28 Aug 2007, Manu Abraham wrote: >> Trent Piepho wrote: >>> That's really inefficient. You've got about 250k of table there. I don't >>> think a 250k+ module is going to be very popular. >>> >> I do agree that's not the most effecient way. But given the short time >> span and lack of FP operations, the quick way that came to my mind was >> precomputed values. >> >> That said, the popularity of a module is not defined by it's size. as an >> eg: the nVidia module is over 1 Meg. Considering the quality/performance >> of the tuners in the tree we have, i don't think there would even be a >> popularity contest, coming anywhere near. > > But nobody likes how huge the nvidia module is. Well, does anyone care about the size of that module in comparison to the features that which provides ? I for one, don't mind that size for the features it provides. When you don't have other alternatives, does it really matter ? That said, the module size is not that big. in fact it is smaller compared to the others in the same league: 16 -rw-r--r-- 1 root root 13756 2007-08-29 14:43 bcm3510.ko 100 -rw-r--r-- 1 root root 95704 2007-08-29 14:43 dvb-core.ko 20 -rw-r--r-- 1 root root 17472 2007-08-29 14:43 dvb-pll.ko 56 -rw-r--r-- 1 root root 50144 2007-08-29 14:43 dvb-usb-af9015.ko 32 -rw-r--r-- 1 root root 31040 2007-08-29 14:43 dvb-usb.ko 8 -rw-r--r-- 1 root root 6420 2007-08-29 14:43 mc44s80x.ko 12 -rw-r--r-- 1 root root 8392 2007-08-29 14:43 mt2060.ko 16 -rw-r--r-- 1 root root 15940 2007-08-29 14:43 mxl500x.ko 16 -rw-r--r-- 1 root root 12364 2007-08-29 14:43 or51132.ko 16 -rw-r--r-- 1 root root 12600 2007-08-29 14:43 or51211.ko 12 -rw-r--r-- 1 root root 9380 2007-08-29 14:43 qt1010.ko Also, it might be interesting to have a comparison between the dissipation on the various devices that we have support now (though not driver related), as dissipation is the culprit for thermal noise, and thermal instabilities on silicon tuners. XC3028: 300mA @5v = 1.5W MT2060: 370mA @3.3v = 1.221W QT1010: 300mA @3.6v = 1.08W MC44S802(3): 210mA @1.8v = 0.378W MXL5003(5): 165mA @1.9v = 0.3135W TDA18291: 54mA @2.8v = 0.1521W Also of all the devices the Freescale tuner has the largest package and a thermal spreader, thermal runaway should be the minimal of these devices. >> That said, if it can be optimized, there is no reason why it shouldn't >> be done. >> >> I had a comment from Andrew Morton as well on it: "It's large, but heck, >> it's not a lot of memory. The nvidia driver is a meg or something." > > Huge tables like that can be a problem just for the code itself. Huge > patches, show checkouts and diffs, slow compiles, etc. Compilation time 0.6 s and 0.7 s, I don't think anyone would care for that difference. The table would change for the Nominal frequency and the BPF setting, so shouldn't matter much since the table won't change unless the hardware changes. When the hardware changes (this is not a firmware based device) well, anyway we need a new driver, for the new chip. > There are also problems with maintaining such code. Say you need to tweak > the tuning algorithm? Instead of just changing one constant or making an > expression round up instead of down, you have to create a whole new table. No need to tweak the algorithm, it is fairly fixed for the hardware and RF settings. Yeah true though you have recreate the table. > And what if there is a new device revision, which is slightly different? > You have to have another huge table, when you could probably support both > with just a small addition to the code. Or say someone has a fake Russian > copy, that uses a 26 Mhz crystal instead of 27 MHz? Much harder to support > if you look at the driver and just see some huge table with no idea what > made it, instead of one simple constant to change. Fake Russian copy. ;-) > For instance, when radio support for my tuner didn't work, I was able to > figure out from the tuner-simple code that it was using the wrong 1st IF > offset for the tda9887 radio demodulator and 2nd IF output. If there was > just some huge table of random settings, it would have been much harder to > fix. > >> If anyone's interested, the original version with FP is here: >> >> http://paste.debian.net/35554 >> >> It 's a userspace application with which i did some crude tests. >> >> The tables are here >> >> http://jusst.de/manu/1086M_36.125M.c >> http://jusst.de/manu/1086M_36.166M.c > > I looked at this code, but there is code there that isn't used, so I'm not > sure exactly what it's supposed to. > > There is a 'optimal_settings_index' parameter to mc44s80x_set_1st_lo(), but > it's never used. > > There is still a huge table, 'tuner_step', where did that come from? The tuner_step is derived from the the 2 tables that i have put up above and the user application. > The > whole table isn't used, only index 1. And lo2_divider isn't used either. > Is there some kind of formula that can calculate this table? Based on the frequency, LO2 is extracted, it does some calculations, but simpler. > The 'lo1_adjustment' code is very odd. Are you sure it's really doing what > it's supposed to do? Think, it should be. Yes. > The actual code in mc44s80x_set_1st_lo() is just a round-about way of doing > a rather simple bit of math. > > So, here is a integer version of your floating point code. It's actually > _more_ accurate than the FP version. For your sample (226500000 Hz, > lo1_divider 13, lo1_adjustment 0), my code finds the same divider as yours, > 630. But the 'first_lo' value I find is 1,312,615,385 Hz, while your FP > code returns 1,312,615,336 Hz. Mine is within 1 Hz of the true value of > 1312615384.615384. Thanks, i will take a look at this. Will try applying this in to the driver and see how it works out. Some few days needed though > > #ifdef __KERNEL__ > #include <asm/div64.h> > #else > #define do_div(n, base) ({ unsigned long __rem = (n)%(base); (n)/=(base); __rem; }) > typedef unsigned long long u64; > #endif > > /* freq = input frequency in Hz > refdiv1 = first reference frequency divisor (10 to 15, or 26) > adj1 = mystery adjustment (signed number!) */ > > /* To turn the lo1_adjustment value into adj1, do this: > adj1 = (lo1_adjustment+1) / ((lo1_adjustment&1)?-2:2); */ > #define REF_FREQ 27000000 /* 27 MHz reference */ > #define IF1 1086000000 > void mc44s80x_set_1st_lo(unsigned int freq, unsigned int refdiv1, int adj1) > { > > u64 div1; > unsigned int remainder, freq1, if1, lo1ref; > signed int if1_offset; > > div1 = ((u64)freq + IF1) * refdiv1 + REF_FREQ/2; /* Subtract 1 to round XXX.5 down instead of up */ > printf("freq+IF * refdiv1 = %llu\n", div1); > remainder = do_div(div1, REF_FREQ); > > /* adjustment sign is flipped if we didn't round div1 down. > Very stange, I don't understand what this is for. */ > if (remainder <= REF_FREQ/2) > adj1 *= -1; > > div1 += adj1; > > /* Actual frequency PLL tuned, taking into account step size, the IF > * offset, and the mystery adjustment. */ > freq1 = (div1 * REF_FREQ + refdiv1/2) / refdiv1; > > /* Actual IF used, taking into acount everything freq1 does */ > if1 = freq1 - freq; > > /* Offset of actual IF from nominal IF */ > if1_offset = if1 - IF1; > > /* LO1REF, whatever that is */ > /* BTW, do you really want to round down, not to the nearest? */ > lo1ref = 15 * (8/2) * div1 / refdiv1; > > /* The "n+2" divider */ > div1 = (div1<2) ? 0 : div1-2; > > printf("frequency: %u lo1: %u ", freq, freq1); > printf("ref div1: %u ", refdiv1); > printf("if1: %u off: %d\n", if1, if1_offset); > printf("n=%d\n", (int)div1); > printf("LO1REF: %d\n", lo1ref); > } > _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
