On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote: > The case statement in get_ap_information() should not use literal integers > to parse information element IDs when these values are provided by name > in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'. Nice. Magic number removal, I like it. > Signed-off-by: Quytelda Kahja <quytelda@xxxxxxxxxxx> > --- > drivers/staging/ks7010/ks_hostif.c | 31 +++++++++++++++---------------- > drivers/staging/ks7010/ks_hostif.h | 1 + > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c > index 74a08417bd0b..67cf32433023 100644 > --- a/drivers/staging/ks7010/ks_hostif.c > +++ b/drivers/staging/ks7010/ks_hostif.c > @@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info, > offset = 0; > > while (bsize > offset) { > - /* DPRINTK(4, "Element ID=%d\n",*bp); */ nit: Probably shouldn't remove debugging output in the same patch as doing magic number removal. (super nit-picky though.) > - switch (*bp) { > - case 0: /* ssid */ > + switch (*bp) { /* Information Element ID */ > + case WLAN_EID_SSID: > if (*(bp + 1) <= SSID_MAX_SIZE) { > ap->ssid.size = *(bp + 1); > } else { > @@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info, > } > memcpy(ap->ssid.body, bp + 2, ap->ssid.size); > break; > - case 1: /* rate */ > - case 50: /* ext rate */ > + case WLAN_EID_SUPP_RATES: > + case WLAN_EID_EXT_SUPP_RATES: > if ((*(bp + 1) + ap->rate_set.size) <= > RATE_SET_MAX_SIZE) { > memcpy(&ap->rate_set.body[ap->rate_set.size], > @@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info, > (RATE_SET_MAX_SIZE - ap->rate_set.size); > } > break; > - case 3: /* DS parameter */ > + case WLAN_EID_DS_PARAMS: > break; > - case 48: /* RSN(WPA2) */ > + case WLAN_EID_RSN: > ap->rsn_ie.id = *bp; > if (*(bp + 1) <= RSN_IE_BODY_MAX) { > ap->rsn_ie.size = *(bp + 1); > @@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info, > } > memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size); > break; > - case 221: /* WPA */ > - if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* WPA OUI check */ > + case WLAN_EID_VENDOR_SPECIFIC: /* WPA */ > + if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* WPA OUI check */ Shouldn't have this line (unnecessary white space change) > ap->wpa_ie.id = *bp; > if (*(bp + 1) <= RSN_IE_BODY_MAX) { > ap->wpa_ie.size = *(bp + 1); > @@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info, > } > break; > > - case 2: /* FH parameter */ > - case 4: /* CF parameter */ > - case 5: /* TIM */ > - case 6: /* IBSS parameter */ > - case 7: /* Country */ > - case 42: /* ERP information */ > - case 47: /* Reserve ID 47 Broadcom AP */ > + case WLAN_EID_FH_PARAMS: > + case WLAN_EID_CF_PARAMS: > + case WLAN_EID_TIM: > + case WLAN_EID_IBSS_PARAMS: > + case WLAN_EID_COUNTRY: > + case WLAN_EID_ERP_INFO: > break; > default: > DPRINTK(4, "unknown Element ID=%d\n", *bp); > break; > } > + FTR try not to include unrelated whitespace changes. I see you changed the case statement but the whitespace is after it and not really related. Again quite nit-picky but I'm guessing you are in staging to learn so might as well learn it now than when you are patching the core kernel :) Good work, Tobin. _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel