Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755477AbbGPPml (ORCPT ); Thu, 16 Jul 2015 11:42:41 -0400 Received: from mail-wg0-f48.google.com ([74.125.82.48]:33346 "EHLO mail-wg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752086AbbGPPmk (ORCPT ); Thu, 16 Jul 2015 11:42:40 -0400 References: <20150716020408.GA6689@sreenath-gentoo.lan> From: Jakub Sitnicki To: Sreenath Madasu Cc: gregkh@linuxfoundation.org, somyaanand214@gmail.com, devel@driverdev.osuosl.org, tapaswenipathak@gmail.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: rtl8188eu: core: find and remove code valid only for 5 HGz. In-reply-to: <20150716020408.GA6689@sreenath-gentoo.lan> Date: Thu, 16 Jul 2015 17:42:37 +0200 Message-ID: <87wpy085xu.fsf@frog.home> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3466 Lines: 93 On Thu, Jul 16, 2015 at 04:04 AM CEST, Sreenath Madasu wrote: > This one of the TODO tasks for staging rtl8188eu driver. I have removed > the code referring to channel > 14 for rtw_ap.c, rtw_ieee80211.c and > rtw_mlme.c files. Please review. > > Signed-off-by: Sreenath Madasu I would consider rewording the subject and the description to say what the change does ("remove ...") as opposed to saying what you did ("find and remove..."). The "Description, description, description" section from the article below, if you haven't seen it yet, explains it better than I can: https://github.com/gregkh/kernel-tutorial/blob/master/lxf_article/write_kernel_patch.txt > --- > drivers/staging/rtl8188eu/core/rtw_ap.c | 31 +++++++------------------- > drivers/staging/rtl8188eu/core/rtw_ieee80211.c | 24 ++++++-------------- > drivers/staging/rtl8188eu/core/rtw_mlme.c | 5 +---- > 3 files changed, 16 insertions(+), 44 deletions(-) > [snip] > @@ -584,15 +576,8 @@ static void update_bmc_sta(struct adapter *padapter) > tx_ra_bitmap |= rtw_get_bit_value_from_ieee_value(psta->bssrateset[i]&0x7f); > } > > - if (pcur_network->Configuration.DSConfig > 14) { > - /* force to A mode. 5G doesn't support CCK rates */ > - network_type = WIRELESS_11A; > - tx_ra_bitmap = 0x150; /* 6, 12, 24 Mbps */ > - } else { > - /* force to b mode */ > - network_type = WIRELESS_11B; > - tx_ra_bitmap = 0xf; > - } > + network_type = WIRELESS_11B; > + tx_ra_bitmap = 0xf; > > raid = networktype_to_raid(network_type); > init_rate = get_highest_rate_idx(tx_ra_bitmap&0x0fffffff)&0x3f; Is the dropped comment ("force to b mode") worth leaving just to draw attention? There is something suspicious going on in update_bmc_sta() because just a few lines above we determine the network_type: network_type = rtw_check_network_type((u8 *)&pcur_network->SupportedRates, supportRateNum, 1); ... only to force to it WIRELESS_11B later on. Unfortunately I'm not familiar enough with the driver to know why. > diff --git a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c > index 11b780d..f55dae1 100644 > --- a/drivers/staging/rtl8188eu/core/rtw_ieee80211.c > +++ b/drivers/staging/rtl8188eu/core/rtw_ieee80211.c > @@ -113,19 +113,12 @@ uint rtw_is_cckratesonly_included(u8 *rate) > > int rtw_check_network_type(unsigned char *rate, int ratelen, int channel) > { > - if (channel > 14) { > - if ((rtw_is_cckrates_included(rate)) == true) > - return WIRELESS_INVALID; > - else > - return WIRELESS_11A; > - } else { /* could be pure B, pure G, or B/G */ > - if ((rtw_is_cckratesonly_included(rate)) == true) > - return WIRELESS_11B; > - else if ((rtw_is_cckrates_included(rate)) == true) > - return WIRELESS_11BG; > - else > - return WIRELESS_11G; > - } > + if ((rtw_is_cckratesonly_included(rate)) == true) > + return WIRELESS_11B; > + else if ((rtw_is_cckrates_included(rate)) == true) > + return WIRELESS_11BG; > + else > + return WIRELESS_11G; > } > That makes the 'channel' parameter unused. A candidate for a clean-up together with 'ratelen'. Cheers, Jakub -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/