[linux-dvb] AverTVHD/NXT2004 patches

[Patrick Boettcher]

Hi Jean-Francois,

On Fri, 8 Jul 2005, Jean-Francois Thibert (SageTV) wrote:

> Hello Patrick,
>
> Here is the patch for the dvb-kernel cvs. This will create also the 2 new
> files, let me know if there is something else.
>
> Thanks
>
> Jean-Francois

I'll try to point out some things which IMHO are better suited to kernel 
coding-style. Johannes and others, please correct me, if I'm wrong.


+static int i2c_writebytes (struct nxt2004_state* state, u8 reg, u8 *buf, u8 len)
+{
+	/* probbably a much better way or doing this */
+	u8 buf2 [256],x;

What about buf2[len+1]; ?

+	int err;
+	struct i2c_msg msg = { .addr = state->config->demod_address, .flags = 0, .buf = buf2, .len = len + 1 };
+
+	buf2[0] = reg;
+	for (x = 0 ; x < len ; x++)
+		buf2[x+1] = buf[x];

memcpy(&buf[1],buf); does the same and looks IMHO cleaner here.

+	if ((err = i2c_transfer (state->i2c, &msg, 1)) != 1) {
+		dprintk ("%s: i2c write error (addr %02x, err == %i)\n",
+			__FUNCTION__, state->config->demod_address, err);
+		return -EREMOTEIO;
+	}
+
+	return 0;
+	return 0;

One return is sufficient ;)

+static int nxt2004_writetuner (struct nxt2004_state* state, u8* data)
[..]
+	/* set tuner i2c address */
+	buf = 0xC2;
+	i2c_writebytes(state,0x35,&buf,1);

Here would be the appropriate place for calling a pll_set callback, when 
pll-programming would have been moved to the device-driver.

+static int nxt2004_load_firmware (struct dvb_frontend* fe, const struct firmware *fw)
[..]
+	position = 0;
+

Here e.g. is an empty line filled with spaces. There are several places in 
the file where you have trailing spaces, please remove. (when you're using 
vim: let c_space_errors=1). It would also be good if all indentions are 
made with tabs instead of spaces. I did not found any of these in your 
files, though.

+/* Modified for our alps tuner... don't know what other tuner was */
+static int nxt2004_setup_frontend_parameters (struct dvb_frontend* fe,
+					     struct dvb_frontend_parameters *p)
+{
+	struct nxt2004_state* state = fe->demodulator_priv;
+	u32 freq = 0;
+	u16 tunerfreq = 0;
+	u8 buf[4];
+
+	freq = 44000 + ( p->frequency / 1000 );
+
+	dprintk("freq = %d      p->frequency = %d\n",freq,p->frequency);
+
+	tunerfreq = freq * 2/125;
+
+	buf[0] = (tunerfreq >> 8) & 0x7F;
+	buf[1] = (tunerfreq & 0xFF);
+
+	if (p->frequency <= 162000000) {
+		buf[2] = 0x85;
+		buf[3] = 0x01;
+	} else if (p->frequency <= 426000000) {
+		buf[2] = 0x85;
+		buf[3] = 0x02;
+	} else if (p->frequency <= 782000000) {
+		buf[2] = 0x85;
+		buf[3] = 0x08;
+	} else {
+		buf[2] = 0x85;
+		buf[3] = 0x88;
+	}
+
+	/* write frequency information */
+	nxt2004_writetuner(state,buf);

Please try to implement pll-programming like all the other 
frontend-drivers are doing it (by adding and calling a pll_set and 
pll_init-callback inthe nxt2004_config or by using struct dvb_pll_desc). I 
know the nxt2002 doesn't implement a pll-programming-callback, but this is 
extremely helpful, when the frontend appears on other boards with another 
pll.

Please also add a "Signed-off-by: Name <Emailaddress>" - line below your 
next Emails which include patches for (dvb-)kernel. See <file:dvb-kernel/SubmittingPatches>

Please resend your patches with the modifications. Thanks for your 
efforts.

Feel free to ask question or point out corrections if I got something 
wrong.

best regards,
Patrick.