[PATCH RESEND] USB: serial: cp210x: Add tx_empty()

[PATCH RESEND] USB: serial: cp210x: Add tx_empty()

[Konstantin Shkolnyy]

Without this function, when the port is closed the data in the chip's
transmit FIFO are lost. If the actual byte count is reported the close
can be delayed until all data are sent.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
 drivers/usb/serial/cp210x.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index e91b654..756e432 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios *);
 static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios*);
+static bool cp210x_tx_empty(struct usb_serial_port *port);
 static int cp210x_tiocmget(struct tty_struct *);
 static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
@@ -215,6 +216,7 @@ static struct usb_serial_driver cp210x_device = {
 	.close			= cp210x_close,
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
+	.tx_empty		= cp210x_tx_empty,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
 	.port_probe		= cp210x_port_probe,
@@ -301,6 +303,18 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CONTROL_WRITE_DTR	0x0100
 #define CONTROL_WRITE_RTS	0x0200
 
+/* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
+#define CP210X_COMM_STATUS_SIZE 0x13
+struct cp210x_comm_status {
+	__le32   ulErrors;
+	__le32   ulHoldReasons;
+	__le32   ulAmountInInQueue;
+	__le32   ulAmountInOutQueue;
+	u8       bEofReceived;
+	u8       bWaitForImmediate;
+	u8       bReserved;
+};
+
 /*
  * CP210X_PURGE - 16 bits passed in wValue of USB request.
  * SiLabs app note AN571 gives a strange description of the 4 bits:
@@ -551,6 +565,52 @@ static void cp210x_close(struct usb_serial_port *port)
 }
 
 /*
+ * Read how many bytes are waiting in the TX queue.
+ */
+static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port,
+	u32 *count)
+{
+	struct usb_serial *serial = port->serial;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	struct cp210x_comm_status *sts;
+	int result;
+
+	sts = kmalloc(CP210X_COMM_STATUS_SIZE, GFP_KERNEL);
+	if (!sts)
+		return -ENOMEM;
+
+	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+		CP210X_GET_COMM_STATUS, REQTYPE_INTERFACE_TO_HOST, 0x0000,
+		port_priv->bInterfaceNumber, sts, CP210X_COMM_STATUS_SIZE,
+		USB_CTRL_GET_TIMEOUT);
+	if (result == CP210X_COMM_STATUS_SIZE) {
+		*count = le32_to_cpu(sts->ulAmountInOutQueue);
+		result = 0;
+	} else {
+		dev_dbg(&port->dev, "%s error: size=%d result=%d\n",
+			__func__, CP210X_COMM_STATUS_SIZE, result);
+		if (result >= 0)
+			result = -EPROTO;
+	}
+
+	kfree(sts);
+
+	return result;
+}
+
+static bool cp210x_tx_empty(struct usb_serial_port *port)
+{
+	int err;
+	u32 count;
+
+	err = cp210x_get_tx_queue_byte_count(port, &count);
+	if (!err && count)
+		return false;
+
+	return true;
+}
+
+/*
  * cp210x_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
  * from the device, corrects any unsupported values, and configures the
-- 
1.8.4.5

Re: [PATCH RESEND] USB: serial: cp210x: Add tx_empty()

[Johan Hovold]

On Wed, Nov 11, 2015 at 03:47:21PM -0600, Konstantin Shkolnyy wrote:

Please make the commit message self-contained even if it means repeating
what callback you're implementing here.

> Without this function, when the port is closed the data in the chip's
> transmit FIFO are lost. If the actual byte count is reported the close
> can be delayed until all data are sent.

You could mention that the tx_empty callback is used to implement
generic wait-until-sent support.

> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index e91b654..756e432 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct usb_serial_port *,
>  							struct ktermios *);
>  static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *,
>  							struct ktermios*);
> +static bool cp210x_tx_empty(struct usb_serial_port *port);
>  static int cp210x_tiocmget(struct tty_struct *);
>  static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
>  static int cp210x_tiocmset_port(struct usb_serial_port *port,
> @@ -215,6 +216,7 @@ static struct usb_serial_driver cp210x_device = {
>  	.close			= cp210x_close,
>  	.break_ctl		= cp210x_break_ctl,
>  	.set_termios		= cp210x_set_termios,
> +	.tx_empty		= cp210x_tx_empty,
>  	.tiocmget		= cp210x_tiocmget,
>  	.tiocmset		= cp210x_tiocmset,
>  	.port_probe		= cp210x_port_probe,
> @@ -301,6 +303,18 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CONTROL_WRITE_DTR	0x0100
>  #define CONTROL_WRITE_RTS	0x0200
>  
> +/* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
> +#define CP210X_COMM_STATUS_SIZE 0x13
> +struct cp210x_comm_status {
> +	__le32   ulErrors;
> +	__le32   ulHoldReasons;
> +	__le32   ulAmountInInQueue;
> +	__le32   ulAmountInOutQueue;
> +	u8       bEofReceived;
> +	u8       bWaitForImmediate;
> +	u8       bReserved;
> +};

Add a __packed attribute here so that you can use sizeof and drop the
size-define.

> +
>  /*
>   * CP210X_PURGE - 16 bits passed in wValue of USB request.
>   * SiLabs app note AN571 gives a strange description of the 4 bits:
> @@ -551,6 +565,52 @@ static void cp210x_close(struct usb_serial_port *port)
>  }
>  
>  /*
> + * Read how many bytes are waiting in the TX queue.
> + */
> +static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port,
> +	u32 *count)

Indent continuation lines at least two tabs (also below).

> +{
> +	struct usb_serial *serial = port->serial;
> +	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> +	struct cp210x_comm_status *sts;
> +	int result;
> +
> +	sts = kmalloc(CP210X_COMM_STATUS_SIZE, GFP_KERNEL);
> +	if (!sts)
> +		return -ENOMEM;
> +
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +		CP210X_GET_COMM_STATUS, REQTYPE_INTERFACE_TO_HOST, 0x0000,

Just use 0 here.

> +		port_priv->bInterfaceNumber, sts, CP210X_COMM_STATUS_SIZE,

And sizeof(*sts) here (and below).

> +		USB_CTRL_GET_TIMEOUT);
> +	if (result == CP210X_COMM_STATUS_SIZE) {
> +		*count = le32_to_cpu(sts->ulAmountInOutQueue);
> +		result = 0;
> +	} else {
> +		dev_dbg(&port->dev, "%s error: size=%d result=%d\n",
> +			__func__, CP210X_COMM_STATUS_SIZE, result);

This should be a dev_err, skip the __func__ and spell out the error
(e.g. "failed to get comm status: %d\n"). No need to report the constant
size.

> +		if (result >= 0)
> +			result = -EPROTO;
> +	}
> +
> +	kfree(sts);
> +
> +	return result;
> +}

Thanks,
Johan