Henrik Kurelid wrote: > Apart from the writing new iso setup code I have added CI descrambling > support and some CI MMI support. I have created a patch at > http://firesat.kurelid.se/firesat-ci-2.6.24.3-v3.patch with my additions. > (It is to be applied after the patch at > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/bad/ldp/dvb-add-firesat-driver.patch). Just a side note: Try to convert to regular coding style when yo do functional or merely formatting changes. One thing that sticks out is the comment style, e.g. > -/* > - printk(KERN_INFO "resp=0x%x\n",resp->resp); > - printk(KERN_INFO "cts=0x%x\n",resp->cts); > - printk(KERN_INFO "suid=0x%x\n",resp->suid); > - printk(KERN_INFO "sutyp=0x%x\n",resp->sutyp); > - printk(KERN_INFO "opcode=0x%x\n",resp->opcode); > - printk(KERN_INFO "length=%d\n",resp->length); > -*/ > + > +// printk(KERN_INFO "resp=0x%x\n",resp->resp); > +// printk(KERN_INFO "cts=0x%x\n",resp->cts); > +// printk(KERN_INFO "suid=0x%x\n",resp->suid); > +// printk(KERN_INFO "sutyp=0x%x\n",resp->sutyp); > +// printk(KERN_INFO "opcode=0x%x\n",resp->opcode); > +// printk(KERN_INFO "length=%d\n",resp->length); > + or > - CmdFrm.operand[4] = (firesat->type == FireSAT_DVB_T)?0x0c:0x11; // system_specific_multiplex selection_length > + // system_specific_multiplex selection_length > + CmdFrm.operand[4] = (firesat->type == FireSAT_DVB_T)?0x0c:0x11; Don't use // comments, except perhaps in hacks that will go away in another iteration before review requests, let alone merge requests. Instead use the styles /* * canonical multiline * comment style */ /* single line comment */ #if 0 temporarily_disabled_code; even_more_disabled_code; #endif /** * kerneldoc comment of subsystem API elements */ linux/scripts/checkpatch.pl will also reveal a few other formatting nits in your patch. ;-) > - BYTE ctype : 4 ; // command type > - BYTE cts : 4 ; // always 0x0 for AVC > - BYTE suid : 3 ; // subunit ID > - BYTE sutyp : 5 ; // subunit_typ > - BYTE opcode : 8 ; // opcode > - BYTE operand[509] ; // array of operands [1-507] > + __u8 ctype : 4 ; // command type > + __u8 cts : 4 ; // always 0x0 for AVC > + __u8 suid : 3 ; // subunit ID > + __u8 sutyp : 5 ; // subunit_typ > + __u8 opcode : 8 ; // opcode > + __u8 operand[509] ; // array of operands [1-507] Hmm, wouldn't plain and simple u8 instead of __u8 be appropriate here? But more importantly, these most certainly should not be bitfields. From what I have been told, bitfields aren't as strictly specified in the C language spec as required for on-the-wire data (and for userspace ABIs for example). I.e. ctype/cts and suid/sutyp in the above example would need to be collapsed into a single struct member. If there are several places in the code accessing these bifields, then you could supply inline functions (or second choice: preprocessor macros) as accessors. Somebody correct me if I confused something here. > #ifdef __LITTLE_ENDIAN > - BYTE ActiveSystem; > - BYTE reserved:5; > - BYTE NoRF:1; > - BYTE Moving:1; > - BYTE Searching:1; > + __u8 ActiveSystem; > + __u8 reserved:5; > + __u8 NoRF:1; > + __u8 Moving:1; > + __u8 Searching:1; [...] > #else [...] > + __u8 CaInitializationStatus:1; > + __u8 reserved6:1; > + > #endif These definitions need to be replaced eventually. Nowadays we typically define such data structures in the endianess of the hardware (I suppose big endian), with sparse-annotated types if available, and use accessors like cpu_to_beXX and so on to write and read such data. I'm not saying that /you/ should do all these cleanups; but you definitely should for example clean up comment style at places where you cleaned up 80 column line lengths and the likes. Thanks for keeping the ball rolling, -- Stefan Richter -=====-==--- =--- ----= http://arcgraph.de/sr/
