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@xxxxxxxxx> > > > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
