Hi Marco,
I've some comments bellow about your patch.
Cheers,
Mauro
Em Dom, 2007-04-15 às 19:55 +0200, Marco Gittler escreveu:
> +config DVB_USB_OPERA1
> + tristate "Opera1 DVB-S USB2.0 receiver"
> + depends on DVB_USB
> + select DVB_STV0299
> + #if !DVB_FE_CUSTOMISE
Why the above is commented? If DVB_FE_CUSTOMISE is not yet supported,
just remove it, but the better is to allow supporting it.
> +static const u8* xilinx_init="\x90\x6e\xc5\xfa\x32\xd0\x73\x23\x1e";
Instead of writing as a string, better to write as a char array:
static const u8* xilinx_init= {0x90, 0x6e, 0xc5, ...
Better yet if you can better document the values at the array, by
replacing it by aliases, like:
static const u8* xilinx_init= { R90_INIT_DEV, R60_SELECT_MODE, ...
(of course if you know what the init is doing)
> + memset(r,0,10);
Avoid using magic numbers. Instead, do:
memset (r,0,sizeof(*r));
> + //if (mutex_lock_interruptible(&mymutex)){
> + // return -EAGAIN;
> + //}
Instead, use #if 0 for commenting codes like the above
Also, to follow kernel Documentation/CodingStyle, you should do:
if (foo) {
instead of
if (foo){
The same trouble (and other CodingStyle things, like column max size)
happens also on other parts of the code. Maybe the better is to run
indent with the following options:
indent -kr -i8 -ts8 -sob -l80 -ss -ncs <files>
This will apply several common CodingStyle used on kernel. You should
review the code, since sometimes it mess some stuff and remove spaces
used for alignment on some undesirable places.
So, the above should be something like this:
#if 0
if (mutex_lock_interruptible(&mymutex)) {
return -EAGAIN;
}
#endif
> + ret=usb_control_msg(dev, pipe,request, request_type| USB_TYPE_VENDOR,value, 0x0 /*index*/, u8buf,len, 2000);
The /*index*/ comment here is useless and can be removed.
> + }else
> + *state = REMOTE_NO_KEY_PRESSED;
> + return 0;
Hmm... return 0 is wrongly indented here.
> + if(ret==0)
> + {
Another CodingStyle. Should be, instead
if ( ret==0 ) {
> + // clear fpga ?
Generally, we avoid using c++ style comments, using instead:
/* clear fpga */
> + .entries = {
> + { 1064000, 500, 0xe5, 0xc6 },
> + { 1169000, 500, 0xe5, 0xe6 },
> + { 1299000, 500, 0xe5, 0x24 },
Hmm... this went missaligned at the email. Probably, you've mixed tabs
with spaces. Better to use or one or the other at the table.
Cheers,
Mauro
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
