Mostly by using the mask/set helpers. Some temporary variables have
also been eliminated.
Signed-off-by: Harvey Harrison <[email protected]>
---
drivers/net/wireless/b43/lo.c | 79 +++++++++++++++-------------------------
1 files changed, 30 insertions(+), 49 deletions(-)
diff --git a/drivers/net/wireless/b43/lo.c b/drivers/net/wireless/b43/lo.c
index 6a18a14..a6c185b 100644
--- a/drivers/net/wireless/b43/lo.c
+++ b/drivers/net/wireless/b43/lo.c
@@ -225,14 +225,12 @@ static void lo_measure_txctl_values(struct b43_wldev *dev)
radio_pctl_reg = tmp;
}
}
- b43_radio_write16(dev, 0x43, (b43_radio_read16(dev, 0x43)
- & 0xFFF0) | radio_pctl_reg);
+ b43_radio_maskset(dev, 0x43, 0xFFF0, radio_pctl_reg);
b43_gphy_set_baseband_attenuation(dev, 2);
reg = lo_txctl_register_table(dev, &mask, NULL);
mask = ~mask;
- b43_radio_write16(dev, reg, b43_radio_read16(dev, reg)
- & mask);
+ b43_radio_mask(dev, reg, mask);
if (has_tx_magnification(phy)) {
int i, j;
@@ -242,14 +240,10 @@ static void lo_measure_txctl_values(struct b43_wldev *dev)
for (i = 0; i < ARRAY_SIZE(tx_magn_values); i++) {
tx_magn = tx_magn_values[i];
- b43_radio_write16(dev, 0x52,
- (b43_radio_read16(dev, 0x52)
- & 0xFF0F) | tx_magn);
+ b43_radio_maskset(dev, 0x52, 0xFF0F, tx_magn);
for (j = 0; j < ARRAY_SIZE(tx_bias_values); j++) {
tx_bias = tx_bias_values[j];
- b43_radio_write16(dev, 0x52,
- (b43_radio_read16(dev, 0x52)
- & 0xFFF0) | tx_bias);
+ b43_radio_maskset(dev, 0x52, 0xFFF0, tx_bias);
feedthrough =
lo_measure_feedthrough(dev, 0, pga,
trsw_rx);
@@ -261,16 +255,13 @@ static void lo_measure_txctl_values(struct b43_wldev *dev)
if (lo->tx_bias == 0)
break;
}
- b43_radio_write16(dev, 0x52,
- (b43_radio_read16(dev, 0x52)
- & 0xFF00) | lo->tx_bias | lo->
- tx_magn);
+ b43_radio_maskset(dev, 0x52, 0xFF00,
+ (lo->tx_bias | lo->tx_magn));
}
} else {
lo->tx_magn = 0;
lo->tx_bias = 0;
- b43_radio_write16(dev, 0x52, b43_radio_read16(dev, 0x52)
- & 0xFFF0); /* TX bias == 0 */
+ b43_radio_mask(dev, 0x52, 0xFFF0); /* TX bias == 0 */
}
lo->txctl_measured_time = jiffies;
}
@@ -301,7 +292,6 @@ static void lo_measure_gain_values(struct b43_wldev *dev,
{
struct b43_phy *phy = &dev->phy;
struct b43_phy_g *gphy = phy->g;
- u16 tmp;
if (max_rx_gain < 0)
max_rx_gain = 0;
@@ -349,12 +339,10 @@ static void lo_measure_gain_values(struct b43_wldev *dev,
}
}
- tmp = b43_radio_read16(dev, 0x7A);
if (gphy->lna_lod_gain == 0)
- tmp &= ~0x0008;
+ b43_radio_mask(dev, 0x7A, ~0x0008);
else
- tmp |= 0x0008;
- b43_radio_write16(dev, 0x7A, tmp);
+ b43_radio_set(dev, 0x7A, 0x0008);
}
struct lo_g_saved_values {
@@ -397,7 +385,6 @@ static void lo_measure_setup(struct b43_wldev *dev,
struct b43_phy *phy = &dev->phy;
struct b43_phy_g *gphy = phy->g;
struct b43_txpower_lo_control *lo = gphy->lo_control;
- u16 tmp;
if (b43_has_hardware_pctl(dev)) {
sav->phy_lo_mask = b43_phy_read(dev, B43_PHY_LO_MASK);
@@ -459,17 +446,16 @@ static void lo_measure_setup(struct b43_wldev *dev,
}
sav->reg_3F4 = b43_read16(dev, 0x3F4);
sav->reg_3E2 = b43_read16(dev, 0x3E2);
- sav->radio_43 = b43_radio_read16(dev, 0x43);
- sav->radio_7A = b43_radio_read16(dev, 0x7A);
+ sav->radio_43 = b43_radio_read(dev, 0x43);
+ sav->radio_7A = b43_radio_read(dev, 0x7A);
sav->phy_pgactl = b43_phy_read(dev, B43_PHY_PGACTL);
sav->phy_cck_2A = b43_phy_read(dev, B43_PHY_CCK(0x2A));
sav->phy_syncctl = b43_phy_read(dev, B43_PHY_SYNCCTL);
sav->phy_dacctl = b43_phy_read(dev, B43_PHY_DACCTL);
- if (!has_tx_magnification(phy)) {
- sav->radio_52 = b43_radio_read16(dev, 0x52);
- sav->radio_52 &= 0x00F0;
- }
+ if (!has_tx_magnification(phy))
+ sav->radio_52 = b43_radio_read(dev, 0x52) & 0x00F0;
+
if (phy->type == B43_PHYTYPE_B) {
sav->phy_cck_30 = b43_phy_read(dev, B43_PHY_CCK(0x30));
sav->phy_cck_06 = b43_phy_read(dev, B43_PHY_CCK(0x06));
@@ -482,14 +468,13 @@ static void lo_measure_setup(struct b43_wldev *dev,
b43_write16(dev, 0x3F4, b43_read16(dev, 0x3F4)
& 0xF000);
- tmp =
- (phy->type == B43_PHYTYPE_G) ? B43_PHY_LO_MASK : B43_PHY_CCK(0x2E);
- b43_phy_write(dev, tmp, 0x007F);
+ if (phy->type == B43_PHYTYPE_G)
+ b43_phy_write(dev, B43_PHY_LO_MASK, 0x007F);
+ else
+ b43_phy_write(dev, B43_PHY_CCK(0x2E), 0x007F);
- tmp = sav->phy_syncctl;
- b43_phy_write(dev, B43_PHY_SYNCCTL, tmp & 0xFF7F);
- tmp = sav->radio_7A;
- b43_radio_write16(dev, 0x007A, tmp & 0xFFF0);
+ b43_phy_write(dev, B43_PHY_SYNCCTL, sav->phy_syncctl & 0xFF7F);
+ b43_radio_write(dev, 0x007A, sav->radio_7A & 0xFFF0);
b43_phy_write(dev, B43_PHY_CCK(0x2A), 0x8A3);
if (phy->type == B43_PHYTYPE_G ||
@@ -501,7 +486,7 @@ static void lo_measure_setup(struct b43_wldev *dev,
if (phy->rev >= 2)
b43_dummy_transmission(dev);
b43_gphy_channel_switch(dev, 6, 0);
- b43_radio_read16(dev, 0x51); /* dummy read */
+ b43_radio_read(dev, 0x51); /* dummy read */
if (phy->type == B43_PHYTYPE_G)
b43_phy_write(dev, B43_PHY_CCK(0x2F), 0);
@@ -554,13 +539,10 @@ static void lo_measure_restore(struct b43_wldev *dev,
b43_phy_write(dev, B43_PHY_CCK(0x2A), sav->phy_cck_2A);
b43_phy_write(dev, B43_PHY_SYNCCTL, sav->phy_syncctl);
b43_phy_write(dev, B43_PHY_DACCTL, sav->phy_dacctl);
- b43_radio_write16(dev, 0x43, sav->radio_43);
- b43_radio_write16(dev, 0x7A, sav->radio_7A);
- if (!has_tx_magnification(phy)) {
- tmp = sav->radio_52;
- b43_radio_write16(dev, 0x52, (b43_radio_read16(dev, 0x52)
- & 0xFF0F) | tmp);
- }
+ b43_radio_write(dev, 0x43, sav->radio_43);
+ b43_radio_write(dev, 0x7A, sav->radio_7A);
+ if (!has_tx_magnification(phy))
+ b43_radio_maskset(dev, 0x52, 0xFF0F, sav->radio_52);
b43_write16(dev, 0x3E2, sav->reg_3E2);
if (phy->type == B43_PHYTYPE_B &&
phy->radio_ver == 0x2050 && phy->radio_rev <= 5) {
@@ -778,12 +760,11 @@ struct b43_lo_calib * b43_calibrate_lo_setting(struct b43_wldev *dev,
txctl_reg = lo_txctl_register_table(dev, &txctl_value, &pad_mix_gain);
- b43_radio_write16(dev, 0x43,
- (b43_radio_read16(dev, 0x43) & 0xFFF0)
- | rfatt->att);
- b43_radio_write16(dev, txctl_reg,
- (b43_radio_read16(dev, txctl_reg) & ~txctl_value)
- | (rfatt->with_padmix) ? txctl_value : 0);
+ b43_radio_maskset(dev, 0x43, 0xFFF0, rfatt->att);
+ if (rfatt->with_padmix)
+ b43_radio_maskset(dev, txctl_reg, ~txctl_value, txctl_value);
+ else
+ b43_radio_maskset(dev, txctl_reg, ~txctl_value, 0);
max_rx_gain = rfatt->att * 2;
max_rx_gain += bbatt->att / 2;
--
1.6.0.2.471.g47a76
On Fri, 2008-10-03 at 23:17 +0200, Michael Buesch wrote:
> On Friday 03 October 2008 22:48:46 Harvey Harrison wrote:
> > Mostly by using the mask/set helpers. Some temporary variables have
> > also been eliminated.
>
> I already told you two times that I do not like these patches.
> I also explained in great detail why this is the case.
>
> Please drop these patches!
> If you send them one more time, I will immediately add your email address to
> my exim blacklist.
>
OK, but you should still look at patch 1 for the unaligned access
helpers.
Patch 2 is small and easy to verify (only two lines), but fair enough.
Patch 3 makes the code flow a lot more obvious rather than relying on
a tmp variable to compose the masking/setting.
The rest I'll kick to the bitbucket...although I think you should look
at the portion of patch 5 that touches the function b43_radio_init2060
as it points out the _one_ place where a register is masking the value
of some other register...which is absolutely impossible to see unless
you do a series of patches like this.
It may be that it is intentional, and it has been this way as far back
in the git history as I can find, but at least now it can be seen.
- b43_radio_write16(dev, 0x0005,
- (b43_radio_read16(dev, 0x0081) & ~0x0008) | 0x0008);
Cheers,
Harvey
Just for kicks, a combined diffstat:
drivers/net/wireless/b43/lo.c | 110 +++++--------
drivers/net/wireless/b43/main.c | 58 ++-----
drivers/net/wireless/b43/phy_a.c | 219 ++++++++++---------------
drivers/net/wireless/b43/phy_common.c | 10 +-
drivers/net/wireless/b43/phy_common.h | 2 -
drivers/net/wireless/b43/phy_g.c | 285 ++++++++++++++------------------
drivers/net/wireless/b43/phy_n.c | 62 ++++----
drivers/net/wireless/b43/tables_nphy.c | 2 +-
drivers/net/wireless/b43/wa.c | 5 +-
9 files changed, 300 insertions(+), 453 deletions(-)
On Friday 03 October 2008 23:56:38 Michael Buesch wrote:
> On Friday 03 October 2008 23:33:04 Harvey Harrison wrote:
> > OK, but you should still look at patch 1 for the unaligned access
> > helpers.
>
> Yeah it's broken (it removes bounds checks at the end)
> and unaligned is not needed.
>
> > The rest I'll kick to the bitbucket...although I think you should look
> > at the portion of patch 5 that touches the function b43_radio_init2060
> > as it points out the _one_ place where a register is masking the value
> > of some other register...which is absolutely impossible to see unless
> > you do a series of patches like this.
> >
> > It may be that it is intentional, and it has been this way as far back
> > in the git history as I can find, but at least now it can be seen.
> >
> > - b43_radio_write16(dev, 0x0005,
> > - (b43_radio_read16(dev, 0x0081) & ~0x0008) | 0x0008);
>
> That's a bug in the dead code. We have more such bugs in the A-PHY code,
> but nobody cares.
> If you want to fix it, please read the specs and find out what should be
> here instead. Then send a patch that fixes this single line of code.
>
ah and you can also send a patch that removes all radio_read16/write16, if
you like to.
This can be proven correct by comparing the .ko sha1sum checksum before
and after applying it.
But no replacing of expressions by maskset and stuff. These can't be proven
correct (easily) and are simply not worth the danger of typo-regressions.
--
Greetings Michael.
On Friday 03 October 2008 23:33:04 Harvey Harrison wrote:
> OK, but you should still look at patch 1 for the unaligned access
> helpers.
Yeah it's broken (it removes bounds checks at the end)
and unaligned is not needed.
> The rest I'll kick to the bitbucket...although I think you should look
> at the portion of patch 5 that touches the function b43_radio_init2060
> as it points out the _one_ place where a register is masking the value
> of some other register...which is absolutely impossible to see unless
> you do a series of patches like this.
>
> It may be that it is intentional, and it has been this way as far back
> in the git history as I can find, but at least now it can be seen.
>
> - b43_radio_write16(dev, 0x0005,
> - (b43_radio_read16(dev, 0x0081) & ~0x0008) | 0x0008);
That's a bug in the dead code. We have more such bugs in the A-PHY code,
but nobody cares.
If you want to fix it, please read the specs and find out what should be
here instead. Then send a patch that fixes this single line of code.
--
Greetings Michael.
On Friday 03 October 2008 22:48:46 Harvey Harrison wrote:
> Mostly by using the mask/set helpers. Some temporary variables have
> also been eliminated.
I already told you two times that I do not like these patches.
I also explained in great detail why this is the case.
Please drop these patches!
If you send them one more time, I will immediately add your email address to
my exim blacklist.
--
Greetings Michael.