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

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

[YAMANE Toshiaki]

Fixed a description of the procedure call dev_dbg().

Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   59 +++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index 099bc69..9bc8923 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -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)
-- 
1.7.9.5

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

[YAMANE Toshiaki]

Modified to eliminate the deep nesting and redundant description.

Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |  147 +++++++++++++++++--------------
 1 file changed, 80 insertions(+), 67 deletions(-)

diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index 9bc8923..0395bdf 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -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);
 		}
-- 
1.7.9.5

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

[YAMANE Toshiaki]

Modified to eliminate the deep nesting.

Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   51 ++++++++++++++++---------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index 0395bdf..4c475ed 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -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);
-- 
1.7.9.5

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

[YAMANE Toshiaki]

Modified to eliminate the deep nesting.

Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   39 ++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index 4c475ed..f68a855 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -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);
 }
-- 
1.7.9.5

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

[Dan Carpenter]

On Sat, Nov 10, 2012 at 06:33:56AM +0900, YAMANE Toshiaki wrote:
> Modified to eliminate the deep nesting and redundant description.
> 
> Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
> ---
>  drivers/staging/serqt_usb2/serqt_usb2.c |  147 +++++++++++++++++--------------
>  1 file changed, 80 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
> index 9bc8923..0395bdf 100644
> --- a/drivers/staging/serqt_usb2/serqt_usb2.c
> +++ b/drivers/staging/serqt_usb2/serqt_usb2.c
> @@ -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) {

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.

> +		dev_dbg(&port->dev,
> +			"Illegal escape seuences in received data\n");
> +		return 0;
> +	}
> +
> +	(*fn)(qt_port, data[i + 3]);
> +
> +	return 1;
> +}
> +

[snip]

>  	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

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

[YAMANE Toshiaki]

On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sat, Nov 10, 2012 at 06:33:56AM +0900, YAMANE Toshiaki wrote:
>> Modified to eliminate the deep nesting and redundant description.
>>
>> Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
>> ---
>>  drivers/staging/serqt_usb2/serqt_usb2.c |  147 +++++++++++++++++--------------
>>  1 file changed, 80 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
>> index 9bc8923..0395bdf 100644
>> --- a/drivers/staging/serqt_usb2/serqt_usb2.c
>> +++ b/drivers/staging/serqt_usb2/serqt_usb2.c
>> @@ -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) {
>
> 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.
>
>> +             dev_dbg(&port->dev,
>> +                     "Illegal escape seuences in received data\n");
>> +             return 0;
>> +     }
>> +
>> +     (*fn)(qt_port, data[i + 3]);
>> +
>> +     return 1;
>> +}
>> +
>
> [snip]
>
>>       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.

Dan-san,

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



-- 

Regards,

YAMANE Toshiaki

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

[YAMANE Toshiaki]

On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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.

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.

>>       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.

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
yamanetoshi@gmail.com

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

[Dan Carpenter]

On Fri, Nov 16, 2012 at 05:01:55AM +0900, YAMANE Toshiaki wrote:
> On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > 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.
> 
> 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.

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.

> 
> >>       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.
> 
> 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

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

[YAMANE Toshiaki]

On Fri, Nov 16, 2012 at 5:18 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Nov 16, 2012 at 05:01:55AM +0900, YAMANE Toshiaki wrote:
>> On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >
>> > 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.
>>
>> 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.
>
> 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.

>> >>       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.
>>
>> 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.

I understand it.
Thanks for your comments.


-- 

Regards,

YAMANE Toshiaki

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

[Dan Carpenter]

Btw:
> +             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.

> +             fn = ProcessLineStatus;
> +     } else {
> +             dev_dbg(&port->dev, "Modem status status.\n");
> +             fn = ProcessModemStatus;

regards,
dan carpenter

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

[YAMANE Toshiaki]

On Fri, Nov 16, 2012 at 5:43 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Btw:
>> +             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!

>> +             fn = ProcessLineStatus;
>> +     } else {
>> +             dev_dbg(&port->dev, "Modem status status.\n");
>> +             fn = ProcessModemStatus;
>
> regards,
> dan carpenter
>



-- 

Regards,

YAMANE Toshiaki