[linux-dvb] pcHDTV HD2000 card (ATSC) driver

Johannes Stezenbach js at linuxtv.org
Thu Feb 17 21:53:40 CET 2005


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




More information about the linux-dvb mailing list