[PATCH 2/5] drivers: staging: rtl8192u: fixing coding style issues in r8192U_core.c

[m999 at openmailbox.org]

On Fri, Jul 17, 2015 at 05:11:34PM -0700, Joe Perches wrote:
> On Fri, 2015-07-17 at 15:59 +0200, Joseph-Eugene Winzer wrote:
> > Reformatting the code without introducing other warnings
> > like 'Avoid unnecessary line continuations' or breaking strings.
> 
> Very few of these seem to be improvements and many
> of them should likely be in separate patches.
> 
> > diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> []
> > @@ -143,17 +143,35 @@ struct CHANNEL_LIST {
> >  };
> >  
> >  static struct CHANNEL_LIST ChannelPlan[] = {
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 36, 40, 44, 48, 52, 56, 60, 64, 149, 153, 157, 161, 165}, 24}, /* FCC */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}, 11}, /* IC */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 36, 40, 44, 48, 52, 56, 60, 64}, 21}, /* ETSI */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}, 13}, /* Spain. Change to ETSI. */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}, 13}, /* France. Change to ETSI. */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /* MKK //MKK */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /* MKK1 */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}, 13}, /* Israel. */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /*  For 11a , TELEC */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 36, 40, 44, 48, 52, 56, 60, 64}, 22}, /* MIC */
> > -	{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}, 14} /* For Global Domain. 1-11:active scan, 12-14 passive scan. //+YJ, 080626 */
> 
> If these were to be reformatted, it'd probably be better
> to use 10 per line so that the Len value that follows is
> more obvious.  Likely this struct should be const too.
> Maybe something like:
> 
> static const struct CHANNEL_LIST ChannelPlan[] = {
> 	{
> 		.Channel = {
> 			  1,  2,  3,  4,  5,  6,  7,  8,  9, 10,
> 			 11, 36, 40, 44, 48, 52, 56, 60, 64,149,
> 			153,157,161,165
> 		},
> 		.Len = 24,
> 	},
> 
> etc.  Add more spaces between channels if you really want.
> 
> > @@ -179,7 +197,9 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
> >  			min_chan = 1;
> >  			max_chan = 14;
> >  		} else {
> > -			RT_TRACE(COMP_ERR, "unknown rf chip, can't set channel map in function:%s()\n", __func__);
> > +			RT_TRACE(COMP_ERR,
> > +				 "unknown rf chip, can't set channel map in function:%s()\n"
> > +				 , __func__);
> 
> No leading commas please.  Put the comma after the format.
> 
> > @@ -187,7 +207,10 @@ static void rtl819x_set_channel_map(u8 channel_plan, struct r8192_priv *priv)
> >  			       sizeof(GET_DOT11D_INFO(ieee)->channel_map));
> >  			/*  Set new channel map */
> >  			for (i = 0; i < ChannelPlan[channel_plan].Len; i++) {
> > -				if (ChannelPlan[channel_plan].Channel[i] < min_chan || ChannelPlan[channel_plan].Channel[i] > max_chan)
> > +				if (ChannelPlan[channel_plan].Channel[i] <
> > +								min_chan ||
> > +				    ChannelPlan[channel_plan].Channel[i] >
> > +								max_chan)
> 
> Likely better to use a temporary for ChannelPlan[channel_plan]
> so these could become:
> 
> 				if (cp->Channel[i] < min_chan ||
> 				    cp->Channel[i] > max_chan)
> 					etc...
> 
> > @@ -1490,7 +1523,8 @@ static u8 QueryIsShort(u8 TxHT, u8 TxRate, cb_desc *tcb_desc)
> >  {
> >  	u8   tmp_Short;
> >  
> > -	tmp_Short = (TxHT == 1) ? ((tcb_desc->bUseShortGI) ? 1 : 0) : ((tcb_desc->bUseShortPreamble) ? 1 : 0);
> > +	tmp_Short = (TxHT == 1) ? ((tcb_desc->bUseShortGI) ? 1 : 0) :
> > +				  ((tcb_desc->bUseShortPreamble) ? 1 : 0);
> >  
> >  	if (TxHT == 1 && TxRate != DESC90_RATEMCS15)
> >  		tmp_Short = 0;
> 
> Maybe using bool would be better.
> 
> > @@ -1673,7 +1711,8 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
> >  					  0, tx_zero_isr, dev);
> >  			status = usb_submit_urb(tx_urb_zero, GFP_ATOMIC);
> >  			if (status) {
> > -				RT_TRACE(COMP_ERR, "Error TX URB for zero byte %d, error %d", atomic_read(&priv->tx_pending[tcb_desc->queue_index]), status);
> > +				RT_TRACE(COMP_ERR,
> > +					 "Error TX URB for zero byte %d, error %d", atomic_read(&priv->tx_pending[tcb_desc->queue_index]), status);
> 
> Probably better line wrapped after format
> 
> etc...
> 

Thanks that you took the time to look over it.
I haven't worked with an 80 character limit before, so it is hard for me to
decide whether 100+ character or chopped up lines hurt readability more.
Pleasing checkpatch.pl won't necessarily get you better code.

I see now that my approach of changing the whole file at once was wrong and a
'function to function' approach would probably be better to avoid complexity
introduced by my 'enhancments' and to get smaller patches.

In several sections the code has four or five levels of indentation.
So instead of trying to reformat I guess following CodingStyle's advise
to rewrite it is recommended..

Rather than polluting the main mailing list I should look at the kernelnewbies
mailing list for further criticism on my patch attemps.

I appreciate the input you gave me and anyone who likes to comment on it
but this patch should be ignored for the lack of any techincal merit.