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

[Joe Perches <joe@perches.com>]

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...