Re: [PATCH] Opera S1- DVB-USB

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 16 Apr 2007, Mauro Carvalho Chehab wrote:
> On Mon, 16 Apr 2007, Marco Gittler wrote:
> > so i this first enough to put it in and have some more testern for this
> > device ?
> For me, it seems ok now. I'll apply it later at master tree, to allow more
> people to test it. This will bring us about 3 weeks for testing and decide
> if this is ready for 2.6.22 or not.
>
> This will also give opportunity to other guys to send their comments,
> patches and sugestions to improve your driver.

diff -r b5be3479f070 linux/drivers/media/dvb/dvb-usb/Kconfig
--- a/linux/drivers/media/dvb/dvb-usb/Kconfig	Sat Apr 14 16:19:13 2007 -0300
+++ b/linux/drivers/media/dvb/dvb-usb/Kconfig	Mon Apr 16 12:06:05 2007 +0200
@@ -110,6 +110,7 @@ config DVB_USB_CXUSB

 	  Medion MD95700 hybrid USB2.0 device.
 	  DViCO FusionHDTV (Bluebird) USB2.0 devices
+

Random blank line inserted.

diff -r b5be3479f070 linux/drivers/media/dvb/dvb-usb/opera1.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/dvb/dvb-usb/opera1.c	Mon Apr 16 13:16:17 2007 +0200
[...]
+#include "opera1.h"
+#include "linux/module.h"
+#include "linux/init.h"
+#include "linux/firmware.h"
+#include "dvb-usb.h"
+#include "dvb-pll.h"
+#include "stv0299.h"

Includes should be first <linux/*.h> includes (with <> not ""), then
include "compat.h", then sub-system includes (e.g., "dvb-usb.h"), then your
includes.  The <linux/*> then compat.h order is important, the rest not as
much.

+       int ret = 0, t = 0;
+	u8 r[10];
+	u8 u8buf[len];
[...]
+	memset(r, 0, sizeof(*r));

You don't need to initialize ret or t.  It would easier to initialize r (I
don't think you need to anyway, but if you do) with "u8 r[10] = {0};" Instead
of adding an extra call to memset().  Also the memset is wrong, it should
be "memset(r, 0, sizeof(r));" as sizeof(*r) is 1, not 10.

Linux coding style is for one blank line after variables are declared and
before statements start.

The variable sized u8buf[len] array is a C99 feature.  Are we allowed to
use this in the kernel?  I'm not sure if this is allowed or not.

+		t = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+				OPERA_TUNER_REQ, USB_DIR_IN | USB_TYPE_VENDOR,
+				0x01, 0x0, r, 10, 2000);

You never use t anywhere.  You should probably check if usb_con...() failed
and return an error or print something.

static int opera1_usb_i2c_msgxfer(struct dvb_usb_device *dev, u16 addr,
[...]
+	request = (addr & 0xff00) >> 8;
+	if (!request)
+		request = 0xb1;
+	value = (addr & 0xff);
+	if (flag & OPERA_READ_MSG) {
+		value |= 0x01;

+static int opera1_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[],
[...]
+			if ((tmp = opera1_usb_i2c_msgxfer(d,
+						msg[i].addr,

The Linux I2C subsystem uses 7-bit addresses, while your code is using
8-bit addresses.  You should either change opera1_usb_i2c_msgxfer() so it
uses 7-bit addreses, or change opera1_i2c_xfer() so it passes
"msg[i].addr<<1" to opera1_usb_i2c_msgxfer().  This problem keeps comming
up again and again, you can check the list archives to find more
explainations.

opera1_i2c_xfer() would be better if you had this at the first line of
code:
	if (!d)
		return -ENODEV;

And the got rid of the extra if(d){ ...  } part.  Note that if d is NULL,
you will crash on the line that does "mutex_lock_interruptible(&d->i2c_mutex)",
which is run _before_ you check if d is null.  So the current if(d){ } is
pointless, you will crash before you get there.

+static struct stv0299_config opera1_stv0299_config = {
+	.demod_address = 0xd0,
[...]
+static int opera1_tuner_attach(struct dvb_usb_adapter *adap)
+{
+	adap->pll_addr = 0xc0;

The addresses are wrong, it should be 0x68 and 0x60.  It works because your
i2c code is also wrong, and the mistakes cancel each other out.

+static int opera1_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
+{
+	u8 buf_start[2] = { 0xff, 0x03 };
+	u8 buf_stop[2] = { 0xff, 0x00 };

Declare constant arrays like this as static const.

+static int opera1_rc_query(struct dvb_usb_device *dev, u32 * event, int *state)
[...]
+	memset(rcbuffer, 0, 32);

This memset isn't necessary, you're just going to over-write it with the i2c
command.

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

[Index of Archives]     [Linux Media]     [Video 4 Linux]     [Asterisk]     [Samba]     [Xorg]     [Xfree86]     [Linux USB]

  Powered by Linux