On Mon, Feb 08, 2010 at 04:41:16PM +0900, Magnus Damm wrote: > Hi Dmitry, > > On Mon, Feb 8, 2010 at 4:18 PM, Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx> wrote: > > On Mon, Feb 08, 2010 at 03:32:34PM +0900, Magnus Damm wrote: > >> > >> +#define WRAP(fn, m...) bitmap_##fn(m, SH_KEYSC_MAXKEYS) > >> +#define sh_keysc_map_zero(m) WRAP(zero, (m)->b) > >> +#define sh_keysc_map_fill(m) WRAP(fill, (m)->b) > >> +#define sh_keysc_map_and(m, m2) WRAP(and, (m)->b, (m)->b, (m2)->b) > >> +#define sh_keysc_map_or(m, m2) WRAP(or, (m)->b, (m)->b, (m2)->b) > >> +#define sh_keysc_map_complement(m) WRAP(complement, (m)->b, (m)->b) > >> +#define sh_keysc_map_set(m, n) set_bit((n), (m)->b) > >> +#define sh_keysc_map_clear(m, n) clear_bit((n), (m)->b) > >> +#define sh_keysc_map_test(m, n) test_bit((n), (m)->b) > >> + > > > > Why do you need these wrappers? For me they simply create a distraction, > > later when I read the code I will have to go and look up what > > sh_keysc_map_set() means but if I see __set_bit() I'd know right away. > > To avoid duplicating SH_KEYSC_MAXKEYS all over the place I started out > by wrapping bitmap_zero/fill/and/or/complement(). To be consistent I > decided to wrap the set/clear/test_bit() functions as well, but it may > be better to leave them alone. > > Are you ok with wrapping the bitmap_...() functions to avoid > duplicating SH_KEYSC_MAXKEYS? > I don't see the point, either. The bitmap routines take the maximum size of the bitmap as an argument, and so code that uses the API heavily and has the size defined in a macro is going to have it sprinkled around, who cares? Wrapping up the bitmap API in your own wrappers to hide this fact only makes it more confusing and really doesn't buy you anything. While things like sh_keysc_map_set/clear() might seem obvious enough, it's also impossible to tell whether these employ __set_bit()/__clear_bit() or set_bit()/clear_bit() semantics without having to scroll back and look them up. The added obfuscation to save yourself from having to sprinkle SH_KEYSC_MAXKEYS around simply isn't worth it. Any time you start out wrapping an existing API for private driver use outside of things like I/O accessors, you're almost certainly going to run in to trouble. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
