Re: [PATCH] staging: winbond: mds.c whitspace, indentation etc.

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Lars Lindley wrote:
> I fixed most of the problems found by checkpatch.pl.
> Some long lines are left and some KERN_..
> This is a new revised patch against master with changes
> after comments from Stefan Richter and Dan Carpenter.
> Forget the old one.
> 
> Signed-off-by: Lars Lindley <lindley@coyote.org>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
>  drivers/staging/winbond/mds.c |  652 ++++++++++++++++++++++-------------------
>  1 files changed, 351 insertions(+), 301 deletions(-)

Lars, thanks for the revision.  The patch can surely go in as-is.  But
let me comment on some small things to take into consideration in future
updates of this kind.

[...]
> +	if (!boGroupAddr) {
> +		/*NOTE : If the protection mode is enabled and the MSDU will be
> +		 *	 fragmented, the tx rates of MPDUs will all be DSSS
> +		 *	 rates. So it will not use CTS-to-self in this case.
> +		 *	 CTS-To-self will only be used when without
> +		 *	 fragmentation. -- 20050112
> +		 */

/*
 * In the canonical multi-line comment form, the first line contains
 * only the /* characters but no text.
 */

[...]
> +			} else {
> +				/* CTS + 1 SIFS + CTS duration
> +				 * CTS Rate : ?? Mega bps
> +				 * CTS frame length = 14 bytes
> +				 */
> +			    if (pT01->T01_plcp_header_length) /*long preamble*/
> +				Duration += LONG_PREAMBLE_PLUS_PLCPHEADER_TIME;
> +			    else
> +				Duration += SHORT_PREAMBLE_PLUS_PLCPHEADER_TIME;
> +
> +				Duration += (((112 + Rate - 1) / Rate)
> +						+ DEFAULT_SIFSTIME);
>  			}
>  		}

There is still this one indentation by 3 tabs plus 4 spaces, which
should be 4 tabs (and the next level 5 tabs of course).  However, this
only shows the /real/ problem here:  Mds_DurationSet() is too long a
function, doing too much with too deep nesting.  Other functions in
mds.c with this problem are Mds_BodyCopy(), Mds_Tx(),
Mds_SendComplete()... IOW the majority of this file.

Eventually, somebody who knows what the sub-blocks of these functions
are for should break them out into own small functions with fitting
names.  Since I am entirely oblivious about WLAN things, I can't propose
a patch myself though.

(Needless to say, such a refactoring update should not be done within in
a large-scale whitespace cleanup patch to minimize the risk of mistakes.)

[...]
> -			//----20061009 add by anson's endian
> +			/* ----20061009 add by anson's endian */
>  			pNextT00->value = cpu_to_le32(pNextT00->value);
> -			pT01->value = cpu_to_le32( pT01->value );
> -			//----end 20061009 add by anson's endian
> +			pT01->value = cpu_to_le32(pT01->value);
> +			/* ----end 20061009 add by anson's endian */

Code history information went into a comment here.  Elsewhere too.

>  			buffer += OffsetSize;
> -			pT01 = (PT01_DESCRIPTOR)(buffer+4);
> -			if (i != 1)	//The last fragment will not have the next fragment
> -				pNextT00 = (PT00_DESCRIPTOR)(buffer+OffsetSize);
> +			pT01 = (PT01_DESCRIPTOR)(buffer + 4);
> +			if (i != 1)	/* The last fragment will not have the
> +					 * next fragment
> +					 */
> +				pNextT00 = (PT00_DESCRIPTOR)(buffer + OffsetSize);
>  		}

This iss seen multiple times in your patch.  IMO it would have been
better for readability if you kept such comments as single-line
comments, crossing the 80 characters boundary (which is a guideline, not
a dogma).  I.e.:

if (i != 1) /* last fragment won't have the next fragment */
	pNextT00 = (PT00_DESCRIPTOR)(buffer + OffsetSize);

or much better:

/* The last fragment will not have the next fragment */
if (i != 1)
	pNextT00 = (PT00_DESCRIPTOR)(buffer + OffsetSize);

There are many instances in this file where a comment would IMO be
better placed above a statement rather than behind a statement.

[...]
> -	// Set more frag bit
> -	TargetBuffer[1] |= 0x04;// Set more frag bit
> +	/* Set more frag bit */
> +	TargetBuffer[1] |= 0x04;/* Set more frag bit*/

Duplicate comment.

[...]
> -		if( ctmp1 == 108) ctmp2 = 7;
> -		else if( ctmp1 == 96 ) ctmp2 = 6; // Rate convert for USB
> -		else if( ctmp1 == 72 ) ctmp2 = 5;
> -		else if( ctmp1 == 48 ) ctmp2 = 4;
> -		else if( ctmp1 == 36 ) ctmp2 = 3;
> -		else if( ctmp1 == 24 ) ctmp2 = 2;
> -		else if( ctmp1 == 18 ) ctmp2 = 1;
> -		else if( ctmp1 == 12 ) ctmp2 = 0;
> -		else if( ctmp1 == 22 ) ctmp2 = 3;
> -		else if( ctmp1 == 11 ) ctmp2 = 2;
> -		else if( ctmp1 == 4  ) ctmp2 = 1;
> -		else ctmp2 = 0; // if( ctmp1 == 2  ) or default
> -
> -		if( i == 0 )
> +		pMds->TxRate[pDes->Descriptor_ID][i] = ctmp1; /* backup the ta
> +							       * rate and fall
> +							       * back rate
> +							       */
> +
> +		if (ctmp1 == 108)
> +			ctmp2 = 7;
> +		else if (ctmp1 == 96)
> +			ctmp2 = 6;	/* Rate convert for USB */
> +		else if (ctmp1 == 72)
> +			ctmp2 = 5;
> +		else if (ctmp1 == 48)
> +			ctmp2 = 4;
> +		else if (ctmp1 == 36)
> +			ctmp2 = 3;
> +		else if (ctmp1 == 24)
> +			ctmp2 = 2;
> +		else if (ctmp1 == 18)
> +			ctmp2 = 1;
> +		else if (ctmp1 == 12)
> +			ctmp2 = 0;
> +		else if (ctmp1 == 22)
> +			ctmp2 = 3;
> +		else if (ctmp1 == 11)
> +			ctmp2 = 2;
> +		else if (ctmp1 == 4)
> +			ctmp2 = 1;
> +		else
> +			ctmp2 = 0;	/* if (ctmp1 == 2) or default */

This should be written as a "switch (ctmp1)" block.  (Also something
that is better done separately from a wholesale whitespace cleanup.)
-- 
Stefan Richter
-=====-==-=- --== -====
http://arcgraph.de/sr/