Sergei Shtylyov <[email protected]> writes: > Hello, I wrote: > >>>> Signed-off-by: Steve Chen <[email protected]> >>>> Signed-off-by: Mark Greer <[email protected]> >>>> Signed-off-by: Sergei Shtylyov <[email protected]> > >>> Mostly, it looks pretty good. Some comments below... > >>>> Index: linux-2.6/arch/arm/common/cp_intc.c >>>> =================================================================== >>>> --- /dev/null >>>> +++ linux-2.6/arch/arm/common/cp_intc.c >>>> @@ -0,0 +1,159 @@ > > [...] > >>>> +static int cp_intc_set_irq_type(unsigned int irq, unsigned int flow_type) >>>> +{ >>>> + unsigned reg = irq >> 5; > >>> minor nitpick: for a little more clarity, you can use BIT_WORD() here. > >> OK. > >>>> + unsigned mask = irq & 0x1f; > >>> maybe a #define here instead of hard-coded constant. > >> What kind of #define you'd want? This is symmetric to the above, >> i.e. it gets a bit number within the 32-bit word. I'm not seeing >> anything giving that in <linux/bitops.h>... though well, I can come >> up with something. > > When writing this, I started to suspect that this looks fishy, and > it does indeed. This code is simply wrong, it should be 1 << (irq & > 0x1f), i.e. BIT_MASK(irq). Sigh, Steve, this one I've missed... :-/ >
I thought it looked fishy as well when the value being used wasn't BITMASK(irq). Thanks for double-checking. Kevin _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
