Re: [PATCH] SC26XX: New serial driver for SC2681 uarts

[Andrew Morton <akpm@linux-foundation.org>]

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.