Linux kernel patch

Linux kernel patch

[Steve Cave]

Please find enclosed a patch (my first) to fix checkpatch.pl issues with the
 usbduxfast.c in the comedi driver. Please note that there is one outstanding
issue, with semaphores and mutex, that I do not feel qualified to patch.

Regards

Steve Cave

---
>From 93c6eae8685fe87c4796e5e177ea136a155bfe2f Mon Sep 17 00:00:00 
2001
From: Steve Cave <steve.cave@ntlworld.com>
Date: Wed, 17 Mar 2010 20:58:35 +0000
Subject: [PATCH] Staging: comedi: fix line lengths and indentation in 
usbduxfast.c

This is a patch to the usbduxfast.c file that reformats and breaks up a
number of code lines greater than 80 characters long, standardizing
indents and replacing some spaces with tabs to fix warnings found by the
checkpatch tool; also replacing a foo * bar with foo *bar to fix an error
found by checkpatch

Signed-off-by: Steve Cave <steve.cave@ntlworld.com>
---
 drivers/staging/comedi/drivers/usbduxfast.c |   66 
++++++++++++++-------------
 1 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbduxfast.c 
b/drivers/staging/comedi/drivers/usbduxfast.c
index e89b818..b407bc0 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -177,8 +177,8 @@ struct usbduxfastsub_s {
 	int16_t *insnBuffer;	/* input buffer for single insn */
 	int ifnum;		/* interface number */
 	struct usb_interface *interface;	/* interface structure */
-	struct comedi_device *comedidev;	/* comedi device for the 
interrupt
-						   context */
+	struct comedi_device *comedidev;	/* comedi device for the
+						   interrupt context */
 	short int ai_cmd_running;	/* asynchronous command is running */
 	short int ai_continous;	/* continous aquisition */
 	long int ai_sample_count;	/* number of samples to acquire */
@@ -271,7 +271,8 @@ static int usbduxfast_ai_stop(struct 
usbduxfastsub_s *udfs, int do_unlink)
 	udfs->ai_cmd_running = 0;
 
 	if (do_unlink)
-		ret = usbduxfastsub_unlink_InURBs(udfs);	/* stop aquistion */
+		ret = usbduxfastsub_unlink_InURBs(udfs);
+						/* stop acquistion */
 
 	return ret;
 }
@@ -451,13 +452,14 @@ static int usbduxfastsub_start(struct 
usbduxfastsub_s *udfs)
 
 	/* 7f92 to zero */
 	local_transfer_buffer[0] = 0;
-	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
0), USBDUXFASTSUB_FIRMWARE,	/* bRequest, "Firmware" */
-			      VENDOR_DIR_OUT,	/* bmRequestType */
-			      USBDUXFASTSUB_CPUCS,	/* Value */
-			      0x0000,	/* Index */
-			      local_transfer_buffer,	/* address of the transfer buffer 
*/
-			      1,	/* Length */
-			      EZTIMEOUT);	/* Timeout */
+	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
0),
+			USBDUXFASTSUB_FIRMWARE,	/* bRequest, 
"Firmware" */
+			VENDOR_DIR_OUT,	/* bmRequestType */
+			USBDUXFASTSUB_CPUCS,	/* Value */
+			0x0000,	/* Index */
+			local_transfer_buffer,	/* transfer buffer address  */
+			1,	/* Length */
+			EZTIMEOUT);	/* Timeout */
 	if (ret < 0) {
 		printk("comedi_: usbduxfast_: control msg failed (start)\n");
 		return ret;
@@ -473,12 +475,13 @@ static int usbduxfastsub_stop(struct 
usbduxfastsub_s *udfs)
 
 	/* 7f92 to one */
 	local_transfer_buffer[0] = 1;
-	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
0), USBDUXFASTSUB_FIRMWARE,	/* bRequest, "Firmware" */
-			      VENDOR_DIR_OUT,	/* bmRequestType */
-			      USBDUXFASTSUB_CPUCS,	/* Value */
-			      0x0000,	/* Index */
-			      local_transfer_buffer, 1,	/* Length */
-			      EZTIMEOUT);	/* Timeout */
+	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
0),
+			USBDUXFASTSUB_FIRMWARE,	/* bRequest, 
"Firmware" */
+			VENDOR_DIR_OUT,	/* bmRequestType */
+			USBDUXFASTSUB_CPUCS,	/* Value */
+			0x0000,	/* Index */
+			local_transfer_buffer, 1,	/* Length */
+			EZTIMEOUT);	/* Timeout */
 	if (ret < 0) {
 		printk(KERN_ERR "comedi_: usbduxfast: control msg failed "
 		       "(stop)\n");
@@ -499,13 +502,14 @@ static int usbduxfastsub_upload(struct 
usbduxfastsub_s *udfs,
 	printk(KERN_DEBUG " to addr %d, first byte=%d.\n",
 	       startAddr, local_transfer_buffer[0]);
 #endif
-	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
0), USBDUXFASTSUB_FIRMWARE,	/* brequest, firmware */
-			      VENDOR_DIR_OUT,	/* bmRequestType */
-			      startAddr,	/* value */
-			      0x0000,	/* index */
-			      local_transfer_buffer,	/* our local safe buffer */
-			      len,	/* length */
-			      EZTIMEOUT);	/* timeout */
+	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
0),
+			USBDUXFASTSUB_FIRMWARE,	/* brequest, firmware 
*/
+			VENDOR_DIR_OUT,	/* bmRequestType */
+			startAddr,	/* value */
+			0x0000,	/* index */
+			local_transfer_buffer,	/* our local safe buffer */
+			len,	/* length */
+			EZTIMEOUT);	/* timeout */
 
 #ifdef CONFIG_COMEDI_DEBUG
 	printk(KERN_DEBUG "comedi_: usbduxfast: result=%d\n", ret);
@@ -1347,7 +1351,7 @@ static int usbduxfast_ai_insn_read(struct 
comedi_device *dev,
 #define FIRMWARE_MAX_LEN 0x2000
 
 static int firmwareUpload(struct usbduxfastsub_s *usbduxfastsub,
-			  const u8 * firmwareBinary, int sizeFirmware)
+			  const u8 *firmwareBinary, int sizeFirmware)
 {
 	int ret;
 	uint8_t *fwBuf;
@@ -1357,7 +1361,7 @@ static int firmwareUpload(struct usbduxfastsub_s 
*usbduxfastsub,
 
 	if (sizeFirmware > FIRMWARE_MAX_LEN) {
 		dev_err(&usbduxfastsub->interface->dev,
-			"comedi_: usbduxfast firmware binary it too large for FX2.
\n");
+		"comedi_: usbduxfast firmware binary too large for FX2.\n");
 		return -ENOMEM;
 	}
 
@@ -1567,12 +1571,12 @@ static int usbduxfastsub_probe(struct 
usb_interface *uinterf,
 	up(&start_stop_sem);
 
 	ret = request_firmware_nowait(THIS_MODULE,
-				      FW_ACTION_HOTPLUG,
-				      "usbduxfast_firmware.bin",
-				      &udev->dev,
-				      GFP_KERNEL,
-				      usbduxfastsub + index,
-				      usbduxfast_firmware_request_complete_handler);
+				FW_ACTION_HOTPLUG,
+				"usbduxfast_firmware.bin",
+				&udev->dev,
+				GFP_KERNEL,
+				usbduxfastsub + index,
+				usbduxfast_firmware_request_complete_handler);
 
 	if (ret) {
 		dev_err(&udev->dev, "could not load firmware (err=%d)\n", ret);
-- 
1.6.4.2

Re: Linux kernel patch

[Arnd Bergmann]

On Tuesday 23 March 2010, Steve Cave wrote:
> Please find enclosed a patch (my first) to fix checkpatch.pl issues with the
>  usbduxfast.c in the comedi driver. Please note that there is one outstanding
> issue, with semaphores and mutex, that I do not feel qualified to patch.

Hi Steve,

Thanks for your contribution. Since this is your first patch, let me go
through it in detail and comment on all issues. Many of these are details
that might otherwise get ignored, but getting this right will help you
get future patches accepted faster.

First of all, the email subject line and introduction above is misplaced.
The subject should be the line you have below, as it comes from
git-format-patch. Try using git-send-email to send that file to yourself
to see what it should look like.

Your description above could have gone below the '---' line, which is
the part that does not get merged into the changelog.

> ---
> From 93c6eae8685fe87c4796e5e177ea136a155bfe2f Mon Sep 17 00:00:00 
> 2001

This looks like it got linewrapped, indicating that you are using a broken
email client. See Documentation/email-clients.txt for more information
on this.

Further, this line is not generally put into a submission, just leave it out.

> From: Steve Cave <steve.cave@ntlworld.com>
> Date: Wed, 17 Mar 2010 20:58:35 +0000
> Subject: [PATCH] Staging: comedi: fix line lengths and indentation in 
> usbduxfast.c
> 
> This is a patch to the usbduxfast.c file that reformats and breaks up a
> number of code lines greater than 80 characters long, standardizing
> indents and replacing some spaces with tabs to fix warnings found by the
> checkpatch tool; also replacing a foo * bar with foo *bar to fix an error
> found by checkpatch

This description is mostly ok. It's probably better to leave out the
redundant 'This is a patch to the usbduxfast.c file' part, which is
totally obvious.

Note that this kind of patch is good and welcome for staging drivers,
but please don't send reformatting patches for stuff outside of
staging. Even for staging, I'd encourage you to read up on topics like
the semaphore to mutex conversion, because changes that really improve
the code are generally appreciated more than those that just change
formatting.

Ideally, you should follow the rule to only send formatting patches
when you also have real code changes for the same files in the same
series (never mix the two in the same patch though). For some new staging
drivers, doing just the formatting can often help as well, but
comedi has probably moved beyond that stage.

> @@ -271,7 +271,8 @@ static int usbduxfast_ai_stop(struct 
> usbduxfastsub_s *udfs, int do_unlink)
>  	udfs->ai_cmd_running = 0;
>  
>  	if (do_unlink)
> -		ret = usbduxfastsub_unlink_InURBs(udfs);	/* stop aquistion */
> +		ret = usbduxfastsub_unlink_InURBs(udfs);
> +						/* stop acquistion */
>  
>  	return ret;
>  }

The comment now looks misplaced IMHO, it may be better to put it into the
line before the ret = .

> @@ -451,13 +452,14 @@ static int usbduxfastsub_start(struct 
> usbduxfastsub_s *udfs)
>  
>  	/* 7f92 to zero */
>  	local_transfer_buffer[0] = 0;
> -	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
> 0), USBDUXFASTSUB_FIRMWARE,	/* bRequest, "Firmware" */
> -			      VENDOR_DIR_OUT,	/* bmRequestType */
> -			      USBDUXFASTSUB_CPUCS,	/* Value */
> -			      0x0000,	/* Index */
> -			      local_transfer_buffer,	/* address of the transfer buffer 
> */
> -			      1,	/* Length */
> -			      EZTIMEOUT);	/* Timeout */
> +	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
> 0),
> +			USBDUXFASTSUB_FIRMWARE,	/* bRequest, 
> "Firmware" */
> +			VENDOR_DIR_OUT,	/* bmRequestType */
> +			USBDUXFASTSUB_CPUCS,	/* Value */
> +			0x0000,	/* Index */
> +			local_transfer_buffer,	/* transfer buffer address  */
> +			1,	/* Length */
> +			EZTIMEOUT);	/* Timeout */
>  	if (ret < 0) {
>  		printk("comedi_: usbduxfast_: control msg failed (start)\n");
>  		return ret;

This one is not really an improvement. The arguments are aligned with the
opening braces already, so you should leave that. In this case, it may actually
be better to remove some or all of the comments in this call in order to
improve readability. Often (but not in this particular case), you can
fix code like this by moving parts into a separate function.

> @@ -473,12 +475,13 @@ static int usbduxfastsub_stop(struct 
> usbduxfastsub_s *udfs)
>  
>  	/* 7f92 to one */
>  	local_transfer_buffer[0] = 1;
> -	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
> 0), USBDUXFASTSUB_FIRMWARE,	/* bRequest, "Firmware" */
> -			      VENDOR_DIR_OUT,	/* bmRequestType */
> -			      USBDUXFASTSUB_CPUCS,	/* Value */
> -			      0x0000,	/* Index */
> -			      local_transfer_buffer, 1,	/* Length */
> -			      EZTIMEOUT);	/* Timeout */
> +	ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 
> 0),
> +			USBDUXFASTSUB_FIRMWARE,	/* bRequest, 
> "Firmware" */
> +			VENDOR_DIR_OUT,	/* bmRequestType */
> +			USBDUXFASTSUB_CPUCS,	/* Value */
> +			0x0000,	/* Index */
> +			local_transfer_buffer, 1,	/* Length */
> +			EZTIMEOUT);	/* Timeout */
>  	if (ret < 0) {
>  		printk(KERN_ERR "comedi_: usbduxfast: control msg failed "
>  		       "(stop)\n");

same as above.

> @@ -1347,7 +1351,7 @@ static int usbduxfast_ai_insn_read(struct 
> comedi_device *dev,
>  #define FIRMWARE_MAX_LEN 0x2000
>  
>  static int firmwareUpload(struct usbduxfastsub_s *usbduxfastsub,
> -			  const u8 * firmwareBinary, int sizeFirmware)
> +			  const u8 *firmwareBinary, int sizeFirmware)
>  {
>  	int ret;
>  	uint8_t *fwBuf;

ok

> @@ -1357,7 +1361,7 @@ static int firmwareUpload(struct usbduxfastsub_s 
> *usbduxfastsub,
>  
>  	if (sizeFirmware > FIRMWARE_MAX_LEN) {
>  		dev_err(&usbduxfastsub->interface->dev,
> -			"comedi_: usbduxfast firmware binary it too large for FX2.
> \n");
> +		"comedi_: usbduxfast firmware binary too large for FX2.\n");
>  		return -ENOMEM;
>  	}
  
The original was better here, you should never align function arguments with the
function name itself. You can split up overly long lines like

			"comedi_: usbduxfast firmware binary too large "
			"for FX2.\n"

but in this case, it's probably better to go over the 80 columns. It's really
not such a hard rule.

	Arnd