Re: [PATCH] b43: use the bitrev helpers rather than rolling a private one

[Michael Buesch <mb@bu3sch.de>]

On Wednesday 14 May 2008 02:53:56 Harvey Harrison wrote:
> On Wed, 2008-05-14 at 01:24 +0200, Michael Buesch wrote:
> > On Tuesday 13 May 2008 21:55:18 Harvey Harrison wrote:
> > > The 4-bit reversal flip_4bit is replaced with the bitrev helper
> > > bitrev8 and a 4-bit shift.  The B43_WARN is moved to the location
> > > where a register is read from for checking there.  The other caller
> > > explicitly passes an array index which is guaranteed to be within range
> > > and so a B43_WARN is not added there.
> > > 
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > 
> > ACK
> > 
> > But I'd prefer if we had something like the following
> > and use that:
> > 
> > #define bitrev4(x)	(bitrev8(x) >> 4)
> > 
> > This way the confusing (confusing to me :) ) shifts in the code would go away.
> > I have a hard time realizing that
> > bitrev8(x) >> 3
> > does actually mean
> > bitrev4(x) << 1
> > Maybe I'm just stupid, though. :)
> > 
> 
> You're not, this was only valid because x < 16....if bit 4 could be set
> this doesn't work anymore as bitrev8(x) >> 4 would truncate bit 4
> before shifting left.  So there was some subtlety here that makes >> 3
> ok.

I don't understand. These are all 4 bit values.
The old flip helper enforced this with a B43_WARN_ON().

The >>3 actually is

((bitrev8(x) >> 4) << 1)
which is the same as
(bitrev4(x) << 1)

The <<1 is there, because the hardware wants is this way in the register.
The <<1 is not related to the actual flip operation.

> Do you want an incremental patch adding a bitrev4 to phy.c or are you
> going to add it?

Can you just respin this one instead of doing an incremental one?

-- 
Greetings Michael.