On Wed, Mar 16, 2011 at 6:05 PM, Ping Cheng <[email protected]> wrote: > Hi Chris, > > Please see my comments inline. > > Ping > > On Wed, Mar 16, 2011 at 6:43 AM, <[email protected]> wrote: >> >> From: Chris Bagwell <[email protected]> >> >> Previously, buttons and ring events from tablet pad itself >> where sent as a tool called BTN_TOOL_FINGER and that > > nitpicking, a typo. > >> >> finger event never toggled back to zero. Its serial >> number also never transition to zero. >> >> This approach had some issues in user land when switching >> between stylus and pad buttons and initial event was usually >> sent before user apps ever started. This could be improved >> by toggling BTN_TOOL_FINGER back to zero when all buttons >> released; like most other wacom drivers are doing. >> >> Instead, convert to more "standard" approach where tablet pad buttons >> are sent always without announcing them as tools. This aligns >> with newer Bamboo P&T in same file. >> >> Note: REL_WHEEL can be on both mouse tool and tablet pad. >> Userland must always treat them as coming from tablet pad to avoid >> confusion (don't ignore while mouse tool is out of proximity). > > Can we use another bit for the pad wheel, such are REL_HWHEEL so both > wheels can be reported and processed in the userland?
I thought about it but from generic point of view it doesn't help. As long as xf86-input-wacom knows make and model of tablet then yes we can say "this tablet uses REL_HWHEEL on pad and REL_WHEEL on mouse" but in generic view were we do not hard code make/model then REL_WHEEL and REL_HWHEEL are both valid mouse events with the later more for those +1/-1 movements when pushing scroll wheel sideways. In addition, we still have user land issue where BTN_LEFT/RIGHT could be a mouse of a pad. In other email, I mentioned I think its easier to treat REL_WHEEL as PAD event *always* in xf86-input-wacom. User won't know difference. > >> Signed-off-by: Chris Bagwell <[email protected]> >> --- >> drivers/input/tablet/wacom_wac.c | 148 >> ++++++++++++++++++-------------------- >> 1 files changed, 70 insertions(+), 78 deletions(-) >> >> diff --git a/drivers/input/tablet/wacom_wac.c >> b/drivers/input/tablet/wacom_wac.c >> index 74056b9..15bb104 100644 >> --- a/drivers/input/tablet/wacom_wac.c >> +++ b/drivers/input/tablet/wacom_wac.c >> @@ -195,109 +195,104 @@ static int wacom_dtu_irq(struct wacom_wac *wacom) >> static int wacom_graphire_irq(struct wacom_wac *wacom) >> { >> struct wacom_features *features = &wacom->features; >> - unsigned char *data = wacom->data; >> struct input_dev *input = wacom->input; >> - int prox; >> - int rw = 0; >> - int retval = 0; >> + unsigned char *data = wacom->data; >> + int prox = 0, x = 0, y = 0, p = 0, d = 0, rw = 0, aw = 0, >> + pen = 0, btns1 = 0, btns2 = 0, btnl = 0, btnm = 0, btnr = 0, >> + btn0 = 0, btn1 = 0, btn3 = 0, btn4 = 0; >> >> if (data[0] != WACOM_REPORT_PENABLED) { >> dbg("wacom_graphire_irq: received unknown report #%d", >> data[0]); >> - goto exit; >> + return 0; >> } >> >> prox = data[1] & 0x80; >> - if (prox || wacom->id[0]) { >> - if (prox) { >> - switch ((data[1] >> 5) & 3) { >> >> + /* >> + * firmware can send invalid data when out of proximity so >> + * send zeros to be safe. Also gives apps a fresh starting >> + * point when entering proximity. >> + */ >> + if (prox) { >> + switch ((data[1] >> 5) & 3) { >> case 0: /* Pen */ >> wacom->tool[0] = BTN_TOOL_PEN; >> - wacom->id[0] = STYLUS_DEVICE_ID; >> break; >> >> case 1: /* Rubber */ >> wacom->tool[0] = BTN_TOOL_RUBBER; >> - wacom->id[0] = ERASER_DEVICE_ID; >> break; >> >> case 2: /* Mouse with wheel */ >> - input_report_key(input, BTN_MIDDLE, >> data[1] & 0x04); > > Shouldn't we assign btnm to data[1] & 0x04 here? I'm 99.999% sure that firmware will correctly set bit 0x04 to zero on mouse that doesn't support middle button but... > > To reduce regression testing effort, do you mind we only change the lines > that we have to change? I know the code can be improved. But there will be > no new devices going through this routine. Minimal changes to fix the in/out > prox issue and to bring the routine to the generic format would require > least effort in testing. > > What do you think? The key part is finding someone that can test this. If it helps that tester then yes I've no problem with that small change. Matter of fact, I can make a patch that touches only 3 lines of today's code and fixes all known issues but keeps it protocol 4. So my goal #1 is to help get it working again with today's xf86-input-wacom. Goal #2 is move to generic protocol. If anyone will be able to do the testing, let me know and I'll provide what ever patch set is best for their setup. Chris > > Ping > >> >> - /* fall through */ >> - >> case 3: /* Mouse without wheel */ >> wacom->tool[0] = BTN_TOOL_MOUSE; >> - wacom->id[0] = CURSOR_DEVICE_ID; >> break; >> - } >> } >> - input_report_abs(input, ABS_X, le16_to_cpup((__le16 >> *)&data[2])); >> - input_report_abs(input, ABS_Y, le16_to_cpup((__le16 >> *)&data[4])); >> - if (wacom->tool[0] != BTN_TOOL_MOUSE) { >> - input_report_abs(input, ABS_PRESSURE, data[6] | >> ((data[7] & 0x01) << 8)); >> - input_report_key(input, BTN_TOUCH, data[1] & >> 0x01); >> - input_report_key(input, BTN_STYLUS, data[1] & >> 0x02); >> - input_report_key(input, BTN_STYLUS2, data[1] & >> 0x04); >> - } else { >> - input_report_key(input, BTN_LEFT, data[1] & 0x01); >> - input_report_key(input, BTN_RIGHT, data[1] & >> 0x02); >> + >> + x = le16_to_cpup((__le16 *)&data[2]); >> + y = le16_to_cpup((__le16 *)&data[4]); >> + >> + if (wacom->tool[0] == BTN_TOOL_MOUSE ) { >> if (features->type == WACOM_G4 || >> - features->type == WACOM_MO) { >> - input_report_abs(input, ABS_DISTANCE, >> data[6] & 0x3f); >> + features->type == WACOM_MO) { >> + d = data[6] & 0x3f; >> rw = (data[7] & 0x04) - (data[7] & 0x03); >> } else { >> - input_report_abs(input, ABS_DISTANCE, >> data[7] & 0x3f); >> + d = data[7] & 0x3f; >> rw = -(signed char)data[6]; >> } >> - input_report_rel(input, REL_WHEEL, rw); >> + btnl = data[1] & 0x01; >> + btnr = data[1] & 0x02; >> + btnm = data[1] & 0x04; >> + } else { >> + p = data[6] | ((data[7] & 0x01) << 8); >> + pen = data[1] & 0x01; >> + btns1 = data[1] & 0x02; >> + btns2 = data[1] & 0x04; >> } >> - >> - if (!prox) >> - wacom->id[0] = 0; >> - input_report_abs(input, ABS_MISC, wacom->id[0]); /* report >> tool id */ >> - input_report_key(input, wacom->tool[0], prox); >> - input_sync(input); /* sync last event */ >> } >> >> - /* send pad data */ >> - switch (features->type) { >> - case WACOM_G4: >> - prox = data[7] & 0xf8; >> - if (prox || wacom->id[1]) { >> - wacom->id[1] = PAD_DEVICE_ID; >> - input_report_key(input, BTN_0, (data[7] & 0x40)); >> - input_report_key(input, BTN_4, (data[7] & 0x80)); >> + /* Buttons and wheels on tablet are always reported */ >> + if (features->type == WACOM_G4) { >> + btn0 = data[7] & 0x40; >> + btn1 = data[7] & 0x80; >> + /* >> + * Mouse wheel and tablet wheel share event but mouse >> + * event is only valid when tool is in proximity. >> + * When both are in use, give priority to mouse. >> + */ >> + if (!rw) >> rw = ((data[7] & 0x18) >> 3) - ((data[7] & 0x20) >> >> 3); >> - input_report_rel(input, REL_WHEEL, rw); >> - input_report_key(input, BTN_TOOL_FINGER, 0xf0); >> - if (!prox) >> - wacom->id[1] = 0; >> - input_report_abs(input, ABS_MISC, wacom->id[1]); >> - input_event(input, EV_MSC, MSC_SERIAL, 0xf0); >> - retval = 1; >> - } >> - break; >> - >> - case WACOM_MO: >> - prox = (data[7] & 0xf8) || data[8]; >> - if (prox || wacom->id[1]) { >> - wacom->id[1] = PAD_DEVICE_ID; >> - input_report_key(input, BTN_0, (data[7] & 0x08)); >> - input_report_key(input, BTN_1, (data[7] & 0x20)); >> - input_report_key(input, BTN_4, (data[7] & 0x10)); >> - input_report_key(input, BTN_5, (data[7] & 0x40)); >> - input_report_abs(input, ABS_WHEEL, (data[8] & >> 0x7f)); >> - input_report_key(input, BTN_TOOL_FINGER, 0xf0); >> - if (!prox) >> - wacom->id[1] = 0; >> - input_report_abs(input, ABS_MISC, wacom->id[1]); >> - input_event(input, EV_MSC, MSC_SERIAL, 0xf0); >> - } >> - retval = 1; >> - break; >> + } else if (features->type == WACOM_MO) { >> + btn0 = data[7] & 0x08; >> + btn1 = data[7] & 0x10; >> + btn3 = data[7] & 0x20; >> + btn4 = data[7] & 0x40; >> + aw = data[8] & 0x7f; >> } >> -exit: >> - return retval; >> + >> + input_report_key(input, BTN_TOUCH, pen); >> + input_report_key(input, BTN_STYLUS, btns1); >> + input_report_key(input, BTN_STYLUS2, btns2); >> + input_report_key(input, BTN_LEFT, btnl); >> + input_report_key(input, BTN_MIDDLE, btnm); >> + input_report_key(input, BTN_RIGHT, btnr); >> + input_report_key(input, BTN_0, btn0); >> + input_report_key(input, BTN_1, btn1); >> + input_report_key(input, BTN_3, btn3); >> + input_report_key(input, BTN_4, btn4); >> + >> + input_report_abs(input, ABS_X, x); >> + input_report_abs(input, ABS_Y, y); >> + input_report_abs(input, ABS_PRESSURE, p); >> + input_report_abs(input, ABS_DISTANCE, d); >> + input_report_abs(input, ABS_WHEEL, aw); >> + >> + input_report_rel(input, REL_WHEEL, rw); >> + >> + input_report_key(input, wacom->tool[0], prox); >> + >> + return 1; >> } >> >> static int wacom_intuos_inout(struct wacom_wac *wacom) >> @@ -1055,18 +1050,15 @@ void wacom_setup_input_capabilities(struct >> input_dev *input_dev, >> >> switch (wacom_wac->features.type) { >> case WACOM_MO: >> - __set_bit(BTN_1, input_dev->keybit); >> - __set_bit(BTN_5, input_dev->keybit); >> + __set_bit(BTN_3, input_dev->keybit); >> + __set_bit(BTN_4, input_dev->keybit); >> >> input_set_abs_params(input_dev, ABS_WHEEL, 0, 71, 0, 0); >> /* fall through */ >> >> case WACOM_G4: >> - input_set_capability(input_dev, EV_MSC, MSC_SERIAL); >> - >> - __set_bit(BTN_TOOL_FINGER, input_dev->keybit); >> __set_bit(BTN_0, input_dev->keybit); >> - __set_bit(BTN_4, input_dev->keybit); >> + __set_bit(BTN_1, input_dev->keybit); >> /* fall through */ >> >> case GRAPHIRE: >> -- >> 1.7.4 >> >> >> >> ------------------------------------------------------------------------------ >> Colocation vs. Managed Hosting >> A question and answer guide to determining the best fit >> for your organization - today and in the future. >> http://p.sf.net/sfu/internap-sfd2d >> _______________________________________________ >> Linuxwacom-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel > > ------------------------------------------------------------------------------ Colocation vs. Managed Hosting A question and answer guide to determining the best fit for your organization - today and in the future. http://p.sf.net/sfu/internap-sfd2d _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
