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@xxxxxxxxxx>
> Acked-by: Pavel Machek <pavel@xxxxxx>
> ---
> 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/
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
