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

[Johan Hovold <johan@kernel.org>]

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