Rusty Scott wrote:
> I have finished testing DVB support of the pcHDTV HD2000 card and it is
> working well. Attached is a patch to CVS and some additional files that need
> to be added to dvb/frontends and the firmware directory. If someone with CVS
> access could look them over and add them (or return with comments) it would be
> appreciated.
Thanks a lot!
I've added your patch to CVS as-is, but have some comments if case
you care to send an incremental clean-up patch:
> /*
> * This driver needs external firmware. Please use the command
> * "<kerneldir>/Documentation/dvb/get_dvb_firmware or51211" to
> * download/extract it, and then copy it to /usr/lib/hotplug/firmware.
> */
Do you have a patch for get_dvb_firmware or at least info where
to download and how to extract the firmware?
> struct or51211_state* state = (struct or51211_state*) fe->demodulator_priv;
cast from void* is unnecessary (multiple issues)
> if(i2c_writebytes(state,0x50,tudata,1)) {
a space between "if" and "(" would be appreciated (repeats all over)
> for (i = 0; i < 145; i++) {
> tudata[i] = fw->data[i];
> }
broken indentation (1 tab, please); the braces are superflous, too
> msleep(1); /* 1ms */
it's unnecessary to comment what msleep() does
> msleep(10); /* 20ms */
hmm
> /* log10-1 table at .5 increments from 1 to 100.5 */
> unsigned int i100x20log10[] = {
> 0, 352, 602, 795, 954, 1088, 1204, 1306, 1397, 1480,
> 1556, 1625, 1690, 1750, 1806, 1858, 1908, 1955, 2000, 2042,
> 2082, 2121, 2158, 2193, 2227, 2260, 2292, 2322, 2352, 2380,
> 2408, 2434, 2460, 2486, 2510, 2534, 2557, 2580, 2602, 2623,
> 2644, 2664, 2684, 2704, 2723, 2742, 2760, 2778, 2795, 2813,
> 2829, 2846, 2862, 2878, 2894, 2909, 2924, 2939, 2954, 2968,
> 2982, 2996, 3010, 3023, 3037, 3050, 3062, 3075, 3088, 3100,
> 3112, 3124, 3136, 3148, 3159, 3170, 3182, 3193, 3204, 3214,
> 3225, 3236, 3246, 3256, 3266, 3276, 3286, 3296, 3306, 3316,
> 3325, 3334, 3344, 3353, 3362, 3371, 3380, 3389, 3397, 3406,
> 3415, 3423, 3432, 3440, 3448, 3456, 3464, 3472, 3480, 3488,
> 3496, 3504, 3511, 3519, 3526, 3534, 3541, 3549, 3556, 3563,
> 3570, 3577, 3584, 3591, 3598, 3605, 3612, 3619, 3625, 3632,
> 3639, 3645, 3652, 3658, 3665, 3671, 3677, 3683, 3690, 3696,
> 3702, 3708, 3714, 3720, 3726, 3732, 3738, 3744, 3750, 3755,
> 3761, 3767, 3772, 3778, 3784, 3789, 3795, 3800, 3806, 3811,
> 3816, 3822, 3827, 3832, 3838, 3843, 3848, 3853, 3858, 3863,
> 3868, 3874, 3879, 3884, 3888, 3893, 3898, 3903, 3908, 3913,
> 3918, 3922, 3927, 3932, 3936, 3941, 3946, 3950, 3955, 3960,
> 3964, 3969, 3973, 3978, 3982, 3986, 3991, 3995, 4000, 4004,
> };
>
> unsigned int denom[] = {1,1,100,1000,10000,100000,1000000,10000000,100000000};
>
> unsigned int i20Log10(unsigned short val)
> {
> unsigned int rntval = 100;
> unsigned int tmp = val;
> unsigned int exp = 1;
>
> while(tmp > 100) {tmp /= 100; exp++;}
>
> val = (2 * val)/denom[exp];
> if(exp > 1) rntval = 2000*exp;
>
> rntval += i100x20log10[val];
> return rntval;
> }
...
> /* The value reported back from the frontend will be FFFF=100% 0000=0% */
> *strength = (((5334 - i20Log10(snr_equ))/3+5)*65535)/1000;
This is kind of neat, but since none of the other frontends does it
I ask myself if we should either drop it, or factor it out into
dvb_frontend.c so the other frontends could use it, too.
> msleep(3); /* 30ms */
hmm
> static int or51211_read_ber(struct dvb_frontend* fe, u32* ber)
> {
> *ber = -ENOSYS;
*ber = 0;
> return 0;
I wonder if we should return -ENOSYS if unsupported, but none of the
other drivers does it so I guess not...
> static int or51211_read_ucblocks(struct dvb_frontend* fe, u32* ucblocks)
> {
> *ucblocks = -ENOSYS;
*ucblocks = 0;
> /* Request the firmware, this will block until someone uploads it */
> printk(KERN_WARNING "or51211: Waiting for firmware upload (%s)...\n", OR51211_DEFAULT_FIRMWARE);
> ret = state->config->request_firmware(fe, &fw, OR51211_DEFAULT_FIRMWARE);
> printk(KERN_WARNING "or51211: Waiting for firmware upload(2)...\n");
should be KERN_INFO, second printk is redundant
> msleep(30); /* 20ms */
hmm
> if (state == NULL) goto error;
please add a line break after if
Johannes
