[linux-dvb] Re: [video4linux-cvs] [hg:v4l-dvb] Add support for Opera S1- DVB-USB

[Michael Krufky]

Marco Gittler wrote:
> here the new patch against main hg.
> -the tuner i2c addr now without define (as wanted).
> -now 7 bit addr are used (the power_ctrl fkt ist ok so, because this
> is a raw write)
> -the addr >> 1 , addr << 1 is ok so, i think beause the read write is
> now taken from the last bit.
> -now i have no datasheet for the device, all taken from usb-logs
>
> i hope i answered all asked questions.
>
> Signed-off-by: Marco Gittler <g.marco at freenet.de>
Looks good... Still some trivial issues that can be fixed after the
fact.  See below for more comments. Meanwhile,

Mauro,

Please pull from:

http://linuxtv.org/hg/~mkrufky/opera

for Marco's patch:

- opera: use 7-bit i2c addresses

 dvb-usb-ids.h |    2
 opera1.c      |   80 +++++++++++++++++++++++---------------
 2 files changed, 50 insertions(+), 32 deletions(-)


Marco,

The only outstanding issues left that I see are whitespace-related
problems.  The repository whitespace stripper made a few cleanups,
besides that, you should still fix up some of these statements by
inserting spaces between operators.  For example:
> +	}
> +	ret = opera1_xilinx_rw(dev->udev, request,
> +		value, buf, len,
> +		addr&0x01?OPERA_READ_MSG:OPERA_WRITE_MSG);
>
>   
should be:

+		addr & 0x01 ? OPERA_READ_MSG : OPERA_WRITE_MSG);


...This cleanup is needed in numerous places throughout opera1.c

>  
>  	mutex_unlock(&dev->usb_mutex);
>  	return ret;
> @@ -122,13 +141,10 @@ static int opera1_i2c_xfer(struct i2c_ad
>  
>  	for (i = 0; i < num; i++) {
>  		if ((tmp = opera1_usb_i2c_msgxfer(d,
> -					msg[i].addr,
> +					(msg[i].addr<<1)|(msg[i].flags&I2C_M_RD?0x01:0),
>  					msg[i].buf,
> -					msg[i].len,
> -					(msg[i].flags ==
> -					I2C_M_RD ?
> -					OPERA_READ_MSG :
> -					OPERA_WRITE_MSG))!= msg[i].len)) {
> +					msg[i].len
> +					)!= msg[i].len)) {
>   
whitespace is all screwed up here...  The only way I know how to fix
this is by using emacs.  I'll clean it up for you if you give the
go-ahead.  Last time I did this, the patch was nacked, so I dont want to
waste my time without your prior knowledge.

>  			break;
>  		}
>  		if (dvb_usb_opera1_debug & 0x10)
> @@ -153,12 +169,12 @@ static int opera1_set_voltage(struct dvb
>  	static u8 command_13v[1]={0x00};
>  	static u8 command_18v[1]={0x01};
>  	struct i2c_msg msg[] = {
> -		{.addr = 0xb600,.flags = 0,.buf = command_13v,.len = 1},
> +		{.addr = ADDR_B600_VOLTAGE_13V,.flags = 0,.buf = command_13v,.len = 1},
>   
^^^ spaces needed after the commas (,)
>  	};
>  	struct dvb_usb_adapter *udev_adap =
>  	    (struct dvb_usb_adapter *)(fe->dvb->priv);
>  	if (voltage == SEC_VOLTAGE_18) {
> -		msg[0].addr = 0xb601;
> +		msg[0].addr = ADDR_B601_VOLTAGE_18V;
>  		msg[0].buf = command_18v;
>  	}
>  	i2c_transfer(&udev_adap->dev->i2c_adap, msg, 1);
> @@ -231,7 +247,7 @@ static u8 opera1_inittab[] = {
>  };
>  
>  static struct stv0299_config opera1_stv0299_config = {
> -	.demod_address = 0xd0,
> +	.demod_address = 0xd0>>1,
>   
0x68, please .. for the sake of consistency.  If you look at the other
driver sources, you'll notice other stv0299's configured at 0x68.  we
should keep this standard.
>  	.min_delay_ms = 100,
>  	.mclk = 88000000UL,
>  	.invert = 1,
> @@ -256,19 +272,21 @@ static int opera1_frontend_attach(struct
>  
>  static int opera1_tuner_attach(struct dvb_usb_adapter *adap)
>  {
> -	adap->pll_addr = 0xc0;
> -	adap->pll_desc = &dvb_pll_opera1;
> -	adap->fe->ops.tuner_ops.set_params = dvb_usb_tuner_set_params_i2c;
> +	dvb_attach(
> +		dvb_pll_attach, adap->fe, 0xc0>>1,
> +		&adap->dev->i2c_adap, &dvb_pll_opera1
> +	);
>   
Again, for the sake of remaining consistent with all other callers of
dvb_pll_attach, lets use 0x60 instead of "0xc0>>1"
>  	return 0;
>  }
>  
>  static int opera1_power_ctrl(struct dvb_usb_device *d, int onoff)
>  {
> -	int addr = onoff ? 0xb701 : 0xb700;
>  	u8 val = onoff ? 0x01 : 0x00;
> +	
>  	if (dvb_usb_opera1_debug)
>  		info("power %s", onoff ? "on" : "off");
> -	return opera1_usb_i2c_msgxfer(d, addr, &val, 1, OPERA_WRITE_MSG);
> +	return opera1_xilinx_rw(d->udev, 0xb7, val,
> +				&val, 1, OPERA_WRITE_MSG);
>  }
>  
>  static int opera1_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff)
> @@ -276,7 +294,7 @@ static int opera1_streaming_ctrl(struct 
>  	static u8 buf_start[2] = { 0xff, 0x03 };
>  	static u8 buf_stop[2] = { 0xff, 0x00 };
>  	struct i2c_msg start_tuner[] = {
> -		{.addr = 0xb1a6,.buf = onoff ? buf_start : buf_stop,.len = 2},
> +		{.addr = ADDR_B1A6_STREAM_CTRL,.buf = onoff ? buf_start : buf_stop,.len = 2},
>   
spaces needed after commas
>  	};
>  	if (dvb_usb_opera1_debug)
>  		info("streaming %s", onoff ? "on" : "off");
> @@ -289,7 +307,7 @@ static int opera1_pid_filter(struct dvb_
>  {
>  	u8 b_pid[3];
>  	struct i2c_msg msg[] = {
> -		{.addr = 0xb1a6,.buf = b_pid,.len = 3},
> +		{.addr = ADDR_B1A6_STREAM_CTRL,.buf = b_pid,.len = 3},
>   
again
>  	};
>  	if (dvb_usb_opera1_debug)
>  		info("pidfilter index: %d pid: %d %s", index, pid,
> @@ -306,7 +324,7 @@ static int opera1_pid_filter_control(str
>  	int u = 0x04;
>  	u8 b_pid[3];
>  	struct i2c_msg msg[] = {
> -		{.addr = 0xb1a6,.buf = b_pid,.len = 3},
> +		{.addr = ADDR_B1A6_STREAM_CTRL,.buf = b_pid,.len = 3},
>   
again
>  	};
>  	if (dvb_usb_opera1_debug)
>  		info("%s hw-pidfilter", onoff ? "enable" : "disable");
> @@ -356,7 +374,7 @@ static int opera1_rc_query(struct dvb_us
>  	const u16 startmarker1 = 0x10ed;
>  	const u16 startmarker2 = 0x11ec;
>  	struct i2c_msg read_remote[] = {
> -		{.addr = 0xb880,.buf = rcbuffer,.flags = I2C_M_RD,.len = 32},
> +		{.addr = ADDR_B880_READ_REMOTE,.buf = rcbuffer,.flags = I2C_M_RD,.len = 32},
>   
again
>  	};
>  	int i = 0;
>  	u32 send_key = 0;
> @@ -478,7 +496,7 @@ static struct dvb_usb_device_properties 
>  static struct dvb_usb_device_properties opera1_properties = {
>  	.caps = DVB_USB_IS_AN_I2C_ADAPTER,
>  	.usb_ctrl = CYPRESS_FX2,
> -	.firmware = "opera.fw",
> +	.firmware = "dvb-usb-opera-01.fw",
>  	.size_of_priv = sizeof(struct opera1_state),
>  
>  	.power_ctrl = opera1_power_ctrl,
> @@ -533,7 +551,7 @@ static int opera1_probe(struct usb_inter
>  	if (udev->descriptor.idProduct == USB_PID_OPERA1_WARM &&
>  		udev->descriptor.idVendor == USB_VID_OPERA1 &&
>  		(d == NULL
> -			|| opera1_xilinx_load_firmware(udev, "opera1-fpga.fw") != 0)
> +			|| opera1_xilinx_load_firmware(udev, "dvb-usb-opera1-fpga.fw") != 0)
>  		) {
>  		return -EINVAL;
>  	}
>   

Usually I would do these cleanups myself, but here I am giving you the
opportunity to do them yourself.  Please either fix these trivial
issues, or give me the go-ahead to fix them.

Either way, this doesn't hold up your pending patch.  Let me know if you
have any questions.

Cheers,

Mike Krufky