Re: [PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

[Johan Hovold <johan@kernel.org>]

On Mon, Feb 16, 2015 at 03:57:54PM +0800, Peter Hung wrote:
> The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
> , We will do read MSR with interrupt_work worker to do f81232_read_msr() func.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 109 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 100 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..ec4609d 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,6 +23,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/usb.h>
>  #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
>  
>  static const struct usb_device_id id_table[] = {
>  	{ USB_DEVICE(0x1934, 0x0706) },
> @@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST 0xA0
> +#define F81232_GET_REGISTER 0xc0

Indent these define values using a tab as well.

> +
> +#define SERIAL_BASE_ADDRESS	   0x0120
> +#define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
> +
>  #define CONTROL_DTR			0x01
>  #define CONTROL_RTS			0x02
>  
> @@ -48,19 +56,92 @@ struct f81232_private {
>  	spinlock_t lock;
>  	u8 line_control;
>  	u8 modem_status;
> +

Newline not needed.

> +	struct work_struct interrupt_work;
> +	struct usb_serial_port *port;
>  };
>  
> -static void f81232_update_line_status(struct usb_serial_port *port,
> +static int f81232_get_register(struct usb_serial_port *port,
> +							  u16 reg, u8 *data)

No need to break this line any more.

> +{
> +	int status;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	status = usb_control_msg(dev,
> +			 usb_rcvctrlpipe(dev, 0),
> +			 F81232_REGISTER_REQUEST,
> +			 F81232_GET_REGISTER,
> +			 reg,
> +			 0,
> +			 data,
> +			 sizeof(*data),
> +			 USB_CTRL_GET_TIMEOUT);

There's some odd indentation above and in other patches as well (in this
case two tabs and one space). Please either just tabs or, if you prefer,
add additional spaces to align all the arguments with the first.

> +	if (status < 0)

Zero is actually also an error here (returned length was 0). You may
want to let this function return -EIO in that case (and perhaps also
return 0 on success) or you need to fix this at every call site.

> +		dev_err(&port->dev, "%s status: %d\n", __func__, status);

Please spell out that there was an error, for example,

	"%s failed: %d\n"

> +
> +	return status;
> +}

Could you add the get/set register helpers in a separate preparatory patch?

> +
> +static void f81232_read_msr(struct usb_serial_port *port)
> +{
> +	int status;
> +	unsigned long flags;
> +	u8 current_msr;
> +	struct tty_struct *tty;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> +	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
> +			&current_msr);

You cannot use a variable on the stack for usb_control_msg as the buffer
must be DMA-able (on all platforms). Use kzalloc.

> +	if (status < 0) {
> +		/* Retain the error even reported in f81232_get_register()
> +		     to make debug easily :D */

No need for this comment.

> +		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
> +		return;
> +	}
> +
> +	if (!(current_msr & UART_MSR_ANY_DELTA))
> +		return;
> +
> +	tty = tty_port_tty_get(&port->port);
> +	if (tty) {
> +		if (current_msr & UART_MSR_DDCD) {
> +			usb_serial_handle_dcd_change(port, tty,
> +				current_msr & UART_MSR_DCD);

Please indent continuation lines at least two tabs further (i.e add one
more tab here).

> +		}
> +
> +		tty_kref_put(tty);
> +	}

You should rewrite this so you only get the tty reference if you're
actually gonna use it.

> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	priv->modem_status = current_msr;
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	wake_up_interruptible(&port->port.delta_msr_wait);
> +}
> +
> +static void f81232_update_modem_status(struct usb_serial_port *port,

This one should still be called f81232_update_line_status.

>  				      unsigned char *data,
>  				      unsigned int actual_length)

Just call it length or size, you can use size_t.

>  {
> -	/*
> -	 * FIXME: Update port->icount, and call
> -	 *
> -	 *		wake_up_interruptible(&port->port.delta_msr_wait);
> -	 *
> -	 *	  on MSR changes.
> -	 */
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> +	if (!actual_length)
> +		return;
> +
> +	switch (data[0] & 0x07) {
> +	case 0x00: /* msr change */
> +		dev_dbg(&port->dev, "IIR: MSR Change: %x\n", data[0]);

%02x

> +		schedule_work(&priv->interrupt_work);
> +		break;
> +	case 0x02: /* tx-empty */
> +		break;
> +	case 0x04: /* rx data available */
> +		break;
> +	case 0x06: /* lsr change */
> +		/* we can forget it. the LSR will read from bulk-in */
> +		dev_dbg(&port->dev, "IIR: LSR Change: %x\n", data[0]);

%02x

> +		break;
> +	}
>  }

Johan