on patch "DVB: add firesat driver" in staging.git

[Stefan Richter]

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/