max3100 driver
PATCH 1/1 max3100 driver
This patch adds support for the MAX3100 SPI UART.
Generated on 20071205 against v2.6.23
Signed-off-by Christian Pellegrin <[email protected]>
--- drivers/serial/Kconfig | 7 + drivers/serial/Makefile | 1 + drivers/serial/max3100.c | 960 ++++++++++++++++++++++++++++++++++++++++ include/linux/serial_max3100.h | 25 + 4 files changed, 993 insertions(+), 0 deletions(-)
drivers/serial/Kconfig +7 -0
@@ -461,6 +461,13 @@ config SERIAL_S3C2410_CONSOLE your boot loader about how to pass options to the kernel at boot time.) +config SERIAL_MAX3100 + tristate "MAX3100 support" + depends on SPI + help + MAX3100 chip support + + config SERIAL_DZ bool "DECstation DZ serial driver" depends on MACH_DECSTATION && 32BIT
drivers/serial/Makefile +1 -0
@@ -29,6 +29,7 @@ obj-$(CONFIG_SERIAL_PNX8XXX) += pnx8xxx_uart.o obj-$(CONFIG_SERIAL_SA1100) += sa1100.o obj-$(CONFIG_SERIAL_BFIN) += bfin_5xx.o obj-$(CONFIG_SERIAL_S3C2410) += s3c2410.o +obj-$(CONFIG_SERIAL_MAX3100) += max3100.o obj-$(CONFIG_SERIAL_SUNCORE) += suncore.o obj-$(CONFIG_SERIAL_SUNHV) += sunhv.o obj-$(CONFIG_SERIAL_SUNZILOG) += sunzilog.o
drivers/serial/max3100.c new file +960 -0
960 lines not on this page · raw message
include/linux/serial_max3100.h new file +25 -0
@@ -0,0 +1,25 @@ + +/* +* +* Copyright (C) 2007 Christian Pellegrin +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +*/ + + +#ifndef _LINUX_SERIAL_MAX3100_H +#define _LINUX_SERIAL_MAX3100_H 1 + +struct plat_max3100 { +int loopback; +/* force MAX3100 in loopback */ +int crystal; +/* 0 for 3.6864 Mhz, 1 for 1.8432 */ +int only_edge_irq; +/* for archs like PXA with only edge irqs */ +}; + +#endif
--
1.4.4.4
Quoted 2 lines · On Wed, 5 Dec 2007 11:26:45 +0100 [email protected] wrote:
This patch adds support for the MAX3100 SPI UART.
Doesn't compile on x86_64:
drivers/serial/max3100.c: In function 'startup':drivers/serial/max3100.c:706: error: 'IRQT_FALLING' undeclared (first use in this function) drivers/serial/max3100.c:706: error: (Each undeclared identifier is reported only once drivers/serial/max3100.c:706: error: for each function it appears in.) drivers/serial/max3100.c:708: error: 'IRQT_LOW' undeclared (first use in this function)
Quoted 8 lines
include/linux/serial_max3100.h | 25 + 4 files changed, 993 insertions(+), 0 deletions(-)drivers/serial/Kconfig +0
-0
drivers/serial/Kconfig config SERIAL_S3C2410_CONSOLE · line 469
Quoted 9 lines
drivers/serial/Kconfig +6
-0@@ -461,6 +461,13 @@ config SERIAL_S3C2410_CONSOLE your boot loader about how to pass options to the kernel at boot time.) +config SERIAL_MAX3100 + tristate "MAX3100 support" + depends on SPI + help + MAX3100 chip support +
Putting a depends on ARM in here might be sufficient.
Quoted 4 lines
drivers/serial/Kconfig +3
-0+++ b/drivers/serial/max3100.c@@ -0,0 +1,960 @@ + +/*
drivers/serial/Kconfig line 12
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ * + * Copyright (C) 2007 Christian Pellegrin <[email protected]> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * version 0.1 31/5/2006 first public release + * version 0.2 11/11/2007 ported to kernel 2.6.23
Normally we skip info like this - it's all in the git tree, in better form.
drivers/serial/Kconfig line 13
Quoted 1 line
drivers/serial/Kconfig +1
-0+static struct tty_driver *serial_driver = NULL;
This initialisation is unneeded and is undesirable because it increases the kernel image's .data.
Please run the diff through scripts/checkpatch.pl. It would have mentioned this, and a whole lot of other things, too.
drivers/serial/Kconfig line 14
Quoted 1 line
drivers/serial/Kconfig +1
-0+/* global since we do register the driver only once */
I assume this comment refers to the preceding line?
Please put comments like this above the code which they're talking about. Doing it this way is quite confusing to kernel developers.
drivers/serial/Kconfig line 16
Quoted 2 lines
drivers/serial/Kconfig +2
-0+#define MAX_MAX3100 4 /* 4 MAX3100s should be enough for everyone */ +static struct max3100_port_s * max3100s[MAX_MAX3100]; /* the chips */
No space after the *, please (checkpatch should detect this).
drivers/serial/Kconfig line 19
Quoted 3 lines
drivers/serial/Kconfig +3
-0+ +#define MAX3100_TX_BUF_N 16 +/* our buffer for sendind chars */
Typo. Misplaced comment.
drivers/serial/Kconfig line 21
Quoted 2 lines
drivers/serial/Kconfig +2
-0+#define MAX3100_FLIP_EVERY 8 +/* how many chars we wait befor flipping tty buffer */
Misplaced comment.
drivers/serial/Kconfig line 22
Quoted 1 line
drivers/serial/Kconfig +1
-0+static void irq_state(struct max3100_port_s *s, int on) {
That brace should go in column 1.
drivers/serial/Kconfig line 32
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ unsigned long flags; + + spin_lock_irqsave(&s->irq_lock, flags); + if (s->irq_status != on) { + if (on) { + enable_irq(s->irq); + } + else { + disable_irq(s->irq); + }
The `else' should be like this:
} else {and as the brqaces are unneeded we'd prefer
if (on) enable_irq(s->irq); else disable_irq(s->irq);
but checkpatch knows about this too.
Quoted 14 lines
drivers/serial/Kconfig +14
-0+ s->irq_status = on; + } + spin_unlock_irqrestore(&s->irq_lock, flags); +} + + +static int max3100_do_parity(struct max3100_port_s *s, u16 c) +{ + int parity; + int i,n; + + if (s->parity & MAX3100_PARITY_ODD) + parity = 0; + else
drivers/serial/Kconfig line 56
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ parity = 1; + + if (s->parity & MAX3100_7BIT) + n = 7; + else + n = 8; + + for(i=0;i<n;i++) { + parity = parity ^ ( (c>>i) & 1); + }
Those three lines will cause a checkpatch frenzy.
Quoted 14 lines
drivers/serial/Kconfig +14
-0+ return parity; +} + +static inline int max3100_check_parity(struct max3100_port_s *s, u16 c) +{ + return max3100_do_parity(s, c) == ( (c>>9) & 1); +} + +static inline void max3100_calc_parity(struct max3100_port_s *s, u16 *c) +{ + *c &= ~MAX3100_PT; + *c |= max3100_do_parity(s, *c)<<8; +} +
drivers/serial/Kconfig line 80
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ +static void max3100_work(struct work_struct *w); + +static inline void max3100_dowork(struct max3100_port_s *s) +{ + if (!s->force_end_work) { + PREPARE_WORK(&s->work, max3100_work); + queue_work(s->workqueue, &s->work); + } +}
You'd be surprised how small an inlined function can be and yet still be too large to be inlined (if that sentence makes sense).
I'd suggest that you try removing inlines then recmpiling and doing `size drivers/serial/max3100.o'. You'll probably find that most of them are either unneeded or plain wrong.
drivers/serial/Kconfig line 92
Quoted 12 lines
drivers/serial/Kconfig +12
-0+static int max3100_sr(struct max3100_port_s *s, u16 tx, u16 *rx, int push) +{ + /* note: we suppose that the T bit fron conf_status is in sync + * with the hw one, so all the communication must go through + * this function */ + struct spi_message message; + struct spi_transfer x; + int status, flag; + u16 etx,erx; + int got_char = 0; + + down(&s->spi_txrx);
The semaphore API is deprecated unless one is actually using its counting feature. Please prefer to use struct mutex.
I'd have expected checkpatch to warn about that too...
drivers/serial/Kconfig line 102
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ if (s->loopback) { + if ((tx & MAX3100_CMD) == MAX3100_RC) + tx |= 1; + } + etx = htons(tx); + spi_message_init(&message); + memset(&x, 0, sizeof(x)); + x.tx_buf = &etx; + x.rx_buf = &erx; + x.len = 2;
`x' is a truly awful identifier. Please try to come up with something which means something?
drivers/serial/Kconfig line 110
Quoted 8 lines
drivers/serial/Kconfig +8
-0+ spi_message_add_tail(&x, &message); + status = spi_sync(s->spi, &message); + if (status) { + dev_warn(&s->spi->dev, "error while calling spi_sync\n"); + return 0; + } + *rx = ntohs(erx); + //printk("%04x - %04x\n", tx, *rx);
checkpatch won't like the c99-style comment.
Quoted 16 lines
drivers/serial/Kconfig +16
-0+ if ((tx & MAX3100_CMD) == MAX3100_RD || + (tx & MAX3100_CMD) == MAX3100_WD) { + s->last_cts_rx = *rx; + } + + if (*rx & MAX3100_R && + ((tx & MAX3100_CMD) == MAX3100_RD || + (tx & MAX3100_CMD) == MAX3100_WD)) { + unsigned int flags; + + got_char = 1; + if (tty_buffer_request_room(s->tty, 1) == 0) { + dev_warn(&s->spi->dev, "no room in tty buffer\n"); + tty_schedule_flip(s->tty); + } +
drivers/serial/Kconfig line 136
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ flag = TTY_NORMAL; + if (*rx & MAX3100_RAFE) { + s->port.icount.frame++; + flags = TTY_FRAME; + } + if (s->parity & MAX3100_PARITY_ON && max3100_check_parity(s, *rx)) { + s->port.icount.parity++; + flags = TTY_PARITY; + } + tty_insert_flip_char(s->tty, *rx & MAX3100_DATA, flag);
Local variable 'flags' never gets used. If it _did_ get used, you'd get a used-uninitialised-variable warning.
drivers/serial/Kconfig line 143
Quoted 7 lines
drivers/serial/Kconfig +7
-0+ } + + up(&s->spi_txrx); + + if (push && got_char) { + tty_schedule_flip(s->tty); + }
unneeded braces.
Quoted 20 lines
drivers/serial/Kconfig +20
-0+ return got_char; +} + + +static void max3100_send_conf(struct max3100_port_s *s) +{ + u16 tx=0,rx; + + tx = MAX3100_WC | (s->conf & 0x0fff); + max3100_sr(s, tx, &rx, 1); +} + +static inline int max3100_ok_to_send(struct max3100_port_s *s) +{ + if (C_CRTSCTS(s->tty)) { + if ((s->last_cts_rx & MAX3100_CTS) == 0) + return 0; + } + return ! s->tx_stopped; +}
drivers/serial/Kconfig line 173
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ +static void max3100_work(struct work_struct *w) +{ + struct max3100_port_s *s = container_of(w, struct max3100_port_s, work); + u16 tx,rx; + unsigned long flags; + int nchars; /* number of char waiting in the send buf */ + int rxchars = 0; /* chars received */ + + do {
local variable `flags' could be local to this block. I think that restricting variable scope in large functions like this improves readability.
Quoted 27 lines
drivers/serial/Kconfig +27
-0+ tx = MAX3100_RD; + rxchars += max3100_sr(s, tx, &rx, 0); + /* rx a char so we get MAX3100_T and CTS current */ + + spin_lock_irqsave(&s->tx_buf_lock, flags); + nchars = s->tx_buf_tot - s->tx_buf_cur; + tx = MAX3100_WD | s->rts; + tx |= s->tx_buf[s->tx_buf_cur]; + spin_unlock_irqrestore(&s->tx_buf_lock, flags); + + if ((rx & MAX3100_T) && /* tx buffer empty*/ + max3100_ok_to_send(s) && /* fw ctrl decision */ + nchars > 0 && /* more to send */ + ! s->force_end_work) { + if (s->parity & MAX3100_PARITY_ON) + max3100_calc_parity(s, &tx); + rxchars += max3100_sr(s, tx, &rx, 0); + nchars -= 1; + spin_lock_irqsave(&s->tx_buf_lock, flags); + s->tx_buf_cur += 1; + spin_unlock_irqrestore(&s->tx_buf_lock, flags); + if (nchars == 0) { + tty_wakeup(s->tty); + } + } + if (rxchars > MAX3100_FLIP_EVERY) { + tty_schedule_flip(s->tty);
drivers/serial/Kconfig line 210
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ rxchars = 0; + } + } + while ((nchars > 0 || /* more to send */ + rx & MAX3100_R) && /* try to empy all the RX buffer */ + ! s->force_end_work); + if (rxchars) { + tty_schedule_flip(s->tty); + } + if (! s->only_edge_irq && ! s->force_end_work)
No space after the !, please (checkpatch)
This patch adds large amounts of trailing whitespace. I always chop that off, but it's best that it never arrives, please.
Quoted 22 lines
drivers/serial/Kconfig +22
-0+ irq_state(s, 1); +} + +static irqreturn_t max3100_irq(int irqno, void *dev_id) +{ + struct max3100_port_s *s = dev_id; + + if (! s->only_edge_irq) + irq_state(s, 0); /* avoid irq storm */ + max3100_dowork(s); + return IRQ_HANDLED; +} + +static void rs_stop(struct tty_struct *tty) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + + s->tx_stopped = 1; +} + +static void rs_start(struct tty_struct *tty) +{
drivers/serial/Kconfig line 242
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + + s->tx_stopped = 0; +} + +static void rs_flush_chars(struct tty_struct *tty) +{ +} + +static void rs_waint_until_sent(struct tty_struct *tty, int timeout)
waint?
drivers/serial/Kconfig line 253
Quoted 11 lines
drivers/serial/Kconfig +11
-0+{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + unsigned long jstop; + + jstop = jiffies + timeout; + do { + if (s->tx_buf_tot == 0 || s->tx_buf_tot == s->tx_buf_cur) + return; + schedule(); + } while (!timeout || time_before(jiffies, jstop)); +}
eek, that's a busy-wait. Can something smarter be done here?
Quoted 5 lines
drivers/serial/Kconfig +5
-0+static void rs_flush_buffer(struct tty_struct *tty) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + unsigned long flags; +
drivers/serial/Kconfig line 268
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ spin_lock_irqsave(&s->tx_buf_lock, flags); + s->tx_buf_cur = 0; + s->tx_buf_tot = 0; + spin_unlock_irqrestore(&s->tx_buf_lock, flags); +} + +static int rs_write(struct tty_struct * tty, + const unsigned char *buf, int count) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data;
Unneded cast of void*. This is unsightly and defeats typechecking. PLease just remove the cast (several instances of this).
drivers/serial/Kconfig line 275
Quoted 7 lines
drivers/serial/Kconfig +7
-0+ unsigned long flags; + + spin_lock_irqsave(&s->tx_buf_lock, flags); + if (s->tx_buf_cur < s->tx_buf_tot) { + count = 0; + } + else {
checkpatch...
Quoted 41 lines
drivers/serial/Kconfig +41
-0+ if (count > MAX3100_TX_BUF_N) + count = MAX3100_TX_BUF_N; + memcpy(s->tx_buf, buf, count); + s->tx_buf_cur = 0; + s->tx_buf_tot = count; + } + spin_unlock_irqrestore(&s->tx_buf_lock, flags); + max3100_dowork(s); + return count; +} + +static int rs_write_room(struct tty_struct *tty) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + int ret; + unsigned long flags; + + spin_lock_irqsave(&s->tx_buf_lock, flags); + if (s->tx_buf_cur < s->tx_buf_tot) + ret = 0; + else + ret = MAX3100_TX_BUF_N; + spin_unlock_irqrestore(&s->tx_buf_lock, flags); + return ret; +} + +static int rs_chars_in_buffer(struct tty_struct *tty) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + int ret; + unsigned long flags; + + spin_lock_irqsave(&s->tx_buf_lock, flags); + if (s->tx_buf_cur < s->tx_buf_tot) + ret = MAX3100_TX_BUF_N; + else + ret = 0; + spin_unlock_irqrestore(&s->tx_buf_lock, flags); + return ret; +} +
drivers/serial/Kconfig line 326
Quoted 10 lines
drivers/serial/Kconfig +10
-0+static void internal_throttle(struct tty_struct * tty, char ch, int rts) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + u16 tx=0,rx; + int old_tx_stopped; + + old_tx_stopped = s->tx_stopped; + s->tx_stopped = 1; + tx = MAX3100_RD; + do { /* wait fot tx buf empty */
typo in comment.
drivers/serial/Kconfig line 328
Quoted 2 lines
drivers/serial/Kconfig +2
-0+ max3100_sr(s, tx, &rx, 1); + } while ((rx & MAX3100_T) == 0);
Another busy-wait. Avoidable?
Quoted 36 lines
drivers/serial/Kconfig +36
-0+ if (I_IXOFF(tty)) { + tx = MAX3100_WD | s->rts | (ch&0xff); + } + if (C_CRTSCTS(tty)) { + s->rts = rts ? (MAX3100_RTS) : 0; + tx = MAX3100_WD | s->rts | MAX3100_TE; + } + if (tx) { + max3100_sr(s, tx, &rx, 1); + } + s->tx_stopped = old_tx_stopped; +} + +static void rs_throttle(struct tty_struct * tty) +{ + internal_throttle(tty, STOP_CHAR(tty), 0); +} + +static void rs_unthrottle(struct tty_struct * tty) +{ + internal_throttle(tty, START_CHAR(tty), 1); +} + +static int get_lsr_info(struct max3100_port_s *s, unsigned int *value) +{ + unsigned char status; + u16 tx=0,rx; + + tx = MAX3100_RD; + max3100_sr(s, tx, &rx, 1); + status = (rx & MAX3100_CTS) > 0; + put_user(status,value); + return 0; +} + +static int rs_ioctl(struct tty_struct *tty, struct file * file,
drivers/serial/Kconfig line 374
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ unsigned int cmd, unsigned long arg) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + int retval = 0; + + switch (cmd) { + + case TIOCSERGETLSR: /* Get line status register */ + if (access_ok(VERIFY_WRITE, (void *) arg, sizeof(unsigned int))) + return get_lsr_info(s, (unsigned int *) arg);
get_lsr_info() uses put_user() which already does the access_ok() check, so it should be removed from here.
get_lsr_info() should be changed to return the return value from put_user().
Quoted 7 lines
drivers/serial/Kconfig +7
-0+ return -EFAULT; + + default: + retval = -ENOIOCTLCMD ; + } + return retval; +}
drivers/serial/Kconfig line 391
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ +static void change_speed(struct max3100_port_s *s) +{ + unsigned cflag; + unsigned i; + u32 param_new; + u32 param_mask; + + if (!s->tty || !s->tty->termios) + return;
Can this happen?
Quoted 80 lines
drivers/serial/Kconfig +80
-0+ cflag = s->tty->termios->c_cflag; + param_new = param_mask = 0; + + i = cflag & CBAUD; + /* note: supposed 3.6864 Mhz xtal */ + switch(i) { + case B300: + if (s->crystal) { + param_new = 16; + } + else { + dev_warn(&s->spi->dev, "unsupported baud rate!\n"); + param_new = 15; + } + break; + case B600: + param_new = 15; + break; + case B1200: + param_new = 14; + break; + case B2400: + param_new = 13; + break; + case B4800: + param_new = 12; + break; + case B9600: + param_new = 11; + break; + case B19200: + param_new = 10; + break; + case B38400: + param_new = 9; + break; + case B57600: + param_new = 2; + break; + case B115200: + param_new = 1; + break; + case B230400: + if (s->crystal) { + param_new = 1; + dev_warn(&s->spi->dev, "unsupported baud rate!\n"); + } + else { + param_new = 0; + } + break; + default: + param_new = 1; + dev_warn(&s->spi->dev, "invalid baudrate\n"); + } + param_new = (param_new - s->crystal) & MAX3100_BAUD; + param_mask |= MAX3100_BAUD; + + if ((cflag & CSIZE) == CS8) { + param_new &= ~MAX3100_L; + s->parity &= ~MAX3100_7BIT; + } + else { + param_new |= MAX3100_L; + s->parity |= MAX3100_7BIT; + } + param_mask |= MAX3100_L; + + if (cflag & CSTOPB) + param_new |= MAX3100_ST; + else + param_new &= ~MAX3100_ST; + param_mask |= MAX3100_ST; + + if (cflag & PARENB) { + param_new |= MAX3100_PE; + s->parity |= MAX3100_PARITY_ON; + } + else { + param_new &= ~MAX3100_PE;
drivers/serial/Kconfig line 481
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ s->parity &= ~MAX3100_PARITY_ON; + } + param_mask |= MAX3100_PE; + + if (cflag & PARODD) { + s->parity |= MAX3100_PARITY_ODD; + } + else { + s->parity &=~ MAX3100_PARITY_ODD; + }
Lots of checkpatch fodder there.
Quoted 11 lines
drivers/serial/Kconfig +11
-0+ s->conf = (s->conf & ~param_mask) | (param_new & param_mask); + max3100_send_conf(s); + + return; +} + + +static void rs_set_termios(struct tty_struct *tty, struct ktermios *old_termios) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; +
drivers/serial/Kconfig line 502
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ if (tty->termios->c_cflag == old_termios->c_cflag) + return; + + change_speed(s); + + if ((old_termios->c_cflag & CRTSCTS) && + !(tty->termios->c_cflag & CRTSCTS)) { + max3100_dowork(s); + } +}
Something went wrong with whitespace in rs_set_termios().
Quoted 8 lines
drivers/serial/Kconfig +8
-0+static void shutdown(struct max3100_port_s *s) +{ + s->force_end_work = 1; + if (s->workqueue) { + flush_workqueue(s->workqueue); + destroy_workqueue(s->workqueue); + } + if (s->irq)
drivers/serial/Kconfig line 520
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ free_irq(s->irq, s); + kfree(s->tx_buf); + s->tx_buf = NULL; + if (s->tty) + set_bit(TTY_IO_ERROR, &s->tty->flags); +} + +static void rs_hangup(struct tty_struct *tty) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data;
unneeded cast..
drivers/serial/Kconfig line 527
Quoted 7 lines
drivers/serial/Kconfig +7
-0+ shutdown(s); +} + +static int startup(struct max3100_port_s *s) +{ + char b[10]; + s->tx_buf = kmalloc(MAX3100_TX_BUF_N, GFP_KERNEL);
Unchecked kmalloc().
Quoted 9 lines
drivers/serial/Kconfig +9
-0+ s->tx_buf_cur = 0; + s->tx_buf_tot = 0; + + s->force_end_work = 0; + s->parity = 0; + s->rts = 0; + s->tx_stopped = 0; + s->conf = MAX3100_RM; +
drivers/serial/Kconfig line 546
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ sprintf(b, "max3100-%d", s->minor); + s->workqueue = create_singlethread_workqueue(b); + if (!s->workqueue) { + dev_warn(&s->spi->dev, "cannot create workqueue\n"); + return -EBUSY; + } + INIT_WORK(&s->work, max3100_work); + + s->irq_status = 1; /* IRQS are on after request */ + if (request_irq(s->irq, max3100_irq, 0, "max3100", s) < 0) {
No need for IRQ sharing?
drivers/serial/Kconfig line 549
Quoted 3 lines
drivers/serial/Kconfig +3
-0+ dev_warn(&s->spi->dev, "cannot allocate irq %d\n", s->irq); + s->irq = 0; + return -EBUSY;
We just leaked the memory at s->tx_buf.
Quoted 24 lines
drivers/serial/Kconfig +24
-0+ } + if (s->only_edge_irq) + set_irq_type(s->irq, IRQT_FALLING); + else + set_irq_type(s->irq, IRQT_LOW); + + if (s->tty) + clear_bit(TTY_IO_ERROR, &s->tty->flags); + + change_speed(s); + + if (s->loopback) { + u16 tx,rx; + tx = 0x4001; + max3100_sr(s, tx, &rx, 1); + } + + return 0; +} + +int rs_open(struct tty_struct *tty, struct file * filp) +{ + struct max3100_port_s *s; + int retval=0, line;
drivers/serial/Kconfig line 583
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ + line = tty->index; + + if (line >= MAX_MAX3100 || line < 0) + return -ENODEV; + + s = max3100s[line]; + + if (!s) { + printk("inexistent max3100 for index %d\n", line);
"Nonexistent"
Quoted 18 lines
drivers/serial/Kconfig +18
-0+ return -ENODEV; + } + + down(&s->sem); + + s->ref_count++; + + if (s->ref_count == 1) { + tty->driver_data = s; + s->tty = tty; + + retval = startup(s); + } + + up(&s->sem); + + return retval; +}
drivers/serial/Kconfig line 611
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ +static void rs_close(struct tty_struct *tty, struct file * filp) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + + down(&s->sem); + + s->ref_count--; + if (s->ref_count == 0) { + rs_waint_until_sent(tty, 5*HZ);
hm, a magic constant. What's happening here?
drivers/serial/Kconfig line 622
Quoted 11 lines
drivers/serial/Kconfig +11
-0+ shutdown(s); + } + + up(&s->sem); +} + +static int rs_tiocmget(struct tty_struct *tty, struct file *file) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + u16 tx,rx; + unsigned int result = 0;
unneeded initialisation.
Quoted 36 lines
drivers/serial/Kconfig +36
-0+ tx = MAX3100_RD; + max3100_sr(s, tx, &rx, 1); + + result = ((s->rts) ? TIOCM_RTS : 0) + | ((rx & MAX3100_CTS) ? TIOCM_CTS : 0); + return result; +} + +static int rs_tiocmset(struct tty_struct *tty, struct file *file, + unsigned int set, unsigned int clear) +{ + struct max3100_port_s *s = (struct max3100_port_s *) tty->driver_data; + int old_rts = s->rts; + u16 tx,rx; + + if (set & TIOCM_RTS) + s->rts = MAX3100_RTS; + if (clear & TIOCM_RTS) + s->rts = 0; + + if (s->rts != old_rts) { + tx = MAX3100_WD | s->rts | MAX3100_TE; + max3100_sr(s, tx, &rx, 1); + } + return 0; +} + +struct tty_operations rs_ops = { + .open = rs_open, + .close = rs_close, + .write = rs_write, + .flush_chars = rs_flush_chars, + .wait_until_sent = rs_waint_until_sent, + .write_room = rs_write_room, + .chars_in_buffer = rs_chars_in_buffer, + .flush_buffer = rs_flush_buffer,
drivers/serial/Kconfig line 668
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ .ioctl = rs_ioctl, + .throttle = rs_throttle, + .unthrottle = rs_unthrottle, + .set_termios = rs_set_termios, + .stop = rs_stop, + .start = rs_start, + .hangup = rs_hangup, + .tiocmget = rs_tiocmget, + .tiocmset = rs_tiocmset, +};
Can this have static scope?
Quoted 35 lines
drivers/serial/Kconfig +35
-0+static int __devinit max3100_probe(struct spi_device *spi) +{ + int i; + struct plat_max3100 *pdata; + + if (!serial_driver) { + serial_driver = alloc_tty_driver(MAX_MAX3100); + if (!serial_driver) { + printk("Cannot allocate serial driver\n"); + return -ENOMEM; + } + + serial_driver->name = "ttyMAX"; + serial_driver->driver_name = "ttyMAX"; + serial_driver->major = TTY_MAJOR; + serial_driver->minor_start = 128; /* this should prevent clashes */ + serial_driver->type = TTY_DRIVER_TYPE_SERIAL; + serial_driver->subtype = SERIAL_TYPE_NORMAL; + serial_driver->init_termios = tty_std_termios; + serial_driver->init_termios.c_cflag = + B9600 | CS8 | CREAD | HUPCL | CLOCAL; + serial_driver->flags = TTY_DRIVER_REAL_RAW |TTY_DRIVER_DYNAMIC_DEV; + tty_set_operations(serial_driver, &rs_ops); + + if (tty_register_driver(serial_driver)) { + put_tty_driver(serial_driver); + printk("Couldn't register serial driver\n"); + return -EINVAL; + } + } + + for(i=0; i<MAX_MAX3100; i++) + if (!max3100s[i]) + break; + if (i == MAX_MAX3100) {
drivers/serial/Kconfig line 713
Quoted 10 lines
drivers/serial/Kconfig +10
-0+ dev_warn(&spi->dev, "too many MAX3100 chips\n"); + return -ENOMEM; + } + + max3100s[i] = kmalloc(sizeof(struct max3100_port_s), GFP_KERNEL); + if (!max3100s[i]) { + dev_warn(&spi->dev, "kmalloc for max3100 structure %d failed!\n", i); + return -ENOMEM; + } + memset(max3100s[i], 0, sizeof(struct max3100_port_s));
use kzalloc() above, remove the memset.
drivers/serial/Kconfig line 717
Quoted 4 lines
drivers/serial/Kconfig +4
-0+ max3100s[i]->spi = spi; + max3100s[i]->irq = spi->irq; + init_MUTEX(&max3100s[i]->sem); + init_MUTEX(&max3100s[i]->spi_txrx);
Use struct mutex.
Quoted 93 lines
drivers/serial/Kconfig +87
-0+ spin_lock_init(&max3100s[i]->tx_buf_lock); + spin_lock_init(&max3100s[i]->irq_lock); + dev_set_drvdata(&spi->dev, &max3100s[i]); + pdata = spi->dev.platform_data; + max3100s[i]->crystal = pdata->crystal; + max3100s[i]->loopback = pdata->loopback; + max3100s[i]->only_edge_irq = pdata->only_edge_irq; + max3100s[i]->minor = i; + tty_register_device(serial_driver, i, &spi->dev); + + return 0; +} + +static int __devexit max3100_remove(struct spi_device *spi) +{ + int i; + + i = 0; + + for(i=0; i<MAX_MAX3100; i++) + if (max3100s[i] && max3100s[i]->ref_count > 0) + return -EBUSY; + + for(i=0; i<MAX_MAX3100; i++) + if (max3100s[i]) { + tty_unregister_device(serial_driver, i); + kfree(max3100s[i]); + max3100s[i] = NULL; + } + + if (serial_driver) + tty_unregister_driver(serial_driver); + return 0; +} + +#ifdef CONFIG_PM +static int max3100_suspend(struct spi_device *spi, pm_message_t state) +{ + struct max3100_port_s *s = (struct max3100_port_s *) dev_get_drvdata(&spi->dev); + u16 tx,rx; + + tx = MAX3100_WC | MAX3100_SHDN; + max3100_sr(s, tx, &rx, 0); + return 0; +} + +static int max3100_resume(struct spi_device *spi) { + struct max3100_port_s *s = (struct max3100_port_s *) dev_get_drvdata(&spi->dev); + + max3100_send_conf(s); + + return 0; +} + +#else +#define max3100_suspend NULL +#define max3100_resume NULL +#endif + +static struct spi_driver max3100_driver = { + .driver = { + .name = "max3100", + .bus = &spi_bus_type, + .owner = THIS_MODULE, + }, + + .probe = max3100_probe, + .remove = __devexit_p(max3100_remove), + .suspend = max3100_suspend, + .resume = max3100_resume, +}; + + +static int __init max3100_init(void) +{ + return spi_register_driver(&max3100_driver); +} +module_init(max3100_init); + +static void __exit max3100_exit(void) +{ + spi_unregister_driver(&max3100_driver); +} +module_exit(max3100_exit); + +MODULE_DESCRIPTION("MAX3100 driver"); +MODULE_LICENSE("GPL");include/linux/serial_max3100.h new file +0
-0
include/linux/serial_max3100.h line 1
Quoted 1 line
include/linux/serial_max3100.h new file +1
-0@@ -0,0 +1,25 @@ +
Stray blank line at start of file(s)
Quoted 12 lines
include/linux/serial_max3100.h new file +12
-0+/* +* +* Copyright (C) 2007 Christian Pellegrin +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +*/ + + +#ifndef _LINUX_SERIAL_MAX3100_H
include/linux/serial_max3100.h line 23
Quoted 10 lines
include/linux/serial_max3100.h new file +10
-0+#define _LINUX_SERIAL_MAX3100_H 1 + +struct plat_max3100 { +int loopback; +/* force MAX3100 in loopback */ +int crystal; +/* 0 for 3.6864 Mhz, 1 for 1.8432 */ +int only_edge_irq; +/* for archs like PXA with only edge irqs */ +};
Does this header file need to exist? afaict plat_max3100 is only used by drivers/serial/max3100.c and hence could be defined there?
Hi,
thank you for your extensive review, I will fix and resubmitt. Anyway I learned an important lesson: checkpatch.pl will be my best friend. Sorry I haven't read about it earlier.
Just a question:
Quoted 18 lines
Quoted 18 lines
include/linux/serial_max3100.h new file +17
-0+/* +* +* Copyright (C) 2007 Christian Pellegrin +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +*/ + + +#ifndef _LINUX_SERIAL_MAX3100_H +#define _LINUX_SERIAL_MAX3100_H 1 + +struct plat_max3100 { + int loopback; +/* force MAX3100 in loopback */
Quoted 9 lines
include/linux/serial_max3100.h line 23
Quoted 5 lines
include/linux/serial_max3100.h new file +5
-0+int crystal; +/* 0 for 3.6864 Mhz, 1 for 1.8432 */ +int only_edge_irq; +/* for archs like PXA with only edge irqs */ +};Does this header file need to exist? afaict
plat_max3100is only used bydrivers/serial/max3100.cand hence could be defined there?
It's used in the machine definition file for platform devices that cannot be autoprobed. Anyway where is the best place to put such struct definition / include files? And what is the best way to document them (if not inline)? For example I put in my machine definition file (let's say arch/arm/mach-s3c2410/mach-smdk2410.c):
static struct plat_max3100 max3100_plat_data = {
.loopback = 0,
.crystal = 0,
.only_edge_irq = 0,
};static struct spi_board_info spi_board_info[] = {
{
.modalias = "max3100",
.platform_data = &max3100_plat_data,
.irq = IRQ_EINT12,
.max_speed_hz = 5*1000*1000,
.chip_select = 0,
},
};...
spi_register_board_info(spi_board_info, ARRAY_SIZE(spi_board_info));
Signature · 5 lines
Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."