Re: PULL request http://linuxtv.org/hg/~pb/v4l-dvb/

[Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>]

On Thu, 11 Dec 2008 01:00:36 +0100 (CET)
Patrick Boettcher <patrick.boettcher@xxxxxxx> wrote:

> Hi Mauro,
> 
> please pull from my repository for the following two changes:

Committed, thanks.

> - Add support for the CX24113 DVB-S tuner driver

Great to add support for cx24113!

A few notes:

+    dprintk("1 N: %d, F: %lld, R: %d\n", N, F, R);
+       do_div(F, state->config->xtal_khz*1000 * factor * 2);
+    dprintk("2 N: %d, F: %lld, R: %d\n", N, F, R);
+       F -= (N + 32) * 262144;
+
+    dprintk("3 N: %d, F: %lld, R: %d\n", N, F, R);

Hmm... it seems that you have some troubles with indenting at the driver. The
dprintk are using 4 spaces instead of a tab. The same also occurs on other
parts of cx24113_set_frequency()

+       switch (state->rev = cx24113_readreg(state, 0x00)) {
+       case 0x43:
+               info("unknown device\n");
+               break;
+       case REV_CX24113:
+               info("CX24113\n");
+               break;
+       default:
+               err("unsupported revision: %x\n", state->rev);
+               goto error;
+       }


Especially with SkyStar2, the above code is not very nice, since an i2c probing
is done by b2c2 code. So, if the device is not rev 2.8, the cx24113_readreg()
will return an error code, instead of a version. The err() will say that the device
has a cx24113 with an unknown version, instead of printing that the device
doesn't have a cx24113. Also, it is a codingstyle violation to assign the value
of a var inside another clause.

I would change this to something like:

	int rc;

	rc = cx24113_readreg(state, 0x00);
	if (rc < 0) {
                info("cx24113 not found.\n");
		goto err;
	}
	state->rev = rc;
	switch (state->rev) {
...
	}

Btw, we probably should add some code at b2c2 i2c code, disabling I2C error
messages during the I2C scanning while seeking for an specific revision, since
it is not really an error to not found a cx24113 or a cx24123 or a stv0299 on a
rev 2.3 board.

While there, this doesn't make much sense to me:

+       case 0x43:
+               info("unknown device\n");
+               break;

Why are you allowing this revision (you aren't jumping to error), but printing
that it is an unknown device?

If the code is known to work with this rev, but you don't know what chip
revision is used, I would change the message to something like:

info("detected a CX24113 variant\n");

+       case REV_CX24113:
+               info("CX24113\n");
+               break;

This will print this:

CX24113: CX24113 

with doesn't make much sense to me ;)

What about something like (using the same style as used on cx24123):
info("detected CX24113")?

Please, send me later the patches fixing the pointed issues. 

Cheers,
Mauro

_______________________________________________
v4l-dvb-maintainer mailing list
v4l-dvb-maintainer@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer