Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753591AbbBYUpG (ORCPT ); Wed, 25 Feb 2015 15:45:06 -0500 Received: from mail-la0-f48.google.com ([209.85.215.48]:35189 "EHLO mail-la0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753567AbbBYUpB (ORCPT ); Wed, 25 Feb 2015 15:45:01 -0500 X-Google-Original-Sender: Date: Wed, 25 Feb 2015 14:52:00 +0700 From: Johan Hovold To: Peter Hung Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw, Peter Hung Subject: Re: [PATCH V7 08/11] USB: f81232: implement set_termios() Message-ID: <20150225075200.GZ12405@localhost> References: <1424772986-5542-1-git-send-email-hpeter+linux_kernel@gmail.com> <1424772986-5542-9-git-send-email-hpeter+linux_kernel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1424772986-5542-9-git-send-email-hpeter+linux_kernel@gmail.com> User-Agent: Mutt/1.5.22 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4955 Lines: 150 On Tue, Feb 24, 2015 at 06:16:23PM +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 in f81232_set_termios(). > > This patch also implement DTR/RTS control when baudrate B0. > We drop DTR/RTS when B0, otherwise enable it. > > Signed-off-by: Peter Hung > --- > drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 102 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c > index f5c9060..0c96b9a 100644 > --- a/drivers/usb/serial/f81232.c > +++ b/drivers/usb/serial/f81232.c > @@ -31,14 +31,19 @@ static const struct usb_device_id id_table[] = { > }; > MODULE_DEVICE_TABLE(usb, id_table); > > +/* Maximum baudrate for F81232 */ > +#define F81232_MAX_BAUDRATE 115200 > + > /* 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_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS) > #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS) > > @@ -64,6 +69,14 @@ struct f81232_private { > struct usb_serial_port *port; > }; > > +static int calc_baud_divisor(u32 baudrate) > +{ > + if (!baudrate) > + return 0; Check no longer needed since you added the check at the call site. Just add a comment. In fact it look like you can get rid of this function now. > + else > + return DIV_ROUND_CLOSEST(F81232_MAX_BAUDRATE, baudrate); > +} > + > static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data) > { > int status; > @@ -369,6 +382,52 @@ 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, lcr; > + int status = 0; > + > + if (!baudrate) > + return; > + > + divisor = calc_baud_divisor(baudrate); > + > + status = f81232_get_register(port, LINE_CONTROL_REGISTER, > + &lcr); /* get LCR */ > + if (status) { > + dev_err(&port->dev, "%s failed to get LCR: %d\n", > + __func__, status); > + } > + > + status = f81232_set_register(port, LINE_CONTROL_REGISTER, > + lcr | UART_LCR_DLAB); /* Enable DLAB */ > + if (status) { > + dev_err(&port->dev, "%s failed to set DLAB: %d\n", > + __func__, status); > + } > + > + status = f81232_set_register(port, RECEIVE_BUFFER_REGISTER, > + divisor & 0x00ff); /* low */ > + if (status) { > + dev_err(&port->dev, "%s failed to set baudrate MSB: %d\n", > + __func__, status); > + } > + > + status = f81232_set_register(port, INTERRUPT_ENABLE_REGISTER, > + (divisor & 0xff00) >> 8); /* high */ > + if (status) { > + dev_err(&port->dev, "%s failed to set baudrate LSB: %d\n", > + __func__, status); > + } > + > + status = f81232_set_register(port, LINE_CONTROL_REGISTER, > + lcr & ~UART_LCR_DLAB); > + if (status) { > + dev_err(&port->dev, "%s failed to set DLAB: %d\n", > + __func__, status); > + } > +} Again, what about the error handling? No need to continue setting the other registers should the first one fail, right? And you always want to restore LCR to its previous value. > + > static int f81232_port_enable(struct usb_serial_port *port, int enable) > { > u8 data = 0; > @@ -399,15 +458,54 @@ static int f81232_port_enable(struct usb_serial_port *port, int enable) > 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; > > - /* Do the real work here... */ > - if (old_termios) > - tty_termios_copy_hw(&tty->termios, old_termios); > + if (C_BAUD(tty) == B0) > + f81232_set_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS); > + else if (old_termios && (old_termios->c_cflag & CBAUD) == B0) > + f81232_set_mctrl(port, TIOCM_DTR | TIOCM_RTS, 0); > + > + f81232_set_baudrate(port, tty_get_baud_rate(tty)); Check for B0 here instead of in the helper. And what about unsupported baudrates (e.g. > 115200 baud)? You should return the baudrate actually used in that case using tty_termios_encode_baud_rate. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/