Re: [PATCH V6 04/10] USB: f81232: implement set_termios

[Johan Hovold <johan@kernel.org>]

On Mon, Feb 16, 2015 at 03:57:56PM +0800, Peter Hung wrote:
> The original driver had do not any h/w change in driver.
> This patch implements with configure H/W for
> baud/parity/word length/stop bits functional.
> 
> Some init step extract to f81232_port_init(), called once with open().
> And refine baudrate setting to f81232_set_baudrate()
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 145 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 138 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 9ea498a..06d1eb0 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -31,11 +31,19 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* Maximum baudrate for F81232 */
> +#define F81232_MAX_BAUDRATE 115200L

No need for L.

> +
>  /* USB Control EP parameter */
>  #define F81232_REGISTER_REQUEST 0xA0
>  #define F81232_GET_REGISTER 0xc0
> +#define F81232_SET_REGISTER 0x40
>  
>  #define SERIAL_BASE_ADDRESS	   0x0120
> +#define RECEIVE_BUFFER_REGISTER    (0x00 + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_ENABLE_REGISTER  (0x01 + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER      (0x02 + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER      (0x03 + SERIAL_BASE_ADDRESS)
>  #define MODEM_STATUS_REGISTER      (0x06 + SERIAL_BASE_ADDRESS)
>  
>  #define CONTROL_DTR			0x01
> @@ -61,6 +69,14 @@ struct f81232_private {
>  	struct usb_serial_port *port;
>  };
>  
> +static int calc_baud_divisor(u32 baudrate)
> +{
> +	if (!baudrate)
> +		return 0;
> +	else
> +		return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate);
> +}

You should handle B0 at the call site (and in that case keep the current
baudrate).

> +
>  static int f81232_get_register(struct usb_serial_port *port,
>  							  u16 reg, u8 *data)
>  {
> @@ -82,6 +98,27 @@ static int f81232_get_register(struct usb_serial_port *port,
>  	return status;
>  }
>  
> +static int f81232_set_register(struct usb_serial_port *port,
> +							  u16 reg, u8 data)

No need to break line.

> +{
> +	int status;
> +	struct usb_device *dev = port->serial->dev;
> +
> +	status = usb_control_msg(dev,
> +			usb_sndctrlpipe(dev, 0),
> +			F81232_REGISTER_REQUEST,
> +			F81232_SET_REGISTER,
> +			reg,
> +			0,
> +			&data,
> +			sizeof(data),
> +			USB_CTRL_SET_TIMEOUT);

As for f81232_get_register, you must use a DMA-able buffer for the
data buffer. Either allocate it in this function or at the call site.

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

Same comments apply as for f81232_set_register on the previous patch.

> +
>  static void f81232_read_msr(struct usb_serial_port *port)
>  {
>  	int status;
> @@ -247,18 +284,106 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
>  	 */
>  }
>  
> +static void f81232_set_baudrate(struct usb_serial_port *port, int baudrate)
> +{
> +	u8 divisor;
> +	int status = 0;
> +
> +	divisor = calc_baud_divisor(baudrate);
> +
> +	status = f81232_set_register(port, LINE_CONTROL_REGISTER,
> +			 UART_LCR_DLAB); /* DLAB */
> +	if (status < 0) {

As with f81232_get_register 0 is also an error.

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

Don't use __LINE__ in your error messages. The dynamic debugging
infrastructure can add that if ever needed.

Spell out the errors instead (e.g. "failed to set DLAB").

Shouldn't you be ORing DLAB to the current state of LCR by the way?

And make sure to bail out on errors here.

> +	}
> +
> +	status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER,
> +			 divisor & 0x00ff); /* low */
> +	if (status < 0) {
> +		dev_err(&port->dev, "%s status: %d line:%d\n",
> +			__func__, status, __LINE__);
> +	}
> +
> +	status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER,
> +			 (divisor & 0xff00) >> 8); /* high */
> +	if (status < 0) {
> +		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
> +			status, __LINE__);
> +	}
> +
> +	status = f81232_set_register(port, LINE_CONTROL_REGISTER, 0x00);

Only clear DLAB?

> +	if (status < 0) {
> +		dev_err(&port->dev, "%s status: %d line:%d\n", __func__,
> +			status, __LINE__);
> +	}
> +}
> +
> +static int f81232_port_init(struct usb_serial_port *port)
> +{
> +	u8 data;
> +	int status = 0;
> +
> +	/* fifo on, trigger8, clear TX/RX*/
> +	data = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR
> +			| UART_FCR_CLEAR_XMIT;
> +
> +	status |= f81232_set_register(port, FIFO_CONTROL_REGISTER, data);

Just assign return value and don't initialise above.

And just bail out on errors directly here.

> +
> +	/* MSR Interrupt only, LSR will read from Bulk-in odd byte */
> +	data = UART_IER_MSI;
> +
> +	/* IER */
> +	status |= f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, data);

Just use = here as well.

> +	if (status < 0) {
> +		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
>  static void f81232_set_termios(struct tty_struct *tty,
>  		struct usb_serial_port *port, struct ktermios *old_termios)
>  {
> -	/* FIXME - Stubbed out for now */
> +	u8 new_lcr = 0;
> +	int status = 0;
>  
> -	/* Don't change anything if nothing has changed */
> -	if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> -		return;
> +	f81232_set_baudrate(port, tty_get_baud_rate(tty));

So here you should check for B0 and in that case simply not update the
baudrate.

B0 is, as I believe I already mentioned, used to hang up any modem by
dropping DTR.

> +
> +	if (C_PARENB(tty)) {
> +		new_lcr |= UART_LCR_PARITY;
> +
> +		if (!C_PARODD(tty))
> +			new_lcr |= UART_LCR_EPAR;
> +
> +		if (C_CMSPAR(tty))
> +			new_lcr |= UART_LCR_SPAR;
> +	}
> +
> +	if (C_CSTOPB(tty))
> +		new_lcr |= UART_LCR_STOP;
> +
> +	switch (C_CSIZE(tty)) {
> +	case CS5:
> +		new_lcr |= UART_LCR_WLEN5;
> +		break;
> +	case CS6:
> +		new_lcr |= UART_LCR_WLEN6;
> +		break;
> +	case CS7:
> +		new_lcr |= UART_LCR_WLEN7;
> +		b<reak;
> +	default:z
> +	case CS8:
> +		new_lcr |= UART_LCR_WLEN8;
> +		break;
> +	}
> +
> +	status = f81232_set_register(port, LINE_CONTROL_REGISTER, new_lcr);
> +	if (status < 0)
> +		dev_err(&port->dev, "%s set error: %d\n", __func__, status);
>  
> -	/* Do the real work here... */
> -	if (old_termios)
> -		tty_termios_copy_hw(&tty->termios, old_termios);
>  }
>  
>  static int f81232_tiocmget(struct tty_struct *tty)
> @@ -278,6 +403,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>  {
>  	int result;
>  
> +	result = f81232_port_init(port);
> +	if (result) {
> +		dev_err(&port->dev, "%s - init fail: %d\n", __func__, result);
> +		return result;
> +	}

Don't you wan't to reverse f81232_port_init at close (e.g. disable
interrupts)?

> +
>  	/* Setup termios */
>  	if (tty)
>  		f81232_set_termios(tty, port, NULL);

Johan