On Sun, 2 Dec 2007 20:43:46 +0100 (CET)
Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx> wrote:
> New serial driver for SC2681/SC2691 uarts. Older SNI RM400 machines are
> using these chips for onboard serial ports.
>
Little things...
> --- /dev/null
> +++ b/drivers/serial/sc26xx.c
> @@ -0,0 +1,757 @@
> +/*
> + * SC268xx.c: Serial driver for Philiphs SC2681/SC2692 devices.
> + *
> + * Copyright (C) 2006,2007 Thomas Bogend__rfer (tsbogend@xxxxxxxxxxxxxxxx)
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/major.h>
> +#include <linux/circ_buf.h>
> +#include <linux/serial.h>
> +#include <linux/sysrq.h>
> +#include <linux/console.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/irq.h>
> +
> +#if defined(CONFIG_MAGIC_SYSRQ)
> +#define SUPPORT_SYSRQ
> +#endif
> +
> +#include <linux/serial_core.h>
> +
> +#define SC26XX_MAJOR 204
> +#define SC26XX_MINOR_START 205
> +#define SC26XX_NR 2
> +
> +struct uart_sc26xx_port {
> + struct uart_port port[2];
> + u8 dsr_mask[2];
> + u8 cts_mask[2];
> + u8 dcd_mask[2];
> + u8 ri_mask[2];
> + u8 dtr_mask[2];
> + u8 rts_mask[2];
> + u8 imr;
> +};
> +
> +/* register common to both ports */
> +#define RD_ISR 0x14
> +#define RD_IPR 0x34
> +
> +#define WR_ACR 0x10
> +#define WR_IMR 0x14
> +#define WR_OPCR 0x34
> +#define WR_OPR_SET 0x38
> +#define WR_OPR_CLR 0x3C
> +
> +/* access common register */
> +#define READ_SC(p, r) readb ((p)->membase + RD_##r)
> +#define WRITE_SC(p, r, v) writeb ((v), (p)->membase + WR_##r)
No space before the (. checkpatch misses this.
> +/* register per port */
> +#define RD_PORT_MRx 0x00
> +#define RD_PORT_SR 0x04
> +#define RD_PORT_RHR 0x0c
> +
> +#define WR_PORT_MRx 0x00
> +#define WR_PORT_CSR 0x04
> +#define WR_PORT_CR 0x08
> +#define WR_PORT_THR 0x0c
> +
> +/* access port register */
> +#define READ_SC_PORT(p, r) \
> + readb((p)->membase + (p)->line * 0x20 + RD_PORT_##r)
> +#define WRITE_SC_PORT(p, r, v) \
> + writeb((v), (p)->membase + (p)->line * 0x20 + WR_PORT_##r)
eww, ugly. Why not have a nice C function which is passed RD_PORT_SR,
RD_PORT_RHR, etc?
That has the (minor) advantage that it won't malfunction if someone does
WRITE_SC_PORT(foo++, r, v);
(the macro references an arg more than once)
> +/* SR bits */
> +#define SR_BREAK (1 << 7)
> +#define SR_FRAME (1 << 6)
> +#define SR_PARITY (1 << 5)
> +#define SR_OVERRUN (1 << 4)
> +#define SR_TXRDY (1 << 2)
> +#define SR_RXRDY (1 << 0)
> +
> +#define CR_RES_MR (1 << 4)
> +#define CR_RES_RX (2 << 4)
> +#define CR_RES_TX (3 << 4)
> +#define CR_STRT_BRK (6 << 4)
> +#define CR_STOP_BRK (7 << 4)
> +#define CR_DIS_TX (1 << 3)
> +#define CR_ENA_TX (1 << 2)
> +#define CR_DIS_RX (1 << 1)
> +#define CR_ENA_RX (1 << 0)
> +
> +/* ISR bits */
> +#define ISR_RXRDYB (1 << 5)
> +#define ISR_TXRDYB (1 << 4)
> +#define ISR_RXRDYA (1 << 1)
> +#define ISR_TXRDYA (1 << 0)
> +
> +/* IMR bits */
> +#define IMR_RXRDY (1 << 1)
> +#define IMR_TXRDY (1 << 0)
> +
> +static void sc26xx_enable_irq(struct uart_port *port, int mask)
> +{
> + struct uart_sc26xx_port *up;
> + int line = port->line;
> +
> + port -= line;
> + up = (struct uart_sc26xx_port *)port;
Yeah, lots of serial drivers do this and it's old-fashioned. It would be
clearer to use container_of() rather than the open-coded typecast. That
way, the uart_port doesn't need to be the first member of uart_sc26xx_port,
too.
> + up->imr |= mask << (line * 4);
> + WRITE_SC(port, IMR, up->imr);
> +}
>
> ...
>
> +
> +/* port->lock is not held. */
> +static unsigned int sc26xx_tx_empty(struct uart_port *port)
> +{
> + unsigned long flags;
> + unsigned int ret;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + ret = (READ_SC_PORT(port, SR) & SR_TXRDY) ? TIOCSER_TEMT : 0;
> + spin_unlock_irqrestore(&port->lock, flags);
> + return ret;
> +}
I suspect the locking here doesn't actually do anything?
> +/* port->lock is not held. */
> +static void sc26xx_set_termios(struct uart_port *port, struct ktermios *termios,
> + struct ktermios *old)
hm, termios stuff. I'll cc Alan on the commit...
> +static struct uart_port *sc26xx_port;
> +
> +#ifdef CONFIG_SERIAL_SC26XX_CONSOLE
> +static inline void sc26xx_console_putchar(struct uart_port *port, char c)
> +{
> + unsigned long flags;
> + int limit = 1000000;
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + while (limit-- > 0) {
> + if (READ_SC_PORT(port, SR) & SR_TXRDY) {
> + WRITE_SC_PORT(port, THR, c);
> + break;
> + }
> + udelay(2);
> + }
> +
> + spin_unlock_irqrestore(&port->lock, flags);
> +}
This is far too large to be inlined.
> +static void sc26xx_console_write(struct console *con, const char *s, unsigned n)
> +{
> + struct uart_port *port = sc26xx_port;
> + int i;
> +
> + for (i = 0; i < n; i++) {
> + if (*s == '\n')
> + sc26xx_console_putchar(port, '\r');
> + sc26xx_console_putchar(port, *s++);
> + }
> +}
> +
> +static int __init sc26xx_console_setup(struct console *con, char *options)
> +{
> + struct uart_port *port = sc26xx_port;
> + int baud = 9600;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (port->type != PORT_SC26XX)
> + return -1;
> +
> + printk(KERN_INFO "Console: ttySC%d (SC26XX)\n", con->index);
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(port, con, baud, parity, bits, flow);
> +}
> +
> +static struct uart_driver sc26xx_reg;
> +static struct console sc26xx_console = {
> + .name = "ttySC",
> + .write = sc26xx_console_write,
> + .device = uart_console_device,
> + .setup = sc26xx_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data = &sc26xx_reg,
> +};
> +#define SC26XX_CONSOLE &sc26xx_console
> +#else
> +#define SC26XX_CONSOLE NULL
> +#endif
> +
> +static struct uart_driver sc26xx_reg = {
> + .owner = THIS_MODULE,
> + .driver_name = "SC26xx",
> + .dev_name = "ttySC",
> + .major = SC26XX_MAJOR,
> + .minor = SC26XX_MINOR_START,
> + .nr = SC26XX_NR,
> + .cons = SC26XX_CONSOLE,
> +};
> +
> +static inline u8 sc26xx_flags2mask(unsigned int flags, unsigned int bitpos)
> +{
> + unsigned int bit = (flags >> bitpos) & 15;
> +
> + return bit ? (1 << (bit - 1)) : 0;
> +}
This might be too big as well. It's less of an issue for __devinit text
though.
