[PATCH 00/11] treewide: mask then shift defects and style updates

linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 00/11] treewide: mask then shift defects and style updates
@ 2014-10-27  5:24 Joe Perches
  2014-10-27  5:24 ` [PATCH 03/11] aiptek: Fix probable mask then right shift defects Joe Perches
  0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2014-10-27  5:24 UTC (permalink / raw)
  To: linux-kernel, linux-nvme, dri-devel, ivtv-devel, linux-media,
	patches, netdev, linux-usb, linux-parisc
  Cc: alsa-devel, linux-wireless, linux-input

logical mask has lower precedence than shift but should be
done before the shift so parentheses are generally required.

And when masking with a fixed value after a shift, normal kernel
style has the shift on the left, then the shift on the right so
convert a few non-conforming uses.

Joe Perches (11):
  block: nvme-scsi: Fix probable mask then right shift defects
  radeon: evergreen: Fix probable mask then right shift defects
  aiptek: Fix probable mask then right shift defects
  dvb-net: Fix probable mask then right shift defects
  cx25840/cx18: Use standard ordering of mask and shift
  wm8350-core: Fix probable mask then right shift defect
  iwlwifi: dvm: Fix probable mask then right shift defect
  ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect
  tty: ipwireless: Fix probable mask then right shift defects
  hwa-hc: Fix probable mask then right shift defect
  sound: ad1889: Fix probable mask then right shift defects

 drivers/block/nvme-scsi.c                | 12 ++++++------
 drivers/gpu/drm/radeon/evergreen.c       |  3 ++-
 drivers/input/tablet/aiptek.c            |  6 +++---
 drivers/media/dvb-core/dvb_net.c         |  4 +++-
 drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
 drivers/media/pci/cx18/cx18-av-core.c    | 16 ++++++++--------
 drivers/mfd/wm8350-core.c                |  2 +-
 drivers/net/wireless/iwlwifi/dvm/lib.c   |  4 ++--
 drivers/ssb/driver_chipcommon_pmu.c      |  4 ++--
 drivers/tty/ipwireless/hardware.c        | 12 ++++++------
 drivers/usb/host/hwa-hc.c                |  2 +-
 sound/pci/ad1889.c                       |  8 ++++----
 12 files changed, 44 insertions(+), 41 deletions(-)

-- 
2.1.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 03/11] aiptek: Fix probable mask then right shift defects
  2014-10-27  5:24 [PATCH 00/11] treewide: mask then shift defects and style updates Joe Perches
@ 2014-10-27  5:24 ` Joe Perches
  2014-10-27 14:44   ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2014-10-27  5:24 UTC (permalink / raw)
  To: linux-kernel; +Cc: Dmitry Torokhov, linux-input

Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.

Here the shifts are unnecessary and a non-zero value can be used
as the test to set 1 or zero.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/input/tablet/aiptek.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
index e7f966d..dee2bb9 100644
--- a/drivers/input/tablet/aiptek.c
+++ b/drivers/input/tablet/aiptek.c
@@ -489,9 +489,9 @@ static void aiptek_irq(struct urb *urb)
 			 */
 			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 = data[1] & aiptek->curSetting.mouseButtonLeft ? 1 : 0;
+			right = data[1] & aiptek->curSetting.mouseButtonRight ? 1 : 0;
+			middle = data[1] & aiptek->curSetting.mouseButtonMiddle ? 1 : 0;
 
 			input_report_key(inputdev, BTN_LEFT, left);
 			input_report_key(inputdev, BTN_MIDDLE, middle);
-- 
2.1.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects
  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
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2014-10-27 14:44 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel, linux-input

Hi Joe,

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.

Thanks.

> 
> Here the shifts are unnecessary and a non-zero value can be used
> as the test to set 1 or zero.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/input/tablet/aiptek.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
> index e7f966d..dee2bb9 100644
> --- a/drivers/input/tablet/aiptek.c
> +++ b/drivers/input/tablet/aiptek.c
> @@ -489,9 +489,9 @@ static void aiptek_irq(struct urb *urb)
>  			 */
>  			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 = data[1] & aiptek->curSetting.mouseButtonLeft ? 1 : 0;
> +			right = data[1] & aiptek->curSetting.mouseButtonRight ? 1 : 0;
> +			middle = data[1] & aiptek->curSetting.mouseButtonMiddle ? 1 : 0;
>  
>  			input_report_key(inputdev, BTN_LEFT, left);
>  			input_report_key(inputdev, BTN_MIDDLE, middle);
> -- 
> 2.1.2
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects
  2014-10-27 14:44   ` Dmitry Torokhov
@ 2014-10-27 17:56     ` Joe Perches
  2014-10-27 18:01       ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2014-10-27 17:56 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-kernel, linux-input

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;



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects
  2014-10-27 17:56     ` Joe Perches
@ 2014-10-27 18:01       ` Dmitry Torokhov
  2014-10-27 18:03         ` Joe Perches
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2014-10-27 18:01 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel, linux-input

On Monday, October 27, 2014 10:56:54 AM Joe Perches wrote:
> 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;

Unless you have the device I'd rather kept the changes (which are mostly
cosmetic in nature and do not fix any bugs) to a minimum.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects
  2014-10-27 18:01       ` Dmitry Torokhov
@ 2014-10-27 18:03         ` Joe Perches
  0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2014-10-27 18:03 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-kernel, linux-input

On Mon, 2014-10-27 at 11:01 -0700, Dmitry Torokhov wrote:
> On Monday, October 27, 2014 10:56:54 AM Joe Perches wrote:
> > On Mon, 2014-10-27 at 07:44 -0700, Dmitry Torokhov wrote:
> > > 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.
[]
> Unless you have the device I'd rather kept the changes (which are mostly
> cosmetic in nature and do not fix any bugs) to a minimum.

I don't have the device.
I think you should do what you think appropriate.

cheers, Joe

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-10-27 18:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-27  5:24 [PATCH 00/11] treewide: mask then shift defects and style updates Joe Perches
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
2014-10-27 18:01       ` Dmitry Torokhov
2014-10-27 18:03         ` Joe Perches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).