[linux-dvb] [PATCH] Opera S1- DVB-USB

Mauro Carvalho Chehab mchehab at infradead.org
Mon Apr 16 17:20:44 CEST 2007


On Mon, 16 Apr 2007, Marco Gittler wrote:

> Thanks for your answer
>
> if fixed the things you wrote, and now it look a bit nicer.
>
> the values used here for many command here are unknown to me, because i have 
> no doc for this part of dvb box.
It would be nice if you can add some comments for things that you've 
already guessed, pointing that it is just a guess.

> 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.

Cheers,
Mauro.

>
> mchehab at infradead.org schrieb:
>> 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
>>
>> 
>
>

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab at infradead.org


More information about the linux-dvb mailing list