Skip to contentSkip to conversation

staging/serqt_usb2: fixed line over issue in serqt_usb2.c

PATCH In review YAMANE Toshiaki 2 participants 11 messages Nov 9, 2012 – Nov 15, 2012 Staging

lore
Share & tools
lore

YAMANE Toshiaki

author
#0 lore raw

PATCH 1/4 staging/serqt_usb2: fixed line over issue in serqt_usb2.c

Fixed a description of the procedure call dev_dbg().

Signed-off-by YAMANE Toshiaki <[email protected]>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   59 +++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 19 deletions(-)
drivers/staging/serqt_usb2/serqt_usb2.c +40 -19
index 099bc69..9bc8923 100644
@@ -272,7 +272,8 @@ static void qt_write_bulk_callback(struct urb *urb)
 	status = urb->status;
 
 	if (status) {
-		dev_dbg(&urb->dev->dev, "nonzero write bulk status received:%d\n", status);
+		dev_dbg(&urb->dev->dev,
+			"nonzero write bulk status received:%d\n", status);
 		return;
 	}
 

@@ -321,7 +322,8 @@ static void qt_read_bulk_callback(struct urb *urb)
 	/* index = MINOR(port->tty->device) - serial->minor; */
 	index = tty->index - serial->minor;
 
-	dev_dbg(&port->dev, "%s - port->RxHolding = %d\n", __func__, qt_port->RxHolding);
+	dev_dbg(&port->dev,
+		"%s - port->RxHolding = %d\n", __func__, qt_port->RxHolding);
 
 	if (port_paranoia_check(port, __func__) != 0) {
 		qt_port->ReadBulkStopped = 1;

@@ -333,7 +335,8 @@ static void qt_read_bulk_callback(struct urb *urb)
 
 	if (qt_port->closePending == 1) {
 		/* Were closing , stop reading */
-		dev_dbg(&port->dev, "%s - (qt_port->closepending == 1\n", __func__);
+		dev_dbg(&port->dev,
+			"%s - (qt_port->closepending == 1\n", __func__);
 		qt_port->ReadBulkStopped = 1;
 		goto exit;
 	}

@@ -912,10 +915,14 @@ static int qt_open(struct tty_struct *tty,
 
 	dev_dbg(&port->dev, "port number is %d\n", port->number);
 	dev_dbg(&port->dev, "serial number is %d\n", port->serial->minor);
-	dev_dbg(&port->dev, "Bulkin endpoint is %d\n", port->bulk_in_endpointAddress);
-	dev_dbg(&port->dev, "BulkOut endpoint is %d\n", port->bulk_out_endpointAddress);
-	dev_dbg(&port->dev, "Interrupt endpoint is %d\n", port->interrupt_in_endpointAddress);
-	dev_dbg(&port->dev, "port's number in the device is %d\n", quatech_port->port_num);
+	dev_dbg(&port->dev,
+		"Bulkin endpoint is %d\n", port->bulk_in_endpointAddress);
+	dev_dbg(&port->dev,
+		"BulkOut endpoint is %d\n", port->bulk_out_endpointAddress);
+	dev_dbg(&port->dev, "Interrupt endpoint is %d\n",
+		port->interrupt_in_endpointAddress);
+	dev_dbg(&port->dev, "port's number in the device is %d\n",
+		quatech_port->port_num);
 	quatech_port->read_urb = port->read_urb;
 
 	/* set up our bulk in urb */

@@ -928,7 +935,8 @@ static int qt_open(struct tty_struct *tty,
 			  quatech_port->read_urb->transfer_buffer_length,
 			  qt_read_bulk_callback, quatech_port);
 
-	dev_dbg(&port->dev, "qt_open: bulkin endpoint is %d\n", port->bulk_in_endpointAddress);
+	dev_dbg(&port->dev, "qt_open: bulkin endpoint is %d\n",
+		port->bulk_in_endpointAddress);
 	quatech_port->read_urb_busy = true;
 	result = usb_submit_urb(quatech_port->read_urb, GFP_KERNEL);
 	if (result) {

@@ -1021,15 +1029,18 @@ static void qt_close(struct usb_serial_port *port)
 	/* Close uart channel */
 	status = qt_close_channel(serial, index);
 	if (status < 0)
-		dev_dbg(&port->dev, "%s - port %d qt_close_channel failed.\n", __func__, port->number);
+		dev_dbg(&port->dev,
+			"%s - port %d qt_close_channel failed.\n",
+			__func__, port->number);
 
 	port0->open_ports--;
 
-	dev_dbg(&port->dev, "qt_num_open_ports in close%d:in port%d\n", port0->open_ports, port->number);
+	dev_dbg(&port->dev, "qt_num_open_ports in close%d:in port%d\n",
+		port0->open_ports, port->number);
 
 	if (port0->open_ports == 0) {
 		if (serial->port[0]->interrupt_in_urb) {
-			dev_dbg(&port->dev, "%s", "Shutdown interrupt_in_urb\n");
+			dev_dbg(&port->dev, "Shutdown interrupt_in_urb\n");
 			usb_kill_urb(serial->port[0]->interrupt_in_urb);
 		}
 

@@ -1053,7 +1064,8 @@ static int qt_write(struct tty_struct *tty, struct usb_serial_port *port,
 		return -ENODEV;
 
 	if (count == 0) {
-		dev_dbg(&port->dev, "%s - write request of 0 bytes\n", __func__);
+		dev_dbg(&port->dev,
+			"%s - write request of 0 bytes\n", __func__);
 		return 0;
 	}
 

@@ -1080,7 +1092,8 @@ static int qt_write(struct tty_struct *tty, struct usb_serial_port *port,
 		/* send the data out the bulk port */
 		result = usb_submit_urb(port->write_urb, GFP_ATOMIC);
 		if (result)
-			dev_dbg(&port->dev, "%s - failed submitting write urb, error %d\n",
+			dev_dbg(&port->dev,
+				"%s - failed submitting write urb, error %d\n",
 				__func__, result);
 		else
 			result = count;

@@ -1163,7 +1176,8 @@ static int qt_ioctl(struct tty_struct *tty,
 		return 0;
 	}
 
-	dev_dbg(&port->dev, "%s -No ioctl for that one.  port = %d\n", __func__, port->number);
+	dev_dbg(&port->dev, "%s -No ioctl for that one.  port = %d\n",
+		__func__, port->number);
 	return -ENOIOCTLCMD;
 }
 

@@ -1238,7 +1252,8 @@ static void qt_set_termios(struct tty_struct *tty,
 
 	/* Now determine flow control */
 	if (cflag & CRTSCTS) {
-		dev_dbg(&port->dev, "%s - Enabling HW flow control port %d\n", __func__, port->number);
+		dev_dbg(&port->dev, "%s - Enabling HW flow control port %d\n",
+			__func__, port->number);
 
 		/* Enable RTS/CTS flow control */
 		status = BoxSetHW_FlowCtrl(port->serial, index, 1);

@@ -1249,7 +1264,9 @@ static void qt_set_termios(struct tty_struct *tty,
 		}
 	} else {
 		/* Disable RTS/CTS flow control */
-		dev_dbg(&port->dev, "%s - disabling HW flow control port %d\n", __func__, port->number);
+		dev_dbg(&port->dev,
+			"%s - disabling HW flow control port %d\n",
+			__func__, port->number);
 
 		status = BoxSetHW_FlowCtrl(port->serial, index, 0);
 		if (status < 0) {

@@ -1268,17 +1285,21 @@ static void qt_set_termios(struct tty_struct *tty,
 		    BoxSetSW_FlowCtrl(port->serial, index, stop_char,
 				      start_char);
 		if (status < 0)
-			dev_dbg(&port->dev, "BoxSetSW_FlowCtrl (enabled) failed\n");
+			dev_dbg(&port->dev,
+				"BoxSetSW_FlowCtrl (enabled) failed\n");
 
 	} else {
 		/* disable SW flow control */
 		status = BoxDisable_SW_FlowCtrl(port->serial, index);
 		if (status < 0)
-			dev_dbg(&port->dev, "BoxSetSW_FlowCtrl (diabling) failed\n");
+			dev_dbg(&port->dev,
+				"BoxSetSW_FlowCtrl (diabling) failed\n");
 
 	}
 	termios->c_cflag &= ~CMSPAR;
-	/* FIXME: Error cases should be returning the actual bits changed only */
+	/* FIXME:
+	   Error cases should be returning the actual bits changed only
+	*/
 }
 
 static void qt_break(struct tty_struct *tty, int break_state)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 2/4 staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Modified to eliminate the deep nesting and redundant description.

Signed-off-by YAMANE Toshiaki <[email protected]>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |  147 +++++++++++++++++--------------
 1 file changed, 80 insertions(+), 67 deletions(-)
drivers/staging/serqt_usb2/serqt_usb2.c +80 -67
index 9bc8923..0395bdf 100644
@@ -291,22 +291,89 @@ static void qt_interrupt_callback(struct urb *urb)
 	/* FIXME */
 }
 
+static int qt_status_change(unsigned int limit,
+			    unsigned char *data,
+			    int i,
+			    struct quatech_port *qt_port,
+			    struct usb_serial_port *port)
+{
+	void (*fn)(struct quatech_port *, unsigned char);
+
+	if (0x00 == data[i + 2]) {
+		dev_dbg(&port->dev, "Line status status.\n");
+		fn = ProcessLineStatus;
+	} else {
+		dev_dbg(&port->dev, "Modem status status.\n");
+		fn = ProcessModemStatus;
+	}
+
+	if (i > limit) {
+		dev_dbg(&port->dev,
+			"Illegal escape seuences in received data\n");
+		return 0;
+	}
+
+	(*fn)(qt_port, data[i + 3]);
+
+	return 1;
+}
+
+static void qt_status_change_check(struct tty_struct *tty,
+				   struct urb *urb,
+				   struct quatech_port *qt_port,
+				   struct usb_serial_port *port)
+{
+	int flag, i;
+	unsigned char *data = urb->transfer_buffer;
+	unsigned int RxCount = urb->actual_length;
+
+	for (i = 0; i < RxCount; ++i) {
+		/* Look ahead code here */
+		if ((i <= (RxCount - 3)) && (data[i] == 0x1b)
+		    && (data[i + 1] == 0x1b)) {
+			flag = 0;
+			switch (data[i + 2]) {
+			case 0x00:
+			case 0x01:
+				flag = qt_status_change((RxCount - 4), data, i,
+							qt_port, port);
+				if (flag == 1)
+					i += 3;
+				break;
+
+			case 0xff:
+				dev_dbg(&port->dev, "No status sequence.\n");
+
+				ProcessRxChar(tty, port, data[i]);
+				ProcessRxChar(tty, port, data[i + 1]);
+
+				i += 2;
+				break;
+			}
+			if (flag == 1)
+				continue;
+		}
+
+		if (tty && urb->actual_length)
+			tty_insert_flip_char(tty, data[i], TTY_NORMAL);
+
+	}
+	tty_flip_buffer_push(tty);
+}
+
 static void qt_read_bulk_callback(struct urb *urb)
 {
 
 	struct usb_serial_port *port = urb->context;
 	struct usb_serial *serial = get_usb_serial(port, __func__);
 	struct quatech_port *qt_port = qt_get_port_private(port);
-	unsigned char *data;
 	struct tty_struct *tty;
-	unsigned int index;
-	unsigned int RxCount;
-	int i, result;
-	int flag, flag_data;
+	int result;
 
 	if (urb->status) {
 		qt_port->ReadBulkStopped = 1;
-		dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n",
+		dev_dbg(&urb->dev->dev,
+			"%s - nonzero write bulk status received: %d\n",
 			__func__, urb->status);
 		return;
 	}

@@ -315,13 +382,6 @@ static void qt_read_bulk_callback(struct urb *urb)
 	if (!tty)
 		return;
 
-	data = urb->transfer_buffer;
-
-	RxCount = urb->actual_length;
-
-	/* index = MINOR(port->tty->device) - serial->minor; */
-	index = tty->index - serial->minor;
-
 	dev_dbg(&port->dev,
 		"%s - port->RxHolding = %d\n", __func__, qt_port->RxHolding);
 

@@ -354,62 +414,14 @@ static void qt_read_bulk_callback(struct urb *urb)
 	if (urb->status) {
 		qt_port->ReadBulkStopped = 1;
 
-		dev_dbg(&port->dev, "%s - nonzero read bulk status received: %d\n",
+		dev_dbg(&port->dev,
+			"%s - nonzero read bulk status received: %d\n",
 			__func__, urb->status);
 		goto exit;
 	}
 
-	if (RxCount) {
-		flag_data = 0;
-		for (i = 0; i < RxCount; ++i) {
-			/* Look ahead code here */
-			if ((i <= (RxCount - 3)) && (data[i] == 0x1b)
-			    && (data[i + 1] == 0x1b)) {
-				flag = 0;
-				switch (data[i + 2]) {
-				case 0x00:
-					/* line status change 4th byte must follow */
-					if (i > (RxCount - 4)) {
-						dev_dbg(&port->dev, "Illegal escape seuences in received data\n");
-						break;
-					}
-					ProcessLineStatus(qt_port, data[i + 3]);
-					i += 3;
-					flag = 1;
-					break;
-
-				case 0x01:
-					/* Modem status status change 4th byte must follow */
-					dev_dbg(&port->dev, "Modem status status.\n");
-					if (i > (RxCount - 4)) {
-						dev_dbg(&port->dev, "Illegal escape sequences in received data\n");
-						break;
-					}
-					ProcessModemStatus(qt_port,
-							   data[i + 3]);
-					i += 3;
-					flag = 1;
-					break;
-				case 0xff:
-					dev_dbg(&port->dev, "No status sequence.\n");
-
-					if (tty) {
-						ProcessRxChar(tty, port, data[i]);
-						ProcessRxChar(tty, port, data[i + 1]);
-					}
-					i += 2;
-					break;
-				}
-				if (flag == 1)
-					continue;
-			}
-
-			if (tty && urb->actual_length)
-				tty_insert_flip_char(tty, data[i], TTY_NORMAL);
-
-		}
-		tty_flip_buffer_push(tty);
-	}
+	if (urb->actual_length)
+		qt_status_change_check(tty, urb, qt_port, port);
 
 	/* Continue trying to always read  */
 	usb_fill_bulk_urb(port->read_urb, serial->dev,

@@ -420,10 +432,11 @@ static void qt_read_bulk_callback(struct urb *urb)
 			  qt_read_bulk_callback, port);
 	result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
 	if (result)
-		dev_dbg(&port->dev, "%s - failed resubmitting read urb, error %d",
+		dev_dbg(&port->dev,
+			"%s - failed resubmitting read urb, error %d",
 			__func__, result);
 	else {
-		if (RxCount) {
+		if (urb->actual_length) {
 			tty_flip_buffer_push(tty);
 			tty_schedule_flip(tty);
 		}
Signature · 1 line
1.7.9.5
#2 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Quoted 22 lines · YAMANE Toshiaki

Modified to eliminate the deep nesting and redundant description.

Signed-off-by YAMANE Toshiaki <[email protected]>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |  147 +++++++++++++++++--------------
 1 file changed, 80 insertions(+), 67 deletions(-)
drivers/staging/serqt_usb2/serqt_usb2.c +7 -0
index 9bc8923..0395bdf 100644
@@ -291,22 +291,89 @@ static void qt_interrupt_callback(struct urb *urb)
 	/* FIXME */
 }
 
+static int qt_status_change(unsigned int limit,
+			    unsigned char *data,
+			    int i,
+			    struct quatech_port *qt_port,
+			    struct usb_serial_port *port)
+{
+	void (*fn)(struct quatech_port *, unsigned char);

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 310

Quoted 10 lines
drivers/staging/serqt_usb2/serqt_usb2.c +10 -0
+
+	if (0x00 == data[i + 2]) {
+		dev_dbg(&port->dev, "Line status status.\n");
+		fn = ProcessLineStatus;
+	} else {
+		dev_dbg(&port->dev, "Modem status status.\n");
+		fn = ProcessModemStatus;
+	}
+
+	if (i > limit) {

Why can't we test whether i == (RxCount - 3) earlier and handle the errors there? That way we wouldn't need to pass the limit variable.

In fact, this whole function is sort of nasty. We start by doing a switch (data[i + 2]) { then we combine the 0x00 and 0x01 and call this function which separates them out and sets a function pointer and then calls the function point? Get rid of this whole function.

You shouldn't need to use function pointers to do this; that's too many levels of abstraction.

Quoted 10 lines
drivers/staging/serqt_usb2/serqt_usb2.c +10 -0
+		dev_dbg(&port->dev,
+			"Illegal escape seuences in received data\n");
+		return 0;
+	}
+
+	(*fn)(qt_port, data[i + 3]);
+
+	return 1;
+}
+

[snip]

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 377

Quoted 6 lines
drivers/staging/serqt_usb2/serqt_usb2.c +2 -1
 	if (urb->status) {
 		qt_port->ReadBulkStopped = 1;
-		dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n",
+		dev_dbg(&urb->dev->dev,
+			"%s - nonzero write bulk status received: %d\n",
 			__func__, urb->status);

Don't mix in these unrelated 80 character limit changes.

regards,
dan carpenter

author
#3 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Quoted 59 lines · Dan Carpenter
Quoted 22 lines · YAMANE Toshiaki

Modified to eliminate the deep nesting and redundant description.

Signed-off-by YAMANE Toshiaki <[email protected]>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |  147 +++++++++++++++++--------------
 1 file changed, 80 insertions(+), 67 deletions(-)
drivers/staging/serqt_usb2/serqt_usb2.c +7 -0
index 9bc8923..0395bdf 100644
@@ -291,22 +291,89 @@ static void qt_interrupt_callback(struct urb *urb)
      /* FIXME */
 }
 
+static int qt_status_change(unsigned int limit,
+                         unsigned char *data,
+                         int i,
+                         struct quatech_port *qt_port,
+                         struct usb_serial_port *port)
+{
+     void (*fn)(struct quatech_port *, unsigned char);

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 310

Quoted 10 lines
drivers/staging/serqt_usb2/serqt_usb2.c +10 -0
+
+     if (0x00 == data[i + 2]) {
+             dev_dbg(&port->dev, "Line status status.\n");
+             fn = ProcessLineStatus;
+     } else {
+             dev_dbg(&port->dev, "Modem status status.\n");
+             fn = ProcessModemStatus;
+     }
+
+     if (i > limit) {

Why can't we test whether i == (RxCount - 3) earlier and handle the errors there? That way we wouldn't need to pass the limit variable.

In fact, this whole function is sort of nasty. We start by doing a switch (data[i + 2]) { then we combine the 0x00 and 0x01 and call this function which separates them out and sets a function pointer and then calls the function point? Get rid of this whole function.

You shouldn't need to use function pointers to do this; that's too many levels of abstraction.

Quoted 10 lines
drivers/staging/serqt_usb2/serqt_usb2.c +10 -0
+             dev_dbg(&port->dev,
+                     "Illegal escape seuences in received data\n");
+             return 0;
+     }
+
+     (*fn)(qt_port, data[i + 3]);
+
+     return 1;
+}
+

[snip]

Quoted 8 lines

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 377

Quoted 6 lines
      if (urb->status) {
              qt_port->ReadBulkStopped = 1;
drivers/staging/serqt_usb2/serqt_usb2.c +2 -1
-             dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n",
+             dev_dbg(&urb->dev->dev,
+                     "%s - nonzero write bulk status received: %d\n",
                      __func__, urb->status);

Don't mix in these unrelated 80 character limit changes.

Dan-san,

Thanks for your follow-ups.
I will try to resend this patch.

Signature · 3 lines
Regards,

YAMANE Toshiaki
author
#4 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

In reply to Dan Carpenter
Quoted 5 lines · Dan Carpenter

Why can't we test whether i == (RxCount - 3) earlier and handle the errors there? That way we wouldn't need to pass the limit variable.

Quoted 7 lines

In fact, this whole function is sort of nasty. We start by doing a switch (data[i + 2]) { then we combine the 0x00 and 0x01 and call this function which separates them out and sets a function pointer and then calls the function point? Get rid of this whole function.

You shouldn't need to use function pointers to do this; that's too many levels of abstraction.

I feel it so diffcult to consider the fixing this patch more.

There are some reasons why I have become such a description.
- The purpose of this patch is the resolution of the line over 80 characters issue
- I Wrote the code to be aware of the following:
-- Do not change the procedure
-- The shallow nest
-- To avoid the redundancy

If I do not use a function pointer, which take the form below.

        if (0x00 == data[i + 2])
                dev_dbg(&port->dev, "Line status status.\n");
        else
                dev_dbg(&port->dev, "Modem status status.\n");

        if (i > limit) {
                dev_dbg(&port->dev,
                        "Illegal escape seuences in received data\n");
                return 0;
        }

        if (0x00 == data[i + 2])
                ProcessLineStatus(qt_port, data[i + 3]);
        else
                ProcessModemStatus(qt_port, data[i + 3]);

	return 1;

I also feel it may be...

And I am against to move the dev_dbg procedure call to qt_status_change_check procedure because the nesting will be so deep.

Quoted 8 lines

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 377

Quoted 6 lines
      if (urb->status) {
              qt_port->ReadBulkStopped = 1;
drivers/staging/serqt_usb2/serqt_usb2.c +2 -1
-             dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n",
+             dev_dbg(&urb->dev->dev,
+                     "%s - nonzero write bulk status received: %d\n",
                      __func__, urb->status);

Don't mix in these unrelated 80 character limit changes.

I think the purpose of refactoring is the resolution of the line over 80 characters issue. I think that the separation of the patch should stop taking because they are already applied in the linux-next tree.

Thanks.

YAMANE Toshiaki
[email protected]

#5 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Quoted 44 lines · YAMANE Toshiaki
Quoted 5 lines · Dan Carpenter

Why can't we test whether i == (RxCount - 3) earlier and handle the errors there? That way we wouldn't need to pass the limit variable.

Quoted 7 lines

In fact, this whole function is sort of nasty. We start by doing a switch (data[i + 2]) { then we combine the 0x00 and 0x01 and call this function which separates them out and sets a function pointer and then calls the function point? Get rid of this whole function.

You shouldn't need to use function pointers to do this; that's too many levels of abstraction.

I feel it so diffcult to consider the fixing this patch more.

There are some reasons why I have become such a description.
- The purpose of this patch is the resolution of the line over 80 characters issue
- I Wrote the code to be aware of the following:
-- Do not change the procedure
-- The shallow nest
-- To avoid the redundancy

If I do not use a function pointer, which take the form below.

        if (0x00 == data[i + 2])
                dev_dbg(&port->dev, "Line status status.\n");
        else
                dev_dbg(&port->dev, "Modem status status.\n");

        if (i > limit) {
                dev_dbg(&port->dev,
                        "Illegal escape seuences in received data\n");
                return 0;
        }

        if (0x00 == data[i + 2])
                ProcessLineStatus(qt_port, data[i + 3]);
        else
                ProcessModemStatus(qt_port, data[i + 3]);

	return 1;
Quoted 4 lines

I also feel it may be...

And I am against to move the dev_dbg procedure call to qt_status_change_check procedure because the nesting will be so deep.

In the end, the new version is more confusing than the original code. Checkpatch.pl is not a king which must be obeyed. The only thing which matters is how easy it is for a human to understand the code.

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
      if (urb->status) {
              qt_port->ReadBulkStopped = 1;
drivers/staging/serqt_usb2/serqt_usb2.c +2 -1
-             dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n",
+             dev_dbg(&urb->dev->dev,
+                     "%s - nonzero write bulk status received: %d\n",
                      __func__, urb->status);
Quoted 6 lines
Quoted 1 line

Don't mix in these unrelated 80 character limit changes.

I think the purpose of refactoring is the resolution of the line over 80 characters issue. I think that the separation of the patch should stop taking because they are already applied in the linux-next tree.

Yes, once it is merged into linux-next then it is too late to send a version 2 patch.

I'm explaining that as a reviewer it is confusing for me to figure out when you do unrelated things in the same patch and mix everything up.

regards,
dan carpenter

author
#6 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Quoted 47 lines · Dan Carpenter
Quoted 46 lines · YAMANE Toshiaki
Quoted 5 lines · Dan Carpenter

Why can't we test whether i == (RxCount - 3) earlier and handle the errors there? That way we wouldn't need to pass the limit variable.

Quoted 7 lines

In fact, this whole function is sort of nasty. We start by doing a switch (data[i + 2]) { then we combine the 0x00 and 0x01 and call this function which separates them out and sets a function pointer and then calls the function point? Get rid of this whole function.

You shouldn't need to use function pointers to do this; that's too many levels of abstraction.

I feel it so diffcult to consider the fixing this patch more.

There are some reasons why I have become such a description.
- The purpose of this patch is the resolution of the line over 80 characters issue
- I Wrote the code to be aware of the following:
-- Do not change the procedure
-- The shallow nest
-- To avoid the redundancy

If I do not use a function pointer, which take the form below.

        if (0x00 == data[i + 2])
                dev_dbg(&port->dev, "Line status status.\n");
        else
                dev_dbg(&port->dev, "Modem status status.\n");

        if (i > limit) {
                dev_dbg(&port->dev,
                        "Illegal escape seuences in received data\n");
                return 0;
        }

        if (0x00 == data[i + 2])
                ProcessLineStatus(qt_port, data[i + 3]);
        else
                ProcessModemStatus(qt_port, data[i + 3]);

      return 1;

I also feel it may be...

Quoted 7 lines
Quoted 2 lines

And I am against to move the dev_dbg procedure call to qt_status_change_check procedure because the nesting will be so deep.

In the end, the new version is more confusing than the original code. Checkpatch.pl is not a king which must be obeyed. The only thing which matters is how easy it is for a human to understand the code.

Yes. I understand it.
I wil condider the improvement this.

Quoted 14 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
      if (urb->status) {
              qt_port->ReadBulkStopped = 1;
drivers/staging/serqt_usb2/serqt_usb2.c +2 -1
-             dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n",
+             dev_dbg(&urb->dev->dev,
+                     "%s - nonzero write bulk status received: %d\n",
                      __func__, urb->status);
Quoted 6 lines
Quoted 1 line

Don't mix in these unrelated 80 character limit changes.

I think the purpose of refactoring is the resolution of the line over 80 characters issue. I think that the separation of the patch should stop taking because they are already applied in the linux-next tree.

Quoted 6 lines

Yes, once it is merged into linux-next then it is too late to send a version 2 patch.

I'm explaining that as a reviewer it is confusing for me to figure out when you do unrelated things in the same patch and mix everything up.

I understand it.
Thanks for your comments.

Signature · 3 lines
Regards,

YAMANE Toshiaki
#7 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Btw:

Quoted 1 line

+ dev_dbg(&port->dev, "Line status status.\n");

                                     ^^^^^^^^^^^^^^^^^^^

These kind of debug statements which just tell which function is being called can be deleted. The function tracer already provides that information.

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 307

Quoted 4 lines
drivers/staging/serqt_usb2/serqt_usb2.c +4 -0
+             fn = ProcessLineStatus;
+     } else {
+             dev_dbg(&port->dev, "Modem status status.\n");
+             fn = ProcessModemStatus;

regards,
dan carpenter

author
#8 lore raw

Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c

Quoted 7 lines · Dan Carpenter

Btw:

Quoted 1 line

+ dev_dbg(&port->dev, "Line status status.\n");

                                     ^^^^^^^^^^^^^^^^^^^

These kind of debug statements which just tell which function is being called can be deleted. The function tracer already provides that information.

I did not know that. Thanks!

Quoted 8 lines

drivers/staging/serqt_usb2/serqt_usb2.c static void qt_interrupt_callback(struct urb *urb) · line 307

Quoted 4 lines
drivers/staging/serqt_usb2/serqt_usb2.c +4 -0
+             fn = ProcessLineStatus;
+     } else {
+             dev_dbg(&port->dev, "Modem status status.\n");
+             fn = ProcessModemStatus;

regards,
dan carpenter

Signature · 3 lines
Regards,

YAMANE Toshiaki
author
#9 lore raw

PATCH 3/4 staging/serqt_usb2: refactor qt_open() in serqt_usb2.c

In reply to YAMANE Toshiaki

Modified to eliminate the deep nesting.

Signed-off-by YAMANE Toshiaki <[email protected]>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   51 ++++++++++++++++---------------
 1 file changed, 27 insertions(+), 24 deletions(-)
drivers/staging/serqt_usb2/serqt_usb2.c +27 -24
index 0395bdf..4c475ed 100644
@@ -840,6 +840,31 @@ static void qt_release(struct usb_serial *serial)
 
 }
 
+static void qt_submit_urb_from_open(struct usb_serial *serial,
+				    struct usb_serial_port *port)
+{
+	int result;
+	struct usb_serial_port *port0 = serial->port[0];
+
+	/* set up interrupt urb */
+	usb_fill_int_urb(port0->interrupt_in_urb,
+			 serial->dev,
+			 usb_rcvintpipe(serial->dev,
+					port0->interrupt_in_endpointAddress),
+			 port0->interrupt_in_buffer,
+			 port0->interrupt_in_urb->transfer_buffer_length,
+			 qt_interrupt_callback, serial,
+			 port0->interrupt_in_urb->interval);
+
+	result = usb_submit_urb(port0->interrupt_in_urb,
+				GFP_KERNEL);
+	if (result) {
+		dev_err(&port->dev,
+			"%s - Error %d submitting interrupt urb\n",
+			__func__, result);
+	}
+}
+
 static int qt_open(struct tty_struct *tty,
 		   struct usb_serial_port *port)
 {

@@ -900,30 +925,8 @@ static int qt_open(struct tty_struct *tty,
 
 	/*  Check to see if we've set up our endpoint info yet */
 	if (port0->open_ports == 1) {
-		if (serial->port[0]->interrupt_in_buffer == NULL) {
-			/* set up interrupt urb */
-			usb_fill_int_urb(serial->port[0]->interrupt_in_urb,
-					 serial->dev,
-					 usb_rcvintpipe(serial->dev,
-							serial->port[0]->interrupt_in_endpointAddress),
-					 serial->port[0]->interrupt_in_buffer,
-					 serial->port[0]->
-					 interrupt_in_urb->transfer_buffer_length,
-					 qt_interrupt_callback, serial,
-					 serial->port[0]->
-					 interrupt_in_urb->interval);
-
-			result =
-			    usb_submit_urb(serial->port[0]->interrupt_in_urb,
-					   GFP_KERNEL);
-			if (result) {
-				dev_err(&port->dev,
-					"%s - Error %d submitting "
-					"interrupt urb\n", __func__, result);
-			}
-
-		}
-
+		if (serial->port[0]->interrupt_in_buffer == NULL)
+			qt_submit_urb_from_open(serial, port);
 	}
 
 	dev_dbg(&port->dev, "port number is %d\n", port->number);
Signature · 1 line
1.7.9.5
author
#10 lore raw

PATCH 4/4 staging/serqt_usb2: refactor qt_unthrottle() in serqt_usb2.c

In reply to YAMANE Toshiaki

Modified to eliminate the deep nesting.

Signed-off-by YAMANE Toshiaki <[email protected]>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   39 ++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 16 deletions(-)
drivers/staging/serqt_usb2/serqt_usb2.c +23 -16
index 4c475ed..f68a855 100644
@@ -1473,12 +1473,32 @@ static void qt_throttle(struct tty_struct *tty)
 	mutex_unlock(&qt_port->lock);
 }
 
+static void qt_submit_urb_from_unthrottle(struct usb_serial_port *port,
+					  struct usb_serial *serial)
+{
+	int result;
+
+	/* Start reading from the device */
+	usb_fill_bulk_urb(port->read_urb, serial->dev,
+			  usb_rcvbulkpipe(serial->dev,
+					  port->bulk_in_endpointAddress),
+			  port->read_urb->transfer_buffer,
+			  port->read_urb->transfer_buffer_length,
+			  qt_read_bulk_callback, port);
+
+	result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
+
+	if (result)
+		dev_err(&port->dev,
+			"%s - failed restarting read urb, error %d\n",
+			__func__, result);
+}
+
 static void qt_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = get_usb_serial(port, __func__);
 	struct quatech_port *qt_port;
-	unsigned int result;
 
 	if (!serial)
 		return;

@@ -1494,21 +1514,8 @@ static void qt_unthrottle(struct tty_struct *tty)
 		dev_dbg(&port->dev, "%s - qt_port->RxHolding = 0\n", __func__);
 
 		/* if we have a bulk endpoint, start it up */
-		if ((serial->num_bulk_in) && (qt_port->ReadBulkStopped == 1)) {
-			/* Start reading from the device */
-			usb_fill_bulk_urb(port->read_urb, serial->dev,
-					  usb_rcvbulkpipe(serial->dev,
-							  port->bulk_in_endpointAddress),
-					  port->read_urb->transfer_buffer,
-					  port->read_urb->
-					  transfer_buffer_length,
-					  qt_read_bulk_callback, port);
-			result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
-			if (result)
-				dev_err(&port->dev,
-					"%s - failed restarting read urb, error %d\n",
-					__func__, result);
-		}
+		if ((serial->num_bulk_in) && (qt_port->ReadBulkStopped == 1))
+			qt_submit_urb_from_unthrottle(port, serial);
 	}
 	mutex_unlock(&qt_port->lock);
 }
Signature · 1 line
1.7.9.5