[PATCH v2 00/22] usb: serial: ti_usb_3410_5052: clean driver

[PATCH v2 00/22] usb: serial: ti_usb_3410_5052: clean driver

[Mathieu OTHACEHE]

Hi Johan,

Thanks for your review ! Here is the v2 of the serie.
I didn't resubmit patches related to the switch to generic
implementation (open, close, read and write).

I will work on them later when this first batch will be
pushed.

Thank you,

Mathieu

Mathieu OTHACEHE (22):
  usb: serial: ti_usb_3410_5052: Do not use __uX types
  usb: serial: ti_usb_3410_5052: Remove useless dev_dbg messages
  usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc
  usb: serial: ti_usb_3410_5052: Remove useless NULL-testing
  usb: serial: ti_usb_3410_5052: Use C_X macros instead of c_cflag
    manipulation
  usb: serial: ti_usb_3410_5052: Remove unused variables
  usb: serial: ti_usb_3410_5052: Use macros instead of magic values
  usb: serial: ti_usb_3410_5052: Remove in_sync and out_sync functions
  usb: serial: ti_usb_3410_5052: Remove useless tty_wakeup
  usb: serial: ti_usb_3410_5052: Change ti_write_byte function arguments
  usb: serial: ti_usb_3410_5052: Do not modify interrupt context
  usb: serial: ti_usb_3410_5052: Remove usb_serial pointer in ti_port
  usb: serial: ti_usb_3410_5052: Change ti_get/set_serial_info function
    arguments
  usb: serial: ti_usb_3410_5052: Do not set shadow mcr in open callback
  usb: serial: ti_usb_3410_5052: Check old_termios parameter in
    set_termios
  usb: serial: ti_usb_3410_5052: Raise DTR and RTS flags if speed is not
    null anymore
  usb: serial: ti_usb_3410_5052: Fix firmware downloading
  usb: serial: ti_usb_3410_5052: Standardize debug and error messages
  usb: serial: ti_usb_3410_5052: Use variables for vendor and product
  usb: serial: ti_usb_3410_5052: Set shadow msr before waking up waiters
  usb: serial: ti_usb_3410_5052: Add CMSPAR support
  usb: serial: ti_usb_3410_5052: Fix indentation problems

 drivers/usb/serial/ti_usb_3410_5052.c | 712 +++++++++++++++++-----------------
 1 file changed, 346 insertions(+), 366 deletions(-)

-- 
2.9.0

[PATCH v2 01/22] usb: serial: ti_usb_3410_5052: Do not use __uX types

[Mathieu OTHACEHE]

__uX types should only be used for user-space interactions.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---

Changelog:
v2:
* Replace cpu_to_be16s calls by cpu_to_be16
* Remove other useless casts

 drivers/usb/serial/ti_usb_3410_5052.c | 101 +++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 07b4bf0..ebeea51 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -179,23 +179,23 @@
 
 /* Config struct */
 struct ti_uart_config {
-	__u16	wBaudRate;
-	__u16	wFlags;
-	__u8	bDataBits;
-	__u8	bParity;
-	__u8	bStopBits;
+	__be16	wBaudRate;
+	__be16	wFlags;
+	u8	bDataBits;
+	u8	bParity;
+	u8	bStopBits;
 	char	cXon;
 	char	cXoff;
-	__u8	bUartMode;
+	u8	bUartMode;
 } __packed;
 
 /* Get port status */
 struct ti_port_status {
-	__u8	bCmdCode;
-	__u8	bModuleId;
-	__u8	bErrorCode;
-	__u8	bMSR;
-	__u8	bLSR;
+	u8 bCmdCode;
+	u8 bModuleId;
+	u8 bErrorCode;
+	u8 bMSR;
+	u8 bLSR;
 } __packed;
 
 /* Purge modes */
@@ -218,12 +218,12 @@ struct ti_port_status {
 #define TI_RW_DATA_DOUBLE_WORD		0x04
 
 struct ti_write_data_bytes {
-	__u8	bAddrType;
-	__u8	bDataType;
-	__u8	bDataCounter;
+	u8	bAddrType;
+	u8	bDataType;
+	u8	bDataCounter;
 	__be16	wBaseAddrHi;
 	__be16	wBaseAddrLo;
-	__u8	bData[0];
+	u8	bData[0];
 } __packed;
 
 struct ti_read_data_request {
@@ -258,7 +258,7 @@ struct ti_interrupt {
 /* Firmware image header */
 struct ti_firmware_header {
 	__le16	wLength;
-	__u8	bCheckSum;
+	u8	bCheckSum;
 } __packed;
 
 /* UART addresses */
@@ -288,9 +288,9 @@ struct ti_firmware_header {
 
 struct ti_port {
 	int			tp_is_open;
-	__u8			tp_msr;
-	__u8			tp_shadow_mcr;
-	__u8			tp_uart_mode;	/* 232 or 485 modes */
+	u8			tp_msr;
+	u8			tp_shadow_mcr;
+	u8			tp_uart_mode;	/* 232 or 485 modes */
 	unsigned int		tp_uart_base_addr;
 	int			tp_flags;
 	struct ti_device	*tp_tdev;
@@ -343,7 +343,7 @@ static int ti_get_serial_info(struct ti_port *tport,
 	struct serial_struct __user *ret_arg);
 static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 	struct serial_struct __user *new_arg);
-static void ti_handle_new_msr(struct ti_port *tport, __u8 msr);
+static void ti_handle_new_msr(struct ti_port *tport, u8 msr);
 
 static void ti_stop_read(struct ti_port *tport, struct tty_struct *tty);
 static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty);
@@ -354,7 +354,7 @@ static int ti_command_in_sync(struct ti_device *tdev, __u8 command,
 	__u16 moduleid, __u16 value, __u8 *data, int size);
 
 static int ti_write_byte(struct usb_serial_port *port, struct ti_device *tdev,
-			 unsigned long addr, __u8 mask, __u8 byte);
+			 unsigned long addr, u8 mask, u8 byte);
 
 static int ti_download_firmware(struct ti_device *tdev);
 
@@ -647,9 +647,11 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	struct urb *urb;
 	int port_number;
 	int status;
-	__u16 open_settings = (__u8)(TI_PIPE_MODE_CONTINUOUS |
-			     TI_PIPE_TIMEOUT_ENABLE |
-			     (TI_TRANSFER_TIMEOUT << 2));
+	u16 open_settings;
+
+	open_settings = (TI_PIPE_MODE_CONTINUOUS |
+			 TI_PIPE_TIMEOUT_ENABLE |
+			 (TI_TRANSFER_TIMEOUT << 2));
 
 	if (tport == NULL)
 		return -ENODEV;
@@ -959,6 +961,8 @@ static void ti_set_termios(struct tty_struct *tty,
 	int status;
 	int port_number = port->port_number;
 	unsigned int mcr;
+	u16 wbaudrate;
+	u16 wflags = 0;
 
 	cflag = tty->termios.c_cflag;
 	iflag = tty->termios.c_iflag;
@@ -974,12 +978,10 @@ static void ti_set_termios(struct tty_struct *tty,
 	if (!config)
 		return;
 
-	config->wFlags = 0;
-
 	/* these flags must be set */
-	config->wFlags |= TI_UART_ENABLE_MS_INTS;
-	config->wFlags |= TI_UART_ENABLE_AUTO_START_DMA;
-	config->bUartMode = (__u8)(tport->tp_uart_mode);
+	wflags |= TI_UART_ENABLE_MS_INTS;
+	wflags |= TI_UART_ENABLE_AUTO_START_DMA;
+	config->bUartMode = tport->tp_uart_mode;
 
 	switch (cflag & CSIZE) {
 	case CS5:
@@ -1002,14 +1004,14 @@ static void ti_set_termios(struct tty_struct *tty,
 
 	if (cflag & PARENB) {
 		if (cflag & PARODD) {
-			config->wFlags |= TI_UART_ENABLE_PARITY_CHECKING;
+			wflags |= TI_UART_ENABLE_PARITY_CHECKING;
 			config->bParity = TI_UART_ODD_PARITY;
 		} else {
-			config->wFlags |= TI_UART_ENABLE_PARITY_CHECKING;
+			wflags |= TI_UART_ENABLE_PARITY_CHECKING;
 			config->bParity = TI_UART_EVEN_PARITY;
 		}
 	} else {
-		config->wFlags &= ~TI_UART_ENABLE_PARITY_CHECKING;
+		wflags &= ~TI_UART_ENABLE_PARITY_CHECKING;
 		config->bParity = TI_UART_NO_PARITY;
 	}
 
@@ -1021,8 +1023,8 @@ static void ti_set_termios(struct tty_struct *tty,
 	if (cflag & CRTSCTS) {
 		/* RTS flow control must be off to drop RTS for baud rate B0 */
 		if ((cflag & CBAUD) != B0)
-			config->wFlags |= TI_UART_ENABLE_RTS_IN;
-		config->wFlags |= TI_UART_ENABLE_CTS_OUT;
+			wflags |= TI_UART_ENABLE_RTS_IN;
+		wflags |= TI_UART_ENABLE_CTS_OUT;
 	} else {
 		ti_restart_read(tport, tty);
 	}
@@ -1032,21 +1034,21 @@ static void ti_set_termios(struct tty_struct *tty,
 		config->cXoff = STOP_CHAR(tty);
 
 		if (I_IXOFF(tty))
-			config->wFlags |= TI_UART_ENABLE_X_IN;
+			wflags |= TI_UART_ENABLE_X_IN;
 		else
 			ti_restart_read(tport, tty);
 
 		if (I_IXON(tty))
-			config->wFlags |= TI_UART_ENABLE_X_OUT;
+			wflags |= TI_UART_ENABLE_X_OUT;
 	}
 
 	baud = tty_get_baud_rate(tty);
 	if (!baud)
 		baud = 9600;
 	if (tport->tp_tdev->td_is_3410)
-		config->wBaudRate = (__u16)((923077 + baud/2) / baud);
+		wbaudrate = (923077 + baud/2) / baud;
 	else
-		config->wBaudRate = (__u16)((461538 + baud/2) / baud);
+		wbaudrate = (461538 + baud/2) / baud;
 
 	/* FIXME: Should calculate resulting baud here and report it back */
 	if ((cflag & CBAUD) != B0)
@@ -1054,12 +1056,12 @@ static void ti_set_termios(struct tty_struct *tty,
 
 	dev_dbg(&port->dev,
 		"%s - BaudRate=%d, wBaudRate=%d, wFlags=0x%04X, bDataBits=%d, bParity=%d, bStopBits=%d, cXon=%d, cXoff=%d, bUartMode=%d\n",
-		__func__, baud, config->wBaudRate, config->wFlags,
+		__func__, baud, wbaudrate, wflags,
 		config->bDataBits, config->bParity, config->bStopBits,
 		config->cXon, config->cXoff, config->bUartMode);
 
-	cpu_to_be16s(&config->wBaudRate);
-	cpu_to_be16s(&config->wFlags);
+	config->wBaudRate = cpu_to_be16(wbaudrate);
+	config->wFlags = cpu_to_be16(wflags);
 
 	status = ti_command_out_sync(tport->tp_tdev, TI_SET_CONFIG,
 		(__u8)(TI_UART1_PORT + port_number), 0, (__u8 *)config,
@@ -1189,7 +1191,7 @@ static void ti_interrupt_callback(struct urb *urb)
 	int function;
 	int status = urb->status;
 	int retval;
-	__u8 msr;
+	u8 msr;
 
 	switch (status) {
 	case 0:
@@ -1522,7 +1524,7 @@ static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 }
 
 
-static void ti_handle_new_msr(struct ti_port *tport, __u8 msr)
+static void ti_handle_new_msr(struct ti_port *tport, u8 msr)
 {
 	struct async_icount *icount;
 	struct tty_struct *tty;
@@ -1634,8 +1636,8 @@ static int ti_command_in_sync(struct ti_device *tdev, __u8 command,
 
 
 static int ti_write_byte(struct usb_serial_port *port,
-			struct ti_device *tdev, unsigned long addr,
-			__u8 mask, __u8 byte)
+			 struct ti_device *tdev, unsigned long addr,
+			 u8 mask, u8 byte)
 {
 	int status;
 	unsigned int size;
@@ -1669,7 +1671,7 @@ static int ti_write_byte(struct usb_serial_port *port,
 }
 
 static int ti_do_download(struct usb_device *dev, int pipe,
-						u8 *buffer, int size)
+			  u8 *buffer, int size)
 {
 	int pos;
 	u8 cs = 0;
@@ -1679,11 +1681,10 @@ static int ti_do_download(struct usb_device *dev, int pipe,
 	int len;
 
 	for (pos = sizeof(struct ti_firmware_header); pos < size; pos++)
-		cs = (__u8)(cs + buffer[pos]);
+		cs = (u8)(cs + buffer[pos]);
 
 	header = (struct ti_firmware_header *)buffer;
-	header->wLength = cpu_to_le16((__u16)(size
-					- sizeof(struct ti_firmware_header)));
+	header->wLength = cpu_to_le16(size - sizeof(*header));
 	header->bCheckSum = cs;
 
 	dev_dbg(&dev->dev, "%s - downloading firmware\n", __func__);
@@ -1701,7 +1702,7 @@ static int ti_download_firmware(struct ti_device *tdev)
 {
 	int status;
 	int buffer_size;
-	__u8 *buffer;
+	u8 *buffer;
 	struct usb_device *dev = tdev->td_serial->dev;
 	unsigned int pipe = usb_sndbulkpipe(dev,
 		tdev->td_serial->port[0]->bulk_out_endpointAddress);
-- 
2.9.0

Re: [PATCH v2 01/22] usb: serial: ti_usb_3410_5052: Do not use __uX types

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:41PM +0200, Mathieu OTHACEHE wrote:
> __uX types should only be used for user-space interactions.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
> 
> Changelog:
> v2:
> * Replace cpu_to_be16s calls by cpu_to_be16
> * Remove other useless casts

You should have mentioned this in the commit message as well (i.e. that
you're doing more than just replacing __uX types).

>  drivers/usb/serial/ti_usb_3410_5052.c | 101 +++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index 07b4bf0..ebeea51 100644
 
>  static int ti_do_download(struct usb_device *dev, int pipe,
> -						u8 *buffer, int size)
> +			  u8 *buffer, int size)
>  {
>  	int pos;
>  	u8 cs = 0;

I dropped this unrelated change, and amended the commit message before
applying.

Thanks,
Johan

[PATCH v2 02/22] usb: serial: ti_usb_3410_5052: Remove useless dev_dbg messages

[Mathieu OTHACEHE]

Remove useless or redundant dev_dbg messages.
Fix debug-message typos.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
Changelog:
v2:
* Keep some debug messages

 drivers/usb/serial/ti_usb_3410_5052.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index ebeea51..50a74b3 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -688,7 +688,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (tty)
 		ti_set_termios(tty, port, &tty->termios);
 
-	dev_dbg(&port->dev, "%s - sending TI_OPEN_PORT\n", __func__);
 	status = ti_command_out_sync(tdev, TI_OPEN_PORT,
 		(__u8)(TI_UART1_PORT + port_number), open_settings, NULL, 0);
 	if (status) {
@@ -697,7 +696,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 		goto unlink_int_urb;
 	}
 
-	dev_dbg(&port->dev, "%s - sending TI_START_PORT\n", __func__);
 	status = ti_command_out_sync(tdev, TI_START_PORT,
 		(__u8)(TI_UART1_PORT + port_number), 0, NULL, 0);
 	if (status) {
@@ -706,7 +704,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 		goto unlink_int_urb;
 	}
 
-	dev_dbg(&port->dev, "%s - sending TI_PURGE_PORT\n", __func__);
 	status = ti_command_out_sync(tdev, TI_PURGE_PORT,
 		(__u8)(TI_UART1_PORT + port_number), TI_PURGE_INPUT, NULL, 0);
 	if (status) {
@@ -730,7 +727,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (tty)
 		ti_set_termios(tty, port, &tty->termios);
 
-	dev_dbg(&port->dev, "%s - sending TI_OPEN_PORT (2)\n", __func__);
 	status = ti_command_out_sync(tdev, TI_OPEN_PORT,
 		(__u8)(TI_UART1_PORT + port_number), open_settings, NULL, 0);
 	if (status) {
@@ -739,7 +735,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 		goto unlink_int_urb;
 	}
 
-	dev_dbg(&port->dev, "%s - sending TI_START_PORT (2)\n", __func__);
 	status = ti_command_out_sync(tdev, TI_START_PORT,
 		(__u8)(TI_UART1_PORT + port_number), 0, NULL, 0);
 	if (status) {
@@ -749,7 +744,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	}
 
 	/* start read urb */
-	dev_dbg(&port->dev, "%s - start read urb\n", __func__);
 	urb = port->read_urb;
 	if (!urb) {
 		dev_err(&port->dev, "%s - no read urb\n", __func__);
@@ -775,7 +769,6 @@ unlink_int_urb:
 		usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
 release_lock:
 	mutex_unlock(&tdev->td_open_close_lock);
-	dev_dbg(&port->dev, "%s - exit %d\n", __func__, status);
 	return status;
 }
 
@@ -805,7 +798,6 @@ static void ti_close(struct usb_serial_port *port)
 
 	port_number = port->port_number;
 
-	dev_dbg(&port->dev, "%s - sending TI_CLOSE_PORT\n", __func__);
 	status = ti_command_out_sync(tdev, TI_CLOSE_PORT,
 		     (__u8)(TI_UART1_PORT + port_number), 0, NULL, 0);
 	if (status)
@@ -832,7 +824,6 @@ static int ti_write(struct tty_struct *tty, struct usb_serial_port *port,
 	struct ti_port *tport = usb_get_serial_port_data(port);
 
 	if (count == 0) {
-		dev_dbg(&port->dev, "%s - write request of 0 bytes\n", __func__);
 		return 0;
 	}
 
@@ -939,11 +930,9 @@ static int ti_ioctl(struct tty_struct *tty,
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		dev_dbg(&port->dev, "%s - TIOCGSERIAL\n", __func__);
 		return ti_get_serial_info(tport,
 				(struct serial_struct __user *)arg);
 	case TIOCSSERIAL:
-		dev_dbg(&port->dev, "%s - TIOCSSERIAL\n", __func__);
 		return ti_set_serial_info(tty, tport,
 				(struct serial_struct __user *)arg);
 	}
@@ -967,9 +956,15 @@ static void ti_set_termios(struct tty_struct *tty,
 	cflag = tty->termios.c_cflag;
 	iflag = tty->termios.c_iflag;
 
-	dev_dbg(&port->dev, "%s - cflag %08x, iflag %08x\n", __func__, cflag, iflag);
-	dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x\n", __func__,
-		old_termios->c_cflag, old_termios->c_iflag);
+	dev_dbg(&port->dev,
+		"%s - cflag 0x%08x, iflag 0x%08x\n", __func__, cflag, iflag);
+
+	if (old_termios) {
+		dev_dbg(&port->dev, "%s - old clfag 0x%08x, old iflag 0x%08x\n",
+			__func__,
+			old_termios->c_cflag,
+			old_termios->c_iflag);
+	}
 
 	if (tport == NULL)
 		return;
-- 
2.9.0

Re: [PATCH v2 02/22] usb: serial: ti_usb_3410_5052: Remove useless dev_dbg messages

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:42PM +0200, Mathieu OTHACEHE wrote:
> Remove useless or redundant dev_dbg messages.
> Fix debug-message typos.

You never fix any typos, and forgot to mention the added NULL-check for
oldtermios, which is currently not needed and really should go in its
own patch.

> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
> Changelog:
> v2:
> * Keep some debug messages
> 
>  drivers/usb/serial/ti_usb_3410_5052.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)

> @@ -967,9 +956,15 @@ static void ti_set_termios(struct tty_struct *tty,
>  	cflag = tty->termios.c_cflag;
>  	iflag = tty->termios.c_iflag;
>  
> -	dev_dbg(&port->dev, "%s - cflag %08x, iflag %08x\n", __func__, cflag, iflag);
> -	dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x\n", __func__,
> -		old_termios->c_cflag, old_termios->c_iflag);
> +	dev_dbg(&port->dev,
> +		"%s - cflag 0x%08x, iflag 0x%08x\n", __func__, cflag, iflag);
> +
> +	if (old_termios) {
> +		dev_dbg(&port->dev, "%s - old clfag 0x%08x, old iflag 0x%08x\n",

Notice that "clfag" is still misspelled.

> +			__func__,
> +			old_termios->c_cflag,
> +			old_termios->c_iflag);
> +	}
>  
>  	if (tport == NULL)
>  		return;

So, I dropped this chunk, and applied the rest.

Thanks,
Johan

[PATCH v2 03/22] usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc

[Mathieu OTHACEHE]

Use kzalloc instead of kmalloc to avoid field initialisation to 0.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 50a74b3..b694d69 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -969,7 +969,7 @@ static void ti_set_termios(struct tty_struct *tty,
 	if (tport == NULL)
 		return;
 
-	config = kmalloc(sizeof(*config), GFP_KERNEL);
+	config = kzalloc(sizeof(*config), GFP_KERNEL);
 	if (!config)
 		return;
 
-- 
2.9.0

Re: [PATCH v2 03/22] usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc

[Oliver Neukum]

On Tue, 2016-07-26 at 19:59 +0200, Mathieu OTHACEHE wrote:
> Use kzalloc instead of kmalloc to avoid field initialisation to 0.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/ti_usb_3410_5052.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index 50a74b3..b694d69 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -969,7 +969,7 @@ static void ti_set_termios(struct tty_struct *tty,
>  	if (tport == NULL)
>  		return;
>  
> -	config = kmalloc(sizeof(*config), GFP_KERNEL);
> +	config = kzalloc(sizeof(*config), GFP_KERNEL);
>  	if (!config)
>  		return;
>  

Hi,

in that case, where is the initialisation to 0 you avoid and hence
can remove from the code?

	Regards
		Oliver

Re: [PATCH v2 03/22] usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc

[Mathieu OTHACEHE]

> in that case, where is the initialisation to 0 you avoid and hence
> can remove from the code?

Hi,

In v1, kzalloc was useful to avoid wFlags initialisation to 0 :

https://lkml.org/lkml/2016/5/12/139

In v2 wFlags initialisation has be removed so this patch has no purpose
anymore, my bad.

Thanks,

Mathieu

Re: [PATCH v2 03/22] usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc

[Johan Hovold]

On Wed, Jul 27, 2016 at 02:46:32PM +0200, Mathieu OTHACEHE wrote:
> 
> > in that case, where is the initialisation to 0 you avoid and hence
> > can remove from the code?
> 
> Hi,
> 
> In v1, kzalloc was useful to avoid wFlags initialisation to 0 :
> 
> https://lkml.org/lkml/2016/5/12/139
> 
> In v2 wFlags initialisation has be removed so this patch has no purpose
> anymore, my bad.

Actually, the kzalloc is now making sure that the cXon and cXoff fields
are always initialised. So I think the change is good, but please update
commit message.

Thanks,
Johan

Re: [PATCH v2 03/22] usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc

[Sergei Shtylyov]

On 7/26/2016 8:59 PM, Mathieu OTHACEHE wrote:

> Use kzalloc instead of kmalloc to avoid field initialisation to 0.

    Avoid? kzalloc() does initialize to 0. :-)

> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>

MBR, Sergei

[PATCH v2 04/22] usb: serial: ti_usb_3410_5052: Remove useless NULL-testing

[Mathieu OTHACEHE]

It is useless to check the return of usb_get_serial_port_data.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index b694d69..c8ed3f9 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -653,9 +653,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 			 TI_PIPE_TIMEOUT_ENABLE |
 			 (TI_TRANSFER_TIMEOUT << 2));
 
-	if (tport == NULL)
-		return -ENODEV;
-
 	dev = port->serial->dev;
 	tdev = tport->tp_tdev;
 
@@ -784,8 +781,6 @@ static void ti_close(struct usb_serial_port *port)
 
 	tdev = usb_get_serial_data(port->serial);
 	tport = usb_get_serial_port_data(port);
-	if (tdev == NULL || tport == NULL)
-		return;
 
 	tport->tp_is_open = 0;
 
@@ -827,7 +822,7 @@ static int ti_write(struct tty_struct *tty, struct usb_serial_port *port,
 		return 0;
 	}
 
-	if (tport == NULL || !tport->tp_is_open)
+	if (!tport->tp_is_open)
 		return -ENODEV;
 
 	count = kfifo_in_locked(&port->write_fifo, data, count,
@@ -845,9 +840,6 @@ static int ti_write_room(struct tty_struct *tty)
 	int room = 0;
 	unsigned long flags;
 
-	if (tport == NULL)
-		return 0;
-
 	spin_lock_irqsave(&tport->tp_lock, flags);
 	room = kfifo_avail(&port->write_fifo);
 	spin_unlock_irqrestore(&tport->tp_lock, flags);
@@ -864,9 +856,6 @@ static int ti_chars_in_buffer(struct tty_struct *tty)
 	int chars = 0;
 	unsigned long flags;
 
-	if (tport == NULL)
-		return 0;
-
 	spin_lock_irqsave(&tport->tp_lock, flags);
 	chars = kfifo_len(&port->write_fifo);
 	spin_unlock_irqrestore(&tport->tp_lock, flags);
@@ -893,9 +882,6 @@ static void ti_throttle(struct tty_struct *tty)
 	struct usb_serial_port *port = tty->driver_data;
 	struct ti_port *tport = usb_get_serial_port_data(port);
 
-	if (tport == NULL)
-		return;
-
 	if (I_IXOFF(tty) || C_CRTSCTS(tty))
 		ti_stop_read(tport, tty);
 
@@ -908,9 +894,6 @@ static void ti_unthrottle(struct tty_struct *tty)
 	struct ti_port *tport = usb_get_serial_port_data(port);
 	int status;
 
-	if (tport == NULL)
-		return;
-
 	if (I_IXOFF(tty) || C_CRTSCTS(tty)) {
 		status = ti_restart_read(tport, tty);
 		if (status)
@@ -925,9 +908,6 @@ static int ti_ioctl(struct tty_struct *tty,
 	struct usb_serial_port *port = tty->driver_data;
 	struct ti_port *tport = usb_get_serial_port_data(port);
 
-	if (tport == NULL)
-		return -ENODEV;
-
 	switch (cmd) {
 	case TIOCGSERIAL:
 		return ti_get_serial_info(tport,
@@ -966,9 +946,6 @@ static void ti_set_termios(struct tty_struct *tty,
 			old_termios->c_iflag);
 	}
 
-	if (tport == NULL)
-		return;
-
 	config = kzalloc(sizeof(*config), GFP_KERNEL);
 	if (!config)
 		return;
@@ -1089,9 +1066,6 @@ static int ti_tiocmget(struct tty_struct *tty)
 	unsigned int mcr;
 	unsigned long flags;
 
-	if (tport == NULL)
-		return -ENODEV;
-
 	spin_lock_irqsave(&tport->tp_lock, flags);
 	msr = tport->tp_msr;
 	mcr = tport->tp_shadow_mcr;
@@ -1119,9 +1093,6 @@ static int ti_tiocmset(struct tty_struct *tty,
 	unsigned int mcr;
 	unsigned long flags;
 
-	if (tport == NULL)
-		return -ENODEV;
-
 	spin_lock_irqsave(&tport->tp_lock, flags);
 	mcr = tport->tp_shadow_mcr;
 
@@ -1152,9 +1123,6 @@ static void ti_break(struct tty_struct *tty, int break_state)
 
 	dev_dbg(&port->dev, "%s - state = %d\n", __func__, break_state);
 
-	if (tport == NULL)
-		return;
-
 	status = ti_write_byte(port, tport->tp_tdev,
 		tport->tp_uart_base_addr + TI_UART_OFFSET_LCR,
 		TI_LCR_BREAK, break_state == -1 ? TI_LCR_BREAK : 0);
-- 
2.9.0

Re: [PATCH v2 04/22] usb: serial: ti_usb_3410_5052: Remove useless NULL-testing

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:44PM +0200, Mathieu OTHACEHE wrote:
> It is useless to check the return of usb_get_serial_port_data.

Please be more specific in your commit messages in general. In this case
it should mention that there's no need to check for NULL private data in
the tty or tty-port callbacks.

> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/ti_usb_3410_5052.c | 34 +---------------------------------
>  1 file changed, 1 insertion(+), 33 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index b694d69..c8ed3f9 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -653,9 +653,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
>  			 TI_PIPE_TIMEOUT_ENABLE |
>  			 (TI_TRANSFER_TIMEOUT << 2));
>  
> -	if (tport == NULL)
> -		return -ENODEV;
> -
>  	dev = port->serial->dev;
>  	tdev = tport->tp_tdev;
>  
> @@ -784,8 +781,6 @@ static void ti_close(struct usb_serial_port *port)
>  
>  	tdev = usb_get_serial_data(port->serial);
>  	tport = usb_get_serial_port_data(port);
> -	if (tdev == NULL || tport == NULL)
> -		return;

You also forgot to mention that you are removing a check for interface
private data in close().

I fixes this up before applying this time.

Thanks,
Johan

[PATCH v2 05/22] usb: serial: ti_usb_3410_5052: Use C_X macros instead of c_cflag manipulation

[Mathieu OTHACEHE]

Use C_X tty.h macros to avoid direct manipulation of termios
c_cflag variable.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index c8ed3f9..29bb62c 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -955,7 +955,7 @@ static void ti_set_termios(struct tty_struct *tty,
 	wflags |= TI_UART_ENABLE_AUTO_START_DMA;
 	config->bUartMode = tport->tp_uart_mode;
 
-	switch (cflag & CSIZE) {
+	switch (C_CSIZE(tty)) {
 	case CS5:
 		    config->bDataBits = TI_UART_5_DATA_BITS;
 		    break;
@@ -974,8 +974,8 @@ static void ti_set_termios(struct tty_struct *tty,
 	/* CMSPAR isn't supported by this driver */
 	tty->termios.c_cflag &= ~CMSPAR;
 
-	if (cflag & PARENB) {
-		if (cflag & PARODD) {
+	if (C_PARENB(tty)) {
+		if (C_PARODD(tty)) {
 			wflags |= TI_UART_ENABLE_PARITY_CHECKING;
 			config->bParity = TI_UART_ODD_PARITY;
 		} else {
@@ -987,14 +987,14 @@ static void ti_set_termios(struct tty_struct *tty,
 		config->bParity = TI_UART_NO_PARITY;
 	}
 
-	if (cflag & CSTOPB)
+	if (C_CSTOPB(tty))
 		config->bStopBits = TI_UART_2_STOP_BITS;
 	else
 		config->bStopBits = TI_UART_1_STOP_BITS;
 
-	if (cflag & CRTSCTS) {
+	if (C_CRTSCTS(tty)) {
 		/* RTS flow control must be off to drop RTS for baud rate B0 */
-		if ((cflag & CBAUD) != B0)
+		if ((C_BAUD(tty)) != B0)
 			wflags |= TI_UART_ENABLE_RTS_IN;
 		wflags |= TI_UART_ENABLE_CTS_OUT;
 	} else {
@@ -1023,7 +1023,7 @@ static void ti_set_termios(struct tty_struct *tty,
 		wbaudrate = (461538 + baud/2) / baud;
 
 	/* FIXME: Should calculate resulting baud here and report it back */
-	if ((cflag & CBAUD) != B0)
+	if ((C_BAUD(tty)) != B0)
 		tty_encode_baud_rate(tty, baud, baud);
 
 	dev_dbg(&port->dev,
@@ -1045,7 +1045,7 @@ static void ti_set_termios(struct tty_struct *tty,
 	/* SET_CONFIG asserts RTS and DTR, reset them correctly */
 	mcr = tport->tp_shadow_mcr;
 	/* if baud rate is B0, clear RTS and DTR */
-	if ((cflag & CBAUD) == B0)
+	if (C_BAUD(tty) == B0)
 		mcr &= ~(TI_MCR_DTR | TI_MCR_RTS);
 	status = ti_set_mcr(tport, mcr);
 	if (status)
-- 
2.9.0

[PATCH v2 06/22] usb: serial: ti_usb_3410_5052: Remove unused variables

[Mathieu OTHACEHE]

Remove variables affected but never read.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 29bb62c..2b7fe89 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -292,7 +292,6 @@ struct ti_port {
 	u8			tp_shadow_mcr;
 	u8			tp_uart_mode;	/* 232 or 485 modes */
 	unsigned int		tp_uart_base_addr;
-	int			tp_flags;
 	struct ti_device	*tp_tdev;
 	struct usb_serial_port	*tp_port;
 	spinlock_t		tp_lock;
@@ -306,7 +305,6 @@ struct ti_device {
 	struct usb_serial	*td_serial;
 	int			td_is_3410;
 	bool			td_rs485_only;
-	int			td_urb_error;
 };
 
 static int ti_startup(struct usb_serial *serial);
@@ -1163,11 +1161,9 @@ static void ti_interrupt_callback(struct urb *urb)
 	case -ENOENT:
 	case -ESHUTDOWN:
 		dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
-		tdev->td_urb_error = 1;
 		return;
 	default:
 		dev_err(dev, "%s - nonzero urb status, %d\n", __func__, status);
-		tdev->td_urb_error = 1;
 		goto exit;
 	}
 
@@ -1240,12 +1236,10 @@ static void ti_bulk_in_callback(struct urb *urb)
 	case -ENOENT:
 	case -ESHUTDOWN:
 		dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
-		tport->tp_tdev->td_urb_error = 1;
 		return;
 	default:
 		dev_err(dev, "%s - nonzero urb status, %d\n",
 			__func__, status);
-		tport->tp_tdev->td_urb_error = 1;
 	}
 
 	if (status == -EPIPE)
@@ -1300,12 +1294,10 @@ static void ti_bulk_out_callback(struct urb *urb)
 	case -ENOENT:
 	case -ESHUTDOWN:
 		dev_dbg(&port->dev, "%s - urb shutting down, %d\n", __func__, status);
-		tport->tp_tdev->td_urb_error = 1;
 		return;
 	default:
 		dev_err_console(port, "%s - nonzero urb status, %d\n",
 			__func__, status);
-		tport->tp_tdev->td_urb_error = 1;
 	}
 
 	/* send any buffered data */
@@ -1455,7 +1447,6 @@ static int ti_get_serial_info(struct ti_port *tport,
 	ret_serial.type = PORT_16550A;
 	ret_serial.line = port->minor;
 	ret_serial.port = port->port_number;
-	ret_serial.flags = tport->tp_flags;
 	ret_serial.xmit_fifo_size = kfifo_size(&port->write_fifo);
 	ret_serial.baud_base = tport->tp_tdev->td_is_3410 ? 921600 : 460800;
 	ret_serial.closing_wait = cwait;
@@ -1480,7 +1471,6 @@ static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 	if (cwait != ASYNC_CLOSING_WAIT_NONE)
 		cwait = msecs_to_jiffies(10 * new_serial.closing_wait);
 
-	tport->tp_flags = new_serial.flags & TI_SET_SERIAL_FLAGS;
 	tport->tp_port->port.closing_wait = cwait;
 
 	return 0;
-- 
2.9.0

Re: [PATCH v2 06/22] usb: serial: ti_usb_3410_5052: Remove unused variables

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:46PM +0200, Mathieu OTHACEHE wrote:
> Remove variables affected but never read.

This commit message is also incomplete. The tp_flags variable was indeed
read, even if it was always ANDed with 0.

I applied it anyway this time, and also dropped the now unused
TI_SET_SERIAL_FLAGS define that you did remove in v1.

> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>

Thanks,
Johan

[PATCH v2 07/22] usb: serial: ti_usb_3410_5052: Use macros instead of magic values

[Mathieu OTHACEHE]

Use macros to define 3410 and 5052 baud bases.
Use macro to define usb download timeout.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 2b7fe89..b5f3328 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -270,10 +270,12 @@ struct ti_firmware_header {
 #define TI_DRIVER_AUTHOR	"Al Borchers <alborchers@steinerpoint.com>"
 #define TI_DRIVER_DESC		"TI USB 3410/5052 Serial Driver"
 
-#define TI_FIRMWARE_BUF_SIZE	16284
+#define TI_3410_BAUD_BASE       923077
+#define TI_5052_BAUD_BASE       461538
 
+#define TI_FIRMWARE_BUF_SIZE	16284
 #define TI_TRANSFER_TIMEOUT	2
-
+#define TI_DOWNLOAD_TIMEOUT	1000
 #define TI_DEFAULT_CLOSING_WAIT	4000		/* in .01 secs */
 
 /* supported setserial flags */
@@ -1016,9 +1018,9 @@ static void ti_set_termios(struct tty_struct *tty,
 	if (!baud)
 		baud = 9600;
 	if (tport->tp_tdev->td_is_3410)
-		wbaudrate = (923077 + baud/2) / baud;
+		wbaudrate = (TI_3410_BAUD_BASE + baud / 2) / baud;
 	else
-		wbaudrate = (461538 + baud/2) / baud;
+		wbaudrate = (TI_5052_BAUD_BASE + baud / 2) / baud;
 
 	/* FIXME: Should calculate resulting baud here and report it back */
 	if ((C_BAUD(tty)) != B0)
@@ -1434,6 +1436,7 @@ static int ti_get_serial_info(struct ti_port *tport,
 	struct usb_serial_port *port = tport->tp_port;
 	struct serial_struct ret_serial;
 	unsigned cwait;
+	int baud_base;
 
 	if (!ret_arg)
 		return -EFAULT;
@@ -1444,11 +1447,16 @@ static int ti_get_serial_info(struct ti_port *tport,
 
 	memset(&ret_serial, 0, sizeof(ret_serial));
 
+	if (tport->tp_tdev->td_is_3410)
+		baud_base = TI_3410_BAUD_BASE;
+	else
+		baud_base = TI_5052_BAUD_BASE;
+
 	ret_serial.type = PORT_16550A;
 	ret_serial.line = port->minor;
 	ret_serial.port = port->port_number;
 	ret_serial.xmit_fifo_size = kfifo_size(&port->write_fifo);
-	ret_serial.baud_base = tport->tp_tdev->td_is_3410 ? 921600 : 460800;
+	ret_serial.baud_base = baud_base;
 	ret_serial.closing_wait = cwait;
 
 	if (copy_to_user(ret_arg, &ret_serial, sizeof(*ret_arg)))
@@ -1643,8 +1651,8 @@ static int ti_do_download(struct usb_device *dev, int pipe,
 	dev_dbg(&dev->dev, "%s - downloading firmware\n", __func__);
 	for (pos = 0; pos < size; pos += done) {
 		len = min(size - pos, TI_DOWNLOAD_MAX_PACKET_SIZE);
-		status = usb_bulk_msg(dev, pipe, buffer + pos, len,
-								&done, 1000);
+		status = usb_bulk_msg(dev, pipe, buffer + pos, len, &done,
+				      TI_DOWNLOAD_TIMEOUT);
 		if (status)
 			break;
 	}
-- 
2.9.0

Re: [PATCH v2 07/22] usb: serial: ti_usb_3410_5052: Use macros instead of magic values

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:47PM +0200, Mathieu OTHACEHE wrote:
> Use macros to define 3410 and 5052 baud bases.
> Use macro to define usb download timeout.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/ti_usb_3410_5052.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index 2b7fe89..b5f3328 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -270,10 +270,12 @@ struct ti_firmware_header {
>  #define TI_DRIVER_AUTHOR	"Al Borchers <alborchers@steinerpoint.com>"
>  #define TI_DRIVER_DESC		"TI USB 3410/5052 Serial Driver"
>  
> -#define TI_FIRMWARE_BUF_SIZE	16284
> +#define TI_3410_BAUD_BASE       923077
> +#define TI_5052_BAUD_BASE       461538
>  
> +#define TI_FIRMWARE_BUF_SIZE	16284
>  #define TI_TRANSFER_TIMEOUT	2
> -
> +#define TI_DOWNLOAD_TIMEOUT	1000
>  #define TI_DEFAULT_CLOSING_WAIT	4000		/* in .01 secs */
>  
>  /* supported setserial flags */
> @@ -1016,9 +1018,9 @@ static void ti_set_termios(struct tty_struct *tty,
>  	if (!baud)
>  		baud = 9600;
>  	if (tport->tp_tdev->td_is_3410)
> -		wbaudrate = (923077 + baud/2) / baud;
> +		wbaudrate = (TI_3410_BAUD_BASE + baud / 2) / baud;
>  	else
> -		wbaudrate = (461538 + baud/2) / baud;
> +		wbaudrate = (TI_5052_BAUD_BASE + baud / 2) / baud;
>  
>  	/* FIXME: Should calculate resulting baud here and report it back */
>  	if ((C_BAUD(tty)) != B0)
> @@ -1434,6 +1436,7 @@ static int ti_get_serial_info(struct ti_port *tport,
>  	struct usb_serial_port *port = tport->tp_port;
>  	struct serial_struct ret_serial;
>  	unsigned cwait;
> +	int baud_base;
>  
>  	if (!ret_arg)
>  		return -EFAULT;
> @@ -1444,11 +1447,16 @@ static int ti_get_serial_info(struct ti_port *tport,
>  
>  	memset(&ret_serial, 0, sizeof(ret_serial));
>  
> +	if (tport->tp_tdev->td_is_3410)
> +		baud_base = TI_3410_BAUD_BASE;
> +	else
> +		baud_base = TI_5052_BAUD_BASE;
> +
>  	ret_serial.type = PORT_16550A;
>  	ret_serial.line = port->minor;
>  	ret_serial.port = port->port_number;
>  	ret_serial.xmit_fifo_size = kfifo_size(&port->write_fifo);
> -	ret_serial.baud_base = tport->tp_tdev->td_is_3410 ? 921600 : 460800;
> +	ret_serial.baud_base = baud_base;

The above is not functionally equivalent, since your now returning a
different baud_base.

This may be fine, but again you're hiding changes in the code without
describing them properly in the commit messages.

Please go through the rest of the series, and make sure that the commit
messages are correct. I'm not gonna look at the rest of the series.

Thanks,
Johan

[PATCH v2 08/22] usb: serial: ti_usb_3410_5052: Remove in_sync and out_sync functions

[Mathieu OTHACEHE]

ti_command_in_sync and ti_command_out_sync shouldn't use userspace
datatypes (__uX), data should be void* to avoid casting and size
should be size_t.

This patch rewrite those functions with new names: ti_send_ctrl_data_urb
and ti_recv_ctrl_urb. Also add a ti_send_ctrl_urb to simplify command
sending.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 157 +++++++++++++++++++---------------
 1 file changed, 88 insertions(+), 69 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index b5f3328..e8515eb 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -348,11 +348,6 @@ static void ti_handle_new_msr(struct ti_port *tport, u8 msr);
 static void ti_stop_read(struct ti_port *tport, struct tty_struct *tty);
 static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty);
 
-static int ti_command_out_sync(struct ti_device *tdev, __u8 command,
-	__u16 moduleid, __u16 value, __u8 *data, int size);
-static int ti_command_in_sync(struct ti_device *tdev, __u8 command,
-	__u16 moduleid, __u16 value, __u8 *data, int size);
-
 static int ti_write_byte(struct usb_serial_port *port, struct ti_device *tdev,
 			 unsigned long addr, u8 mask, u8 byte);
 
@@ -517,6 +512,71 @@ MODULE_DEVICE_TABLE(usb, ti_id_table_combined);
 
 module_usb_serial_driver(serial_drivers, ti_id_table_combined);
 
+static int ti_send_ctrl_data_urb(struct usb_serial *serial, u8 request,
+				 u16 value, u16 index, void *data, size_t size)
+{
+	int status;
+
+	status = usb_control_msg(serial->dev,
+				 usb_sndctrlpipe(serial->dev, 0),
+				 request,
+				 (USB_TYPE_VENDOR | USB_RECIP_DEVICE |
+				  USB_DIR_OUT), value, index,
+				 data, size,
+				 USB_CTRL_SET_TIMEOUT);
+	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;
+}
+
+static int ti_send_ctrl_urb(struct usb_serial *serial,
+			    u8 request, u16 value, u16 index)
+{
+	return ti_send_ctrl_data_urb(serial, request, value, index,
+				     NULL, 0);
+}
+
+static int ti_recv_ctrl_data_urb(struct usb_serial *serial, u8 request,
+				 u16 value, u16 index, void *data, size_t size)
+{
+	int status;
+
+	status = usb_control_msg(serial->dev,
+				 usb_rcvctrlpipe(serial->dev, 0),
+				 request,
+				 (USB_TYPE_VENDOR | USB_RECIP_DEVICE |
+				  USB_DIR_IN), value, index,
+				 data, size,
+				 USB_CTRL_SET_TIMEOUT);
+	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 read (%d / %zd)\n",
+			__func__, status, size);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int ti_startup(struct usb_serial *serial)
 {
 	struct ti_device *tdev;
@@ -642,6 +702,7 @@ static int ti_port_remove(struct usb_serial_port *port)
 static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	struct ti_port *tport = usb_get_serial_port_data(port);
+	struct usb_serial *serial = port->serial;
 	struct ti_device *tdev;
 	struct usb_device *dev;
 	struct urb *urb;
@@ -685,31 +746,32 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (tty)
 		ti_set_termios(tty, port, &tty->termios);
 
-	status = ti_command_out_sync(tdev, TI_OPEN_PORT,
-		(__u8)(TI_UART1_PORT + port_number), open_settings, NULL, 0);
+	status = ti_send_ctrl_urb(serial, TI_OPEN_PORT, open_settings,
+				  TI_UART1_PORT + port_number);
 	if (status) {
 		dev_err(&port->dev, "%s - cannot send open command, %d\n",
 			__func__, status);
 		goto unlink_int_urb;
 	}
 
-	status = ti_command_out_sync(tdev, TI_START_PORT,
-		(__u8)(TI_UART1_PORT + port_number), 0, NULL, 0);
+	status = ti_send_ctrl_urb(serial, TI_START_PORT, 0,
+				  TI_UART1_PORT + port_number);
 	if (status) {
 		dev_err(&port->dev, "%s - cannot send start command, %d\n",
 							__func__, status);
 		goto unlink_int_urb;
 	}
 
-	status = ti_command_out_sync(tdev, TI_PURGE_PORT,
-		(__u8)(TI_UART1_PORT + port_number), TI_PURGE_INPUT, NULL, 0);
+	status = ti_send_ctrl_urb(serial, TI_PURGE_PORT, TI_PURGE_INPUT,
+				  TI_UART1_PORT + port_number);
 	if (status) {
 		dev_err(&port->dev, "%s - cannot clear input buffers, %d\n",
 							__func__, status);
 		goto unlink_int_urb;
 	}
-	status = ti_command_out_sync(tdev, TI_PURGE_PORT,
-		(__u8)(TI_UART1_PORT + port_number), TI_PURGE_OUTPUT, NULL, 0);
+
+	status = ti_send_ctrl_urb(serial, TI_PURGE_PORT, TI_PURGE_OUTPUT,
+				  TI_UART1_PORT + port_number);
 	if (status) {
 		dev_err(&port->dev, "%s - cannot clear output buffers, %d\n",
 							__func__, status);
@@ -724,16 +786,16 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (tty)
 		ti_set_termios(tty, port, &tty->termios);
 
-	status = ti_command_out_sync(tdev, TI_OPEN_PORT,
-		(__u8)(TI_UART1_PORT + port_number), open_settings, NULL, 0);
+	status = ti_send_ctrl_urb(serial, TI_OPEN_PORT, open_settings,
+				  TI_UART1_PORT + port_number);
 	if (status) {
 		dev_err(&port->dev, "%s - cannot send open command (2), %d\n",
 							__func__, status);
 		goto unlink_int_urb;
 	}
 
-	status = ti_command_out_sync(tdev, TI_START_PORT,
-		(__u8)(TI_UART1_PORT + port_number), 0, NULL, 0);
+	status = ti_send_ctrl_urb(serial, TI_START_PORT, 0,
+				  TI_UART1_PORT + port_number);
 	if (status) {
 		dev_err(&port->dev, "%s - cannot send start command (2), %d\n",
 							__func__, status);
@@ -793,8 +855,8 @@ static void ti_close(struct usb_serial_port *port)
 
 	port_number = port->port_number;
 
-	status = ti_command_out_sync(tdev, TI_CLOSE_PORT,
-		     (__u8)(TI_UART1_PORT + port_number), 0, NULL, 0);
+	status = ti_send_ctrl_urb(port->serial, TI_CLOSE_PORT, 0,
+				  TI_UART1_PORT + port_number);
 	if (status)
 		dev_err(&port->dev,
 			"%s - cannot send close port command, %d\n"
@@ -1035,9 +1097,9 @@ static void ti_set_termios(struct tty_struct *tty,
 	config->wBaudRate = cpu_to_be16(wbaudrate);
 	config->wFlags = cpu_to_be16(wflags);
 
-	status = ti_command_out_sync(tport->tp_tdev, TI_SET_CONFIG,
-		(__u8)(TI_UART1_PORT + port_number), 0, (__u8 *)config,
-		sizeof(*config));
+	status = ti_send_ctrl_data_urb(port->serial, TI_SET_CONFIG, 0,
+				       TI_UART1_PORT + port_number, config,
+				       sizeof(*config));
 	if (status)
 		dev_err(&port->dev, "%s - cannot set config on port %d, %d\n",
 					__func__, port_number, status);
@@ -1401,7 +1463,6 @@ static int ti_set_mcr(struct ti_port *tport, unsigned int mcr)
 static int ti_get_lsr(struct ti_port *tport, u8 *lsr)
 {
 	int size, status;
-	struct ti_device *tdev = tport->tp_tdev;
 	struct usb_serial_port *port = tport->tp_port;
 	int port_number = port->port_number;
 	struct ti_port_status *data;
@@ -1411,8 +1472,8 @@ static int ti_get_lsr(struct ti_port *tport, u8 *lsr)
 	if (!data)
 		return -ENOMEM;
 
-	status = ti_command_in_sync(tdev, TI_GET_PORT_STATUS,
-		(__u8)(TI_UART1_PORT+port_number), 0, (__u8 *)data, size);
+	status = ti_recv_ctrl_data_urb(port->serial, TI_GET_PORT_STATUS, 0,
+				       TI_UART1_PORT + port_number, data, size);
 	if (status) {
 		dev_err(&port->dev,
 			"%s - get port status command failed, %d\n",
@@ -1555,47 +1616,6 @@ static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty)
 	return status;
 }
 
-
-static int ti_command_out_sync(struct ti_device *tdev, __u8 command,
-	__u16 moduleid, __u16 value, __u8 *data, int size)
-{
-	int status;
-
-	status = usb_control_msg(tdev->td_serial->dev,
-		usb_sndctrlpipe(tdev->td_serial->dev, 0), command,
-		(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT),
-		value, moduleid, data, size, 1000);
-
-	if (status == size)
-		status = 0;
-
-	if (status > 0)
-		status = -ECOMM;
-
-	return status;
-}
-
-
-static int ti_command_in_sync(struct ti_device *tdev, __u8 command,
-	__u16 moduleid, __u16 value, __u8 *data, int size)
-{
-	int status;
-
-	status = usb_control_msg(tdev->td_serial->dev,
-		usb_rcvctrlpipe(tdev->td_serial->dev, 0), command,
-		(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN),
-		value, moduleid, data, size, 1000);
-
-	if (status == size)
-		status = 0;
-
-	if (status > 0)
-		status = -ECOMM;
-
-	return status;
-}
-
-
 static int ti_write_byte(struct usb_serial_port *port,
 			 struct ti_device *tdev, unsigned long addr,
 			 u8 mask, u8 byte)
@@ -1620,9 +1640,8 @@ static int ti_write_byte(struct usb_serial_port *port,
 	data->bData[0] = mask;
 	data->bData[1] = byte;
 
-	status = ti_command_out_sync(tdev, TI_WRITE_DATA, TI_RAM_PORT, 0,
-		(__u8 *)data, size);
-
+	status = ti_send_ctrl_data_urb(port->serial, TI_WRITE_DATA, 0,
+				       TI_RAM_PORT, data, size);
 	if (status < 0)
 		dev_err(&port->dev, "%s - failed, %d\n", __func__, status);
 
-- 
2.9.0

[PATCH v2 09/22] usb: serial: ti_usb_3410_5052: Remove useless tty_wakeup

[Mathieu OTHACEHE]

tty_wakeup is already called when blocked bulk-out transfers complete.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index e8515eb..5f0d010 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1549,7 +1549,6 @@ static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 static void ti_handle_new_msr(struct ti_port *tport, u8 msr)
 {
 	struct async_icount *icount;
-	struct tty_struct *tty;
 	unsigned long flags;
 
 	dev_dbg(&tport->tp_port->dev, "%s - msr 0x%02X\n", __func__, msr);
@@ -1570,14 +1569,6 @@ static void ti_handle_new_msr(struct ti_port *tport, u8 msr)
 	}
 
 	tport->tp_msr = msr & TI_MSR_MASK;
-
-	/* handle CTS flow control */
-	tty = tty_port_tty_get(&tport->tp_port->port);
-	if (tty && C_CRTSCTS(tty)) {
-		if (msr & TI_MSR_CTS)
-			tty_wakeup(tty);
-	}
-	tty_kref_put(tty);
 }
 
 
-- 
2.9.0

[PATCH v2 10/22] usb: serial: ti_usb_3410_5052: Change ti_write_byte function arguments

[Mathieu OTHACEHE]

Remove useless ti_device pointer, and change addr to u32.
Change size variable in function from int to size_t.

Also fix minor style issue.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
Changelog:
v2:
* Do not delete prototype and move function declaration

 drivers/usb/serial/ti_usb_3410_5052.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 5f0d010..7f53432 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -348,8 +348,8 @@ static void ti_handle_new_msr(struct ti_port *tport, u8 msr);
 static void ti_stop_read(struct ti_port *tport, struct tty_struct *tty);
 static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty);
 
-static int ti_write_byte(struct usb_serial_port *port, struct ti_device *tdev,
-			 unsigned long addr, u8 mask, u8 byte);
+static int ti_write_byte(struct usb_serial_port *port, u32 addr,
+			 u8 mask, u8 byte);
 
 static int ti_download_firmware(struct ti_device *tdev);
 
@@ -1185,10 +1185,9 @@ static void ti_break(struct tty_struct *tty, int break_state)
 
 	dev_dbg(&port->dev, "%s - state = %d\n", __func__, break_state);
 
-	status = ti_write_byte(port, tport->tp_tdev,
+	status = ti_write_byte(port,
 		tport->tp_uart_base_addr + TI_UART_OFFSET_LCR,
 		TI_LCR_BREAK, break_state == -1 ? TI_LCR_BREAK : 0);
-
 	if (status)
 		dev_dbg(&port->dev, "%s - error setting break, %d\n", __func__, status);
 }
@@ -1447,7 +1446,7 @@ static int ti_set_mcr(struct ti_port *tport, unsigned int mcr)
 	unsigned long flags;
 	int status;
 
-	status = ti_write_byte(tport->tp_port, tport->tp_tdev,
+	status = ti_write_byte(tport->tp_port,
 		tport->tp_uart_base_addr + TI_UART_OFFSET_MCR,
 		TI_MCR_RTS | TI_MCR_DTR | TI_MCR_LOOP, mcr);
 
@@ -1607,16 +1606,15 @@ static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty)
 	return status;
 }
 
-static int ti_write_byte(struct usb_serial_port *port,
-			 struct ti_device *tdev, unsigned long addr,
+static int ti_write_byte(struct usb_serial_port *port, u32 addr,
 			 u8 mask, u8 byte)
 {
 	int status;
-	unsigned int size;
+	size_t size;
 	struct ti_write_data_bytes *data;
 
-	dev_dbg(&port->dev, "%s - addr 0x%08lX, mask 0x%02X, byte 0x%02X\n", __func__,
-		addr, mask, byte);
+	dev_dbg(&port->dev, "%s - addr 0x%08X, mask 0x%02X, byte 0x%02X\n",
+		__func__, addr, mask, byte);
 
 	size = sizeof(struct ti_write_data_bytes) + 2;
 	data = kmalloc(size, GFP_KERNEL);
@@ -1626,7 +1624,7 @@ static int ti_write_byte(struct usb_serial_port *port,
 	data->bAddrType = TI_RW_DATA_ADDR_XDATA;
 	data->bDataType = TI_RW_DATA_BYTE;
 	data->bDataCounter = 1;
-	data->wBaseAddrHi = cpu_to_be16(addr>>16);
+	data->wBaseAddrHi = cpu_to_be16(addr >> 16);
 	data->wBaseAddrLo = cpu_to_be16(addr);
 	data->bData[0] = mask;
 	data->bData[1] = byte;
-- 
2.9.0

Re: [PATCH v2 10/22] usb: serial: ti_usb_3410_5052: Change ti_write_byte function arguments

[Oliver Neukum]

On Tue, 2016-07-26 at 19:59 +0200, Mathieu OTHACEHE wrote:
> @@ -1626,7 +1624,7 @@ static int ti_write_byte(struct usb_serial_port
> *port,
>         data->bAddrType = TI_RW_DATA_ADDR_XDATA;
>         data->bDataType = TI_RW_DATA_BYTE;
>         data->bDataCounter = 1;
> -       data->wBaseAddrHi = cpu_to_be16(addr>>16);
> +       data->wBaseAddrHi = cpu_to_be16(addr >> 16);
>         data->wBaseAddrLo = cpu_to_be16(addr);
>         data->bData[0] = mask;
>         data->bData[1] = byte;
> -- 

Hi,

this makes me think something is wrong with the data structure.
We should have a be32 there, it seems to me.

	Regards
		Oliver

Re: [PATCH v2 10/22] usb: serial: ti_usb_3410_5052: Change ti_write_byte function arguments

[Mathieu OTHACEHE]

Hi,

> this makes me think something is wrong with the data structure.
> We should have a be32 there, it seems to me.

You mean something like :

struct ti_write_data_bytes {
	u8	bAddrType;
	u8	bDataType;
	u8	bDataCounter;
	__be32	wBaseAddr;
	u8	bData[0];
} __packed;

and,

data->wBaseAddr = cpu_to_be32(addr) ?

Thanks,

Mathieu

Re: [PATCH v2 10/22] usb: serial: ti_usb_3410_5052: Change ti_write_byte function arguments

[Oliver Neukum]

On Wed, 2016-07-27 at 18:08 +0200, Mathieu OTHACEHE wrote:
> Hi,
> 
> > this makes me think something is wrong with the data structure.
> > We should have a be32 there, it seems to me.
> 
> You mean something like :
> 
> struct ti_write_data_bytes {
> 	u8	bAddrType;
> 	u8	bDataType;
> 	u8	bDataCounter;
> 	__be32	wBaseAddr;
> 	u8	bData[0];
> } __packed;
> 
> and,
> 
> data->wBaseAddr = cpu_to_be32(addr) ?

Yes.

	Regards
		Oliver

[PATCH v2 11/22] usb: serial: ti_usb_3410_5052: Do not modify interrupt context

[Mathieu OTHACEHE]

It is useless to pass a specific context (ti_device) to the interrupt
callback. So use the default context (usb_serial_port).

Remove useless variables in ti_interrupt_callback.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 58 ++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 7f53432..ee8f08c 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -343,7 +343,7 @@ static int ti_get_serial_info(struct ti_port *tport,
 	struct serial_struct __user *ret_arg);
 static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 	struct serial_struct __user *new_arg);
-static void ti_handle_new_msr(struct ti_port *tport, u8 msr);
+static void ti_handle_new_msr(struct usb_serial_port *port, u8 msr);
 
 static void ti_stop_read(struct ti_port *tport, struct tty_struct *tty);
 static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty);
@@ -735,7 +735,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 			status = -EINVAL;
 			goto release_lock;
 		}
-		urb->context = tdev;
 		status = usb_submit_urb(urb, GFP_KERNEL);
 		if (status) {
 			dev_err(&port->dev, "%s - submit interrupt urb failed, %d\n", __func__, status);
@@ -1204,17 +1203,12 @@ static int ti_get_func_from_code(unsigned char code)
 
 static void ti_interrupt_callback(struct urb *urb)
 {
-	struct ti_device *tdev = urb->context;
-	struct usb_serial_port *port;
-	struct usb_serial *serial = tdev->td_serial;
-	struct ti_port *tport;
-	struct device *dev = &urb->dev->dev;
+	struct usb_serial_port *port = urb->context;
 	unsigned char *data = urb->transfer_buffer;
 	int length = urb->actual_length;
 	int port_number;
 	int function;
 	int status = urb->status;
-	int retval;
 	u8 msr;
 
 	switch (status) {
@@ -1223,64 +1217,63 @@ static void ti_interrupt_callback(struct urb *urb)
 	case -ECONNRESET:
 	case -ENOENT:
 	case -ESHUTDOWN:
-		dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
+		dev_dbg(&port->dev, "%s - urb shutting down, %d\n",
+			__func__, status);
 		return;
 	default:
-		dev_err(dev, "%s - nonzero urb status, %d\n", __func__, status);
+		dev_err(&port->dev, "%s - nonzero urb status, %d\n",
+			__func__, status);
 		goto exit;
 	}
 
 	if (length != 2) {
-		dev_dbg(dev, "%s - bad packet size, %d\n", __func__, length);
+		dev_dbg(&port->dev, "%s - bad packet size, %d\n",
+			__func__, length);
 		goto exit;
 	}
 
 	if (data[0] == TI_CODE_HARDWARE_ERROR) {
-		dev_err(dev, "%s - hardware error, %d\n", __func__, data[1]);
+		dev_err(&port->dev, "%s - hardware error, %d\n",
+			__func__, data[1]);
 		goto exit;
 	}
 
 	port_number = ti_get_port_from_code(data[0]);
 	function = ti_get_func_from_code(data[0]);
 
-	dev_dbg(dev, "%s - port_number %d, function %d, data 0x%02X\n",
+	dev_dbg(&port->dev, "%s - port_number %d, function %d, data 0x%02X\n",
 		__func__, port_number, function, data[1]);
 
-	if (port_number >= serial->num_ports) {
-		dev_err(dev, "%s - bad port number, %d\n",
+	if (port_number >= port->serial->num_ports) {
+		dev_err(&port->dev, "%s - bad port number, %d\n",
 						__func__, port_number);
 		goto exit;
 	}
 
-	port = serial->port[port_number];
-
-	tport = usb_get_serial_port_data(port);
-	if (!tport)
-		goto exit;
-
 	switch (function) {
 	case TI_CODE_DATA_ERROR:
-		dev_err(dev, "%s - DATA ERROR, port %d, data 0x%02X\n",
+		dev_err(&port->dev, "%s - DATA ERROR, port %d, data 0x%02X\n",
 			__func__, port_number, data[1]);
 		break;
 
 	case TI_CODE_MODEM_STATUS:
 		msr = data[1];
-		dev_dbg(dev, "%s - port %d, msr 0x%02X\n", __func__, port_number, msr);
-		ti_handle_new_msr(tport, msr);
+		dev_dbg(&port->dev, "%s - port %d, msr 0x%02X\n",
+			__func__, port_number, msr);
+		ti_handle_new_msr(port, msr);
 		break;
 
 	default:
-		dev_err(dev, "%s - unknown interrupt code, 0x%02X\n",
+		dev_err(&port->dev, "%s - unknown interrupt code, 0x%02X\n",
 							__func__, data[1]);
 		break;
 	}
 
 exit:
-	retval = usb_submit_urb(urb, GFP_ATOMIC);
-	if (retval)
-		dev_err(dev, "%s - resubmit interrupt urb failed, %d\n",
-			__func__, retval);
+	status = usb_submit_urb(urb, GFP_ATOMIC);
+	if (status)
+		dev_err(&port->dev, "%s - resubmit interrupt urb failed, %d\n",
+			__func__, status);
 }
 
 
@@ -1545,12 +1538,13 @@ static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 }
 
 
-static void ti_handle_new_msr(struct ti_port *tport, u8 msr)
+static void ti_handle_new_msr(struct usb_serial_port *port, u8 msr)
 {
+	struct ti_port *tport = usb_get_serial_port_data(port);
 	struct async_icount *icount;
 	unsigned long flags;
 
-	dev_dbg(&tport->tp_port->dev, "%s - msr 0x%02X\n", __func__, msr);
+	dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr);
 
 	if (msr & TI_MSR_DELTA_MASK) {
 		spin_lock_irqsave(&tport->tp_lock, flags);
@@ -1563,7 +1557,7 @@ static void ti_handle_new_msr(struct ti_port *tport, u8 msr)
 			icount->dcd++;
 		if (msr & TI_MSR_DELTA_RI)
 			icount->rng++;
-		wake_up_interruptible(&tport->tp_port->port.delta_msr_wait);
+		wake_up_interruptible(&port->port.delta_msr_wait);
 		spin_unlock_irqrestore(&tport->tp_lock, flags);
 	}
 
-- 
2.9.0

[PATCH v2 12/22] usb: serial: ti_usb_3410_5052: Remove usb_serial pointer in ti_port

[Mathieu OTHACEHE]

There is no need to keep a pointer to usb_serial in ti_port
structure.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index ee8f08c..8b1dc1a 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -304,7 +304,6 @@ struct ti_port {
 struct ti_device {
 	struct mutex		td_open_close_lock;
 	int			td_open_port_count;
-	struct usb_serial	*td_serial;
 	int			td_is_3410;
 	bool			td_rs485_only;
 };
@@ -351,7 +350,7 @@ static int ti_restart_read(struct ti_port *tport, struct tty_struct *tty);
 static int ti_write_byte(struct usb_serial_port *port, u32 addr,
 			 u8 mask, u8 byte);
 
-static int ti_download_firmware(struct ti_device *tdev);
+static int ti_download_firmware(struct usb_serial *serial);
 
 static int closing_wait = TI_DEFAULT_CLOSING_WAIT;
 
@@ -597,7 +596,6 @@ static int ti_startup(struct usb_serial *serial)
 		return -ENOMEM;
 
 	mutex_init(&tdev->td_open_close_lock);
-	tdev->td_serial = serial;
 	usb_set_serial_data(serial, tdev);
 
 	/* determine device type */
@@ -622,7 +620,7 @@ static int ti_startup(struct usb_serial *serial)
 
 	/* if we have only 1 configuration and 1 endpoint, download firmware */
 	if (dev->descriptor.bNumConfigurations == 1 && num_endpoints == 1) {
-		status = ti_download_firmware(tdev);
+		status = ti_download_firmware(serial);
 
 		if (status != 0)
 			goto free_tdev;
@@ -729,7 +727,7 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	/* start interrupt urb the first time a port is opened on this device */
 	if (tdev->td_open_port_count == 0) {
 		dev_dbg(&port->dev, "%s - start interrupt in urb\n", __func__);
-		urb = tdev->td_serial->port[0]->interrupt_in_urb;
+		urb = serial->port[0]->interrupt_in_urb;
 		if (!urb) {
 			dev_err(&port->dev, "%s - no interrupt urb\n", __func__);
 			status = -EINVAL;
@@ -824,7 +822,7 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 unlink_int_urb:
 	if (tdev->td_open_port_count == 0)
-		usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
+		usb_kill_urb(serial->port[0]->interrupt_in_urb);
 release_lock:
 	mutex_unlock(&tdev->td_open_close_lock);
 	return status;
@@ -1661,17 +1659,19 @@ static int ti_do_download(struct usb_device *dev, int pipe,
 	return status;
 }
 
-static int ti_download_firmware(struct ti_device *tdev)
+static int ti_download_firmware(struct usb_serial *serial)
 {
 	int status;
 	int buffer_size;
 	u8 *buffer;
-	struct usb_device *dev = tdev->td_serial->dev;
-	unsigned int pipe = usb_sndbulkpipe(dev,
-		tdev->td_serial->port[0]->bulk_out_endpointAddress);
+	struct usb_device *dev = serial->dev;
+	struct ti_device *tdev = usb_get_serial_data(serial);
+	unsigned int pipe;
 	const struct firmware *fw_p;
 	char buf[32];
 
+	pipe = usb_sndbulkpipe(dev, serial->port[0]->bulk_out_endpointAddress);
+
 	if (le16_to_cpu(dev->descriptor.idVendor) == MXU1_VENDOR_ID) {
 		snprintf(buf,
 			sizeof(buf),
-- 
2.9.0

[PATCH v2 13/22] usb: serial: ti_usb_3410_5052: Change ti_get/set_serial_info function arguments

[Mathieu OTHACEHE]

It is sufficient to pass usb_serial_port structure to ti_get_serial_info
and ti_set_serial_info.

Also use unsigned int instead of unsigned for cwait variable.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
Changelog:
v2:
* Do not remove prototypes and move functions declarations.

 drivers/usb/serial/ti_usb_3410_5052.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 8b1dc1a..3a88c2f 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -338,10 +338,10 @@ static void ti_recv(struct usb_serial_port *port, unsigned char *data,
 static void ti_send(struct ti_port *tport);
 static int ti_set_mcr(struct ti_port *tport, unsigned int mcr);
 static int ti_get_lsr(struct ti_port *tport, u8 *lsr);
-static int ti_get_serial_info(struct ti_port *tport,
-	struct serial_struct __user *ret_arg);
-static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
-	struct serial_struct __user *new_arg);
+static int ti_get_serial_info(struct usb_serial_port *port,
+			      struct serial_struct __user *ret_arg);
+static int ti_set_serial_info(struct usb_serial_port *port,
+			      struct serial_struct __user *new_arg);
 static void ti_handle_new_msr(struct usb_serial_port *port, u8 msr);
 
 static void ti_stop_read(struct ti_port *tport, struct tty_struct *tty);
@@ -965,15 +965,14 @@ static int ti_ioctl(struct tty_struct *tty,
 	unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct ti_port *tport = usb_get_serial_port_data(port);
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		return ti_get_serial_info(tport,
-				(struct serial_struct __user *)arg);
+		return ti_get_serial_info(port,
+					  (struct serial_struct __user *)arg);
 	case TIOCSSERIAL:
-		return ti_set_serial_info(tty, tport,
-				(struct serial_struct __user *)arg);
+		return ti_set_serial_info(port,
+					  (struct serial_struct __user *)arg);
 	}
 	return -ENOIOCTLCMD;
 }
@@ -1481,12 +1480,12 @@ free_data:
 }
 
 
-static int ti_get_serial_info(struct ti_port *tport,
-	struct serial_struct __user *ret_arg)
+static int ti_get_serial_info(struct usb_serial_port *port,
+			      struct serial_struct __user *ret_arg)
 {
-	struct usb_serial_port *port = tport->tp_port;
+	struct ti_device *tdev = usb_get_serial_data(port->serial);
 	struct serial_struct ret_serial;
-	unsigned cwait;
+	unsigned int cwait;
 	int baud_base;
 
 	if (!ret_arg)
@@ -1498,7 +1497,7 @@ static int ti_get_serial_info(struct ti_port *tport,
 
 	memset(&ret_serial, 0, sizeof(ret_serial));
 
-	if (tport->tp_tdev->td_is_3410)
+	if (tdev->td_is_3410)
 		baud_base = TI_3410_BAUD_BASE;
 	else
 		baud_base = TI_5052_BAUD_BASE;
@@ -1517,8 +1516,8 @@ static int ti_get_serial_info(struct ti_port *tport,
 }
 
 
-static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
-	struct serial_struct __user *new_arg)
+static int ti_set_serial_info(struct usb_serial_port *port,
+			      struct serial_struct __user *new_arg)
 {
 	struct serial_struct new_serial;
 	unsigned cwait;
@@ -1530,7 +1529,7 @@ static int ti_set_serial_info(struct tty_struct *tty, struct ti_port *tport,
 	if (cwait != ASYNC_CLOSING_WAIT_NONE)
 		cwait = msecs_to_jiffies(10 * new_serial.closing_wait);
 
-	tport->tp_port->port.closing_wait = cwait;
+	port->port.closing_wait = cwait;
 
 	return 0;
 }
-- 
2.9.0

[PATCH v2 14/22] usb: serial: ti_usb_3410_5052: Do not set shadow mcr in open callback

[Mathieu OTHACEHE]

Setting DTR/RTS is handled using dtr_rts in tty_core.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 3a88c2f..b5ea850 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -722,7 +722,6 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	port_number = port->port_number;
 
 	tport->tp_msr = 0;
-	tport->tp_shadow_mcr |= (TI_MCR_RTS | TI_MCR_DTR);
 
 	/* start interrupt urb the first time a port is opened on this device */
 	if (tdev->td_open_port_count == 0) {
-- 
2.9.0

[PATCH v2 15/22] usb: serial: ti_usb_3410_5052: Check old_termios parameter in set_termios

[Mathieu OTHACEHE]

The old_termios parameter is never used in set_termios callback.

Add a check to old_termios to see if we can return right away because
there is nothing to change.

Also pass NULL for old_termios in open callback because it is the
initial call to set_termios.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
Changelog:
v2:
* Deal with DTR and RTS raising in the next patch.

 drivers/usb/serial/ti_usb_3410_5052.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index b5ea850..d488237 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -740,7 +740,7 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	}
 
 	if (tty)
-		ti_set_termios(tty, port, &tty->termios);
+		ti_set_termios(tty, port, NULL);
 
 	status = ti_send_ctrl_urb(serial, TI_OPEN_PORT, open_settings,
 				  TI_UART1_PORT + port_number);
@@ -780,7 +780,7 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	usb_clear_halt(dev, port->read_urb->pipe);
 
 	if (tty)
-		ti_set_termios(tty, port, &tty->termios);
+		ti_set_termios(tty, port, NULL);
 
 	status = ti_send_ctrl_urb(serial, TI_OPEN_PORT, open_settings,
 				  TI_UART1_PORT + port_number);
@@ -993,6 +993,13 @@ static void ti_set_termios(struct tty_struct *tty,
 	cflag = tty->termios.c_cflag;
 	iflag = tty->termios.c_iflag;
 
+	if (old_termios &&
+		!tty_termios_hw_change(&tty->termios, old_termios) &&
+		tty->termios.c_iflag == old_termios->c_iflag) {
+		dev_dbg(&port->dev, "%s - nothing to change\n", __func__);
+		return;
+	}
+
 	dev_dbg(&port->dev,
 		"%s - cflag 0x%08x, iflag 0x%08x\n", __func__, cflag, iflag);
 
-- 
2.9.0

[PATCH v2 16/22] usb: serial: ti_usb_3410_5052: Raise DTR and RTS flags if speed is not null anymore

[Mathieu OTHACEHE]

If speed is non null anymore, we can raise DTR and RTS flags in
ti_set_termios.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index d488237..472971b 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1111,6 +1111,9 @@ static void ti_set_termios(struct tty_struct *tty,
 	/* if baud rate is B0, clear RTS and DTR */
 	if (C_BAUD(tty) == B0)
 		mcr &= ~(TI_MCR_DTR | TI_MCR_RTS);
+	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
+		mcr |= TI_MCR_DTR | TI_MCR_RTS;
+
 	status = ti_set_mcr(tport, mcr);
 	if (status)
 		dev_err(&port->dev,
-- 
2.9.0

[PATCH v2 17/22] usb: serial: ti_usb_3410_5052: Fix firmware downloading

[Mathieu OTHACEHE]

The buffer used to store firmware is allocated to maximum firmware
size (TI_FIRMWARE_BUF_SIZE) + header size.
This buffer is filled with requested firmware (fw_p->size) and padded
with 0xff bytes. The header is written over the 3 first bytes of the
buffer (overwritting the 3 first bytes of the requested firmware)

Then, ti_do_download function is called to send only the first
fw_p->size bytes from the buffer to the device.
The rest of the buffer is never used.

So it is sufficient to allocate a buffer of fw_p->size bytes to store
only the requested firmware. There is no need to do any padding.

This patch also move firmware buffer manipulation to ti_do_download
function.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 61 +++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 472971b..b46a511 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1639,46 +1639,65 @@ static int ti_write_byte(struct usb_serial_port *port, u32 addr,
 	return status;
 }
 
-static int ti_do_download(struct usb_device *dev, int pipe,
-			  u8 *buffer, int size)
+static int ti_do_download(struct usb_serial *serial,
+			  const struct firmware *fw_p)
 {
 	int pos;
 	u8 cs = 0;
 	int done;
+	struct usb_device *dev = serial->dev;
 	struct ti_firmware_header *header;
 	int status = 0;
+	u8 *buffer;
+	int buffer_size;
 	int len;
+	unsigned int pipe;
+
+	pipe = usb_sndbulkpipe(dev, serial->port[0]->bulk_out_endpointAddress);
+
+	buffer_size = fw_p->size;
+	buffer = kmalloc(buffer_size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
 
-	for (pos = sizeof(struct ti_firmware_header); pos < size; pos++)
+	memcpy(buffer, fw_p->data, fw_p->size);
+
+	for (pos = sizeof(*header); pos < buffer_size; pos++)
 		cs = (u8)(cs + buffer[pos]);
 
 	header = (struct ti_firmware_header *)buffer;
-	header->wLength = cpu_to_le16(size - sizeof(*header));
+	header->wLength = cpu_to_le16(buffer_size - sizeof(*header));
 	header->bCheckSum = cs;
 
 	dev_dbg(&dev->dev, "%s - downloading firmware\n", __func__);
-	for (pos = 0; pos < size; pos += done) {
-		len = min(size - pos, TI_DOWNLOAD_MAX_PACKET_SIZE);
+	for (pos = 0; pos < buffer_size; pos += done) {
+		len = min(buffer_size - pos, TI_DOWNLOAD_MAX_PACKET_SIZE);
 		status = usb_bulk_msg(dev, pipe, buffer + pos, len, &done,
 				      TI_DOWNLOAD_TIMEOUT);
 		if (status)
 			break;
 	}
-	return status;
+
+	kfree(buffer);
+
+	if (status) {
+		dev_err(&dev->dev, "failed to download firmware: %d\n", status);
+		return status;
+	}
+
+	dev_dbg(&dev->dev, "%s - download successful\n", __func__);
+
+	return 0;
 }
 
 static int ti_download_firmware(struct usb_serial *serial)
 {
 	int status;
-	int buffer_size;
-	u8 *buffer;
 	struct usb_device *dev = serial->dev;
 	struct ti_device *tdev = usb_get_serial_data(serial);
-	unsigned int pipe;
 	const struct firmware *fw_p;
 	char buf[32];
 
-	pipe = usb_sndbulkpipe(dev, serial->port[0]->bulk_out_endpointAddress);
 
 	if (le16_to_cpu(dev->descriptor.idVendor) == MXU1_VENDOR_ID) {
 		snprintf(buf,
@@ -1733,30 +1752,16 @@ check_firmware:
 		dev_err(&dev->dev, "%s - firmware not found\n", __func__);
 		return -ENOENT;
 	}
+
 	if (fw_p->size > TI_FIRMWARE_BUF_SIZE) {
 		dev_err(&dev->dev, "%s - firmware too large %zu\n", __func__, fw_p->size);
 		release_firmware(fw_p);
 		return -ENOENT;
 	}
 
-	buffer_size = TI_FIRMWARE_BUF_SIZE + sizeof(struct ti_firmware_header);
-	buffer = kmalloc(buffer_size, GFP_KERNEL);
-	if (buffer) {
-		memcpy(buffer, fw_p->data, fw_p->size);
-		memset(buffer + fw_p->size, 0xff, buffer_size - fw_p->size);
-		status = ti_do_download(dev, pipe, buffer, fw_p->size);
-		kfree(buffer);
-	} else {
-		status = -ENOMEM;
-	}
-	release_firmware(fw_p);
-	if (status) {
-		dev_err(&dev->dev, "%s - error downloading firmware, %d\n",
-							__func__, status);
-		return status;
-	}
+	ti_do_download(serial, fw_p);
 
-	dev_dbg(&dev->dev, "%s - download successful\n", __func__);
+	release_firmware(fw_p);
 
 	return 0;
 }
-- 
2.9.0

[PATCH v2 18/22] usb: serial: ti_usb_3410_5052: Standardize debug and error messages

[Mathieu OTHACEHE]

Use the format "error text: error value\n" when possible.
Drop redundant function names from error messages.
Also move a couple err messages to dbg messages.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 105 ++++++++++++++++------------------
 1 file changed, 50 insertions(+), 55 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index b46a511..e59b3c1 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -601,7 +601,7 @@ static int ti_startup(struct usb_serial *serial)
 	/* determine device type */
 	if (serial->type == &ti_1port_device)
 		tdev->td_is_3410 = 1;
-	dev_dbg(&dev->dev, "%s - device type is %s\n", __func__,
+	dev_dbg(&dev->dev, "%s - device type is: %s\n", __func__,
 		tdev->td_is_3410 ? "3410" : "5052");
 
 	vid = le16_to_cpu(dev->descriptor.idVendor);
@@ -728,13 +728,14 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 		dev_dbg(&port->dev, "%s - start interrupt in urb\n", __func__);
 		urb = serial->port[0]->interrupt_in_urb;
 		if (!urb) {
-			dev_err(&port->dev, "%s - no interrupt urb\n", __func__);
+			dev_err(&port->dev, "no interrupt endpoint\n");
 			status = -EINVAL;
 			goto release_lock;
 		}
 		status = usb_submit_urb(urb, GFP_KERNEL);
 		if (status) {
-			dev_err(&port->dev, "%s - submit interrupt urb failed, %d\n", __func__, status);
+			dev_err(&port->dev, "failed to submit interrupt urb: %d\n",
+				status);
 			goto release_lock;
 		}
 	}
@@ -745,32 +746,29 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	status = ti_send_ctrl_urb(serial, TI_OPEN_PORT, open_settings,
 				  TI_UART1_PORT + port_number);
 	if (status) {
-		dev_err(&port->dev, "%s - cannot send open command, %d\n",
-			__func__, status);
+		dev_err(&port->dev, "cannot send open command: %d\n", status);
 		goto unlink_int_urb;
 	}
 
 	status = ti_send_ctrl_urb(serial, TI_START_PORT, 0,
 				  TI_UART1_PORT + port_number);
 	if (status) {
-		dev_err(&port->dev, "%s - cannot send start command, %d\n",
-							__func__, status);
+		dev_err(&port->dev, "cannot send start command: %d\n", status);
 		goto unlink_int_urb;
 	}
 
 	status = ti_send_ctrl_urb(serial, TI_PURGE_PORT, TI_PURGE_INPUT,
 				  TI_UART1_PORT + port_number);
 	if (status) {
-		dev_err(&port->dev, "%s - cannot clear input buffers, %d\n",
-							__func__, status);
+		dev_err(&port->dev, "cannot clear input buffers: %d\n", status);
 		goto unlink_int_urb;
 	}
 
 	status = ti_send_ctrl_urb(serial, TI_PURGE_PORT, TI_PURGE_OUTPUT,
 				  TI_UART1_PORT + port_number);
 	if (status) {
-		dev_err(&port->dev, "%s - cannot clear output buffers, %d\n",
-							__func__, status);
+		dev_err(&port->dev, "cannot clear output buffers: %d\n",
+			status);
 		goto unlink_int_urb;
 	}
 
@@ -785,23 +783,23 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	status = ti_send_ctrl_urb(serial, TI_OPEN_PORT, open_settings,
 				  TI_UART1_PORT + port_number);
 	if (status) {
-		dev_err(&port->dev, "%s - cannot send open command (2), %d\n",
-							__func__, status);
+		dev_err(&port->dev, "cannot send open command (2): %d\n",
+			status);
 		goto unlink_int_urb;
 	}
 
 	status = ti_send_ctrl_urb(serial, TI_START_PORT, 0,
 				  TI_UART1_PORT + port_number);
 	if (status) {
-		dev_err(&port->dev, "%s - cannot send start command (2), %d\n",
-							__func__, status);
+		dev_err(&port->dev, "cannot send start command (2): %d\n",
+			status);
 		goto unlink_int_urb;
 	}
 
 	/* start read urb */
 	urb = port->read_urb;
 	if (!urb) {
-		dev_err(&port->dev, "%s - no read urb\n", __func__);
+		dev_err(&port->dev, "no read urb\n");
 		status = -EINVAL;
 		goto unlink_int_urb;
 	}
@@ -809,8 +807,7 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 	urb->context = tport;
 	status = usb_submit_urb(urb, GFP_KERNEL);
 	if (status) {
-		dev_err(&port->dev, "%s - submit read urb failed, %d\n",
-							__func__, status);
+		dev_err(&port->dev, "failed to submit read urb: %d\n", status);
 		goto unlink_int_urb;
 	}
 
@@ -852,11 +849,10 @@ static void ti_close(struct usb_serial_port *port)
 	port_number = port->port_number;
 
 	status = ti_send_ctrl_urb(port->serial, TI_CLOSE_PORT, 0,
-				  TI_UART1_PORT + port_number);
+				  TI_UART1_PORT + port->port_number);
 	if (status)
 		dev_err(&port->dev,
-			"%s - cannot send close port command, %d\n"
-							, __func__, status);
+			"failed to send close port command: %d\n", status);
 
 	/* if mutex_lock is interrupted, continue anyway */
 	do_unlock = !mutex_lock_interruptible(&tdev->td_open_close_lock);
@@ -955,8 +951,8 @@ static void ti_unthrottle(struct tty_struct *tty)
 	if (I_IXOFF(tty) || C_CRTSCTS(tty)) {
 		status = ti_restart_read(tport, tty);
 		if (status)
-			dev_err(&port->dev, "%s - cannot restart read, %d\n",
-							__func__, status);
+			dev_err(&port->dev, "failed to restart read: %d\n",
+				status);
 	}
 }
 
@@ -1102,9 +1098,10 @@ static void ti_set_termios(struct tty_struct *tty,
 	status = ti_send_ctrl_data_urb(port->serial, TI_SET_CONFIG, 0,
 				       TI_UART1_PORT + port_number, config,
 				       sizeof(*config));
-	if (status)
-		dev_err(&port->dev, "%s - cannot set config on port %d, %d\n",
-					__func__, port_number, status);
+	if (status) {
+		dev_err(&port->dev, "cannot set config on port %d: %d\n",
+			port_number, status);
+	}
 
 	/* SET_CONFIG asserts RTS and DTR, reset them correctly */
 	mcr = tport->tp_shadow_mcr;
@@ -1115,10 +1112,11 @@ static void ti_set_termios(struct tty_struct *tty,
 		mcr |= TI_MCR_DTR | TI_MCR_RTS;
 
 	status = ti_set_mcr(tport, mcr);
-	if (status)
+	if (status) {
 		dev_err(&port->dev,
-			"%s - cannot set modem control on port %d, %d\n",
-						__func__, port_number, status);
+			"cannot set modem control on port %d: %d\n",
+			port_number, status);
+	}
 
 	kfree(config);
 }
@@ -1194,7 +1192,7 @@ static void ti_break(struct tty_struct *tty, int break_state)
 		tport->tp_uart_base_addr + TI_UART_OFFSET_LCR,
 		TI_LCR_BREAK, break_state == -1 ? TI_LCR_BREAK : 0);
 	if (status)
-		dev_dbg(&port->dev, "%s - error setting break, %d\n", __func__, status);
+		dev_dbg(&port->dev, "failed to set break: %d\n", status);
 }
 
 static int ti_get_port_from_code(unsigned char code)
@@ -1223,23 +1221,23 @@ static void ti_interrupt_callback(struct urb *urb)
 	case -ECONNRESET:
 	case -ENOENT:
 	case -ESHUTDOWN:
-		dev_dbg(&port->dev, "%s - urb shutting down, %d\n",
+		dev_dbg(&port->dev, "%s - urb shutting down: %d\n",
 			__func__, status);
 		return;
 	default:
-		dev_err(&port->dev, "%s - nonzero urb status, %d\n",
+		dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
 			__func__, status);
 		goto exit;
 	}
 
 	if (length != 2) {
-		dev_dbg(&port->dev, "%s - bad packet size, %d\n",
+		dev_dbg(&port->dev, "%s - bad packet size: %d\n",
 			__func__, length);
 		goto exit;
 	}
 
 	if (data[0] == TI_CODE_HARDWARE_ERROR) {
-		dev_err(&port->dev, "%s - hardware error, %d\n",
+		dev_err(&port->dev, "%s - hardware error: %d\n",
 			__func__, data[1]);
 		goto exit;
 	}
@@ -1251,14 +1249,13 @@ static void ti_interrupt_callback(struct urb *urb)
 		__func__, port_number, function, data[1]);
 
 	if (port_number >= port->serial->num_ports) {
-		dev_err(&port->dev, "%s - bad port number, %d\n",
-						__func__, port_number);
+		dev_err(&port->dev, "bad port number: %d\n", port_number);
 		goto exit;
 	}
 
 	switch (function) {
 	case TI_CODE_DATA_ERROR:
-		dev_err(&port->dev, "%s - DATA ERROR, port %d, data 0x%02X\n",
+		dev_dbg(&port->dev, "%s - DATA ERROR, port %d, data 0x%02X\n",
 			__func__, port_number, data[1]);
 		break;
 
@@ -1270,16 +1267,16 @@ static void ti_interrupt_callback(struct urb *urb)
 		break;
 
 	default:
-		dev_err(&port->dev, "%s - unknown interrupt code, 0x%02X\n",
-							__func__, data[1]);
+		dev_err(&port->dev, "unknown interrupt code: 0x%02X\n",
+			data[1]);
 		break;
 	}
 
 exit:
 	status = usb_submit_urb(urb, GFP_ATOMIC);
 	if (status)
-		dev_err(&port->dev, "%s - resubmit interrupt urb failed, %d\n",
-			__func__, status);
+		dev_err(&port->dev, "resubmit interrupt urb failed: %d\n",
+			status);
 }
 
 
@@ -1300,7 +1297,7 @@ static void ti_bulk_in_callback(struct urb *urb)
 		dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
 		return;
 	default:
-		dev_err(dev, "%s - nonzero urb status, %d\n",
+		dev_dbg(dev, "%s - nonzero urb status, %d\n",
 			__func__, status);
 	}
 
@@ -1308,7 +1305,7 @@ static void ti_bulk_in_callback(struct urb *urb)
 		goto exit;
 
 	if (status) {
-		dev_err(dev, "%s - stopping read!\n", __func__);
+		dev_err(dev, "stop reading\n");
 		return;
 	}
 
@@ -1336,8 +1333,7 @@ exit:
 
 	spin_unlock(&tport->tp_lock);
 	if (retval)
-		dev_err(dev, "%s - resubmit read urb failed, %d\n",
-			__func__, retval);
+		dev_err(dev, "failed to resubmit urb: %d\n", retval);
 }
 
 
@@ -1358,8 +1354,7 @@ static void ti_bulk_out_callback(struct urb *urb)
 		dev_dbg(&port->dev, "%s - urb shutting down, %d\n", __func__, status);
 		return;
 	default:
-		dev_err_console(port, "%s - nonzero urb status, %d\n",
-			__func__, status);
+		dev_err_console(port, "nonzero urb status: %d\n", status);
 	}
 
 	/* send any buffered data */
@@ -1375,8 +1370,8 @@ static void ti_recv(struct usb_serial_port *port, unsigned char *data,
 	do {
 		cnt = tty_insert_flip_string(&port->port, data, length);
 		if (cnt < length) {
-			dev_err(&port->dev, "%s - dropping data, %d bytes lost\n",
-						__func__, length - cnt);
+			dev_err(&port->dev, "dropping data: %d bytes lost\n",
+				length - cnt);
 			if (cnt == 0)
 				break;
 		}
@@ -1420,8 +1415,8 @@ static void ti_send(struct ti_port *tport)
 
 	result = usb_submit_urb(port->write_urb, GFP_ATOMIC);
 	if (result) {
-		dev_err_console(port, "%s - submit write urb failed, %d\n",
-							__func__, result);
+		dev_err_console(port, "failed to submit write urb: %d\n",
+				result);
 		tport->tp_write_urb_in_use = 0;
 		/* TODO: reschedule ti_send */
 	} else {
@@ -1474,8 +1469,8 @@ static int ti_get_lsr(struct ti_port *tport, u8 *lsr)
 				       TI_UART1_PORT + port_number, data, size);
 	if (status) {
 		dev_err(&port->dev,
-			"%s - get port status command failed, %d\n",
-							__func__, status);
+			"get port status command failed: %d\n",
+			status);
 		goto free_data;
 	}
 
@@ -1749,12 +1744,12 @@ static int ti_download_firmware(struct usb_serial *serial)
 
 check_firmware:
 	if (status) {
-		dev_err(&dev->dev, "%s - firmware not found\n", __func__);
+		dev_err(&dev->dev, "failed to request firmware: %d\n", status);
 		return -ENOENT;
 	}
 
 	if (fw_p->size > TI_FIRMWARE_BUF_SIZE) {
-		dev_err(&dev->dev, "%s - firmware too large %zu\n", __func__, fw_p->size);
+		dev_err(&dev->dev, "firmware too large: %zu\n", fw_p->size);
 		release_firmware(fw_p);
 		return -ENOENT;
 	}
-- 
2.9.0

[PATCH v2 19/22] usb: serial: ti_usb_3410_5052: Use variables for vendor and product

[Mathieu OTHACEHE]

Use variables for vendor and product in download_firmware to improve
readability.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index e59b3c1..24e1d52 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1692,7 +1692,10 @@ static int ti_download_firmware(struct usb_serial *serial)
 	struct ti_device *tdev = usb_get_serial_data(serial);
 	const struct firmware *fw_p;
 	char buf[32];
+	__le16 vendor, product;
 
+	vendor = le16_to_cpu(dev->descriptor.idVendor);
+	product = le16_to_cpu(dev->descriptor.idProduct);
 
 	if (le16_to_cpu(dev->descriptor.idVendor) == MXU1_VENDOR_ID) {
 		snprintf(buf,
@@ -1705,15 +1708,13 @@ static int ti_download_firmware(struct usb_serial *serial)
 	}
 
 	/* try ID specific firmware first, then try generic firmware */
-	sprintf(buf, "ti_usb-v%04x-p%04x.fw",
-			le16_to_cpu(dev->descriptor.idVendor),
-			le16_to_cpu(dev->descriptor.idProduct));
+	sprintf(buf, "ti_usb-v%04x-p%04x.fw", vendor, product);
 	status = request_firmware(&fw_p, buf, &dev->dev);
 
 	if (status != 0) {
 		buf[0] = '\0';
-		if (le16_to_cpu(dev->descriptor.idVendor) == MTS_VENDOR_ID) {
-			switch (le16_to_cpu(dev->descriptor.idProduct)) {
+		if (vendor == MTS_VENDOR_ID) {
+			switch (product) {
 			case MTS_CDMA_PRODUCT_ID:
 				strcpy(buf, "mts_cdma.fw");
 				break;
@@ -1733,6 +1734,7 @@ static int ti_download_firmware(struct usb_serial *serial)
 				strcpy(buf, "mts_mt9234zba.fw");
 				break;			}
 		}
+
 		if (buf[0] == '\0') {
 			if (tdev->td_is_3410)
 				strcpy(buf, "ti_3410.fw");
-- 
2.9.0

[PATCH v2 20/22] usb: serial: ti_usb_3410_5052: Set shadow msr before waking up waiters

[Mathieu OTHACEHE]

Save msr before testing the delta and waking up any waiters.
Also use port directly instead of tport->tp_port.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 24e1d52..168a969 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1547,9 +1547,12 @@ static void ti_handle_new_msr(struct usb_serial_port *port, u8 msr)
 
 	dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr);
 
+	spin_lock_irqsave(&tport->tp_lock, flags);
+	tport->tp_msr = msr & TI_MSR_MASK;
+	spin_unlock_irqrestore(&tport->tp_lock, flags);
+
 	if (msr & TI_MSR_DELTA_MASK) {
-		spin_lock_irqsave(&tport->tp_lock, flags);
-		icount = &tport->tp_port->icount;
+		icount = &port->icount;
 		if (msr & TI_MSR_DELTA_CTS)
 			icount->cts++;
 		if (msr & TI_MSR_DELTA_DSR)
@@ -1558,11 +1561,9 @@ static void ti_handle_new_msr(struct usb_serial_port *port, u8 msr)
 			icount->dcd++;
 		if (msr & TI_MSR_DELTA_RI)
 			icount->rng++;
+
 		wake_up_interruptible(&port->port.delta_msr_wait);
-		spin_unlock_irqrestore(&tport->tp_lock, flags);
 	}
-
-	tport->tp_msr = msr & TI_MSR_MASK;
 }
 
 
-- 
2.9.0

[PATCH v2 21/22] usb: serial: ti_usb_3410_5052: Add CMSPAR support

[Mathieu OTHACEHE]

Add CMSPAR support in set_termios callback.
Move TI_UART_ENABLE_PARITY_CHECKING setting in the upper
block to avoid doing it twice.
Delete useless TI_UART_ENABLE_PARITY_CHECKING unsetting.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 168a969..50324b4 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1031,19 +1031,20 @@ static void ti_set_termios(struct tty_struct *tty,
 		    break;
 	}
 
-	/* CMSPAR isn't supported by this driver */
-	tty->termios.c_cflag &= ~CMSPAR;
-
 	if (C_PARENB(tty)) {
-		if (C_PARODD(tty)) {
-			wflags |= TI_UART_ENABLE_PARITY_CHECKING;
-			config->bParity = TI_UART_ODD_PARITY;
+		wflags |= TI_UART_ENABLE_PARITY_CHECKING;
+		if (C_CMSPAR(tty)) {
+			if (C_PARODD(tty))
+				config->bParity = TI_UART_MARK_PARITY;
+			else
+				config->bParity = TI_UART_SPACE_PARITY;
 		} else {
-			wflags |= TI_UART_ENABLE_PARITY_CHECKING;
-			config->bParity = TI_UART_EVEN_PARITY;
+			if (C_PARODD(tty))
+				config->bParity = TI_UART_ODD_PARITY;
+			else
+				config->bParity = TI_UART_EVEN_PARITY;
 		}
 	} else {
-		wflags &= ~TI_UART_ENABLE_PARITY_CHECKING;
 		config->bParity = TI_UART_NO_PARITY;
 	}
 
-- 
2.9.0

[PATCH v2 22/22] usb: serial: ti_usb_3410_5052: Fix indentation problems

[Mathieu OTHACEHE]

Fix some minor indentation problems.
Also correct a multi-line comment.

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 42 +++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index 50324b4..f987988 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -315,26 +315,27 @@ static int ti_port_remove(struct usb_serial_port *port);
 static int ti_open(struct tty_struct *tty, struct usb_serial_port *port);
 static void ti_close(struct usb_serial_port *port);
 static int ti_write(struct tty_struct *tty, struct usb_serial_port *port,
-		const unsigned char *data, int count);
+		    const unsigned char *data, int count);
 static int ti_write_room(struct tty_struct *tty);
 static int ti_chars_in_buffer(struct tty_struct *tty);
 static bool ti_tx_empty(struct usb_serial_port *port);
 static void ti_throttle(struct tty_struct *tty);
 static void ti_unthrottle(struct tty_struct *tty);
 static int ti_ioctl(struct tty_struct *tty,
-		unsigned int cmd, unsigned long arg);
+		    unsigned int cmd, unsigned long arg);
 static void ti_set_termios(struct tty_struct *tty,
-		struct usb_serial_port *port, struct ktermios *old_termios);
+			   struct usb_serial_port *port,
+			   struct ktermios *old_termios);
 static int ti_tiocmget(struct tty_struct *tty);
 static int ti_tiocmset(struct tty_struct *tty,
-		unsigned int set, unsigned int clear);
+		       unsigned int set, unsigned int clear);
 static void ti_break(struct tty_struct *tty, int break_state);
 static void ti_interrupt_callback(struct urb *urb);
 static void ti_bulk_in_callback(struct urb *urb);
 static void ti_bulk_out_callback(struct urb *urb);
 
 static void ti_recv(struct usb_serial_port *port, unsigned char *data,
-		int length);
+		    int length);
 static void ti_send(struct ti_port *tport);
 static int ti_set_mcr(struct ti_port *tport, unsigned int mcr);
 static int ti_get_lsr(struct ti_port *tport, u8 *lsr);
@@ -772,8 +773,10 @@ static int ti_open(struct tty_struct *tty, struct usb_serial_port *port)
 		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 */
+	/*
+	 * reset the data toggle on the bulk endpoints to work around bug in
+	 * host controllers where things get out of sync some times
+	 */
 	usb_clear_halt(dev, port->write_urb->pipe);
 	usb_clear_halt(dev, port->read_urb->pipe);
 
@@ -868,7 +871,7 @@ static void ti_close(struct usb_serial_port *port)
 
 
 static int ti_write(struct tty_struct *tty, struct usb_serial_port *port,
-			const unsigned char *data, int count)
+		    const unsigned char *data, int count)
 {
 	struct ti_port *tport = usb_get_serial_port_data(port);
 
@@ -957,7 +960,7 @@ static void ti_unthrottle(struct tty_struct *tty)
 }
 
 static int ti_ioctl(struct tty_struct *tty,
-	unsigned int cmd, unsigned long arg)
+		    unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
 
@@ -974,7 +977,8 @@ static int ti_ioctl(struct tty_struct *tty,
 
 
 static void ti_set_termios(struct tty_struct *tty,
-		struct usb_serial_port *port, struct ktermios *old_termios)
+			   struct usb_serial_port *port,
+			   struct ktermios *old_termios)
 {
 	struct ti_port *tport = usb_get_serial_port_data(port);
 	struct ti_uart_config *config;
@@ -1137,13 +1141,13 @@ static int ti_tiocmget(struct tty_struct *tty)
 	mcr = tport->tp_shadow_mcr;
 	spin_unlock_irqrestore(&tport->tp_lock, flags);
 
-	result = ((mcr & TI_MCR_DTR) ? TIOCM_DTR : 0)
-		| ((mcr & TI_MCR_RTS) ? TIOCM_RTS : 0)
-		| ((mcr & TI_MCR_LOOP) ? TIOCM_LOOP : 0)
-		| ((msr & TI_MSR_CTS) ? TIOCM_CTS : 0)
-		| ((msr & TI_MSR_CD) ? TIOCM_CAR : 0)
-		| ((msr & TI_MSR_RI) ? TIOCM_RI : 0)
-		| ((msr & TI_MSR_DSR) ? TIOCM_DSR : 0);
+	result = ((mcr & TI_MCR_DTR)	? TIOCM_DTR	: 0) |
+		 ((mcr & TI_MCR_RTS)	? TIOCM_RTS	: 0) |
+		 ((mcr & TI_MCR_LOOP)   ? TIOCM_LOOP	: 0) |
+		 ((msr & TI_MSR_CTS)	? TIOCM_CTS	: 0) |
+		 ((msr & TI_MSR_CD)	? TIOCM_CAR	: 0) |
+		 ((msr & TI_MSR_RI)	? TIOCM_RI	: 0) |
+		 ((msr & TI_MSR_DSR)	? TIOCM_DSR	: 0);
 
 	dev_dbg(&port->dev, "%s - 0x%04X\n", __func__, result);
 
@@ -1152,7 +1156,7 @@ static int ti_tiocmget(struct tty_struct *tty)
 
 
 static int ti_tiocmset(struct tty_struct *tty,
-				unsigned int set, unsigned int clear)
+		       unsigned int set, unsigned int clear)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct ti_port *tport = usb_get_serial_port_data(port);
@@ -1364,7 +1368,7 @@ static void ti_bulk_out_callback(struct urb *urb)
 
 
 static void ti_recv(struct usb_serial_port *port, unsigned char *data,
-		int length)
+		    int length)
 {
 	int cnt;
 
-- 
2.9.0