On 06/01/2013 01:11 AM, Dan Carpenter wrote:
This one will need to be redone. It introduces new GCC warnings:
drivers/staging/rtl8192u/r8192U_core.c:1958:9: warning: mixing declarations and code
drivers/staging/rtl8192u/r8192U_core.c:2698:25: warning: mixing declarations and code
drivers/staging/rtl8192u/r8192U_core.c:2809:9: warning: mixing declarations and code
drivers/staging/rtl8192u/r8192U_core.c:2895:9: warning: mixing declarations and code
drivers/staging/rtl8192u/r8192U_core.c:3007:17: warning: mixing declarations and code
drivers/staging/rtl8192u/r8192U_core.c:3019:9: warning: mixing declarations and code
People use this trick:
{
int my_local_var;
blah; blah; blah;
}
You will need to move the declaration to start of the function when
you delete the block.
Often the code does this:
#ifdef DEAD_CODE
{
int my_local_var;
blah; blah; blah;
}
#endif
My suggestion is to go through and delete the dead code first in a
separate patch. Also really this patch is pretty huge. With all
the indent changes and everything it's a bit hard to review, and I
knew you were going to have to redo it anyway so I didn't review
until the end.
One way to split this up would be:
[patch 1/3] remove ifdefed out dead code
[patch 2/3] move braces around but don't add or delete any brace or change indent levels
[patch 3/3] remove unneeded block statements and pull things in an indent level
The subjects are bad but you get the idea.
Ok i see. I will try this.
On Fri, May 31, 2013 at 08:10:49PM +0300, Xenia Ragiadakou wrote:
if (priv->ieee80211->iw_mode == IW_MODE_MONITOR || \
You didn't add this, but these '\' characters are pointless. This
isn't a macro.
I have removed these line continuations in RFC 4/5.
- dev->flags& IFF_PROMISC){
+ dev->flags& IFF_PROMISC)
rxconf = rxconf | RCR_AAP;
- } else{
+ else {
You can fix this in a follow on patch, but this isn't right. The
rule is that if either side of the if else statement has curly
braces then both sides get them.
I did not know that. Thx I will fix it.
rxconf = rxconf | RCR_APM;
rxconf = rxconf | RCR_CBSSID;
}
@@ -1441,13 +1429,10 @@ void rtl8192_update_cap(struct net_device *dev, u16 cap)
tmp |= BRSR_AckShortPmb;
write_nic_dword(dev, RRSR, tmp);
- if (net->mode& (IEEE_G|IEEE_N_24G))
- {
+ if (net->mode& (IEEE_G|IEEE_N_24G)) {
u8 slot_time = 0;
- if ((cap& WLAN_CAPABILITY_SHORT_SLOT)&&(!priv->ieee80211->pHTInfo->bCurrentRT2RTLongSlotTime))
- {//short slot time
+ if ((cap& WLAN_CAPABILITY_SHORT_SLOT)&&(!priv->ieee80211->pHTInfo->bCurrentRT2RTLongSlotTime)) //short slot time
^^^^^^^^^^^^^^^
This is a pointless comment. Delete.
I will delete all pointless comments in a following patch.
slot_time = SHORT_SLOT_TIME;
- }
else //long slot time
slot_time = NON_SHORT_SLOT_TIME;
priv->slot_time = slot_time;
@@ -2563,15 +2503,12 @@ static void rtl8192_init_priv_variable(struct net_device *dev)
skb_queue_head_init(&priv->skb_queue);
/* Tx related queue */
- for (i = 0; i< MAX_QUEUE_SIZE; i++) {
+ for (i = 0; i< MAX_QUEUE_SIZE; i++)
skb_queue_head_init(&priv->ieee80211->skb_waitQ [i]);
- }
- for (i = 0; i< MAX_QUEUE_SIZE; i++) {
+ for (i = 0; i< MAX_QUEUE_SIZE; i++)
skb_queue_head_init(&priv->ieee80211->skb_aggQ [i]);
- }
- for (i = 0; i< MAX_QUEUE_SIZE; i++) {
+ for (i = 0; i< MAX_QUEUE_SIZE; i++)
skb_queue_head_init(&priv->ieee80211->skb_drv_aggQ [i]);
- }
In a later patch you can change this to:
for (i = 0; i< MAX_QUEUE_SIZE; i++) {
skb_queue_head_init(&priv->ieee80211->skb_waitQ[i]);
skb_queue_head_init(&priv->ieee80211->skb_aggQ[i]);
skb_queue_head_init(&priv->ieee80211->skb_drv_aggQ[i]);
}
And the 'Q' on the end is bad CamelCase and it's bad kernel style.
"skb_waitQ" is a horrible name. Neither "skb" nor
"waitQ/wait_queue" actually tell you any information at all about
what it's for. :/
Yes, I have noticed it. I will fix it in a following patch.
regards,
dan carpenter
Thanks again for your suggestions. I will update it and resend it.
Ksenia
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
