On Mon, 2014-10-27 at 07:44 -0700, Dmitry Torokhov wrote: > Hi Joe, Hello Dmitry. > On Sun, Oct 26, 2014 at 10:24:59PM -0700, Joe Perches wrote: > > Precedence of & and >> is not the same and is not left to right. > > shift has higher precedence and should be done after the mask. > > Looking at the protocol description the current code is exactly right. > We want to "move" button bits first as in packet type 1 they are in a > different place than in other packets. > > I'll take a patch that adds parenthesis around shifts to make clear it > is intended. I think it's more sensible to do the shift first to a temporary then direct comparisons. Perhaps something like this cleanup that also uses a temporary for aiptek->curSetting and !!(foo & mask) instead of ((foo & mask) != 0) ? 1 : 0; --- drivers/input/tablet/aiptek.c | 149 ++++++++++++++++++++---------------------- 1 file changed, 72 insertions(+), 77 deletions(-) diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c index e7f966d..9c46618 100644 --- a/drivers/input/tablet/aiptek.c +++ b/drivers/input/tablet/aiptek.c @@ -433,6 +433,7 @@ static const char *map_val_to_str(const struct aiptek_map *map, int val) static void aiptek_irq(struct urb *urb) { struct aiptek *aiptek = urb->context; + struct aiptek_settings *settings = &aiptek->curSetting; unsigned char *data = aiptek->data; struct input_dev *inputdev = aiptek->inputdev; struct usb_interface *intf = aiptek->intf; @@ -472,26 +473,31 @@ static void aiptek_irq(struct urb *urb) * tool generated the event. */ if (data[0] == 1) { - if (aiptek->curSetting.coordinateMode == + if (settings->coordinateMode == AIPTEK_COORDINATE_ABSOLUTE_MODE) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_RELATIVE_IN_ABSOLUTE; } else { - x = (signed char) data[2]; - y = (signed char) data[3]; - - /* jitterable keeps track of whether any button has been pressed. - * We're also using it to remap the physical mouse button mask - * to pseudo-settings. (We don't specifically care about it's - * value after moving/transposing mouse button bitmasks, except + /* + * Shift buttons pressed to the curSettings location. + * jitterable keeps track of whether any button has + * been pressed. We're also using it to remap the + * physical mouse button mask to pseudo-settings. + * (We don't specifically care about it's value after + * moving/transposing mouse button bitmasks, except * that a non-zero value indicates that one or more * mouse button was pressed.) */ + unsigned char buttons = data[1] << 2; + + x = (signed char)data[2]; + y = (signed char)data[3]; + jitterable = data[1] & 0x07; - left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0; - right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0; - middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0; + left = !!(buttons & settings->mouseButtonLeft); + right = !!(buttons & settings->mouseButtonRight); + middle = !!(buttons & settings->mouseButtonMiddle); input_report_key(inputdev, BTN_LEFT, left); input_report_key(inputdev, BTN_MIDDLE, middle); @@ -505,10 +511,10 @@ static void aiptek_irq(struct urb *urb) /* Wheel support is in the form of a single-event * firing. */ - if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) { + if (settings->wheel != AIPTEK_WHEEL_DISABLE) { input_report_rel(inputdev, REL_WHEEL, - aiptek->curSetting.wheel); - aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE; + settings->wheel); + settings->wheel = AIPTEK_WHEEL_DISABLE; } if (aiptek->lastMacro != -1) { input_report_key(inputdev, @@ -522,26 +528,26 @@ static void aiptek_irq(struct urb *urb) * absolute coordinates. */ else if (data[0] == 2) { - if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { + if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE; } else if (!AIPTEK_POINTER_ALLOW_STYLUS_MODE - (aiptek->curSetting.pointerMode)) { + (settings->pointerMode)) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED; } else { x = get_unaligned_le16(data + 1); y = get_unaligned_le16(data + 3); z = get_unaligned_le16(data + 6); - dv = (data[5] & 0x01) != 0 ? 1 : 0; - p = (data[5] & 0x02) != 0 ? 1 : 0; - tip = (data[5] & 0x04) != 0 ? 1 : 0; + dv = !!(data[5] & 0x01); + p = !!(data[5] & 0x02); + tip = !!(data[5] & 0x04); /* Use jitterable to re-arrange button masks */ jitterable = data[5] & 0x18; - bs = (data[5] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0; - pck = (data[5] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0; + bs = !!(data[5] & settings->stylusButtonLower); + pck = !!(data[5] & settings->stylusButtonUpper); /* dv indicates 'data valid' (e.g., the tablet is in sync * and has delivered a "correct" report) We will ignore @@ -552,14 +558,14 @@ static void aiptek_irq(struct urb *urb) * tool key, and set the new one. */ if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, + settings->toolMode, 1); aiptek->previousToolMode = - aiptek->curSetting.toolMode; + settings->toolMode; } if (p != 0) { @@ -571,27 +577,25 @@ static void aiptek_irq(struct urb *urb) input_report_key(inputdev, BTN_STYLUS, bs); input_report_key(inputdev, BTN_STYLUS2, pck); - if (aiptek->curSetting.xTilt != - AIPTEK_TILT_DISABLE) { + if (settings->xTilt != AIPTEK_TILT_DISABLE) { input_report_abs(inputdev, ABS_TILT_X, - aiptek->curSetting.xTilt); + settings->xTilt); } - if (aiptek->curSetting.yTilt != AIPTEK_TILT_DISABLE) { + if (settings->yTilt != AIPTEK_TILT_DISABLE) { input_report_abs(inputdev, ABS_TILT_Y, - aiptek->curSetting.yTilt); + settings->yTilt); } /* Wheel support is in the form of a single-event * firing. */ - if (aiptek->curSetting.wheel != - AIPTEK_WHEEL_DISABLE) { + if (settings->wheel != AIPTEK_WHEEL_DISABLE) { input_report_abs(inputdev, ABS_WHEEL, - aiptek->curSetting.wheel); - aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE; + settings->wheel); + settings->wheel = AIPTEK_WHEEL_DISABLE; } } input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_STYLUS); @@ -607,10 +611,10 @@ static void aiptek_irq(struct urb *urb) /* Report 3's come from the mouse in absolute mode. */ else if (data[0] == 3) { - if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { + if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE; } else if (!AIPTEK_POINTER_ALLOW_MOUSE_MODE - (aiptek->curSetting.pointerMode)) { + (settings->pointerMode)) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED; } else { x = get_unaligned_le16(data + 1); @@ -618,25 +622,25 @@ static void aiptek_irq(struct urb *urb) jitterable = data[5] & 0x1c; - dv = (data[5] & 0x01) != 0 ? 1 : 0; - p = (data[5] & 0x02) != 0 ? 1 : 0; - left = (data[5] & aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0; - right = (data[5] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0; - middle = (data[5] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0; + dv = !!(data[5] & 0x01); + p = !!(data[5] & 0x02); + left = !!(data[5] & settings->mouseButtonLeft); + right = !!(data[5] & settings->mouseButtonRight); + middle = !!(data[5] & settings->mouseButtonMiddle); if (dv != 0) { /* If the selected tool changed, reset the old * tool key, and set the new one. */ if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, + settings->toolMode, 1); aiptek->previousToolMode = - aiptek->curSetting.toolMode; + settings->toolMode; } if (p != 0) { @@ -650,11 +654,11 @@ static void aiptek_irq(struct urb *urb) /* Wheel support is in the form of a single-event * firing. */ - if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) { + if (settings->wheel != AIPTEK_WHEEL_DISABLE) { input_report_abs(inputdev, ABS_WHEEL, - aiptek->curSetting.wheel); - aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE; + settings->wheel); + settings->wheel = AIPTEK_WHEEL_DISABLE; } } input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_MOUSE); @@ -672,11 +676,11 @@ static void aiptek_irq(struct urb *urb) else if (data[0] == 4) { jitterable = data[1] & 0x18; - dv = (data[1] & 0x01) != 0 ? 1 : 0; - p = (data[1] & 0x02) != 0 ? 1 : 0; - tip = (data[1] & 0x04) != 0 ? 1 : 0; - bs = (data[1] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0; - pck = (data[1] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0; + dv = !!(data[1] & 0x01); + p = !!(data[1] & 0x02); + tip = !!(data[1] & 0x04); + bs = !!(data[1] & settings->stylusButtonLower); + pck = !!(data[1] & settings->stylusButtonUpper); macro = dv && p && tip && !(data[3] & 1) ? (data[3] >> 1) : -1; z = get_unaligned_le16(data + 4); @@ -685,15 +689,12 @@ static void aiptek_irq(struct urb *urb) /* If the selected tool changed, reset the old * tool key, and set the new one. */ - if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + if (aiptek->previousToolMode != settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, - 1); - aiptek->previousToolMode = - aiptek->curSetting.toolMode; + settings->toolMode, 1); + aiptek->previousToolMode = settings->toolMode; } } @@ -715,24 +716,23 @@ static void aiptek_irq(struct urb *urb) else if (data[0] == 5) { jitterable = data[1] & 0x1c; - dv = (data[1] & 0x01) != 0 ? 1 : 0; - p = (data[1] & 0x02) != 0 ? 1 : 0; - left = (data[1]& aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0; - right = (data[1] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0; - middle = (data[1] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0; + dv = !!(data[1] & 0x01); + p = !!(data[1] & 0x02); + left = !!(data[1]& settings->mouseButtonLeft); + right = !!(data[1] & settings->mouseButtonRight); + middle = !!(data[1] & settings->mouseButtonMiddle); macro = dv && p && left && !(data[3] & 1) ? (data[3] >> 1) : 0; if (dv) { /* If the selected tool changed, reset the old * tool key, and set the new one. */ - if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + if (aiptek->previousToolMode != settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, 1); - aiptek->previousToolMode = aiptek->curSetting.toolMode; + settings->toolMode, 1); + aiptek->previousToolMode = settings->toolMode; } } @@ -770,15 +770,10 @@ static void aiptek_irq(struct urb *urb) /* If the selected tool changed, reset the old tool key, and set the new one. */ - if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { - input_report_key(inputdev, - aiptek->previousToolMode, 0); - input_report_key(inputdev, - aiptek->curSetting.toolMode, - 1); - aiptek->previousToolMode = - aiptek->curSetting.toolMode; + if (aiptek->previousToolMode != settings->toolMode) { + input_report_key(inputdev, aiptek->previousToolMode, 0); + input_report_key(inputdev, settings->toolMode, 1); + aiptek->previousToolMode = settings->toolMode; } input_report_key(inputdev, macroKeyEvents[macro], 1); @@ -802,9 +797,9 @@ static void aiptek_irq(struct urb *urb) */ if (aiptek->previousJitterable != jitterable && - aiptek->curSetting.jitterDelay != 0 && aiptek->inDelay != 1) { + settings->jitterDelay != 0 && aiptek->inDelay != 1) { aiptek->endDelay = jiffies + - ((aiptek->curSetting.jitterDelay * HZ) / 1000); + ((settings->jitterDelay * HZ) / 1000); aiptek->inDelay = 1; } aiptek->previousJitterable = jitterable; -- 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