Re: [Outreachy kernel] [PATCH] Staging: rtl8188eu: Changed array and loop construct

[Jes Sorensen <jes.sorensen@gmail.com>]

On 02/23/15 09:02, Vatika Harlalka wrote:
> This function only required the array from the 14th element
> onwards. Therefore, the array size is reduced and the loop
> counter is modified so as to start from 0.
> Also, the assignment of variable place is redundant as it is
> initialized again in the loop.
> 
> Signed-off-by: Vatika Harlalka <vatikaharlalka@gmail.com>
> ---
>  drivers/staging/rtl8188eu/hal/phy.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/hal/phy.c b/drivers/staging/rtl8188eu/hal/phy.c
> index 3f663fe..e005150 100644
> --- a/drivers/staging/rtl8188eu/hal/phy.c
> +++ b/drivers/staging/rtl8188eu/hal/phy.c
> @@ -386,19 +386,18 @@ void phy_sw_chnl(struct adapter *adapt, u8 channel)
>  
>  static u8 get_right_chnl_for_iqk(u8 chnl)
>  {
> +	u8 place;
>  	u8 channel_all[ODM_TARGET_CHNL_NUM_2G_5G] = {
                       ^^^^^^^^^^^^^^^^^^^^^^^^^

> -		1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
>  		36, 38, 40, 42, 44, 46, 48, 50, 52, 54, 56, 58, 60, 62, 64,
>  		100, 102, 104, 106, 108, 110, 112, 114, 116, 118, 120, 122,
>  		124, 126, 128, 130, 132, 134, 136, 138, 140, 149, 151, 153,
>  		155, 157, 159, 161, 163, 165
>  	};
> -	u8 place = chnl;
>  
>  	if (chnl > 14) {
> -		for (place = 14; place < sizeof(channel_all); place++) {
> +		for (place = 0; place < sizeof(channel_all); place++) {
>  			if (channel_all[place] == chnl)
> -				return place-13;
> +				return ++place;
>  		}
>  	}
>  	return 0;
> 

This is a good catch!

You should reduce the size of the array accordingly as you now only need
space for the 5G channels, since you removed all the 2G channels.

To put it into context, for WiFi, there are 14 traditional 2GHz
channels, which are channels 1 through 14, and all the remaining
channels here are in the 5GHz spectrum.

It would also make sense to change the name of the array from
channel_all to channel_5g to reflect the situation. You can introduce a
#define ODM_TARGET_CHNL_NUM_5G next to the existing definition of
ODM_TARGET_CHNL_NUM_2G_5G.

Cheers,
Jes