Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Grant Likely <grant.likely@secretlab.ca>]

Hi Feng,

I'm really concerned with this driver.  I think it needs a lot more
work before it is ready for prime-time.  Again, you might want to
consider asking Greg to add it to the staging tree while you clean
stuff up and coordinate with the max3100 driver author.

Comments below.

On Thu, Mar 4, 2010 at 12:25 AM, Feng Tang <feng.tang@intel.com> 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 log:
>  since v5:
>        * Make the spi data buffer DMA safe, thanks to Mssakazu Mokuno,
>          David Brownell and Grant Likely for pointing the bug out
>  since v4:
>        * Add explanation why not using DMA
>        * Fix a bug in max3110_read_multi()
>  since v3:
>        * Remove the Designware controller related stuff
>        * Remove some initialization code which should be done in the platform
>          setup code
>        * Add a missing change for drivers/serial/Makefile
>  since v2:
>        * Address comments from Andrew Morton
>        * Use test/set_bit to avoid race condition for SMP/preempt case
>        * Fix bug related with endian order
>  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 6ff1d75c34d9465d4ffce076350473bfaf5404a5 Mon Sep 17 00:00:00 2001
> From: Feng Tang <feng.tang@intel.com>
> Date: Fri, 26 Feb 2010 11:37:29 +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
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Greg KH <greg@kroah.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Alan Cox <alan@linux.intel.com>
> ---
>  drivers/serial/Kconfig      |    8 +
>  drivers/serial/Makefile     |    1 +
>  drivers/serial/max3110.c    |  892 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/serial/max3110.h    |   61 +++

Why the separate .h file?  If it isn't used by multiple .c files, then
it belongs in the .c file itself.

>  include/linux/serial_core.h |    3 +
>  5 files changed, 965 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/serial/max3110.c
>  create mode 100644 drivers/serial/max3110.h
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 9ff47db..94aa282 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -688,6 +688,14 @@ config SERIAL_SA1100_CONSOLE
>          your boot loader (lilo or loadlin) about how to pass options to the
>          kernel at boot time.)
>
> +config SERIAL_MAX3110
> +       tristate "SPI UART driver for Max3110"
> +       depends on SPI_MASTER
> +       select SERIAL_CORE
> +       select SERIAL_CORE_CONSOLE
> +       help
> +         This is the UART protocol driver for MAX3110 device
> +
>  config SERIAL_BFIN
>        tristate "Blackfin serial port support"
>        depends on BLACKFIN
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index 5548fe7..b93d8a0 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SERIAL_S3C24A0) += s3c24a0.o
>  obj-$(CONFIG_SERIAL_S3C6400) += s3c6400.o
>  obj-$(CONFIG_SERIAL_S5PC100) += s3c6400.o
>  obj-$(CONFIG_SERIAL_MAX3100) += max3100.o
> +obj-$(CONFIG_SERIAL_MAX3110) += max3110.o

NAK.  I've looked at the max3100 driver, and the max3110 data sheet
specifically states that it is max3100 compatible.  I'm opposed to the
merging of this driver until you've at least made an attempt at
working with the max3100 driver author to produce a single driver.

>  obj-$(CONFIG_SERIAL_IP22_ZILOG) += ip22zilog.o
>  obj-$(CONFIG_SERIAL_MUX) += mux.o
>  obj-$(CONFIG_SERIAL_68328) += 68328serial.o
> diff --git a/drivers/serial/max3110.c b/drivers/serial/max3110.c
> new file mode 100644
> index 0000000..9b39914
> --- /dev/null
> +++ b/drivers/serial/max3110.c
> @@ -0,0 +1,892 @@
> +/*
> + * max3110.c - spi uart protocol driver for Maxim 3110
> + *
> + * Copyright (c) 2009, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +/*
> + * Note:
> + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
> + *    1 word. If SPI master controller doesn't support sclk frequency change,
> + *    then the char need be sent out one by one with some delay
> + *
> + * 2. Currently only RX availabe interrrupt is used
> + */
> +
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +
> +#include "max3110.h"
> +
> +#define PR_FMT "max3110: "
> +
> +struct uart_max3110 {
> +       struct uart_port port;
> +       struct spi_device *spi;
> +       char *name;
> +
> +       wait_queue_head_t wq;
> +       struct task_struct *main_thread;
> +       struct task_struct *read_thread;
> +       spinlock_t lock;
> +
> +       u32 baud;
> +       u16 cur_conf;
> +       u8 clock;
> +       u8 parity, word_7bits;
> +       u16 irq;
> +
> +       /* bit map for UART status */
> +       unsigned long flags;
> +#define M3110_CON_TX_NEED      0
> +#define M3110_UART_TX_NEED     1
> +#define M3110_IRQ_PENDING      2
> +       unsigned long mthread_up;
> +
> +       /* console buffer */
> +       struct circ_buf con_xmit;
> +};
> +
> +static struct uart_max3110 *pmax;

Don't do this.  To start, it limits the driver to only ever being able
to handle a single instance of the device.  Some drivers use a fixed
size table instead, but that isn't any better either.

Instead of relying on a global variable, store the struct uart_max3110
pointer inside the spi_device.dev.p private data pointer.

For the console portion use the struct console.data pointer.

> +
> +static int use_irq = 1;
> +module_param(use_irq, int, 0444);
> +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");

Why is this a module param?  Shouldn't this be determined on whether
or not the drivers pdata structure is populated with an irq number?

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html