On Wed, 30 Mar 2011 09:27:50 +0200
John Crispin <blogic@xxxxxxxxxxx> wrote:
> This patch adds the driver for the 2 serial ports found inside the Lantiq SoC family
Several comments, and a NAK to the current version.
Looks like it just needs a bit of bringing up to current tty/serial
interface expectations.
> +static void
> +lqasc_start_tx(struct uart_port *port)
> +{
> + unsigned long flags;
> + local_irq_save(flags);
> + lqasc_tx_chars(port);
> + local_irq_restore(flags);
> + return;
> +}
Shouldn't this be using locks ?
(note if the platorm is uniprocessor only then spin_lock_irqsave() turns
into local_irq_save())
> +static void
> +lqasc_rx_chars(struct uart_port *port)
> +{
> + struct tty_struct *tty = port->state->port.tty;
tty ports are refcounted. Look how drivers use tty_port_tty_get() and
tty_kref_put(). Note that a tty can be NULL at this point and you need to
handle it
> +static irqreturn_t
> +lqasc_err_int(int irq, void *_port)
> +{
> + struct uart_port *port = (struct uart_port *)_port;
> + /* clear any pending interrupts */
> + ltq_w32_mask(0, ASCWHBSTATE_CLRPE | ASCWHBSTATE_CLRFE |
> + ASCWHBSTATE_CLRROE, port->membase + LTQ_ASC_WHBSTATE);
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t
> +lqasc_rx_int(int irq, void *_port)
> +{
> + struct uart_port *port = (struct uart_port *)_port;
> + ltq_w32(ASC_IRNCR_RIR, port->membase + LTQ_ASC_IRNCR);
> + lqasc_rx_chars(port);
> + return IRQ_HANDLED;
> +}
The interrupt handlers look like they need to hold the port lock as well ?
> +static void
> +lqasc_set_termios(struct uart_port *port,
> + struct ktermios *new, struct ktermios *old)
> +{
> + unsigned int cflag;
> + unsigned int iflag;
> + unsigned int divisor;
> + unsigned int baud;
> + unsigned int con = 0;
> + unsigned long flags;
> +
> + cflag = new->c_cflag;
> + iflag = new->c_iflag;
> +
> + switch (cflag & CSIZE) {
> + case CS7:
> + con = ASCCON_M_7ASYNC;
> + break;
> +
> + case CS5:
> + case CS6:
> + default:
> + con = ASCCON_M_8ASYNC;
If you can't support a request (eg CS5/CS6 or CMSPAR etc) you need to
clear them from the requested settings - ie
default:
new->c_cflag &= ~ CSIZE;
new->c_cflag |= CS8;
con = ....
> + break;
> + }
> +
> + if (cflag & CSTOPB)
> + con |= ASCCON_STP;
> +
> + if (cflag & PARENB) {
> + if (!(cflag & PARODD))
> + con &= ~ASCCON_ODD;
> + else
> + con |= ASCCON_ODD;
> + }
CMSPAR ?
> + local_irq_save(flags);
Again I'd expect locks not this.
> +
> + /* set up CON */
> + ltq_w32_mask(0, con, port->membase + LTQ_ASC_CON);
> +
> + /* Set baud rate - take a divider of 2 into account */
> + baud = uart_get_baud_rate(port, new, old, 0, port->uartclk / 16);
> + divisor = uart_get_divisor(port, baud);
> + divisor = divisor / 2 - 1;
Actual baud also wants writing back if not set to B0 (see 8250.c)
> +static struct console lqasc_console = {
> + .name = "ttyS",
ttyS is reserved for the 8250 ports
> + .write = lqasc_console_write,
> + .device = uart_console_device,
> + .setup = lqasc_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data = &lqasc_reg,
> +};
> +
> +static int __init
> +lqasc_console_init(void)
> +{
> + register_console(&lqasc_console);
> + return 0;
> +}
> +console_initcall(lqasc_console_init);
> +
> +static struct uart_driver lqasc_reg = {
> + .owner = THIS_MODULE,
> + .driver_name = DRVNAME,
> + .dev_name = "ttyS",
> + .major = TTY_MAJOR,
> + .minor = 64,
This is existing owned and reserved space - do a dynamic allocation and
use a new name.
Alan
