USB: serial: add Moxa UPORT 11x0 driver
Subsystems (2)
PATCH 1/1 USB: serial: add Moxa UPORT 11x0 driver
Add a driver which supports :
- UPort 1110 : 1 port RS-232 USB to Serial Hub.
- UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
- UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
- UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
- UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
This driver is based on GPL MOXA driver written by Hen Huang and available on MOXA website. The original driver was based on io_ti serial driver.
--- drivers/usb/serial/Kconfig | 16 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/mxu11x0.c | 1500 ++++++++++++++++++++++++++++++++++++++++++ drivers/usb/serial/mxu11x0.h | 199 ++++++ 4 files changed, 1716 insertions(+) create mode 100644 drivers/usb/serial/mxu11x0.c create mode 100644 drivers/usb/serial/mxu11x0.h
drivers/usb/serial/Kconfig +16 -0
@@ -475,6 +475,22 @@ config USB_SERIAL_MOS7840 To compile this driver as a module, choose M here: the module will be called mos7840. If unsure, choose N. +config USB_SERIAL_MXUPORT11 + tristate "USB Moxa UPORT 11x0 Serial Driver" + ---help--- + Say Y here if you want to use a MOXA UPort 11x0 Serial hub. + + This driver supports: + + - UPort 1110 : 1 port RS-232 USB to Serial Hub. + - UPort 1130 : 1 port RS-422/485 USB to Serial Hub. + - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation. + - UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub. + - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation. + + To compile this driver as a module, choose M here: the + module will be called mxu11x0. + config USB_SERIAL_MXUPORT tristate "USB Moxa UPORT Serial Driver" ---help---
drivers/usb/serial/Makefile +1 -0
@@ -38,6 +38,7 @@ obj-$(CONFIG_USB_SERIAL_METRO) += metro-usb.o 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_MXUPORT11) += mxu11x0.o obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
drivers/usb/serial/mxu11x0.c new file +1500 -0
1,500 lines not on this page · raw message
drivers/usb/serial/mxu11x0.h new file +199 -0
199 lines not on this page · raw message
Signature · 1 line
2.5.1
Johan Hovold
maintainerQuoted 8 lines · Mathieu OTHACEHE
Add a driver which supports :
- UPort 1110 : 1 port RS-232 USB to Serial Hub.
- UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
- UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
- UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
- UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
Quoted 4 lines
This driver is based on GPL MOXA driver written by Hen Huang and available on MOXA website. The original driver was based on
io_tiserial driver.Signed-off-by Mathieu OTHACEHE <[email protected]>
Thanks for submitting this driver. Code looks clean, but you should be able to reuse a whole lot more of the generic usb-serial implementation, something which should also give better throughput.
Specifically, most of the read and write code can simply be dropped in favour of the generic implementation.
You must also get rid of the Moxa specific ioctl to set the port mode. Have a look at the TIOCSRS485 ioctl as a basis for this change (but note that we may need to extend that interface).
Quoted 5 lines
--- drivers/usb/serial/Kconfig | 16 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/mxu11x0.c | 1500 ++++++++++++++++++++++++++++++++++++++++++ drivers/usb/serial/mxu11x0.h | 199 ++++++
Just merge the header file with the implementation as it is not used outside of it.
Quoted 1 line
+static int
closing_wait=MXU1_DEFAULT_CLOSING_WAIT;
This should be configurable per device using TIOCSSERIAL, just drop the module parameter.
drivers/usb/serial/mxu11x0.c line 187
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+static int mxu1_download_firmware (struct usb_serial *serial, +const struct firmware *fw_p) +{ +int status =0 ; +int buffer_size; +int pos; +int len; +int done; +__u8 cs =0 ; +__u8 *buffer;
Only use __u8 when implementing a user-space API (use u8).
Quoted 14 lines
drivers/usb/serial/mxu11x0.c new file +14
-0+struct usb_device *dev = serial->dev; +struct mxu1_firmware_header *header; +unsigned int pipe =usb_sndbulkpipe (dev, serial->port[0 ]-> + bulk_out_endpointAddress); + + buffer_size = fw_p->size + +sizeof (struct mxu1_firmware_header ); + buffer =kmalloc (buffer_size,GFP_KERNEL ); +if (!buffer) +return -ENOMEM ; + +memcpy (buffer, fw_p->data, fw_p->size); +memset (buffer + fw_p->size,0xff , buffer_size - fw_p->size); +
drivers/usb/serial/mxu11x0.c line 211
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+for (pos =sizeof (struct mxu1_firmware_header ); + pos < buffer_size; pos++) + cs = (__u8)(cs + buffer[pos]); + + header = (struct mxu1_firmware_header *)buffer; + header->wLength =cpu_to_le16 ( + (__u16)(buffer_size -sizeof (struct mxu1_firmware_header ))); + header->bCheckSum = cs; + +pr_debug ("%s - downloading firmware" , __func__);
Drop all pr_debug (and friends) in favour of the more descriptive dev_dbg.
Quoted 16 lines
drivers/usb/serial/mxu11x0.c new file +16
-0+static int mxu1_startup (struct usb_serial *serial) +{ +struct mxu1_device *mxdev; +struct usb_device *dev = serial->dev; +char fw_name[32 ]; +const struct firmware *fw_p =NULL ; +u16 product_id; +int err; + +pr_debug ("%s - product 0x%4X, num configurations %d, configuration value %d" , + __func__,le16_to_cpu (dev->descriptor.idProduct), + dev->descriptor.bNumConfigurations, + dev->actconfig->desc.bConfigurationValue); + +/* create device structure */ + mxdev =kzalloc (sizeof (struct mxu1_device ),GFP_KERNEL );
drivers/usb/serial/mxu11x0.c line 299
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+if (mxdev ==NULL ) +return -ENOMEM ; + +mutex_init (&mxdev->mxd_lock); + mxdev->mxd_serial = serial; +usb_set_serial_data (serial, mxdev); + +/* determine device type */ +if (!usb_match_id (serial->interface, mxuport11_idtable)) +return 0 ;
This would already have been taken care of by usb-serial core.
drivers/usb/serial/mxu11x0.c line 301
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+ + product_id = dev->descriptor.idProduct;
Missing le16_to_cpu().
drivers/usb/serial/mxu11x0.c line 403
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+static int mxu1_set_mcr (struct mxu1_port *mxport,unsigned int mcr) +{ +int status =0 ; + + status =mxu1_write_byte (mxport->mxp_mxdev, + mxport->mxp_uart_base_addr + +MXU1_UART_OFFSET_MCR , +MXU1_MCR_RTS |MXU1_MCR_DTR |MXU1_MCR_LOOP , + mcr); + +if (!status) + mxport->mxp_shadow_mcr = mcr;
No locking?
drivers/usb/serial/mxu11x0.c line 613
Quoted 13 lines
drivers/usb/serial/mxu11x0.c new file +13
-0+static int mxu1_set_serial_info (struct mxu1_port *mxport, +struct serial_struct __user *new_arg) +{ +struct serial_struct new_serial; + +if (copy_from_user (&new_serial, new_arg,sizeof (new_serial))) +return -EFAULT ; + + mxport->mxp_flags = new_serial.flags &MXU1_SET_SERIAL_FLAGS ; + +if (mxport->mxp_mxdev->mxd_model_name !=MXU1_MODEL_1110 ) { +/* UPort 1130, 1150, 1150I */ +switch (new_serial.port) {
Hmm, this is not how to switch the port mode, use the ioctl mentioned above.
drivers/usb/serial/mxu11x0.c line 620
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+case MXU1_RS232 :/* UPort 1150, 1150I */ +if (mxport->mxp_mxdev->mxd_model_name == +MXU1_MODEL_1130 || + mxport->mxp_mxdev->mxd_model_name == +MXU1_MODEL_1131 ) { +return -EINVAL ; + }
Instead of sprinkling the driver with model conditionals, consider abstracting this into device type-flags that you can test for instead (e.g. MXU1_TYPE_RS232_ONLY).
Quoted 41 lines
drivers/usb/serial/mxu11x0.c new file +41
-0+static int mxu1_ioctl (struct tty_struct *tty, +unsigned int cmd,unsigned long arg) +{ +struct usb_serial_port *port = tty->driver_data; + +struct mxu1_port *mxport =usb_get_serial_port_data (port); +struct async_icount cnow; +struct async_icount cprev; + +pr_debug ("%s - port %d, cmd = 0x%04X" , + __func__, port->port_number, cmd); + +if (mxport ==NULL ) +return -ENODEV ; + +switch (cmd) { +case TIOCGSERIAL : +pr_debug ("%s - (%d) TIOCGSERIAL" , __func__, port->port_number); +return mxu1_get_serial_info (mxport, + (struct serial_struct __user *)arg); + +case TIOCSSERIAL : +pr_debug ("%s - (%d) TIOCSSERIAL" , __func__, port->port_number); +return mxu1_set_serial_info (mxport, + (struct serial_struct __user *)arg); + +case TIOCMIWAIT : +pr_debug ("%s - (%d) TIOCMIWAIT" , __func__, port->port_number); + cprev = mxport->mxp_icount; + mxport->mxp_msr_wait_flags =1 ; +while (1 ) { +wait_event_interruptible_timeout + (mxport->mxp_msr_wait, + (mxport->mxp_msr_wait_flags ==0 ), +MXU1_MSR_WAIT_TIMEOUT ); + +if (signal_pending (current)) +return -ERESTARTSYS ; + cnow = mxport->mxp_icount; +if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr && + cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
drivers/usb/serial/mxu11x0.c line 700
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+return -EIO ;/* no change => error */ +if (((arg &TIOCM_RNG ) && (cnow.rng != cprev.rng)) || + ((arg &TIOCM_DSR ) && (cnow.dsr != cprev.dsr)) || + ((arg &TIOCM_CD ) && (cnow.dcd != cprev.dcd)) || + ((arg &TIOCM_CTS ) && (cnow.cts != cprev.cts))) { +return 0 ; + } + cprev = cnow; + } +break ;
Use the generic implementation of this.
drivers/usb/serial/mxu11x0.c line 712
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+ +case TIOCGICOUNT : +pr_debug ("%s - (%d) TIOCGICOUNT RX=%d, TX=%d" , + __func__, + port->port_number, mxport->mxp_icount.rx, + mxport->mxp_icount.tx); + +if (copy_to_user ((void __user *)arg, + &mxport->mxp_icount, +sizeof (mxport->mxp_icount))) +return -EFAULT ; +return 0 ;
And this.
Quoted 24 lines
drivers/usb/serial/mxu11x0.c new file +24
-0+ +case MOXA_SET_INTERFACE : +pr_debug ("%s - port%d MOXA_SET_INTERFACE=%d" , __func__, + port->port_number, (int )arg); + +if (mxport->mxp_mxdev->mxd_model_name !=MXU1_MODEL_1110 ) { +/* UPort 1130, 1150, 1150I */ +switch (arg) { +case MXU1_RS232 :/* UPort 1150, 1150I */ +if (mxport->mxp_mxdev->mxd_model_name + ==MXU1_MODEL_1130 || + mxport->mxp_mxdev->mxd_model_name + ==MXU1_MODEL_1131 ) { +return -EINVAL ; + } + mxport->mxp_uart_mode =MXU1_UART_232 ; +break ; +case MXU1_RS422 :/* UPort 1130, 1150, 1150I */ +case MXU1_RS4854W :/* UPort 1130, 1150, 1150I */ + mxport->mxp_uart_mode = +MXU1_UART_485_RECEIVER_ENABLED ; +break ; + +case MXU1_RS4852W :/* UPort 1130, 1150, 1150I */
drivers/usb/serial/mxu11x0.c line 746
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ mxport->mxp_uart_mode = +MXU1_UART_485_RECEIVER_DISABLED ; +break ; +default : +return -EINVAL ; + } + }else {/* UPort 1110 */ +if (arg !=MXU1_RS232 ) +return -EINVAL ; + }
And drop this one in favour of the standard interface.
Quoted 6 lines
drivers/usb/serial/mxu11x0.c new file +6
-0+static int mxu1_tiocmget (struct tty_struct *tty) +{ +struct usb_serial_port *port = tty->driver_data; + +struct mxu1_port *mxport =usb_get_serial_port_data (port); +unsigned int result;
drivers/usb/serial/mxu11x0.c line 769
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+unsigned int msr; +unsigned int mcr; + +pr_debug ("%s - port %d" , __func__, port->port_number); + +if (mxport ==NULL ) +return -ENODEV ; + + msr = mxport->mxp_msr; + mcr = mxport->mxp_shadow_mcr;
Again, no locking.
Quoted 9 lines
drivers/usb/serial/mxu11x0.c new file +9
-0+static int mxu1_open (struct tty_struct *tty,struct usb_serial_port *port) +{ +struct mxu1_port *mxport =usb_get_serial_port_data (port); +struct mxu1_device *mxdev; +struct usb_device *dev; +struct urb *urb; +int port_number; +int status; + __u16 open_settings = (__u8)(MXU1_PIPE_MODE_CONTINUOUS |
drivers/usb/serial/mxu11x0.c line 930
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+MXU1_PIPE_TIMEOUT_ENABLE | + (MXU1_TRANSFER_TIMEOUT << 2)); +if (!mxport) +return -ENODEV ; + + dev = port->serial->dev; + mxdev = mxport->mxp_mxdev; + +if (port->port.tty) + port->port.low_latency =MXU1_DEFAULT_LOW_LATENCY ;
This flag should not be set.
Quoted 20 lines
drivers/usb/serial/mxu11x0.c new file +20
-0+ + port_number = port->port_number - port->minor; + + mxport->mxp_msr =0 ; + mxport->mxp_shadow_mcr |= (MXU1_MCR_RTS |MXU1_MCR_DTR ); + +if (mutex_lock_interruptible (&mxdev->mxd_lock)) +return -ERESTARTSYS ; + +/* start interrupt urb the first time a port is opened on this device */ +if (mxdev->mxd_open_port_count ==0 ) { +pr_debug ("%s - start interrupt in urb" , __func__); + urb = mxdev->mxd_serial->port[0 ]->interrupt_in_urb; +if (!urb) { +dev_err (&port->dev, +"%s - no interrupt urb\n" , + __func__); + status = -EINVAL ; + goto release_mxd_lock; + }
drivers/usb/serial/mxu11x0.c line 960
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ urb->context = mxdev; + status =usb_submit_urb (urb,GFP_KERNEL ); +if (status) { +dev_err (&port->dev, +"%s - submit interrupt urb failed, %d\n" , + __func__, + status); + goto release_mxd_lock; + } + }
This does not seem to make sense, since this is a single port device. Just submit the interrupt urb unconditionally at open.
drivers/usb/serial/mxu11x0.c line 1500
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+module_param (closing_wait, int,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (closing_wait,"Maximum wait for data to drain, in .01 secs" );
And as mentioned above, just drop this.
drivers/usb/serial/mxu11x0.h line 128
Quoted 10 lines
drivers/usb/serial/mxu11x0.h new file +4
-0+/* Config struct */ +struct mxu1_uart_config { +__u16 wBaudRate; +__u16 wFlags;
Use __be16 (or __le16).
Quoted 9 lines
drivers/usb/serial/mxu11x0.h new file +8
-0+__u8 bDataBits; +__u8 bParity; +__u8 bStopBits; +char cXon; +char cXoff; +__u8 bUartMode; +} __packed; +struct mxu1_write_data_bytes {
drivers/usb/serial/mxu11x0.h line 166
Quoted 10 lines
drivers/usb/serial/mxu11x0.h new file +10
-0+__u8 bAddrType; +__u8 bDataType; +__u8 bDataCounter; +#ifdef __CHECK_ENDIAN__ +__be16 wBaseAddrHi; +__be16 wBaseAddrLo; +#else +__u16 wBaseAddrHi; +__u16 wBaseAddrLo; +#endif
Drop the ifdef.
Quoted 10 lines
drivers/usb/serial/mxu11x0.h new file +10
-0+__u8 bData[0 ]; +} __packed; + +/* Interrupt codes */ +#define MXU1_GET_PORT_FROM_CODE (c) (((c) >> 4) - 3) +#define MXU1_GET_FUNC_FROM_CODE (c) ((c) & 0x0f) +#define MXU1_CODE_HARDWARE_ERROR 0xFF +#define MXU1_CODE_DATA_ERROR 0x03 +#define MXU1_CODE_MODEM_STATUS 0x04 +
drivers/usb/serial/mxu11x0.h line 186
Quoted 10 lines
drivers/usb/serial/mxu11x0.h new file +10
-0+/* Download firmware max packet size */ +#define MXU1_DOWNLOAD_MAX_PACKET_SIZE 64 + +/* Firmware image header */ +struct mxu1_firmware_header { +#ifdef __CHECK_ +__le16 wLength; +#else +__u16 wLength; +#endif
Ditto.
drivers/usb/serial/mxu11x0.h line 188
Quoted 2 lines
drivers/usb/serial/mxu11x0.h new file +2
-0+__u8 bCheckSum; +} __packed;
Thanks,
Johan
PATCH v2 1/1 USB: serial: add Moxa UPORT 11x0 driver
Add a driver which supports :
- UPort 1110 : 1 port RS-232 USB to Serial Hub.
- UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
- UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
- UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
- UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
This driver is based on GPL MOXA driver written by Hen Huang and available on MOXA website. The original driver was based on io_ti serial driver.
---
Changes in v2:
* Use generic implementation for read/write code
* Use TIOCSRS485/TIOCGRS485 instead of moxa specific ioctl
* Other problems pointed out by Johan review
drivers/usb/serial/Kconfig | 16 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/mxu11x0.c | 1254 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1271 insertions(+) create mode 100644 drivers/usb/serial/mxu11x0.c
drivers/usb/serial/Kconfig +16 -0
@@ -475,6 +475,22 @@ config USB_SERIAL_MOS7840 To compile this driver as a module, choose M here: the module will be called mos7840. If unsure, choose N. +config USB_SERIAL_MXUPORT11 + tristate "USB Moxa UPORT 11x0 Serial Driver" + ---help--- + Say Y here if you want to use a MOXA UPort 11x0 Serial hub. + + This driver supports: + + - UPort 1110 : 1 port RS-232 USB to Serial Hub. + - UPort 1130 : 1 port RS-422/485 USB to Serial Hub. + - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation. + - UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub. + - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation. + + To compile this driver as a module, choose M here: the + module will be called mxu11x0. + config USB_SERIAL_MXUPORT tristate "USB Moxa UPORT Serial Driver" ---help---
drivers/usb/serial/Makefile +1 -0
@@ -38,6 +38,7 @@ obj-$(CONFIG_USB_SERIAL_METRO) += metro-usb.o 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_MXUPORT11) += mxu11x0.o obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
drivers/usb/serial/mxu11x0.c new file +1254 -0
1,254 lines not on this page · raw message
Signature · 1 line
2.5.3
Johan Hovold
maintainerQuoted 12 lines · Mathieu OTHACEHE
Add a driver which supports :
- UPort 1110 : 1 port RS-232 USB to Serial Hub.
- UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
- UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
- UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
- UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.This driver is based on GPL MOXA driver written by Hen Huang and available on MOXA website. The original driver was based on
io_tiserial driver.Signed-off-by Mathieu OTHACEHE <[email protected]>
<snip>
drivers/usb/serial/mxu11x0.c line 141
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+/* Operation modes */ +#define MXU1_UART_232 0x00 +#define MXU1_UART_485_RECEIVER_DISABLED 0x01 +#define MXU1_UART_485_RECEIVER_ENABLED 0x02 +#define MXU1_TYPE_RS232 (1 << 0) +#define MXU1_TYPE_RS422 (1 << 1) +#define MXU1_TYPE_RS485 (1 << 2)
You can use BIT(n) for these.
Quoted 58 lines
drivers/usb/serial/mxu11x0.c new file +58
-0+/* Pipe transfer mode and timeout */ +#define MXU1_PIPE_MODE_CONTINUOUS 0x01 +#define MXU1_PIPE_MODE_MASK 0x03 +#define MXU1_PIPE_TIMEOUT_MASK 0x7C +#define MXU1_PIPE_TIMEOUT_ENABLE 0x80 + +/* Config struct */ +struct mxu1_uart_config { +__be16 wBaudRate; +__be16 wFlags; +u8 bDataBits; +u8 bParity; +u8 bStopBits; +char cXon; +char cXoff; +u8 bUartMode; +} __packed; + +/* Purge modes */ +#define MXU1_PURGE_OUTPUT 0x00 +#define MXU1_PURGE_INPUT 0x80 + +/* Read/Write data */ +#define MXU1_RW_DATA_ADDR_SFR 0x10 +#define MXU1_RW_DATA_ADDR_IDATA 0x20 +#define MXU1_RW_DATA_ADDR_XDATA 0x30 +#define MXU1_RW_DATA_ADDR_CODE 0x40 +#define MXU1_RW_DATA_ADDR_GPIO 0x50 +#define MXU1_RW_DATA_ADDR_I2C 0x60 +#define MXU1_RW_DATA_ADDR_FLASH 0x70 +#define MXU1_RW_DATA_ADDR_DSP 0x80 + +#define MXU1_RW_DATA_UNSPECIFIED 0x00 +#define MXU1_RW_DATA_BYTE 0x01 +#define MXU1_RW_DATA_WORD 0x02 +#define MXU1_RW_DATA_DOUBLE_WORD 0x04 + +struct mxu1_write_data_bytes { +u8 bAddrType; +u8 bDataType; +u8 bDataCounter; +__be16 wBaseAddrHi; +__be16 wBaseAddrLo; +u8 bData[0 ]; +} __packed; + +/* Interrupt codes */ +#define MXU1_GET_PORT_FROM_CODE (c) (((c) >> 4) - 3) +#define MXU1_GET_FUNC_FROM_CODE (c) ((c) & 0x0f) +#define MXU1_CODE_HARDWARE_ERROR 0xFF +#define MXU1_CODE_DATA_ERROR 0x03 +#define MXU1_CODE_MODEM_STATUS 0x04 + +/* Download firmware max packet size */ +#define MXU1_DOWNLOAD_MAX_PACKET_SIZE 64 + +/* Firmware image header */ +struct mxu1_firmware_header {
drivers/usb/serial/mxu11x0.c line 210
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+__le16 wLength; +u8 bCheckSum; +} __packed; + +/* UART addresses */ +/* UART 1 base address */ +#define MXU1_UART1_BASE_ADDR 0xFFA0 +/* UART 2 base address*/ +#define MXU1_UART2_BASE_ADDR 0xFFB0 +#define MXU1_UART_OFFSET_LCR 0x0002
Why are these unused defines here? This driver is for one-port devices, right?
drivers/usb/serial/mxu11x0.c line 217
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+/*UART MCR register offset */ +#define MXU1_UART_OFFSET_MCR 0x0004 + +#define MXU1_SET_SERIAL_FLAGS 0 + +#define MXU1_TRANSFER_TIMEOUT 2 +#define MXU1_MSR_WAIT_TIMEOUT (5 * HZ)
This one does not seem to be used either.
drivers/usb/serial/mxu11x0.c line 221
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+ +/* Configuration ids */ +#define MXU1_BOOT_CONFIG 1 +#define MXU1_ACTIVE_CONFIG 2
Nor are these. Please drop unused defines unless useful for future feature additions (e.g. register definitions).
drivers/usb/serial/mxu11x0.c line 231
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ +#define MXU1_DEFAULT_CLOSING_WAIT 4000/* in .01 secs */ + +struct mxu1_port { +u8 mxp_msr; +u8 mxp_lsr; +u8 mxp_shadow_mcr; +u8 mxp_uart_types; +u8 mxp_uart_mode; +unsigned int mxp_uart_base_addr;
u32 or u16?
Quoted 32 lines
drivers/usb/serial/mxu11x0.c new file +32
-0+int mxp_flags; +int mxp_msr_wait_flags; +wait_queue_head_t mxp_msr_wait;/* wait for msr change */ +struct mxu1_device *mxp_mxdev; +struct usb_serial_port *mxp_port; +spinlock_t mxp_lock; +int mxp_send_break; +int mxp_set_B0; +}; + +struct mxu1_device { +struct mutex mxd_lock; +struct usb_serial *mxd_serial; +u16 mxd_model; +}; + +static const struct usb_device_id mxuport11_idtable[] = { + {USB_DEVICE (MXU1_VENDOR_ID ,MXU1_1110_PRODUCT_ID ) }, + {USB_DEVICE (MXU1_VENDOR_ID ,MXU1_1130_PRODUCT_ID ) }, + {USB_DEVICE (MXU1_VENDOR_ID ,MXU1_1150_PRODUCT_ID ) }, + {USB_DEVICE (MXU1_VENDOR_ID ,MXU1_1151_PRODUCT_ID ) }, + {USB_DEVICE (MXU1_VENDOR_ID ,MXU1_1131_PRODUCT_ID ) }, + { } +}; + +MODULE_DEVICE_TABLE (usb, mxuport11_idtable); + +/* Write the given buffer out to the control pipe. */ +static int mxu1_send_ctrl_data_urb (struct usb_serial *serial, +u8 request, +u16 value,u16 index, +u8 *data,size_t size)
drivers/usb/serial/mxu11x0.c line 273
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+{ +int status; + + status =usb_control_msg (serial->dev, +usb_sndctrlpipe (serial->dev,0 ), + request, + (USB_DIR_OUT |USB_TYPE_VENDOR | +USB_RECIP_DEVICE ), value, index, + data, size, +1000 );
Please use a define for the timeout.
Quoted 29 lines
drivers/usb/serial/mxu11x0.c new file +29
-0+if (status <0 ) { +dev_err (&serial->interface->dev, +"%s - usb_control_msg failed (%d)\n" , + __func__, status); +return status; + } + +if (status != size) { +dev_err (&serial->interface->dev, +"%s - short write (%d / %zd)\n" , + __func__, status, size); +return -EIO ; + } + +return 0 ; +} + +/* Send a vendor request without any data */ +static int mxu1_send_ctrl_urb (struct usb_serial *serial, +u8 request,u16 value,u16 index) +{ +return mxu1_send_ctrl_data_urb (serial, request, value, index, +NULL ,0 ); +} + +static int mxu1_download_firmware (struct usb_serial *serial, +const struct firmware *fw_p) +{ +int status =0 ;
drivers/usb/serial/mxu11x0.c line 312
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+int buffer_size; +int pos; +int len; +int done; +u8 cs =0 ; +u8 *buffer; +struct usb_device *dev = serial->dev; +struct mxu1_firmware_header *header; +unsigned int pipe =usb_sndbulkpipe (dev, serial->port[0 ]-> + bulk_out_endpointAddress);
Separate declaration and initialisation here.
drivers/usb/serial/mxu11x0.c line 315
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ + buffer_size = fw_p->size + +sizeof (struct mxu1_firmware_header );
Indent continuation lines at least two tabs further, but this line does appear to need to be broken at all.
drivers/usb/serial/mxu11x0.c line 323
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+ buffer =kmalloc (buffer_size,GFP_KERNEL ); +if (!buffer) +return -ENOMEM ; + +memcpy (buffer, fw_p->data, fw_p->size); +memset (buffer + fw_p->size,0xff , buffer_size - fw_p->size); + + for (pos = sizeof(struct mxu1_firmware_header);
sizeof(*header) to avoid having to break the line?
drivers/usb/serial/mxu11x0.c line 325
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+ pos < buffer_size; pos++) + cs = (u8)(cs + buffer[pos]);
Add brackets unless you can avoid the line break above.
drivers/usb/serial/mxu11x0.c line 329
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+ + header = (struct mxu1_firmware_header *)buffer; + header->wLength =cpu_to_le16 ( + (__u16)(buffer_size -sizeof (struct mxu1_firmware_header )));
No need for cast.
Use sizeof(*header).
drivers/usb/serial/mxu11x0.c line 337
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+ header->bCheckSum = cs; + +dev_dbg (&dev->dev,"%s - downloading firmware" , __func__); + +for (pos =0 ; pos < buffer_size; pos += done) { + len =min (buffer_size - pos,MXU1_DOWNLOAD_MAX_PACKET_SIZE ); + + status =usb_bulk_msg (dev, pipe, buffer+pos, len, &done,1000 );
Spaces around + operator.
Use a define for the timeout.
Quoted 1 line
+
Remove blank line.
drivers/usb/serial/mxu11x0.c line 347
Quoted 9 lines
drivers/usb/serial/mxu11x0.c new file +9
-0+if (status) +break ; + } + +kfree (buffer); + +if (status) { +dev_err (&dev->dev,"%s - error downloading firmware, %d\n" , + __func__, status);
No need for __func__ when you have a self-contained error message (which is preferred). Debug messages can be more brief and rely on __func__.
If possible try to use a consistent style for reporting errnos (e.g. "failed to download firmware: %d").
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+return status; + } + +msleep_interruptible (100 ); + + status =mxu1_send_ctrl_urb (serial,MXU1_RESET_EXT_DEVICE ,0 ,0 ); + +dev_dbg (&dev->dev,"%s - download successful (%d)" , __func__, status);
drivers/usb/serial/mxu11x0.c line 365
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ +return 0 ; +} + +static int mxu1_port_probe (struct usb_serial_port *port) +{ +struct mxu1_port *mxport; + + mxport =kzalloc (sizeof (struct mxu1_port ),GFP_KERNEL ); +
Don't add newline before checking return values.
drivers/usb/serial/mxu11x0.c line 371
Quoted 6 lines
drivers/usb/serial/mxu11x0.c new file +6
-0+if (!mxport) +return -ENOMEM ; + +spin_lock_init (&mxport->mxp_lock); + mxport->mxp_port = port; + mxport->mxp_mxdev =usb_get_serial_data (port->serial);
No need to store the serial data pointer in the port data, you can always access it through port->serial.
drivers/usb/serial/mxu11x0.c line 372
Quoted 1 line
drivers/usb/serial/mxu11x0.c new file +1
-0+ mxport->mxp_uart_base_addr =MXU1_UART1_BASE_ADDR ;
I already asked above, but when is any other base address ever used?
drivers/usb/serial/mxu11x0.c line 374
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+ +init_waitqueue_head (&mxport->mxp_msr_wait);
Use the port msr wait queue instead.
Quoted 13 lines
drivers/usb/serial/mxu11x0.c new file +13
-0+ +switch (mxport->mxp_mxdev->mxd_model) { +case MXU1_1110_PRODUCT_ID : + mxport->mxp_uart_types =MXU1_TYPE_RS232 ; + mxport->mxp_uart_mode =MXU1_UART_232 ; +break ; +case MXU1_1130_PRODUCT_ID : +case MXU1_1131_PRODUCT_ID : + mxport->mxp_uart_types =MXU1_TYPE_RS422 |MXU1_TYPE_RS485 ; + mxport->mxp_uart_mode =MXU1_UART_485_RECEIVER_DISABLED ; +break ; +case MXU1_1150_PRODUCT_ID : +case MXU1_1151_PRODUCT_ID :
drivers/usb/serial/mxu11x0.c line 397
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ mxport->mxp_uart_types = +MXU1_TYPE_RS232 |MXU1_TYPE_RS422 |MXU1_TYPE_RS485 ; + mxport->mxp_uart_mode =MXU1_UART_232 ; +break ; + } + +usb_set_serial_port_data (port, mxport); + + port->port.closing_wait = +msecs_to_jiffies (MXU1_DEFAULT_CLOSING_WAIT *10 );
Indent further.
Quoted 22 lines
drivers/usb/serial/mxu11x0.c new file +22
-0+ port->port.drain_delay =1 ; + +return 0 ; +} + +static int mxu1_startup (struct usb_serial *serial) +{ +struct mxu1_device *mxdev; +struct usb_device *dev = serial->dev; +char fw_name[32 ]; +const struct firmware *fw_p =NULL ; +int err; + +dev_dbg (&dev->dev,"%s - product 0x%4X, num configurations %d, configuration value %d" , + __func__,le16_to_cpu (dev->descriptor.idProduct), + dev->descriptor.bNumConfigurations, + dev->actconfig->desc.bConfigurationValue); + +/* create device structure */ + mxdev =kzalloc (sizeof (struct mxu1_device ),GFP_KERNEL ); +if (mxdev ==NULL ) +return -ENOMEM ;
drivers/usb/serial/mxu11x0.c line 429
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ +mutex_init (&mxdev->mxd_lock); + mxdev->mxd_serial = serial; +usb_set_serial_data (serial, mxdev); + + mxdev->mxd_model =le16_to_cpu (dev->descriptor.idProduct); + +/* if we have only 1 configuration, download firmware */ +if (dev->config->interface[0 ]->cur_altsetting-> + desc.bNumEndpoints ==1 ) {
Use an intermediate variable for the interface descriptors, which are accessible through serial->interface->cur_altsetting, to avoid line break.
drivers/usb/serial/mxu11x0.c line 432
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ + snprintf(fw_name, + sizeof(fw_name) - 1,
No need for -1 as size includes trailing NUL.
Quoted 11 lines
drivers/usb/serial/mxu11x0.c new file +11
-0+"moxa/moxa-%04x.fw" , + mxdev->mxd_model); + + err =request_firmware (&fw_p, fw_name, &serial->interface->dev); +if (err) { +dev_err (&serial->interface->dev,"Firmware %s not found\n" , + fw_name); +kfree (mxdev); +return err; + } +
drivers/usb/serial/mxu11x0.c line 453
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ err =mxu1_download_firmware (serial, fw_p); +if (err) { +kfree (mxdev); +return err; + } + + } + +if (fw_p) +release_firmware (fw_p);
Move this into the conditional block above and remove the NULL-test.
drivers/usb/serial/mxu11x0.c line 459
Quoted 6 lines
drivers/usb/serial/mxu11x0.c new file +6
-0+ +return 0 ; +} + +static int mxu1_write_byte (struct mxu1_device *mxdev,unsigned long addr, +u8 mask,u8 byte)
Pass the usb_serial_port as first parameter for port operations (and use it for any error or debug messages).
u32 for address?
drivers/usb/serial/mxu11x0.c line 462
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+{ +int status =0 ; +unsigned int size;
size_t
drivers/usb/serial/mxu11x0.c line 470
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+struct mxu1_write_data_bytes *data; +struct device *dev = &mxdev->mxd_serial->dev->dev; + +dev_dbg (dev,"%s - addr 0x%08lX, mask 0x%02X, byte 0x%02X" , __func__, + addr, mask, byte); + + size =sizeof (struct mxu1_write_data_bytes ) +2 ; + data =kmalloc (size,GFP_KERNEL );
kzalloc to clear the pad (?) bytes?
drivers/usb/serial/mxu11x0.c line 472
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+if (!data) { +dev_err (dev,"%s - out of memory\n" , __func__);
OOM errors would already have been logged so just return -ENOMEM here.
drivers/usb/serial/mxu11x0.c line 479
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+return -ENOMEM ; + } + + data->bAddrType =MXU1_RW_DATA_ADDR_XDATA ; + data->bDataType =MXU1_RW_DATA_BYTE ; + data->bDataCounter =1 ; + data->wBaseAddrHi =cpu_to_be16 (addr>>16 );
Spaces around binary operator missing.
drivers/usb/serial/mxu11x0.c line 488
Quoted 9 lines
drivers/usb/serial/mxu11x0.c new file +9
-0+ data->wBaseAddrLo =cpu_to_be16 (addr); + data->bData[0 ] = mask; + data->bData[1 ] = byte; + + status =mxu1_send_ctrl_data_urb (mxdev->mxd_serial,MXU1_WRITE_DATA ,0 , +MXU1_RAM_PORT , + (u8 *)data, + size); +
Stray new line again.
drivers/usb/serial/mxu11x0.c line 497
Quoted 9 lines
drivers/usb/serial/mxu11x0.c new file +9
-0+if (status <0 ) +dev_err (dev,"%s - failed, %d\n" , __func__, status); + +kfree (data); + +return status; +} + +static int mxu1_set_mcr (struct mxu1_port *mxport,unsigned int mcr)
So pass usb-serial struct here and elsewhere.
Review
Quoted 2 lines
(quoted diff) +2
-0+{ + int status = 0;
No need to initialise.
drivers/usb/serial/mxu11x0.c line 511
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+unsigned long flags; + + status =mxu1_write_byte (mxport->mxp_mxdev, + mxport->mxp_uart_base_addr + +MXU1_UART_OFFSET_MCR , +MXU1_MCR_RTS |MXU1_MCR_DTR |MXU1_MCR_LOOP , + mcr); + +spin_lock_irqsave (&mxport->mxp_lock, flags); +if (!status) + mxport->mxp_shadow_mcr = mcr; +spin_unlock_irqrestore (&mxport->mxp_lock, flags);
Use a mutex to protect shadow_mcr so that you can to this atomically.
drivers/usb/serial/mxu11x0.c line 522
Quoted 11 lines
drivers/usb/serial/mxu11x0.c new file +11
-0+ +return status; +} + +static void mxu1_set_termios (struct tty_struct *tty1, +struct usb_serial_port *port, +struct ktermios *old_termios) +{ +struct mxu1_port *mxport =usb_get_serial_port_data (port); + +struct tty_struct *tty = port->port.tty;
You cannot access the tty like this. Use the passed first argument instead.
Quoted 1 line
+
Remove stray newline above.
drivers/usb/serial/mxu11x0.c line 526
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+struct mxu1_uart_config *config; +tcflag_t cflag, iflag; +int baud;
speed_t
Review
Quoted 2 lines
(quoted diff) +2
-0+ int status = 0; + int port_number = port->port_number - port->minor;
Again, why do you need this as these are all one-port devices?
drivers/usb/serial/mxu11x0.c line 534
Quoted 6 lines
drivers/usb/serial/mxu11x0.c new file +6
-0+unsigned int mcr; + +dev_dbg (&port->dev,"%s - port %d" , __func__, port->port_number); + +if (!tty) { +dev_dbg (&port->dev,"%s - no tty or termios" , __func__);
A lot of debug messages lack a terminating \n. Fix throughout.
drivers/usb/serial/mxu11x0.c line 542
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+return ; + } + + cflag = tty->termios.c_cflag; + iflag = tty->termios.c_iflag; + +if (old_termios && cflag == old_termios->c_cflag + && iflag == old_termios->c_iflag) {
Use tty_termios_hw_change() if appropriate.
Place && before line break.
drivers/usb/serial/mxu11x0.c line 554
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+dev_dbg (&port->dev,"%s - nothing to change" , __func__); +return ; + } + +dev_dbg (&port->dev, +"%s - clfag %08x, iflag %08x" , __func__, cflag, iflag); + +if (old_termios) +dev_dbg (&port->dev,"%s - old clfag %08x, old iflag %08x" , + __func__, + old_termios->c_cflag, + old_termios->c_iflag);
Add brackets.
drivers/usb/serial/mxu11x0.c line 557
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ +if (mxport ==NULL ) +return ;
Not needed.
drivers/usb/serial/mxu11x0.c line 559
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+ + config =kmalloc (sizeof (*config),GFP_KERNEL );
Use kzalloc.
Quoted 21 lines
drivers/usb/serial/mxu11x0.c new file +21
-0+if (!config) +return ; + + config->wFlags =0 ; + +/* these flags must be set */ + config->wFlags |=MXU1_UART_ENABLE_MS_INTS ; + config->wFlags |=MXU1_UART_ENABLE_AUTO_START_DMA ; +if (mxport->mxp_send_break ==MXU1_LCR_BREAK ) + config->wFlags |=MXU1_UART_SEND_BREAK_SIGNAL ; + config->bUartMode = (u8)(mxport->mxp_uart_mode); + +switch (cflag &CSIZE ) { +case CS5 : + config->bDataBits =MXU1_UART_5_DATA_BITS ; +break ; +case CS6 : + config->bDataBits =MXU1_UART_6_DATA_BITS ; +break ; +case CS7 : + config->bDataBits =MXU1_UART_7_DATA_BITS ;
drivers/usb/serial/mxu11x0.c line 590
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+break ; +default : +case CS8 : + config->bDataBits =MXU1_UART_8_DATA_BITS ; +break ; + } + +if (cflag &PARENB ) { +if (cflag &PARODD ) { + config->wFlags |=MXU1_UART_ENABLE_PARITY_CHECKING ;
Move out of inner conditional.
drivers/usb/serial/mxu11x0.c line 597
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+ config->bParity =MXU1_UART_ODD_PARITY ; + }else { + config->wFlags |=MXU1_UART_ENABLE_PARITY_CHECKING ; + config->bParity =MXU1_UART_EVEN_PARITY ; + } + }else { + config->wFlags &= ~MXU1_UART_ENABLE_PARITY_CHECKING ;
You never set it.
Review
Quoted 2 lines
(quoted diff) +2
-0+ config->bParity = MXU1_UART_NO_PARITY; + }
You should also clear CMSPAR in the termios structure if you do not support it.
drivers/usb/serial/mxu11x0.c line 612
Quoted 13 lines
drivers/usb/serial/mxu11x0.c new file +13
-0+ +if (cflag &CSTOPB ) + config->bStopBits =MXU1_UART_2_STOP_BITS ; +else + config->bStopBits =MXU1_UART_1_STOP_BITS ; + +if (cflag &CRTSCTS ) { +/* RTS flow control must be off to drop RTS for baud rate B0 */ +if ((cflag &CBAUD ) !=B0 ) + config->wFlags |=MXU1_UART_ENABLE_RTS_IN ; + config->wFlags |=MXU1_UART_ENABLE_CTS_OUT ; + }else { + tty->hw_stopped =0 ;
No need to update this.
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+ } + +if (I_IXOFF (tty) ||I_IXON (tty)) { + config->cXon =START_CHAR (tty); + config->cXoff =STOP_CHAR (tty); + +if (I_IXOFF (tty))
drivers/usb/serial/mxu11x0.c line 629
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ config->wFlags |=MXU1_UART_ENABLE_X_IN ; + +if (I_IXON (tty)) + config->wFlags |=MXU1_UART_ENABLE_X_OUT ; + } + + baud =tty_get_baud_rate (tty); +if (!baud) + baud =9600 ; + config->wBaudRate = (__u16)(923077 / baud);
No need for cast. Missing cpu_to_be16 (store in temporary baud and flags variables if needed).
Use a define for the baud base.
drivers/usb/serial/mxu11x0.c line 640
Quoted 11 lines
drivers/usb/serial/mxu11x0.c new file +11
-0+ +dev_dbg (&port->dev,"%s - BaudRate=%d, wBaudRate=%d, wFlags=0x%04X, bDataBits=%d, bParity=%d, bStopBits=%d, cXon=%d, cXoff=%d, bUartMode=%d" , + __func__, baud, config->wBaudRate, config->wFlags, + config->bDataBits, config->bParity, config->bStopBits, + config->cXon, config->cXoff, config->bUartMode); + +cpu_to_be16s (&config->wBaudRate); +cpu_to_be16s (&config->wFlags); + + status = mxu1_send_ctrl_data_urb(port->serial, MXU1_SET_CONFIG, 0, + (u8)(MXU1_UART1_PORT + port_number),
port number again?
drivers/usb/serial/mxu11x0.c line 647
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+ (u8 *)config, + sizeof(*config)); +if (status) +dev_err (&port->dev,"%s - cannot set config on port %d, %d\n" , + __func__, + port_number, + status);
Use brackets around multi-line expressions for readability.
drivers/usb/serial/mxu11x0.c line 650
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ +/* SET_CONFIG asserts RTS and DTR, reset them correctly */ + mcr = mxport->mxp_shadow_mcr;
Locking?
drivers/usb/serial/mxu11x0.c line 655
Quoted 5 lines
drivers/usb/serial/mxu11x0.c new file +5
-0+/* if baud rate is B0, clear RTS and DTR */ +if ((cflag &CBAUD ) ==B0 ) { + + mcr &= ~(MXU1_MCR_DTR |MXU1_MCR_RTS ); + mxport->mxp_set_B0 = true;
Why do you need this?
You should also disable automatic flow control if enabled.
drivers/usb/serial/mxu11x0.c line 660
Quoted 5 lines
drivers/usb/serial/mxu11x0.c new file +5
-0+ }else { +if (mxport->mxp_set_B0) + mcr |=MXU1_MCR_DTR ; + + mxport->mxp_set_B0 = false;
Raise DTR/RTS, if changing from B0 (check old termios).
drivers/usb/serial/mxu11x0.c line 664
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+ } + + status =mxu1_set_mcr (mxport, mcr); +
Stray newline.
drivers/usb/serial/mxu11x0.c line 670
Quoted 6 lines
drivers/usb/serial/mxu11x0.c new file +6
-0+if (status) +dev_err (&port->dev, +"%s - cannot set modem control on port %d, %d\n" , + __func__, + port_number, + status);
Brackets.
drivers/usb/serial/mxu11x0.c line 676
Quoted 6 lines
drivers/usb/serial/mxu11x0.c new file +6
-0+ +kfree (config); +} + +static int mxu1_ioctl_get_rs485 (struct mxu1_port *mxport, +struct serial_rs485 __user *rs485) {
Bracket on new line (below as well).
Quoted 13 lines
drivers/usb/serial/mxu11x0.c new file +13
-0+struct serial_rs485 aux; + +memset (&aux,0 ,sizeof (aux)); + +if (mxport->mxp_uart_mode ==MXU1_UART_485_RECEIVER_ENABLED ) + aux.flags =SER_RS485_ENABLED ; + +if (copy_to_user (rs485, &aux,sizeof (aux))) +return -EFAULT ; + +return 0 ; +} +
drivers/usb/serial/mxu11x0.c line 699
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+static int mxu1_ioctl_set_rs485 (struct mxu1_port *mxport, +struct serial_rs485 __user *rs485_user) { +struct serial_rs485 rs485; +struct usb_serial_port *port = mxport->mxp_port; + +if (copy_from_user (&rs485, rs485_user,sizeof (*rs485_user))) +return -EFAULT ; + +if (mxport->mxp_uart_types & + (MXU1_TYPE_RS422 |MXU1_TYPE_RS485 )) {
Don't break line.
drivers/usb/serial/mxu11x0.c line 707
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+ +if (rs485.flags &SER_RS485_ENABLED ) { + mxport->mxp_uart_mode =MXU1_UART_485_RECEIVER_ENABLED ; + }else { +if (mxport->mxp_uart_types &MXU1_TYPE_RS232 ) + mxport->mxp_uart_mode =MXU1_UART_232 ; +else + mxport->mxp_uart_mode =MXU1_UART_485_RECEIVER_DISABLED ;
Indent using tabs, not spaces.
drivers/usb/serial/mxu11x0.c line 715
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+ } + }else { +dev_err (&port->dev,"%s not handled by MOXA UPort %04x\n" , + __func__, mxport->mxp_mxdev->mxd_model); +return -EINVAL ; + } + +mxu1_set_termios (NULL , mxport->mxp_port,NULL );
Why do you need to use set_termios for this update? Why not a dedicated helper?
Quoted 16 lines
drivers/usb/serial/mxu11x0.c new file +16
-0+ +return 0 ; +} + +static int mxu1_get_serial_info (struct mxu1_port *mxport, +struct serial_struct __user *ret_arg) +{ +struct usb_serial_port *port = mxport->mxp_port; +struct serial_struct ret_serial; +unsigned cwait; + +if (!ret_arg) +return -EFAULT ; + + cwait = port->port.closing_wait; +if (cwait !=ASYNC_CLOSING_WAIT_NONE )
drivers/usb/serial/mxu11x0.c line 741
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ cwait =jiffies_to_msecs (cwait) /10 ; + +memset (&ret_serial,0 ,sizeof (ret_serial)); + + ret_serial.type =PORT_16550A ; + ret_serial.line = port->minor; + ret_serial.port = port->port_number; + ret_serial.flags = mxport->mxp_flags; + ret_serial.xmit_fifo_size = port->bulk_out_size; + ret_serial.baud_base =tty_get_baud_rate (mxport->mxp_port->port.tty);
This is the baud base, not the current the baudrate.
Quoted 14 lines
drivers/usb/serial/mxu11x0.c new file +14
-0+ ret_serial.close_delay =5 *HZ ; + ret_serial.closing_wait = cwait; + +if (copy_to_user (ret_arg, &ret_serial,sizeof (*ret_arg))) +return -EFAULT ; + +return 0 ; +} + + +static int mxu1_set_serial_info (struct mxu1_port *mxport, +struct serial_struct __user *new_arg) +{ +struct serial_struct new_serial;
drivers/usb/serial/mxu11x0.c line 765
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+unsigned cwait; + +if (copy_from_user (&new_serial, new_arg,sizeof (new_serial))) +return -EFAULT ; + + cwait = new_serial.closing_wait; +if (cwait !=ASYNC_CLOSING_WAIT_NONE ) + cwait =msecs_to_jiffies (10 * new_serial.closing_wait); + + mxport->mxp_flags = new_serial.flags &MXU1_SET_SERIAL_FLAGS ;
You're not using the flags so drop them (the define above is also 0).
drivers/usb/serial/mxu11x0.c line 775
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ mxport->mxp_port->port.closing_wait = cwait; + +return 0 ; +} + +static int mxu1_ioctl (struct tty_struct *tty, +unsigned int cmd,unsigned long arg) +{ +struct usb_serial_port *port = tty->driver_data; +
Stray new line (just fix throughout).
drivers/usb/serial/mxu11x0.c line 779
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+struct mxu1_port *mxport =usb_get_serial_port_data (port); + +if (mxport ==NULL ) +return -ENODEV ;
Not needed.
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+ +switch (cmd) { +case TIOCGSERIAL : + return mxu1_get_serial_info(mxport,
drivers/usb/serial/mxu11x0.c line 793
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ (struct serial_struct __user *)arg); + +case TIOCSSERIAL : +return mxu1_set_serial_info (mxport, + (struct serial_struct __user *)arg); + +case TIOCGRS485 : +return mxu1_ioctl_get_rs485 (mxport, + (struct serial_rs485 __user *) + arg);
Don't break line before arg.
drivers/usb/serial/mxu11x0.c line 797
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+case TIOCSRS485 : +return mxu1_ioctl_set_rs485 (mxport, + (struct serial_rs485 __user *) + arg);
Ditto.
Quoted 9 lines
drivers/usb/serial/mxu11x0.c new file +9
-0+ } + +return -ENOIOCTLCMD ; +} + +static int mxu1_tiocmget (struct tty_struct *tty) +{ +struct usb_serial_port *port = tty->driver_data; +
drivers/usb/serial/mxu11x0.c line 816
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+struct mxu1_port *mxport =usb_get_serial_port_data (port); +unsigned int result; +unsigned int msr; +unsigned int mcr; +unsigned long flags; + +dev_dbg (&port->dev,"%s - port %d" , __func__, port->port_number); + +if (mxport ==NULL ) +return -ENODEV ;
Not needed. Fix throughout.
drivers/usb/serial/mxu11x0.c line 829
Quoted 13 lines
drivers/usb/serial/mxu11x0.c new file +13
-0+ +spin_lock_irqsave (&mxport->mxp_lock, flags); + msr = mxport->mxp_msr; + mcr = mxport->mxp_shadow_mcr; +spin_unlock_irqrestore (&mxport->mxp_lock, flags); + + result = ((mcr &MXU1_MCR_DTR ) ?TIOCM_DTR :0 ) + | ((mcr &MXU1_MCR_RTS ) ?TIOCM_RTS :0 ) + | ((mcr &MXU1_MCR_LOOP ) ?TIOCM_LOOP :0 ) + | ((msr &MXU1_MSR_CTS ) ?TIOCM_CTS :0 ) + | ((msr &MXU1_MSR_CD ) ?TIOCM_CAR :0 ) + | ((msr &MXU1_MSR_RI ) ?TIOCM_RI :0 ) + | ((msr &MXU1_MSR_DSR ) ?TIOCM_DSR :0 );
But boolean operators before line break and indent further.
Quoted 62 lines
drivers/usb/serial/mxu11x0.c new file +62
-0+ +dev_dbg (&port->dev,"%s - 0x%04X" , __func__, result); + +return result; +} + +static int mxu1_tiocmset (struct tty_struct *tty, +unsigned int set,unsigned int clear) +{ +struct usb_serial_port *port = tty->driver_data; + +struct mxu1_port *mxport =usb_get_serial_port_data (port); +unsigned int mcr; +unsigned long flags; + +dev_dbg (&port->dev,"%s - port %d" , __func__, port->port_number); + +if (mxport ==NULL ) +return -ENODEV ; + +spin_lock_irqsave (&mxport->mxp_lock, flags); + mcr = mxport->mxp_shadow_mcr; + +if (set &TIOCM_RTS ) + mcr |=MXU1_MCR_RTS ; +if (set &TIOCM_DTR ) + mcr |=MXU1_MCR_DTR ; +if (set &TIOCM_LOOP ) + mcr |=MXU1_MCR_LOOP ; + +if (clear &TIOCM_RTS ) + mcr &= ~MXU1_MCR_RTS ; +if (clear &TIOCM_DTR ) + mcr &= ~MXU1_MCR_DTR ; +if (clear &TIOCM_LOOP ) + mcr &= ~MXU1_MCR_LOOP ; + +spin_unlock_irqrestore (&mxport->mxp_lock, flags); + +return mxu1_set_mcr (mxport, mcr); +} + +static void mxu1_break (struct tty_struct *tty,int break_state) +{ +struct usb_serial_port *port = tty->driver_data; + +struct mxu1_port *mxport =usb_get_serial_port_data (port); + +dev_dbg (&port->dev,"%s - state = %d" , __func__, break_state); + +if (mxport ==NULL ) +return ; + +if (break_state ==-1 ) + mxport->mxp_send_break =MXU1_LCR_BREAK ; +else + mxport->mxp_send_break =0 ; + +mxu1_set_termios (NULL , mxport->mxp_port,NULL ); +} + +static int mxu1_open (struct tty_struct *tty,struct usb_serial_port *port)
drivers/usb/serial/mxu11x0.c line 901
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+{ +struct mxu1_port *mxport =usb_get_serial_port_data (port); +struct mxu1_device *mxdev; +struct usb_device *dev; +struct urb *urb; +int port_number; +int status; +__u16 open_settings = (u8)(MXU1_PIPE_MODE_CONTINUOUS | +MXU1_PIPE_TIMEOUT_ENABLE | + (MXU1_TRANSFER_TIMEOUT <<2 ));
Use u16 and split declatation from initialisation. u8 cast looks odd.
drivers/usb/serial/mxu11x0.c line 911
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+if (!mxport) +return -ENODEV ; + + dev = port->serial->dev; + mxdev = mxport->mxp_mxdev; + + port_number = port->port_number - port->minor; + + mxport->mxp_msr =0 ; + mxport->mxp_shadow_mcr |= (MXU1_MCR_RTS |MXU1_MCR_DTR );
Drop parentheses.
drivers/usb/serial/mxu11x0.c line 914
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ +if (mutex_lock_interruptible (&mxdev->mxd_lock)) +return -ERESTARTSYS ;
Why do you need this? Again, these are one-port devices.
drivers/usb/serial/mxu11x0.c line 917
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ +dev_dbg (&port->dev,"%s - start interrupt in urb" , __func__); + urb = mxdev->mxd_serial->port[0 ]->interrupt_in_urb;
Access directly though port pointer instead.
drivers/usb/serial/mxu11x0.c line 921
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+if (!urb) { +dev_err (&port->dev, +"%s - no interrupt urb\n" , + __func__);
Too many line breaks. Not needed at all here.
Review
Quoted 1 line
(quoted diff) +1
-0+ status = -EINVAL;
If you expect an interrupt endpoint then make sure it's there already at probe.
drivers/usb/serial/mxu11x0.c line 935
Quoted 13 lines
drivers/usb/serial/mxu11x0.c new file +13
-0+ goto release_mxd_lock; + } + urb->context = mxdev; + status =usb_submit_urb (urb,GFP_KERNEL ); +if (status) { +dev_err (&port->dev, +"%s - submit interrupt urb failed, %d\n" , + __func__, + status); + goto release_mxd_lock; + } + +mxu1_set_termios (NULL , port,NULL );
You have a tty argument, which is non-null unless used as a console.
drivers/usb/serial/mxu11x0.c line 937
Quoted 2 lines
drivers/usb/serial/mxu11x0.c new file +2
-0+ +dev_dbg (&port->dev,"%s - sending MXU1_OPEN_PORT" , __func__);
Drop this debugging, just log any errors.
drivers/usb/serial/mxu11x0.c line 940
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ status =mxu1_send_ctrl_urb (mxdev->mxd_serial,MXU1_OPEN_PORT , + open_settings, + (u8)(MXU1_UART1_PORT + port_number));
Use a intermediate variable for the port number if needed at all.
drivers/usb/serial/mxu11x0.c line 944
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+if (status) { +dev_err (&port->dev,"%s - cannot send open command, %d\n" , + __func__, + status);
Unnecessary line breaks again.
Quoted 30 lines
drivers/usb/serial/mxu11x0.c new file +30
-0+ goto unlink_int_urb; + } + +dev_dbg (&port->dev,"%s - sending MXU1_START_PORT" , __func__); + status =mxu1_send_ctrl_urb (mxdev->mxd_serial,MXU1_START_PORT , +0 , (u8)(MXU1_UART1_PORT + port_number)); +if (status) { +dev_err (&port->dev,"%s - cannot send start command, %d\n" , + __func__, + status); + goto unlink_int_urb; + } + +dev_dbg (&port->dev,"%s - sending MXU1_PURGE_PORT" , __func__); + status =mxu1_send_ctrl_urb (mxdev->mxd_serial,MXU1_PURGE_PORT , +MXU1_PURGE_INPUT , + (u8)(MXU1_UART1_PORT + port_number)); +if (status) { +dev_err (&port->dev, +"%s - cannot clear input buffers, %d\n" , + __func__, + status); + + goto unlink_int_urb; + } + status =mxu1_send_ctrl_urb (mxdev->mxd_serial,MXU1_PURGE_PORT , +MXU1_PURGE_OUTPUT , + (u8)(MXU1_UART1_PORT + port_number)); +if (status) { + dev_err(&port->dev,
drivers/usb/serial/mxu11x0.c line 984
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+"%s - cannot clear output buffers, %d\n" , + __func__, + status); + + goto unlink_int_urb; + } + +/* reset the data toggle on the bulk endpoints to work around bug in +* host controllers where things get out of sync some times +*/
Multi-line comments should be on the following format
/* * ... */
Quoted 35 lines
drivers/usb/serial/mxu11x0.c new file +35
-0+usb_clear_halt (dev, port->write_urb->pipe); +usb_clear_halt (dev, port->read_urb->pipe); + +mxu1_set_termios (NULL , port,NULL ); + +dev_dbg (&port->dev,"%s - sending MXU1_OPEN_PORT (2)" , __func__); + status =mxu1_send_ctrl_urb (mxdev->mxd_serial,MXU1_OPEN_PORT , + open_settings, + (u8)(MXU1_UART1_PORT + port_number)); +if (status) { +dev_err (&port->dev,"%s - cannot send open command, %d\n" , + __func__, + status); + goto unlink_int_urb; + } + +dev_dbg (&port->dev,"%s - sending MXU1_START_PORT (2)" , __func__); + status =mxu1_send_ctrl_urb (mxdev->mxd_serial,MXU1_START_PORT , +0 , (u8)(MXU1_UART1_PORT + port_number)); +if (status) { +dev_err (&port->dev,"%s - cannot send start command, %d\n" , + __func__, + status); + goto unlink_int_urb; + } + +/* start read urb */ +dev_dbg (&port->dev,"%s - start read urb" , __func__); + urb = port->read_urb; + +if (!urb) { +dev_err (&port->dev,"%s - no read urb\n" , __func__); + status = -EINVAL ; + goto unlink_int_urb; + }
drivers/usb/serial/mxu11x0.c line 1029
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ + urb->context = port; + urb->dev = dev; + status =usb_submit_urb (urb,GFP_KERNEL ); + +if (status) { +dev_err (&port->dev,"%s - submit read urb failed, %d\n" , + __func__, status); + goto unlink_int_urb; + }
Use usb_serial_generic_open() to submit the read urb(s).
drivers/usb/serial/mxu11x0.c line 1038
Quoted 9 lines
drivers/usb/serial/mxu11x0.c new file +9
-0+ + goto release_mxd_lock; + +unlink_int_urb: +usb_kill_urb (port->serial->port[0 ]->interrupt_in_urb); + +release_mxd_lock: +mutex_unlock (&mxdev->mxd_lock); +dev_dbg (&port->dev,"%s - exit %d" , __func__, status);
Drop debugging.
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+ +return status; +} + +static void mxu1_close (struct usb_serial_port *port) +{ +struct mxu1_device *mxdev; +struct mxu1_port *mxport;
drivers/usb/serial/mxu11x0.c line 1056
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+int port_number; +unsigned long flags; +int status =0 ; + +dev_dbg (&port->dev,"%s - port %d" , __func__, port->port_number); + + mxdev =usb_get_serial_data (port->serial); + mxport =usb_get_serial_port_data (port); +if (mxdev ==NULL || mxport ==NULL ) +return ;
Again, not needed (anywhere).
drivers/usb/serial/mxu11x0.c line 1063
Quoted 7 lines
drivers/usb/serial/mxu11x0.c new file +7
-0+ +usb_kill_urb (port->read_urb); +usb_kill_urb (port->write_urb); + +spin_lock_irqsave (&port->lock, flags); +kfifo_reset_out (&port->write_fifo); +spin_unlock_irqrestore (&port->lock, flags);
Use generic close implementation instead.
drivers/usb/serial/mxu11x0.c line 1075
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+ + port_number = port->port_number - port->minor; + +dev_dbg (&port->dev,"%s - sending MXU1_CLOSE_PORT" , __func__); + status =mxu1_send_ctrl_urb (port->serial, +MXU1_CLOSE_PORT , +0 , (u8)(MXU1_UART1_PORT + port_number)); +if (status) +dev_err (&port->dev, +"%s - cannot send close port command, %d\n" , + __func__, + status);
Close before killing urbs?
drivers/usb/serial/mxu11x0.c line 1079
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+ +usb_kill_urb (port->serial->port[0 ]->interrupt_in_urb); + +dev_dbg (&port->dev,"%s - exit" , __func__);
Remove.
Quoted 14 lines
drivers/usb/serial/mxu11x0.c new file +14
-0+} + +static void mxu1_handle_new_msr (struct mxu1_port *mxport,u8 msr) +{ +struct async_icount *icount; +struct tty_struct *tty; +unsigned long flags; + +dev_dbg (&mxport->mxp_port->dev,"%s - msr 0x%02X" , __func__, msr); + +if (msr &MXU1_MSR_DELTA_MASK ) { +spin_lock_irqsave (&mxport->mxp_lock, flags); + icount = &mxport->mxp_port->icount; +if (msr &MXU1_MSR_DELTA_CTS )
drivers/usb/serial/mxu11x0.c line 1103
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ icount->cts++; +if (msr &MXU1_MSR_DELTA_DSR ) + icount->dsr++; +if (msr &MXU1_MSR_DELTA_CD ) + icount->dcd++; +if (msr &MXU1_MSR_DELTA_RI ) + icount->rng++; +if (mxport->mxp_msr_wait_flags ==1 ) { + mxport->mxp_msr_wait_flags =0 ; +wake_up_interruptible (&mxport->mxp_msr_wait);
Drop wait flags and use port msr wait queue.
drivers/usb/serial/mxu11x0.c line 1111
Quoted 8 lines
drivers/usb/serial/mxu11x0.c new file +8
-0+ } +spin_unlock_irqrestore (&mxport->mxp_lock, flags); + } + + mxport->mxp_msr = msr &MXU1_MSR_MASK ; + +/* handle CTS flow control */ + tty = mxport->mxp_port->port.tty;
You need to use tty_port_tty_get() and put the reference when done.
drivers/usb/serial/mxu11x0.c line 1121
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ +if (tty &&C_CRTSCTS (tty)) { +if (msr &MXU1_MSR_CTS ) { + tty->hw_stopped =0 ; + +tty_wakeup (tty); + }else { + tty->hw_stopped =1 ; + } + }
But I think you drop this bit.
drivers/usb/serial/mxu11x0.c line 1126
Quoted 5 lines
drivers/usb/serial/mxu11x0.c new file +5
-0+} + +static void mxu1_interrupt_callback (struct urb *urb) +{ +struct mxu1_device *mxdev = (struct mxu1_device *)urb->context;
Cast not needed. Usb-serial core would already have setup the context to be the usb-serial port.
drivers/usb/serial/mxu11x0.c line 1138
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+struct usb_serial_port *port; +struct usb_serial *serial = mxdev->mxd_serial; +struct mxu1_port *mxport; +struct device *dev = &urb->dev->dev; +unsigned char *data = urb->transfer_buffer; +int length = urb->actual_length; +int port_number; +int function; +int status =0 ; +u8 msr; + +dev_dbg (&urb->dev->dev,"%s" , __func__);
Unless this is a common interrupt urb for a multi-port device, simply use the port device for all debug and error messages.
drivers/usb/serial/mxu11x0.c line 1142
Quoted 4 lines
drivers/usb/serial/mxu11x0.c new file +4
-0+ +/* Check port is valid or not */ +if (mxdev ==NULL ) +return ;
Not needed.
Review
Quoted 2 lines
(quoted diff) +2
-0+ +
Stray newline.
drivers/usb/serial/mxu11x0.c line 1156
Quoted 12 lines
drivers/usb/serial/mxu11x0.c new file +12
-0+switch (urb->status) { +case 0 : +break ; +case -ECONNRESET : +case -ENOENT : +case -ESHUTDOWN : +dev_dbg (dev,"%s - urb shutting down, %d" , + __func__, urb->status); +return ; +default : +dev_err (dev,"%s - nonzero urb status, %d\n" , + __func__, urb->status);
These should be debug level as you'd usually get these when unplugging an open device.
Quoted 5 lines
drivers/usb/serial/mxu11x0.c new file +5
-0+ goto exit; + } + +if (length !=2 ) { +dev_dbg (dev,"%s - bad packet size, %d" , __func__, length);
drivers/usb/serial/mxu11x0.c line 1171
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ goto exit; + } + +if (data[0 ] ==MXU1_CODE_HARDWARE_ERROR ) { +dev_err (dev,"%s - hardware error, %d\n" , __func__, data[1 ]); + goto exit; + } + + port_number =MXU1_GET_PORT_FROM_CODE (data[0 ]); + function =MXU1_GET_FUNC_FROM_CODE (data[0 ]);
Is this needed if single port devices? Use static functions rather than macros.
Quoted 44 lines
drivers/usb/serial/mxu11x0.c new file +44
-0+ +dev_dbg (dev,"%s - port_number %d, function %d, data 0x%02X" , + __func__, + port_number, + function, + data[1 ]); + +if (port_number >= serial->num_ports) { +dev_err (dev,"%s - bad port number, %d\n" , + __func__, port_number); + goto exit; + } + + port = serial->port[port_number]; + + mxport =usb_get_serial_port_data (port); +if (!mxport) + goto exit; + +switch (function) { +case MXU1_CODE_DATA_ERROR : +dev_dbg (dev,"%s - DATA ERROR, port %d, data 0x%02X\n" , + __func__, + port_number, + data[1 ]); +break ; + +case MXU1_CODE_MODEM_STATUS : + msr = data[1 ]; +dev_dbg (dev,"%s - port %d, msr 0x%02X" , + __func__, port_number, msr); +mxu1_handle_new_msr (mxport, msr); +break ; + +default : +dev_err (dev,"%s - unknown interrupt code, 0x%02X\n" , + __func__, data[1 ]); +break ; + } + +exit: + status =usb_submit_urb (urb,GFP_ATOMIC ); +if (status) + dev_err(dev, "%s - resubmit interrupt urb failed, %d\n",
drivers/usb/serial/mxu11x0.c line 1225
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ __func__, status); +} + +static struct usb_serial_driver mxuport11_device = { + .driver = { + .owner =THIS_MODULE , + .name ="mxuport11" , + }, + .description ="MOXA UPort 11x0" , + .id_table = mxuport11_idtable,
Use mxu1 prefix throughout?
drivers/usb/serial/mxu11x0.c line 1235
Quoted 10 lines
drivers/usb/serial/mxu11x0.c new file +10
-0+ .num_ports = 1, + .port_probe = mxu1_port_probe, + .attach = mxu1_startup, + .open = mxu1_open, + .close = mxu1_close, + .ioctl = mxu1_ioctl, + .set_termios = mxu1_set_termios, + .tiocmget = mxu1_tiocmget, + .tiocmset = mxu1_tiocmset, + .get_icount = usb_serial_generic_get_icount,
You may want to set tiocmiwait to the generic implementation as well.
drivers/usb/serial/mxu11x0.c line 1238
Quoted 3 lines
drivers/usb/serial/mxu11x0.c new file +3
-0+ .break_ctl = mxu1_break, + .read_int_callback = mxu1_interrupt_callback, +};
Johan
PATCH v3 1/1 USB: serial: add Moxa UPORT 11x0 driver
Add a driver which supports :
- UPort 1110 : 1 port RS-232 USB to Serial Hub.
- UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
- UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
- UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
- UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
This driver is based on GPL MOXA driver written by Hen Huang and available on MOXA website. The original driver was based on io_ti serial driver.
---
Hi,
Sorry for late reply, here is the third version of the driver.
Thank you,
Mathieu
drivers/usb/serial/Kconfig | 16 + drivers/usb/serial/Makefile | 1 + drivers/usb/serial/mxu11x0.c | 1100 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1117 insertions(+) create mode 100644 drivers/usb/serial/mxu11x0.c
drivers/usb/serial/Kconfig +16 -0
@@ -475,6 +475,22 @@ config USB_SERIAL_MOS7840 To compile this driver as a module, choose M here: the module will be called mos7840. If unsure, choose N. +config USB_SERIAL_MXUPORT11 + tristate "USB Moxa UPORT 11x0 Serial Driver" + ---help--- + Say Y here if you want to use a MOXA UPort 11x0 Serial hub. + + This driver supports: + + - UPort 1110 : 1 port RS-232 USB to Serial Hub. + - UPort 1130 : 1 port RS-422/485 USB to Serial Hub. + - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation. + - UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub. + - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation. + + To compile this driver as a module, choose M here: the + module will be called mxu11x0. + config USB_SERIAL_MXUPORT tristate "USB Moxa UPORT Serial Driver" ---help---
drivers/usb/serial/Makefile +1 -0
@@ -38,6 +38,7 @@ obj-$(CONFIG_USB_SERIAL_METRO) += metro-usb.o 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_MXUPORT11) += mxu11x0.o obj-$(CONFIG_USB_SERIAL_NAVMAN) += navman.o obj-$(CONFIG_USB_SERIAL_OMNINET) += omninet.o obj-$(CONFIG_USB_SERIAL_OPTICON) += opticon.o
drivers/usb/serial/mxu11x0.c new file +1100 -0
1,100 lines not on this page · raw message
Signature · 1 line
2.6.1