[PATCH 2/3 v2] staging: bcm: Bcmchar.c - Checkpatch fixes

Dan Carpenter dan.carpenter at oracle.com
Sun Oct 28 17:09:22 UTC 2012


On Sat, Oct 27, 2012 at 11:16:54PM +0100, Ceri James wrote:
> >>@@ -985,11 +992,13 @@ cntrlEnd:
> >>  		if (uiData) {
> >>  			/* Allow All Packets */
> >>-			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, "IOCTL_BCM_SWITCH_TRANSFER_MODE: ETH_PACKET_TUNNELING_MODE\n");
> >>-				Adapter->TransferMode = ETH_PACKET_TUNNELING_MODE;
> >>+			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
> >>+					"IOCTL_BCM_SWITCH_TRANSFER_MODE: ETH_PACKET_TUNNELING_MODE\n");
> >>+					Adapter->TransferMode = ETH_PACKET_TUNNELING_MODE;
> >                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >This is indented too far now.  It's better to wait over night and
> >re-review your changes the next morning before resending.
> This is not a hasty mistake, it is not knowing the correct/expected
> indentation rules :)
> So the correct indentation is as below?
> >+			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
> >+				"IOCTL_BCM_SWITCH_TRANSFER_MODE: ETH_PACKET_TUNNELING_MODE\n");
> >+				Adapter->TransferMode = ETH_PACKET_TUNNELING_MODE;

No.  It's a new statement.  It should be:
			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
				"IOCTL_BCM_SWITCH_TRANSFER_MODE: ETH_PACKET_TUNNELING_MODE\n");
			Adapter->TransferMode = ETH_PACKET_TUNNELING_MODE;
> >>@@ -1397,7 +1423,9 @@ cntrlEnd:
> >>  		}
> >>  		do_gettimeofday(&tv1);
> >>-		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, " timetaken by Write/read :%ld msec\n", (tv1.tv_sec - tv0.tv_sec)*1000 + (tv1.tv_usec - tv0.tv_usec)/1000);
> >>+		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
> >>+				" timetaken by Write/read :%ld msec\n",
> >>+				(tv1.tv_sec - tv0.tv_sec)*1000 + (tv1.tv_usec - tv0.tv_usec)/1000);
> >We would normally put spaces around the multiply and divide as well.
> >
> >				(tv1.tv_sec - tv0.tv_sec) * 1000 + (tv1.tv_usec - tv0.tv_usec) / 1000);
> Fair enough, I may have missed this in the checkpatch output. I will
> double check.

Checkpatch.pl doesn't warn about these.  Normally I would ignore
them if you didn't introduce it, but since I was already commenting
on stuff I figured I may as well mention it.

regards,
dan carpenter




More information about the devel mailing list