Re: [PATCH] v2: rtl8187: micro cleanup

[Pavel Roskin <proski@gnu.org>]

On Wed, 2010-02-17 at 20:04 +0100, okias wrote:
> ok, sorry I tested it on x86_64. Here is correct patch:

I'm sorry, but it's not a cleanup.  Initializing reg outside the block
gives a wrong impression that reg may be used outside the block.  Code
is safer is it's more readable, and it's more readable if the logic is
not spread around, but kept in one place.

Unnecessary initialization can prevent gcc from issuing warnings about
using uninitialized variables when the code changes.  Such warnings may
be useful to find real bugs.  I can give you a realistic example, but it
would be off-topic here.

A real cleanup would be to give reg the block scope and to refactor the
code where reg is written to the hardware:

--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1163,9 +1163,10 @@ static void rtl8187_bss_info_changed(struct ieee80211_hw *dev,
 {
 	struct rtl8187_priv *priv = dev->priv;
 	int i;
-	u8 reg;
 
 	if (changed & BSS_CHANGED_BSSID) {
+		u8 reg;
+
 		mutex_lock(&priv->conf_mutex);
 		for (i = 0; i < ETH_ALEN; i++)
 			rtl818x_iowrite8(priv, &priv->map->BSSID[i],
@@ -1176,13 +1177,12 @@ static void rtl8187_bss_info_changed(struct ieee80211_hw *dev,
 		else
 			reg = 0;
 
-		if (is_valid_ether_addr(info->bssid)) {
+		if (is_valid_ether_addr(info->bssid))
 			reg |= RTL818X_MSR_INFRA;
-			rtl818x_iowrite8(priv, &priv->map->MSR, reg);
-		} else {
+		else
 			reg |= RTL818X_MSR_NO_LINK;
-			rtl818x_iowrite8(priv, &priv->map->MSR, reg);
-		}
+
+		rtl818x_iowrite8(priv, &priv->map->MSR, reg);
 
 		mutex_unlock(&priv->conf_mutex);
 	}

But I would never bother with that simply because wireless developers
have more important things to do.

I respectfully suggest that you do your exercises in C programming
elsewhere.

-- 
Regards,
Pavel Roskin