On Mon, Oct 31, 2011 at 7:02 AM, Przemo Firszt <prz...@firszt.eu> wrote: > Dnia 2011-10-24, pon o godzinie 10:52 -0700, Ping Cheng pisze: >> On Sun, Oct 23, 2011 at 5:17 AM, Przemo Firszt <prz...@firszt.eu> wrote: >> > >> > Next part of the I4 WL driver attached - this time pen button reporting. >> >> This is part of I4 WL support. We should merge it into the other patch. > > Ping, > Can you cofirm that your "Acked-by" stil holds (merged version)? I want > to submit this to linux-input.
HI Przemo, I thought maybe you were going to have a few more patches on linuxwacom before forwarding... so I hadn't previously given any nitpicking comments. Here they are... For most part, you can ignore these and send to linux-input but you may get same comments from others there. 1) First, can you please try to get "git send-email" (or some similar solution that doesn't have tab issues) working so that the patches are not attachments? Its very difficult to provide review comments for attachments. I don't think on linux-input people will bother to review attachments. 2) wacom_i4_parse_pen_report() + if (pressure > 1) + input_report_key(input, BTN_TOUCH, 1); + else + input_report_key(input, BTN_TOUCH, 0); + I think there is a preference on linux-input to make that a single statement: input_report_key(input, BTN_TOUCH, pressure > 1); 3) wacom_raw_event() In wacom_raw_event(), I think majority of change are whitespace only as you've shifted graphire logic inside a case statement. It would be much easier for reviewer if you broke out the graphire rearrangement part as its own patch and then have a I4 patch after that. In addition, its probably best to move the graphire parsing logic to its own function; like you've done for I4. If you did that, then there may not even be a whitespace change anymore showing up in diff report and would be even easier to review. 4) wacom_input_mapped() I'd break the removal of this function into its own patch since its not entirely related to I4 support and to make easier to review. Chris ------------------------------------------------------------------------------ Get your Android app more play: Bring it to the BlackBerry PlayBook in minutes. BlackBerry App World™ now supports Android™ Apps for the BlackBerry® PlayBook™. Discover just how easy and simple it is! http://p.sf.net/sfu/android-dev2dev _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
