Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero

On Sun, Aug 31, 2014 at 03:25:03PM +0100, Mark Einon wrote:

On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote:

quoted

On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote:

quoted

Replace a long list of contiguous writel() calls with a for loop iterating
over the same values.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index fffe763..44cc684 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c

@@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
 	u32 sa_lo;
 	u32 sa_hi = 0;
 	u32 pf_ctrl = 0;
+	u32 *wolw;
 
 	/* Disable the MAC while it is being configured (also disable WOL) */
 	writel(0x8, &rxmac->ctrl);

@@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
 	 * its default Values of 0x00000000 because there are not WOL masks
 	 * as of this time.
 	 */
-	writel(0, &rxmac->mask0_word0);
-	writel(0, &rxmac->mask0_word1);
-	writel(0, &rxmac->mask0_word2);
-	writel(0, &rxmac->mask0_word3);
-
-	writel(0, &rxmac->mask1_word0);
-	writel(0, &rxmac->mask1_word1);
-	writel(0, &rxmac->mask1_word2);
-	writel(0, &rxmac->mask1_word3);
-
-	writel(0, &rxmac->mask2_word0);
-	writel(0, &rxmac->mask2_word1);
-	writel(0, &rxmac->mask2_word2);
-	writel(0, &rxmac->mask2_word3);
-
-	writel(0, &rxmac->mask3_word0);
-	writel(0, &rxmac->mask3_word1);
-	writel(0, &rxmac->mask3_word2);
-	writel(0, &rxmac->mask3_word3);
-
-	writel(0, &rxmac->mask4_word0);
-	writel(0, &rxmac->mask4_word1);
-	writel(0, &rxmac->mask4_word2);
-	writel(0, &rxmac->mask4_word3);
+	for (wolw = &rxmac->mask0_word0; wolw <= &rxmac->mask4_word3; wolw++)
+		writel(0, wolw);

You are now only writing to all locations 1 time, instead of 4 times,
like before, are you sure that is ok?  Hardware is flaky, sometimes it
wants to be written to multiple times...

Hi Greg,

Thanks for the review.

As far as my understanding goes, the new code is equivalent to the old
code - it's a little confusing that the name refers to a word, but the
masks are all 32 bit values, and the loop iterates over the contiguous
list of masks found in txmac_regs (et131x.h:891 - the masks are also
unused in the driver after being set).

Or am I missing something here?

No, I was wrong, sorry, your explanation makes sense, sorry for the
noise.

greg k-h