All of lore.kernel.org
 help / color / mirror / Atom feed
From: Harvey Harrison <harvey.harrison@gmail.com>
To: Michael Buesch <mb@bu3sch.de>
Cc: linux-wireless <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH 3/8] b43: remove b43_radio_{read|write}16 from lo.c
Date: Fri, 03 Oct 2008 14:33:04 -0700	[thread overview]
Message-ID: <1223069584.6512.42.camel@brick> (raw)
In-Reply-To: <200810032317.19971.mb@bu3sch.de>

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(-)


  reply	other threads:[~2008-10-03 21:33 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-10-03 20:48 [PATCH 3/8] b43: remove b43_radio_{read|write}16 from lo.c Harvey Harrison
2008-10-03 21:17 ` Michael Buesch
2008-10-03 21:33   ` Harvey Harrison [this message]
2008-10-03 21:56     ` Michael Buesch
2008-10-03 22:35       ` Michael Buesch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1223069584.6512.42.camel@brick \
    --to=harvey.harrison@gmail.com \
    --cc=linux-wireless@vger.kernel.org \
    --cc=mb@bu3sch.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.