From: Joe Perches <joe@perches.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
Subject: Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects
Date: Mon, 27 Oct 2014 10:56:54 -0700 [thread overview]
Message-ID: <1414432614.18896.1.camel@perches.com> (raw)
In-Reply-To: <20141027144411.GA7594@dtor-ws>
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;
next prev parent reply other threads:[~2014-10-27 17:56 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-27 5:24 [PATCH 00/11] treewide: mask then shift defects and style updates Joe Perches
2014-10-27 5:24 ` [PATCH 01/11] block: nvme-scsi: Fix probable mask then right shift defects Joe Perches
2014-11-10 19:33 ` Verma, Vishal L
2014-10-27 5:24 ` [PATCH 02/11] radeon: evergreen: " Joe Perches
2014-10-27 9:14 ` Michel Dänzer
2014-10-27 14:14 ` [PATCH 02/11 V2] radeon: evergreen: Fix probable mask then right shift defect Joe Perches
2014-10-28 3:42 ` Michel Dänzer
2014-10-28 14:06 ` Alex Deucher
2014-10-29 3:03 ` Michel Dänzer
2014-10-27 5:24 ` [PATCH 03/11] aiptek: Fix probable mask then right shift defects Joe Perches
2014-10-27 14:44 ` Dmitry Torokhov
2014-10-27 17:56 ` Joe Perches [this message]
2014-10-27 18:01 ` Dmitry Torokhov
2014-10-27 18:03 ` Joe Perches
2014-10-27 5:25 ` [PATCH 04/11] dvb-net: " Joe Perches
2014-10-27 5:25 ` [PATCH 05/11] cx25840/cx18: Use standard ordering of mask and shift Joe Perches
2014-11-08 13:41 ` Andy Walls
2014-10-27 5:25 ` [PATCH 06/11] wm8350-core: Fix probable mask then right shift defect Joe Perches
2014-10-27 12:47 ` Charles Keepax
2014-11-03 18:02 ` Lee Jones
2014-10-27 5:25 ` [PATCH 07/11] iwlwifi: dvm: " Joe Perches
2014-10-27 6:14 ` Grumbach, Emmanuel
2014-10-27 5:25 ` [PATCH 08/11] ssb: driver_chip_comon_pmu: " Joe Perches
2014-10-27 17:32 ` Michael Büsch
2014-10-27 5:25 ` [PATCH 09/11] tty: ipwireless: Fix probable mask then right shift defects Joe Perches
2014-10-27 13:18 ` David Sterba
2014-10-27 14:17 ` Jiri Kosina
2014-10-28 1:57 ` Greg Kroah-Hartman
2014-10-27 5:25 ` [PATCH 10/11] hwa-hc: Fix probable mask then right shift defect Joe Perches
2014-10-27 5:25 ` [PATCH 11/11] sound: ad1889: Fix probable mask then right shift defects Joe Perches
2014-10-27 7:41 ` Takashi Iwai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1414432614.18896.1.camel@perches.com \
--to=joe@perches.com \
--cc=dmitry.torokhov@gmail.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome