[PATCH 5/5] b43: move the register write into radio2050_rfover_val

[PATCH 5/5] b43: move the register write into radio2050_rfover_val

[Harvey Harrison]

It is always called with the same dev, register value as the
b43_phy_write that wraps around it, make it return void
and move the register write into radio2050_rfover_val.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 drivers/net/wireless/b43/phy.c |  125 ++++++++++++++++++----------------------
 1 files changed, 56 insertions(+), 69 deletions(-)

diff --git a/drivers/net/wireless/b43/phy.c b/drivers/net/wireless/b43/phy.c
index 4f1129b..3c5dc1d 100644
--- a/drivers/net/wireless/b43/phy.c
+++ b/drivers/net/wireless/b43/phy.c
@@ -3022,14 +3022,15 @@ static u16 b43_radio_core_calibration_value(struct b43_wldev *dev)
 }
 
 #define LPD(L, P, D)	(((L) << 2) | ((P) << 1) | ((D) << 0))
-static u16 radio2050_rfover_val(struct b43_wldev *dev,
+static void radio2050_rfover_val(struct b43_wldev *dev,
 				u16 phy_register, unsigned int lpd)
 {
 	struct b43_phy *phy = &dev->phy;
 	struct ssb_sprom *sprom = &(dev->dev->bus->sprom);
+	u16 value = 0;
 
 	if (!phy->gmode)
-		return 0;
+		goto write_value;
 
 	if (has_loopback_gain(phy)) {
 		int max_lb_gain = phy->max_lb_gain;
@@ -3063,37 +3064,46 @@ static u16 radio2050_rfover_val(struct b43_wldev *dev,
 		if ((phy->rev < 7) ||
 		    !(sprom->boardflags_lo & B43_BFL_EXTLNA)) {
 			if (phy_register == B43_PHY_RFOVER) {
-				return 0x1B3;
+				value =  0x1B3;
+				goto write_value;
 			} else if (phy_register == B43_PHY_RFOVERVAL) {
 				extlna |= (i << 8);
 				switch (lpd) {
 				case LPD(0, 1, 1):
-					return 0x0F92;
+					value = 0x0F92;
+					goto write_value;
 				case LPD(0, 0, 1):
 				case LPD(1, 0, 1):
-					return (0x0092 | extlna);
+					value = 0x0092 | extlna;
+					goto write_value;
 				case LPD(1, 0, 0):
-					return (0x0093 | extlna);
+					value = 0x0093 | extlna;
+					goto write_value;
 				}
 				B43_WARN_ON(1);
 			}
 			B43_WARN_ON(1);
 		} else {
 			if (phy_register == B43_PHY_RFOVER) {
-				return 0x9B3;
+				value = 0x9B3;
+				goto write_value;
 			} else if (phy_register == B43_PHY_RFOVERVAL) {
 				if (extlna)
 					extlna |= 0x8000;
 				extlna |= (i << 8);
 				switch (lpd) {
 				case LPD(0, 1, 1):
-					return 0x8F92;
+					value = 0x8F92;
+					goto write_value;
 				case LPD(0, 0, 1):
-					return (0x8092 | extlna);
+					value = 0x8092 | extlna;
+					goto write_value;
 				case LPD(1, 0, 1):
-					return (0x2092 | extlna);
+					value = 0x2092 | extlna;
+					goto write_value;
 				case LPD(1, 0, 0):
-					return (0x2093 | extlna);
+					value = 0x2093 | extlna;
+					goto write_value;
 				}
 				B43_WARN_ON(1);
 			}
@@ -3103,41 +3113,53 @@ static u16 radio2050_rfover_val(struct b43_wldev *dev,
 		if ((phy->rev < 7) ||
 		    !(sprom->boardflags_lo & B43_BFL_EXTLNA)) {
 			if (phy_register == B43_PHY_RFOVER) {
-				return 0x1B3;
+				value = 0x1B3;
+				goto write_value;
 			} else if (phy_register == B43_PHY_RFOVERVAL) {
 				switch (lpd) {
 				case LPD(0, 1, 1):
-					return 0x0FB2;
+					value = 0x0FB2;
+					goto write_value;
 				case LPD(0, 0, 1):
-					return 0x00B2;
+					value = 0x00B2;
+					goto write_value;
 				case LPD(1, 0, 1):
-					return 0x30B2;
+					value = 0x30B2;
+					goto write_value;
 				case LPD(1, 0, 0):
-					return 0x30B3;
+					value = 0x30B3;
+					goto write_value;
 				}
 				B43_WARN_ON(1);
 			}
 			B43_WARN_ON(1);
 		} else {
 			if (phy_register == B43_PHY_RFOVER) {
-				return 0x9B3;
+				value = 0x9B3;
+				goto write_value;
 			} else if (phy_register == B43_PHY_RFOVERVAL) {
 				switch (lpd) {
 				case LPD(0, 1, 1):
-					return 0x8FB2;
+					value = 0x8FB2;
+					goto write_value;
 				case LPD(0, 0, 1):
-					return 0x80B2;
+					value = 0x80B2;
+					goto write_value;
 				case LPD(1, 0, 1):
-					return 0x20B2;
+					value = 0x20B2;
+					goto write_value;
 				case LPD(1, 0, 0):
-					return 0x20B3;
+					value = 0x20B3;
+					goto write_value;
 				}
 				B43_WARN_ON(1);
 			}
 			B43_WARN_ON(1);
 		}
 	}
-	return 0;
+
+write_value:
+	b43_phy_write(dev, phy_register, value);
 }
 
 struct init2050_saved_values {
@@ -3216,11 +3238,8 @@ u16 b43_radio_init2050(struct b43_wldev *dev)
 			b43_phy_write(dev, B43_PHY_LO_CTL, 0);
 		}
 
-		b43_phy_write(dev, B43_PHY_RFOVERVAL,
-			      radio2050_rfover_val(dev, B43_PHY_RFOVERVAL,
-						   LPD(0, 1, 1)));
-		b43_phy_write(dev, B43_PHY_RFOVER,
-			      radio2050_rfover_val(dev, B43_PHY_RFOVER, 0));
+		radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(0, 1, 1));
+		radio2050_rfover_val(dev, B43_PHY_RFOVER, 0);
 	}
 	b43_write16(dev, 0x3E2, b43_read16(dev, 0x3E2) | 0x8000);
 
@@ -3246,16 +3265,12 @@ u16 b43_radio_init2050(struct b43_wldev *dev)
 	if (phy->type == B43_PHYTYPE_B)
 		b43_radio_write16(dev, 0x78, 0x26);
 	if (phy->gmode || phy->rev >= 2) {
-		b43_phy_write(dev, B43_PHY_RFOVERVAL,
-			      radio2050_rfover_val(dev, B43_PHY_RFOVERVAL,
-						   LPD(0, 1, 1)));
+		radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(0, 1, 1));
 	}
 	b43_phy_write(dev, B43_PHY_PGACTL, 0xBFAF);
 	b43_phy_write(dev, B43_PHY_CCK(0x2B), 0x1403);
 	if (phy->gmode || phy->rev >= 2) {
-		b43_phy_write(dev, B43_PHY_RFOVERVAL,
-			      radio2050_rfover_val(dev, B43_PHY_RFOVERVAL,
-						   LPD(0, 0, 1)));
+		radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(0, 0, 1));
 	}
 	b43_phy_write(dev, B43_PHY_PGACTL, 0xBFA0);
 	b43_radio_write16(dev, 0x51, b43_radio_read16(dev, 0x51)
@@ -3274,36 +3289,24 @@ u16 b43_radio_init2050(struct b43_wldev *dev)
 		b43_phy_write(dev, B43_PHY_CCK(0x59), 0xC810);
 		b43_phy_write(dev, B43_PHY_CCK(0x58), 0x000D);
 		if (phy->gmode || phy->rev >= 2) {
-			b43_phy_write(dev, B43_PHY_RFOVERVAL,
-				      radio2050_rfover_val(dev,
-							   B43_PHY_RFOVERVAL,
-							   LPD(1, 0, 1)));
+			radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 1));
 		}
 		b43_phy_write(dev, B43_PHY_PGACTL, 0xAFB0);
 		udelay(10);
 		if (phy->gmode || phy->rev >= 2) {
-			b43_phy_write(dev, B43_PHY_RFOVERVAL,
-				      radio2050_rfover_val(dev,
-							   B43_PHY_RFOVERVAL,
-							   LPD(1, 0, 1)));
+			radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 1));
 		}
 		b43_phy_write(dev, B43_PHY_PGACTL, 0xEFB0);
 		udelay(10);
 		if (phy->gmode || phy->rev >= 2) {
-			b43_phy_write(dev, B43_PHY_RFOVERVAL,
-				      radio2050_rfover_val(dev,
-							   B43_PHY_RFOVERVAL,
-							   LPD(1, 0, 0)));
+			radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 0));
 		}
 		b43_phy_write(dev, B43_PHY_PGACTL, 0xFFF0);
 		udelay(20);
 		tmp1 += b43_phy_read(dev, B43_PHY_LO_LEAKAGE);
 		b43_phy_write(dev, B43_PHY_CCK(0x58), 0);
 		if (phy->gmode || phy->rev >= 2) {
-			b43_phy_write(dev, B43_PHY_RFOVERVAL,
-				      radio2050_rfover_val(dev,
-							   B43_PHY_RFOVERVAL,
-							   LPD(1, 0, 1)));
+			radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 1));
 		}
 		b43_phy_write(dev, B43_PHY_PGACTL, 0xAFB0);
 	}
@@ -3322,40 +3325,24 @@ u16 b43_radio_init2050(struct b43_wldev *dev)
 			b43_phy_write(dev, B43_PHY_CCK(0x59), 0xC810);
 			b43_phy_write(dev, B43_PHY_CCK(0x58), 0x000D);
 			if (phy->gmode || phy->rev >= 2) {
-				b43_phy_write(dev, B43_PHY_RFOVERVAL,
-					      radio2050_rfover_val(dev,
-								   B43_PHY_RFOVERVAL,
-								   LPD(1, 0,
-								       1)));
+				radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 1));
 			}
 			b43_phy_write(dev, B43_PHY_PGACTL, 0xAFB0);
 			udelay(10);
 			if (phy->gmode || phy->rev >= 2) {
-				b43_phy_write(dev, B43_PHY_RFOVERVAL,
-					      radio2050_rfover_val(dev,
-								   B43_PHY_RFOVERVAL,
-								   LPD(1, 0,
-								       1)));
+				radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 1));
 			}
 			b43_phy_write(dev, B43_PHY_PGACTL, 0xEFB0);
 			udelay(10);
 			if (phy->gmode || phy->rev >= 2) {
-				b43_phy_write(dev, B43_PHY_RFOVERVAL,
-					      radio2050_rfover_val(dev,
-								   B43_PHY_RFOVERVAL,
-								   LPD(1, 0,
-								       0)));
+				radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 0));
 			}
 			b43_phy_write(dev, B43_PHY_PGACTL, 0xFFF0);
 			udelay(10);
 			tmp2 += b43_phy_read(dev, B43_PHY_LO_LEAKAGE);
 			b43_phy_write(dev, B43_PHY_CCK(0x58), 0);
 			if (phy->gmode || phy->rev >= 2) {
-				b43_phy_write(dev, B43_PHY_RFOVERVAL,
-					      radio2050_rfover_val(dev,
-								   B43_PHY_RFOVERVAL,
-								   LPD(1, 0,
-								       1)));
+				radio2050_rfover_val(dev, B43_PHY_RFOVERVAL, LPD(1, 0, 1));
 			}
 			b43_phy_write(dev, B43_PHY_PGACTL, 0xAFB0);
 		}
-- 
1.5.5.1.482.g0f174

Re: [PATCH 5/5] b43: move the register write into radio2050_rfover_val

[Michael Buesch]

On Wednesday 14 May 2008 20:56:36 Harvey Harrison wrote:
> It is always called with the same dev, register value as the
> b43_phy_write that wraps around it, make it return void
> and move the register write into radio2050_rfover_val.

NACK to the whole 5 patches.
See the list archives for an explanation.

-- 
Greetings Michael.

Re: [PATCH 5/5] b43: move the register write into radio2050_rfover_val

[Harvey Harrison]

On Wed, 2008-05-14 at 21:22 +0200, Michael Buesch wrote:
> On Wednesday 14 May 2008 20:56:36 Harvey Harrison wrote:
> > It is always called with the same dev, register value as the
> > b43_phy_write that wraps around it, make it return void
> > and move the register write into radio2050_rfover_val.
> 
> NACK to the whole 5 patches.
> See the list archives for an explanation.

Any particular reference for nacking 5/5?

While I respectfully disagree with you Re: 1-4 I can't make you take it.

But if you apply all 5, just look at the two files side-by-side and it
is much easier to follow with them applied.

Cheers,

Harvey

Re: [PATCH 5/5] b43: move the register write into radio2050_rfover_val

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Wed, 2008-05-14 at 12:35 -0700, Harvey Harrison wrote:
> On Wed, 2008-05-14 at 21:22 +0200, Michael Buesch wrote:
> > On Wednesday 14 May 2008 20:56:36 Harvey Harrison wrote:
> > > It is always called with the same dev, register value as the
> > > b43_phy_write that wraps around it, make it return void
> > > and move the register write into radio2050_rfover_val.
> > 
> > NACK to the whole 5 patches.
> > See the list archives for an explanation.
> 
> Any particular reference for nacking 5/5?
> 
> While I respectfully disagree with you Re: 1-4 I can't make you take it.

You could prove that those are correct:

make the function you're going to use an inline, compile &
objdump/md5sum, apply the patch conversion, compile & objdump/md5sum &
compare, make function out of line again

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 5/5] b43: move the register write into radio2050_rfover_val

[Harvey Harrison]

On Wed, 2008-05-14 at 22:00 +0200, Johannes Berg wrote:
> On Wed, 2008-05-14 at 12:35 -0700, Harvey Harrison wrote:
> > On Wed, 2008-05-14 at 21:22 +0200, Michael Buesch wrote:
> > > On Wednesday 14 May 2008 20:56:36 Harvey Harrison wrote:
> > > > It is always called with the same dev, register value as the
> > > > b43_phy_write that wraps around it, make it return void
> > > > and move the register write into radio2050_rfover_val.
> > > 
> > > NACK to the whole 5 patches.
> > > See the list archives for an explanation.
> > 
> > Any particular reference for nacking 5/5?
> > 
> > While I respectfully disagree with you Re: 1-4 I can't make you take it.
> 
> You could prove that those are correct:
> 
> make the function you're going to use an inline, compile &
> objdump/md5sum, apply the patch conversion, compile & objdump/md5sum &
> compare, make function out of line again

At this point I'll let sleeping dogs lie, it's not worth fighting over.

Harvey

Re: [PATCH 5/5] b43: move the register write into radio2050_rfover_val

[Michael Buesch]

On Wednesday 14 May 2008 21:35:57 Harvey Harrison wrote:
> On Wed, 2008-05-14 at 21:22 +0200, Michael Buesch wrote:
> > On Wednesday 14 May 2008 20:56:36 Harvey Harrison wrote:
> > > It is always called with the same dev, register value as the
> > > b43_phy_write that wraps around it, make it return void
> > > and move the register write into radio2050_rfover_val.
> > 
> > NACK to the whole 5 patches.
> > See the list archives for an explanation.
> 
> Any particular reference for nacking 5/5?

Well, I don't see a point.
I designed the function to return the actual value. Therefore it's
named foobar_val().
You patches shuffle a lot of code around, but there's no real gain from it.
There are only huge downsides, like the possibility of introducing a flipped
bitmask or similiar bugs. That happened in the past. And I tell you, it's
really hard to debug the PHY code. _Not_ because it's hard to read, but because
nobody does understand what it really does.
And guess what; it's _me_ who will have to debug it in case bugs appear... .
But I already explained all this earlier.

> But if you apply all 5, just look at the two files side-by-side and it
> is much easier to follow with them applied.

Nobody is actually reading or touching this code. It's Good Code (tm).
We don't know what it does and we must not modify its semantics. So I don't
really see a point in making it more pretty. You won't understand it afterwards
anyway.

_Please_ try to do something more useful, like hacking mac80211 or something like
that. There are lots of places in wireless that need work. But this certainly not
one of them, as it's good and _working_ code.

-- 
Greetings Michael.