Hi Mat, On Fri, Feb 24, 2012 at 9:21 PM, Mat Martineau <mathewm@xxxxxxxxxxxxxx> wrote: > Andrei and Ulisses - > > > On 2/24/2012 9:42 AM, Ulisses Furquim wrote: >> >> Hi Andrei, >> >> On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko >> <andrei.emeltchenko.news@xxxxxxxxx> wrote: >>> >>> Hi Mat, <snip> >>> and for example here: >>> >>> - __put_control(chan, control, skb_put(skb, __ctrl_size(chan))); >>> + /* Control header is populated later */ >>> + if (test_bit(FLAG_EXT_CTRL,&chan->flags)) >>> >>> + put_unaligned_le32(0, skb_put(skb, 4)); >>> + else >>> + put_unaligned_le16(0, skb_put(skb, 2)); > > > This is the only place the macro would drop in cleanly - but it seems like a > macro should be used in more than one place unless it's hiding some really > distracting syntax. In this case, I think it's clearer to see what's going > on. If we don't have a lot of places then I'd rather see what's going on as well. > And - I forgot to fix some magic numbers in those skb_puts. This would be good to fix. >>>> - >>>> -static inline __u8 __ctrl_size(struct l2cap_chan *chan) >>>> -{ >>>> - if (test_bit(FLAG_EXT_CTRL,&chan->flags)) >>>> >>>> - return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE; >>>> - else >>>> - return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE; >>>> -} >> >> Do we have that many places with this test in Mat's code? If not, then >> we might not need to bother having all of these helpers, I think. And >> if we add them, I do think it makes sense to add them to l2cap_core.c >> than in l2cap.h, right? > > Not too many places with the FLAG_EXT_CTRL test, with use of struct > bt_control in most of the code. I would rather have helpers in l2cap_core.c > if they're needed. Agreed. Regards, -- Ulisses Furquim ProFUSION embedded systems http://profusion.mobi Mobile: +55 19 9250 0942 Skype: ulissesffs -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html