On Tue, 2010-06-01 at 19:19 -0700, [email protected] wrote:
> From: Cody Harmon <[email protected]>
Hi Cody.
Some of what you wrote are improvements, others not.
I only comment below on each issue once, though there
are multiple instances of each issue.
cheers, Joe
> diff --git a/drivers/staging/wlan-ng/p80211wext.c
> b/drivers/staging/wlan-ng/p80211wext.c
> index 387194d..d8d3db9 100644
> --- a/drivers/staging/wlan-ng/p80211wext.c
> +++ b/drivers/staging/wlan-ng/p80211wext.c
> @@ -232,9 +232,12 @@ struct iw_statistics
> *p80211wext_get_wireless_stats(netdevice_t *dev)
>
> retval = wlandev->mlmerequest(wlandev, (p80211msg_t *) &quality);
>
> - wstats->qual.qual = qual_as_percent(quality.link.data); /* overall link
> quality */
> - wstats->qual.level = quality.level.data; /* instant signal level
> */
> - wstats->qual.noise = quality.noise.data; /* instant noise level
> */
> + /* overall link quality */
> + wstats->qual.qual = qual_as_percent(quality.link.data);
> + /* instant signal level */
> + wstats->qual.level = quality.level.data;
> + /* instant noise level */
> + wstats->qual.noise = quality.noise.data;
Good
> wstats->qual.updated = IW_QUAL_ALL_UPDATED | IW_QUAL_DBM;
> wstats->discard.code = wlandev->rx.decrypt_err;
> @@ -287,8 +290,7 @@ static int p80211wext_giwfreq(netdevice_t *dev,
> unsigned int value;
>
> result = p80211wext_getmib(wlandev,
> -
> DIDmib_dot11phy_dot11PhyDSSSTable_dot11CurrentChannel,
> - &value);
> + DIDmib_dot11phy_dot11PhyDSSSTable_dot11CurrentChannel, &value);
not so good. 50+ character variable names really
are not easy to wrap at 80 chars and you probably
shouldn't bother.
[]
> @@ -419,7 +420,8 @@ static int p80211wext_giwrange(netdevice_t *dev,
> struct iw_range *range = (struct iw_range *)extra;
> int i, val;
>
> - /* for backward compatability set size and zero everything we don't
> understand */
> + /* for backward compatability set size
> + and zero everything we don't understand */
Comment blocks are not normally written like this
Normally this would be written something like:
/*
* for backward compatibility set size and zero everything
* we don't understand
*/
> data->length = sizeof(*range);
> memset(range, 0, sizeof(*range));
>
> @@ -564,9 +566,9 @@ static int p80211wext_siwencode(netdevice_t *dev,
> /* Set current key number only if no keys are given */
> if (erq->flags & IW_ENCODE_NOKEY) {
> result =
> - p80211wext_setmib(wlandev,
> -
> DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
> - i);
> + p80211wext_setmib(wlandev,
> + Dmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
> + i);
You end up with
result =
p80211wext_setmib(wlandev,
[...]
which is not normally used.
I'd've written:
result = p80211wext_setmib(wlandev,
DIDmib_dot11smt_dot11PrivacyTable_dot11WEPDefaultKeyID,
i);
and not cared about the 80 char warning
[]
> @@ -599,22 +602,22 @@ static int p80211wext_siwencode(netdevice_t *dev,
> switch (i) {
> case 0:
> pstr.did =
> -
> DIDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey0;
> +
> IDmib_dot11smt_dot11WEPDefaultKeysTable_dot11WEPDefaultKey0;
> break;
Remember to compile test before sending patches.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
