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(-)
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
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(-)
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
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(-)
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
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(-)
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
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 <[email protected]>
> ---
> 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
On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <[email protected]> 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
[email protected]
On Fri, Nov 16, 2012 at 05:01:55AM +0900, YAMANE Toshiaki wrote:
> On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter <[email protected]> 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
On Fri, Nov 16, 2012 at 5:18 AM, Dan Carpenter <[email protected]> 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 <[email protected]> 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
On Fri, Nov 16, 2012 at 5:43 AM, Dan Carpenter <[email protected]> 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
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