On Mon, 8 Feb 2010 16:59:46 +0800 Feng Tang <feng.tang@xxxxxxxxx> wrote: > Hi All, > > Here is a driver for Maxim 3110 SPI-UART device, please help to review. > > It has been validated with Designware SPI controller (drivers/spi: dw_spi.c & > dw_spi_pci.c). It supports polling and IRQ mode, supports batch read, and > provides a console. > > change since v1: > * Address comments from Alan Cox > * Use a "use_irq" module parameter to runtime check whether > to use IRQ mode > * Add the suspend/resume implementation > > Thanks! > Feng > > >From 6d14c5d68cdae8d48b6d8a00b6653022f2b100d0 Mon Sep 17 00:00:00 2001 > From: Feng Tang <feng.tang@xxxxxxxxx> > Date: Mon, 8 Feb 2010 16:02:59 +0800 > Subject: [PATCH] serial: spi: add spi-uart driver for Maxim 3110 > > This is the driver for Max3110 SPI-UART device, which connect > to host with SPI interface. It supports baud rates from 300 to > 230400, and supports both polling and IRQ mode, as well as > providing a console for system use > > Its datasheet could be found here: > http://datasheets.maxim-ic.com/en/ds/MAX3110E-MAX3111E.pdf > I wonder if this is an "spi" subsystem thing or a "serial" subsystem thing. It looks more like a serial driver to me. > --- > drivers/serial/Kconfig | 12 + > drivers/serial/max3110.c | 848 +++++++++++++++++++++++++++++++++++++++++++ > drivers/serial/max3110.h | 59 +++ > include/linux/serial_core.h | 3 + > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig > index 9ff47db..f7daf2f 100644 > --- a/drivers/serial/Kconfig > +++ b/drivers/serial/Kconfig > > ... > > +static int use_irq = 1; > +module_param(use_irq, int, 0444); > +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability"); > + > +static void receive_chars(struct uart_max3110 *max, > + unsigned char *str, int len); > +static int max3110_read_multi(struct uart_max3110 *max, int len, u8 *buf); > +static void max3110_con_receive(struct uart_max3110 *max); > + > +int max3110_write_then_read(struct uart_max3110 *max, > + const u8 *txbuf, u8 *rxbuf, unsigned len, int always_fast) The driver has a number of identifiers which have global scope, apparently unnecessarily. Please review all such identifiers, see if they can be made static. > +{ > + struct spi_device *spi = max->spi; > + struct spi_message message; > + struct spi_transfer x; > + int ret; > + > + spi_message_init(&message); > + memset(&x, 0, sizeof x); > + x.len = len; > + x.tx_buf = txbuf; > + x.rx_buf = rxbuf; > + spi_message_add_tail(&x, &message); > + > + if (always_fast) > + x.speed_hz = spi->max_speed_hz; > + else if (max->baud) > + x.speed_hz = max->baud; > + > + /* Do the i/o */ > + ret = spi_sync(spi, &message); > + return ret; > +} > + > +/* Write a u16 to the device, and return one u16 read back */ > +int max3110_out(struct uart_max3110 *max, const u16 out) Another. > +{ > + u16 tmp; > + int ret; > + > + ret = max3110_write_then_read(max, (u8 *)&out, (u8 *)&tmp, 2, 1); Something nasty is happening here. Modifying a u16 via a u8* is to assume a certain endianness, surely. Will the driver break when used on other-endian architectures? > + if (ret) > + return ret; > + > + /* If some valid data is read back */ > + if (tmp & MAX3110_READ_DATA_AVAILABLE) { > + tmp &= 0xff; > + receive_chars(max, (unsigned char *)&tmp, 1); > + } > + > + return ret; > +} > + > +#define MAX_READ_LEN 20 > +/* > + * This is usually used to read data from SPIC RX FIFO, which doesn't > + * need any delay like flushing character out. > + * Returns how many valide bytes are read back > + */ > +static int max3110_read_multi(struct uart_max3110 *max, int len, u8 *buf) > +{ > + u16 out[MAX_READ_LEN], in[MAX_READ_LEN]; > + u8 *pbuf, valid_str[MAX_READ_LEN]; > + int i, j, bytelen; > + > + bytelen = len * 2; > + memset(out, 0, bytelen); > + memset(in, 0, bytelen); > + > + if (max3110_write_then_read(max, (u8 *)out, (u8 *)in, bytelen, 1)) > + return 0; > max3110_write_then_read() can return an error code (from spi_async()). But here that error code simply gets lost. It should be reported to higher-level code somehow? > + /* If caller doesn't provide a buffer, then handle received char */ > + pbuf = buf ? buf : valid_str; > + > + for (i = 0, j = 0; i < len; i++) { > + if (in[i] & MAX3110_READ_DATA_AVAILABLE) > + pbuf[j++] = (u8)(in[i] & 0xff); > + } > + > + if (j && (pbuf == valid_str)) > + receive_chars(max, valid_str, j); > + > + return j; > +} > + > +static void serial_m3110_con_putchar(struct uart_port *port, int ch) > +{ > + struct uart_max3110 *max = > + container_of(port, struct uart_max3110, port); > + struct circ_buf *xmit = &max->con_xmit; > + > + if (uart_circ_chars_free(xmit)) { > + xmit->buf[xmit->head] = (char)ch; > + xmit->head = (xmit->head + 1) & (PAGE_SIZE - 1); > + } > + > + if (!atomic_read(&max->con_tx_need)) { > + atomic_set(&max->con_tx_need, 1); This manipulation of con_tx_need looks racy on SMP or preempt. > + wake_up_process(max->main_thread); > + } > +} > + > > ... > > +#define WORDS_PER_XFER 128 > +static inline void send_circ_buf(struct uart_max3110 *max, > + struct circ_buf *xmit) I suggest that the `inline' be removed. Modern gcc will take care of this, if it is of benefit. > +{ > + int len, left = 0; > + u16 obuf[WORDS_PER_XFER], ibuf[WORDS_PER_XFER]; > + u8 valid_str[WORDS_PER_XFER]; > + int i, j; > + > + while (!uart_circ_empty(xmit)) { > + left = uart_circ_chars_pending(xmit); > + while (left) { > + len = (left >= WORDS_PER_XFER) ? WORDS_PER_XFER : left; Use len = min(left, WORDS_PER_XFER); > + > + memset(obuf, 0, len * 2); > + memset(ibuf, 0, len * 2); Using memset(obuf, 0, sizeof(obuf)); would remove the assumptions that a) obuf is of type u16 and b) that sizeof(u16)==2. > + for (i = 0; i < len; i++) { > + obuf[i] = (u8)xmit->buf[xmit->tail] | WD_TAG; > + xmit->tail = (xmit->tail + 1) & > + (UART_XMIT_SIZE - 1); Could this driver use include/linux/kfifo.h, rather than open-coding it? > + } > + max3110_write_then_read(max, (u8 *)obuf, > + (u8 *)ibuf, len * 2, 0); Error codes are ignored. > + for (i = 0, j = 0; i < len; i++) { > + if (ibuf[i] & MAX3110_READ_DATA_AVAILABLE) > + valid_str[j++] = (u8)(ibuf[i] & 0xff); > + } > + > + if (j) > + receive_chars(max, valid_str, j); > + > + max->port.icount.tx += len; > + left -= len; > + } > + } > +} > + > > ... > > +static void max3110_con_receive(struct uart_max3110 *max) > +{ > + int loop = 1, num, total = 0; > + u8 recv_buf[512], *pbuf; > + > + pbuf = recv_buf; > + do { > + /* 3110 RX buffer is 8 words */ > + num = max3110_read_multi(max, 8, pbuf); > + > + if (num) { > + loop = 5; > + pbuf += num; > + total += num; > + > + if (total >= 500) { > + receive_chars(max, recv_buf, total); > + pbuf = recv_buf; > + total = 0; > + } > + } hm, what do all the magic numbers do? Some comments explaining them would be good. > + } while (--loop); > + > + if (total) > + receive_chars(max, recv_buf, total); > +} > + > +static int max3110_main_thread(void *_max) > +{ > + struct uart_max3110 *max = _max; > + wait_queue_head_t *wq = &max->wq; > + int ret = 0; > + struct circ_buf *xmit = &max->con_xmit; > + > + init_waitqueue_head(wq); > + pr_info(PR_FMT "start main thread\n"); > + > + do { > + wait_event_interruptible(*wq, (atomic_read(&max->irq_pending) || > + atomic_read(&max->con_tx_need) || > + atomic_read(&max->uart_tx_need)) || > + kthread_should_stop()); > + max->mthread_up = 1; > + > + if (use_irq && atomic_read(&max->irq_pending)) { > + max3110_con_receive(max); > + atomic_set(&max->irq_pending, 0); > + } The manipulation of irq_pending looks racy. Perhaps something list test_and_clear_bit() would fix that. > + /* First handle console output */ > + if (atomic_read(&max->con_tx_need)) { > + send_circ_buf(max, xmit); > + atomic_set(&max->con_tx_need, 0); > + } Ditto. > + /* Handle uart output */ > + if (atomic_read(&max->uart_tx_need)) { > + transmit_char(max); > + atomic_set(&max->uart_tx_need, 0); > + } Ditto. > + max->mthread_up = 0; > + } while (!kthread_should_stop()); > + > + return ret; > +} > + > +irqreturn_t static serial_m3110_irq(int irq, void *dev_id) `static irqreturn_t' would be more typical. > +{ > + struct uart_max3110 *max = dev_id; > + > + /* max3110's irq is a falling edge, not level triggered, > + * so no need to disable the irq */ > + if (!atomic_read(&max->irq_pending)) { > + atomic_inc(&max->irq_pending); racy? > + wake_up_process(max->main_thread); > + } > + return IRQ_HANDLED; > +} > + > +/* If don't use RX IRQ, then need a thread to polling read */ > +static int max3110_read_thread(void *_max) > +{ > + struct uart_max3110 *max = _max; > + > + pr_info(PR_FMT "start read thread\n"); > + do { > + if (!max->mthread_up) > + max3110_con_receive(max); > + > + set_current_state(TASK_INTERRUPTIBLE); > + schedule_timeout(HZ / 20); Use schedule_timeout_interruptible() > + } while (!kthread_should_stop()); > + > + return 0; > +} > + > +static int serial_m3110_startup(struct uart_port *port) > +{ > + struct uart_max3110 *max = > + container_of(port, struct uart_max3110, port); > + u16 config = 0; > + int ret = 0; > + > + if (port->line != 0) > + pr_err(PR_FMT "uart port startup failed\n"); > + > + /* Firstly disable all IRQ and config it to 115200, 8n1 */ > + config = WC_TAG | WC_FIFO_ENABLE > + | WC_1_STOPBITS > + | WC_8BIT_WORD > + | WC_BAUD_DR2; > + ret = max3110_out(max, config); > + > + /* As we use thread to handle tx/rx, need set low latency */ > + port->state->port.tty->low_latency = 1; > + > + if (use_irq) { > + ret = request_irq(max->irq, serial_m3110_irq, > + IRQ_TYPE_EDGE_FALLING, "max3110", max); > + if (ret) > + return ret; > + > + /* Enable RX IRQ only */ > + config |= WC_RXA_IRQ_ENABLE; > + max3110_out(max, config); > + } else { > + /* if IRQ is disabled, start a read thread for input data */ > + max->read_thread = > + kthread_run(max3110_read_thread, max, "max3110_read"); Check for kthread_run() failure? > + } > + > + max->cur_conf = config; > + return 0; > +} > + > > ... > > +static int serial_m3110_probe(struct spi_device *spi) Should this be __init or __devinit? > +{ > + struct uart_max3110 *max; > + int ret; > + unsigned char *buffer; > + > + max = kzalloc(sizeof(*max), GFP_KERNEL); > + if (!max) > + return -ENOMEM; > + > + /* set spi info */ > + spi->mode = SPI_MODE_0; > + spi->bits_per_word = 16; > +#ifdef CONFIG_MAX3110_DESIGNWARE > + spi->controller_data = &spi_uart; > +#endif > + spi_setup(spi); > + > + max->clock = MAX3110_HIGH_CLK; > + max->port.type = PORT_MAX3110; > + max->port.fifosize = 2; /* only have 16b buffer */ > + max->port.ops = &serial_m3110_ops; > + max->port.line = 0; > + max->port.dev = &spi->dev; > + max->port.uartclk = 115200; > + > + max->spi = spi; > + max->name = spi->modalias; /* use spi name as the name */ > + max->irq = (u16)spi->irq; > + > + spin_lock_init(&max->lock); > + > + max->word_7bits = 0; > + max->parity = 0; > + max->baud = 0; > + > + max->cur_conf = 0; > + atomic_set(&max->irq_pending, 0); > + > + buffer = (unsigned char *)__get_free_page(GFP_KERNEL); > + if (!buffer) { > + ret = -ENOMEM; > + goto err_get_page; > + } > + max->con_xmit.buf = (unsigned char *)buffer; > + max->con_xmit.head = max->con_xmit.tail = 0; > + > + max->main_thread = kthread_run(max3110_main_thread, > + max, "max3110_main"); > + if (IS_ERR(max->main_thread)) { > + ret = PTR_ERR(max->main_thread); > + goto err_kthread; > + } > + > + pmax = max; > + /* Give membase a psudo value to pass serial_core's check */ > + max->port.membase = (void *)0xff110000; > + uart_add_one_port(&serial_m3110_reg, &max->port); > + > + return 0; > + > +err_kthread: > + free_page((unsigned long)buffer); > +err_get_page: > + pmax = NULL; > + kfree(max); > + return ret; > +} > + > > ... > > +static struct spi_driver uart_max3110_driver = { > + .driver = { > + .name = "spi_max3111", > + .bus = &spi_bus_type, > + .owner = THIS_MODULE, > + }, > + .probe = serial_m3110_probe, > + .remove = __devexit_p(max3110_remove), > + .suspend = serial_m3110_suspend, > + .resume = serial_m3110_resume, > +}; > + > +int __init serial_m3110_init(void) static? > +{ > + int ret = 0; > + > + ret = uart_register_driver(&serial_m3110_reg); > + if (ret) > + return ret; > + > + ret = spi_register_driver(&uart_max3110_driver); > + if (ret) > + uart_unregister_driver(&serial_m3110_reg); > + > + return ret; > +} > + > +void __exit serial_m3110_exit(void) static? > +{ > + spi_unregister_driver(&uart_max3110_driver); > + uart_unregister_driver(&serial_m3110_reg); > +} > + > > ... > -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
