Re: [PATCH] USB: serial: add nt124 usb to serial driver
On Mon, Dec 15, 2014 at 10:09:22AM -0600, George McCollister wrote: On Mon, Dec 15, 2014 at 3:52 AM, Johan Hovold jo...@kernel.org wrote: On Sun, Dec 14, 2014 at 11:51:11AM -0600, George McCollister wrote: Johan, While working on the tx_empty changes you suggested it occurred to me that it might not be obvious to others that the firmware doesn't send a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins transmitting. The practical implication is that if the driver sets tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be reset to false somewhere when more data is transmitted. Perhaps I could add prepare_write_buffer and do it in there before calling usb_serial_generic_prepare_write_buffer(). Does that sound acceptable? Hmm. There's no way to query that flag? And the status is sent (as bulk in data) periodically or only when data has been received? And not when the actual status changes? The bulk in packets are not sent periodically only on TXEMPTY, other line change or received data. There's no way to query the flag, though we're still at the stage we can make modifications to the firmware if there's justification. One of the design goals is to minimize unnecessary USB traffic so if there's a place to clear the flag in the driver without querying it would be preferable. A potential problem with using prepare_write_buffer would be on failures to submit the write urb, in which case this flag might never be cleared. Yes, this could be a problem though I wonder how many (if any) recoverable situations this would happen in that didn't eventually lead to a transmission. Adding a timer with a long timeout that set tx_empty = true crossed my mind but that doesn't really seem any less error prone than the original issue. I could of course duplicate a bunch of code from generic.c and make a few minor changes to set tx_empty = true but that obviously isn't desirable either. If none of the above solutions sound acceptable, let me know I and I guess I'll change the firmware to allow polling of TXEMPTY with a control message and remove it from the bulk in packet. I think it would be best if you could add a way to query the driver. It seems like that is the only way to avoid having write race with the tx_empty bulk-in status reports. Thanks, Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: add nt124 usb to serial driver
On Fri, Dec 12, 2014 at 09:01:03AM -0600, George McCollister wrote:
On Wed, Dec 10, 2014 at 7:04 AM, Johan Hovold jo...@kernel.org wrote:
On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:
+ switch (termios-c_cflag CSIZE) {
C_CSIZE(tty)
Okay
+ case CS5:
+ newline.bDataBits = 5;
+ break;
+ case CS6:
+ newline.bDataBits = 6;
+ break;
I should probably just remove CS5 and CS6 since I don't think they
will work anyway.
Ok. Remember to update the passed termios with the settings that you
actually end up using (i.e. settings from old_termios).
+static int nt124_open(struct tty_struct *tty,
+ struct usb_serial_port *port)
+{
+ struct nt124_private *priv = usb_get_serial_port_data(port);
+ int result = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(port-lock, flags);
+ port-throttled = 0;
+ port-throttle_req = 0;
+ spin_unlock_irqrestore(port-lock, flags);
+
+ priv-flowctrl = 0;
Drop this and keep the current settings.
Okay
+ nt124_set_termios(tty, port, NULL);
+ nt124_set_flowctrl(port, priv-flowctrl);
Drop this. This is handled by tty and dtr_rts().
Okay
+
+ if (port-bulk_in_size)
+ result = usb_serial_generic_submit_read_urbs(port,
GFP_KERNEL);
Call usb_serial_generic_open() instead (and skip the throttle-flags bit
above).
Okay, so looks like the only thing I will do in this function is call
nt124_set_termios() followed by usb_serial_generic_open().
Yes, that should do it.
+static struct usb_serial_driver nt124_device = {
+ .driver = {
+ .owner =THIS_MODULE,
+ .name = nt124,
+ },
+ .id_table = id_table,
+ .num_ports =1,
+ .bulk_in_size = 32,
+ .bulk_out_size =32,
Why do you reduce these buffer sizes? They can be bigger than the
endpoint size for increased throughput.
I'll leave them out like most of the other drivers (and retest) unless
you have another suggestion.
That's perfectly fine, and means that the buffer sizes will match the
endpoint sizes (they will in fact never be smaller than that).
Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: add nt124 usb to serial driver
On Mon, Dec 15, 2014 at 3:52 AM, Johan Hovold jo...@kernel.org wrote: On Sun, Dec 14, 2014 at 11:51:11AM -0600, George McCollister wrote: Johan, While working on the tx_empty changes you suggested it occurred to me that it might not be obvious to others that the firmware doesn't send a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins transmitting. The practical implication is that if the driver sets tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be reset to false somewhere when more data is transmitted. Perhaps I could add prepare_write_buffer and do it in there before calling usb_serial_generic_prepare_write_buffer(). Does that sound acceptable? Hmm. There's no way to query that flag? And the status is sent (as bulk in data) periodically or only when data has been received? And not when the actual status changes? The bulk in packets are not sent periodically only on TXEMPTY, other line change or received data. There's no way to query the flag, though we're still at the stage we can make modifications to the firmware if there's justification. One of the design goals is to minimize unnecessary USB traffic so if there's a place to clear the flag in the driver without querying it would be preferable. A potential problem with using prepare_write_buffer would be on failures to submit the write urb, in which case this flag might never be cleared. Yes, this could be a problem though I wonder how many (if any) recoverable situations this would happen in that didn't eventually lead to a transmission. Adding a timer with a long timeout that set tx_empty = true crossed my mind but that doesn't really seem any less error prone than the original issue. I could of course duplicate a bunch of code from generic.c and make a few minor changes to set tx_empty = true but that obviously isn't desirable either. If none of the above solutions sound acceptable, let me know I and I guess I'll change the firmware to allow polling of TXEMPTY with a control message and remove it from the bulk in packet. Thanks again, George Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: add nt124 usb to serial driver
Johan, While working on the tx_empty changes you suggested it occurred to me that it might not be obvious to others that the firmware doesn't send a packet with the NT124_CTRL_TXEMPTY flag cleared when it begins transmitting. The practical implication is that if the driver sets tx_empty = true when it sees NT124_CTRL_TXEMPTY, tx_empty must be reset to false somewhere when more data is transmitted. Perhaps I could add prepare_write_buffer and do it in there before calling usb_serial_generic_prepare_write_buffer(). Does that sound acceptable? If so I'll also initialize tx_empty = true in nt124_port_probe. Regards, George -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: add nt124 usb to serial driver
Johan,
Thanks for the thorough review.
On Wed, Dec 10, 2014 at 7:04 AM, Johan Hovold jo...@kernel.org wrote:
On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:
This driver is for the NovaTech 124 4x serial expansion board for the
NovaTech OrionLXm.
Firmware source code can be found here:
https://github.com/novatechweb/nt124
Great, and thanks for the patch!
Signed-off-by: George McCollister george.mccollis...@gmail.com
---
drivers/usb/serial/Kconfig | 9 +
drivers/usb/serial/Makefile | 1 +
drivers/usb/serial/nt124.c | 429
3 files changed, 439 insertions(+)
create mode 100644 drivers/usb/serial/nt124.c
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index a69f7cd..6dfc340 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -509,6 +509,15 @@ config USB_SERIAL_NAVMAN
To compile this driver as a module, choose M here: the
module will be called navman.
+config USB_SERIAL_NT124
+ tristate USB nt124 serial device
USB NovaTech 124 Serial Driver (or NovaTech nt124)
I'll use USB NovaTech 124 Serial Driver
+ help
+ Say Y here if you want to use the NovaTech 124 4x USB to serial
+ board.
+
+ To compile this driver as a module, choose M here: the
+ module will be called nt124.
+
config USB_SERIAL_PL2303
tristate USB Prolific 2303 Single Port Serial Driver
help
diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
index 349d9df..f88eaab 100644
--- a/drivers/usb/serial/Makefile
+++ b/drivers/usb/serial/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_USB_SERIAL_MOS7720)+= mos7720.o
obj-$(CONFIG_USB_SERIAL_MOS7840) += mos7840.o
obj-$(CONFIG_USB_SERIAL_MXUPORT) += mxuport.o
obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o
+obj-$(CONFIG_USB_SERIAL_NT124) += nt124.o
obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o
obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
obj-$(CONFIG_USB_SERIAL_OPTION) += option.o
diff --git a/drivers/usb/serial/nt124.c b/drivers/usb/serial/nt124.c
new file mode 100644
index 000..d7557ff
--- /dev/null
+++ b/drivers/usb/serial/nt124.c
@@ -0,0 +1,429 @@
+/*
+ * nt124.c
Put a brief description here instead.
Okay
+ *
+ * Copyright (c) 2014 NovaTech LLC
+ *
+ * Driver for nt124 4x serial board based on STM32F103
For example use something like this above.
Okay
+ *
+ * Portions derived from the cdc-acm driver
+ *
+ * The original intention was to implement a cdc-acm compliant
+ * 4x USB to serial converter in the STM32F103 however several problems
arose.
+ * The STM32F103 didn't have enough end points to implement 4 ports.
+ * CTS control was required by the application.
+ * Accurate notification of transmission completion was required.
+ * RTSCTS flow control support was required.
+ *
+ * The interrupt endpoint was eliminated and the control line information
+ * was moved to the first two bytes of the in endpoint message. CTS control
bulk in endpoint
Okay
+ * and mechanisms to enable RTSCTS flow control and deliver TXEMPTY
+ * information were added.
+ *
+ * Firmware source code can be found here:
+ * https://github.com/novatechweb/nt124
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/kernel.h
+#include linux/slab.h
+#include linux/tty.h
+#include linux/tty_driver.h
+#include linux/tty_flip.h
+#include linux/module.h
+#include linux/usb.h
+#include linux/usb/serial.h
+#include asm/unaligned.h
+
+#define NT124_VID0x2aeb
+#define NT124_USB_PID124
+
+#define DRIVER_AUTHOR George McCollister george.mccollis...@gmail.com
+#define DRIVER_DESC nt124 USB serial driver
Just use the MODULE macros directly (at the end of the file), no need
for the defines.
Okay
+
+static const struct usb_device_id id_table[] = {
+ { USB_DEVICE(NT124_VID, NT124_USB_PID) },
+ { },
+};
+
+MODULE_DEVICE_TABLE(usb, id_table);
+
+/*
+ * Output control lines.
+ */
+
No new line.
Okay
+#define NT124_CTRL_DTR 0x01
+#define NT124_CTRL_RTS 0x02
+
+/*
+ * Input control lines and line errors.
+ */
+
Same here.
Okay
+#define NT124_CTRL_DCD 0x01
+#define NT124_CTRL_DSR 0x02
+#define NT124_CTRL_BRK 0x04
Re: Re: [PATCH] USB: serial: add nt124 usb to serial driver
Johan Hovold jo...@kernel.org wrote: On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote: + newline.bParityType = termios-c_cflag PARENB ? + (termios-c_cflag PARODD ? 1 : 2) + + (termios-c_cflag CMSPAR ? 2 : 0) : 0; This hardly readable. Don't use ?: There's also C_PARENB(tty), C_PARODD(tty), and C_CMSPAR(tty) macros available, in addition to the others that Johan pointed out. Sincerely, Karl P
Re: [PATCH] USB: serial: add nt124 usb to serial driver
On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:
This driver is for the NovaTech 124 4x serial expansion board for the
NovaTech OrionLXm.
Firmware source code can be found here:
https://github.com/novatechweb/nt124
Great, and thanks for the patch!
Signed-off-by: George McCollister george.mccollis...@gmail.com
---
drivers/usb/serial/Kconfig | 9 +
drivers/usb/serial/Makefile | 1 +
drivers/usb/serial/nt124.c | 429
3 files changed, 439 insertions(+)
create mode 100644 drivers/usb/serial/nt124.c
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index a69f7cd..6dfc340 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -509,6 +509,15 @@ config USB_SERIAL_NAVMAN
To compile this driver as a module, choose M here: the
module will be called navman.
+config USB_SERIAL_NT124
+ tristate USB nt124 serial device
USB NovaTech 124 Serial Driver (or NovaTech nt124)
+ help
+ Say Y here if you want to use the NovaTech 124 4x USB to serial
+ board.
+
+ To compile this driver as a module, choose M here: the
+ module will be called nt124.
+
config USB_SERIAL_PL2303
tristate USB Prolific 2303 Single Port Serial Driver
help
diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
index 349d9df..f88eaab 100644
--- a/drivers/usb/serial/Makefile
+++ b/drivers/usb/serial/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_USB_SERIAL_MOS7720)+= mos7720.o
obj-$(CONFIG_USB_SERIAL_MOS7840) += mos7840.o
obj-$(CONFIG_USB_SERIAL_MXUPORT) += mxuport.o
obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o
+obj-$(CONFIG_USB_SERIAL_NT124) += nt124.o
obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o
obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
obj-$(CONFIG_USB_SERIAL_OPTION) += option.o
diff --git a/drivers/usb/serial/nt124.c b/drivers/usb/serial/nt124.c
new file mode 100644
index 000..d7557ff
--- /dev/null
+++ b/drivers/usb/serial/nt124.c
@@ -0,0 +1,429 @@
+/*
+ * nt124.c
Put a brief description here instead.
+ *
+ * Copyright (c) 2014 NovaTech LLC
+ *
+ * Driver for nt124 4x serial board based on STM32F103
For example use something like this above.
+ *
+ * Portions derived from the cdc-acm driver
+ *
+ * The original intention was to implement a cdc-acm compliant
+ * 4x USB to serial converter in the STM32F103 however several problems
arose.
+ * The STM32F103 didn't have enough end points to implement 4 ports.
+ * CTS control was required by the application.
+ * Accurate notification of transmission completion was required.
+ * RTSCTS flow control support was required.
+ *
+ * The interrupt endpoint was eliminated and the control line information
+ * was moved to the first two bytes of the in endpoint message. CTS control
bulk in endpoint
+ * and mechanisms to enable RTSCTS flow control and deliver TXEMPTY
+ * information were added.
+ *
+ * Firmware source code can be found here:
+ * https://github.com/novatechweb/nt124
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/kernel.h
+#include linux/slab.h
+#include linux/tty.h
+#include linux/tty_driver.h
+#include linux/tty_flip.h
+#include linux/module.h
+#include linux/usb.h
+#include linux/usb/serial.h
+#include asm/unaligned.h
+
+#define NT124_VID0x2aeb
+#define NT124_USB_PID124
+
+#define DRIVER_AUTHOR George McCollister george.mccollis...@gmail.com
+#define DRIVER_DESC nt124 USB serial driver
Just use the MODULE macros directly (at the end of the file), no need
for the defines.
+
+static const struct usb_device_id id_table[] = {
+ { USB_DEVICE(NT124_VID, NT124_USB_PID) },
+ { },
+};
+
+MODULE_DEVICE_TABLE(usb, id_table);
+
+/*
+ * Output control lines.
+ */
+
No new line.
+#define NT124_CTRL_DTR 0x01
+#define NT124_CTRL_RTS 0x02
+
+/*
+ * Input control lines and line errors.
+ */
+
Same here.
+#define NT124_CTRL_DCD 0x01
+#define NT124_CTRL_DSR 0x02
+#define NT124_CTRL_BRK 0x04
+#define NT124_CTRL_RI0x08
+#define NT124_CTRL_FRAMING 0x10
+#define NT124_CTRL_PARITY0x20
+#define NT124_CTRL_OVERRUN 0x40
+#define NT124_CTRL_TXEMPTY
Re: [PATCH] USB: serial: add nt124 usb to serial driver
George, On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote: This driver is for the NovaTech 124 4x serial expansion board for the NovaTech OrionLXm. Firmware source code can be found here: https://github.com/novatechweb/nt124 Signed-off-by: George McCollister george.mccollis...@gmail.com --- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/nt124.c | 429 3 files changed, 439 insertions(+) create mode 100644 drivers/usb/serial/nt124.c diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig [...] It applies to next without any errors, there are no checkpatch issues other than recommending updating the MAINTAINERS file, and I can't find any other obvious problems. Looks good to me. Reviewed-by: Jeremiah Mahler jmmah...@gmail.com -- - Jeremiah Mahler -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: add nt124 usb to serial driver
Thanks for the review. I didn't update the MAINTAINERS file because I looked in it and didn't see any other individual usb/serial drivers listed with maintainers. -George On Mon, Dec 8, 2014 at 6:29 PM, Jeremiah Mahler jmmah...@gmail.com wrote: George, On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote: This driver is for the NovaTech 124 4x serial expansion board for the NovaTech OrionLXm. Firmware source code can be found here: https://github.com/novatechweb/nt124 Signed-off-by: George McCollister george.mccollis...@gmail.com --- drivers/usb/serial/Kconfig | 9 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/nt124.c | 429 3 files changed, 439 insertions(+) create mode 100644 drivers/usb/serial/nt124.c diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig [...] It applies to next without any errors, there are no checkpatch issues other than recommending updating the MAINTAINERS file, and I can't find any other obvious problems. Looks good to me. Reviewed-by: Jeremiah Mahler jmmah...@gmail.com -- - Jeremiah Mahler -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
