lkml.org 
[lkml]   [2007]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] SC26XX: New serial driver for SC2681 uarts
    On Sun,  2 Dec 2007 20:43:46 +0100 (CET)
    Thomas Bogendoerfer <tsbogend@alpha.franken.de> 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@alpha.franken.de)
    > + */
    > +
    > +#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.




    \
     
     \ /
      Last update: 2007-12-04 00:57    [W:0.037 / U:3.476 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site