Re: [PATCH] Add sc16is7x2 driver

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

On Fri, 03 Sep 2010 15:11:53 +0200
Manuel Stahl <manuel.stahl@iis.fraunhofer.de> wrote:

> This patch adds support for the sc16is7x2 chips.
> 

The patch was pretty badly wordwrapped.

>
> ...
>
> +#define WRITE_CMD(reg, ch) (REG_WRITE | (reg & 0xf) << 3 | (ch & 0x1) << 1)
> +#define READ_CMD(reg, ch)  (REG_READ  | (reg & 0xf) << 3 | (ch & 0x1) << 1)

It would be better to implement these as lower-case-named inlined C
functions.

> +
> +/* 16bit SPI command to read or write a register */
> +struct sc16is7x2_spi_reg {
> +	u8 cmd;
> +	u8 value;
> +} __attribute__ ((packed));

We have a __packed helper macro for this.

> +struct sc16is7x2_chip;
> +
> +/*
> + * Some registers must be read back to modify.
> + * To save time we cache them here in memory

That implies that the caches have some locking (the mutex, perhaps?). 
Documenting that here would be appropriate.

>
> ...
>
> +/* ******************************** SPI 
> ********************************* */
> +
> +
> +/*
> + * Reserve memory for command sequence
> + * @param cnt number of commands

This "@param" stuff is some form of markup which the kernel does not
use.  Please use standard kerneldoc markup throughout the driver, or
just plain old English-language comments.

> + */
> +static inline struct sc16is7x2_spi_reg *
> +sc16is7x2_alloc_spi_cmds(unsigned cnt)
> +{
> +	return kzalloc(sizeof(struct sc16is7x2_spi_reg)*cnt, GFP_KERNEL);

Use kcalloc() here.

> +}
> +
>
> ...
>
> +/*
> + * sc16is7x2_write_async - Write a new register content (async)
> + */
> +static inline int sc16is7x2_write_async(struct spi_device *spi, u8 reg, 
> u8 ch,
> +		u8 value)
> +{
> +	struct sc16is7x2_spi_reg *cmd = sc16is7x2_alloc_spi_cmds(1);
> +	if (!cmd)
> +		return -ENOMEM;
> +	sc16is7x2_add_write_cmd(cmd, reg, ch, value);
> +	return sc16is7x2_spi_async(spi, cmd, 1);
> +}

It's comventional to place a blank line between end-of-definitions and
start-of-code.  The driver has many instances of this.

> +/*
> + * sc16is7x2_write - Write a new register content (sync)
> + */
> +static int sc16is7x2_write(struct spi_device *spi, u8 reg, u8 ch, u8 val)
> +{
> +	u16 word = REG_WRITE | (reg & 0xf) << 3 | (ch & 0x3) << 1 | val << 8;
> +	return spi_write(spi, (const u8 *)&word, sizeof(word));
> +}

There's another.

> +/**
> + * sc16is7x2_read - Read back register content
> + * @spi: The SPI device
> + * @reg: Register offset
> + *
> + * Returns positive 8 bit value from device if successful or a
> + * negative value on error
> + */

ah-hah.  That's a correct kerneldoc comment.

> +static int sc16is7x2_read(struct spi_device *spi, unsigned reg, 
> unsigned ch)
> +{
> +	u8 cmd = REG_READ | (reg & 0xf) << 3 | (ch & 0x3) << 1;
> +	return spi_w8r8(spi, cmd);
> +}
> +
> +/* ******************************** UART 
> ********************************* */
> +
> +/* Uart divisor latch write */
> +static inline void sc16is7x2_add_dl_write_cmd(struct sc16is7x2_spi_reg 
> *cmd,
> +		u8 ch, int value)
> +{
> +	sc16is7x2_add_write_cmd(&cmd[0], UART_DLL, ch, value & 0xff);
> +	sc16is7x2_add_write_cmd(&cmd[1], UART_DLM, ch, value >> 8 & 0xff);
> +}

Probably the driver doesn't need any explicit "inline" usage at all -
modern gcc's work all that out for themselves.  And if gcc disagreed
with your inline directive, it will just ignore it.

> +static unsigned int sc16is7x2_tx_empty(struct uart_port *port)
> +{
> +	struct sc16is7x2_channel *chan =
> +			container_of(port, struct sc16is7x2_channel, uart);
> +	struct sc16is7x2_chip *ts = chan->chip;
> +	unsigned lsr;
> +
> +	dev_dbg(&ts->spi->dev, "%s\n", __func__);
> +
> +	mutex_lock(&chan->lock);
> +	lsr = chan->lsr;
> +	mutex_unlock(&chan->lock);

It's strange to put locking around a single atomic read.  What are we
trying to do here?

> +	return lsr & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> +}
> +
>
> ...
>
> +static void sc16is7x2_break_ctl(struct uart_port *port, int break_state)
> +{
> +	struct sc16is7x2_channel *chan =
> +			container_of(port, struct sc16is7x2_channel, uart);
> +	struct sc16is7x2_chip *ts = chan->chip;
> +	unsigned ch = port->line & 0x01;
> +	unsigned long flags;
> +
> +	dev_dbg(&ts->spi->dev, "%s\n", __func__);
> +
> +	spin_lock_irqsave(&chan->uart.lock, flags);
> +	if (break_state == -1)
> +		chan->lcr |= UART_LCR_SBC;
> +	else
> +		chan->lcr &= ~UART_LCR_SBC;
> +	spin_unlock_irqrestore(&chan->uart.lock, flags);

hm.  Above we use the mutex to protect char->lcr but here we're using a
spinlock.

> +	sc16is7x2_write_async(ts->spi, UART_LCR, ch, chan->lcr);
> +}
> +
>
> ...
>
> +#define MIN(a, b) ((a < b) ? (a) : (b))

Nope.  Use the min() from include/linux/kernel.h.

>
> ...
>
> +static irqreturn_t sc16is7x2_interrupt(int irq, void *dev_id)
> +{
> +	struct sc16is7x2_chip *ts = dev_id;
> +
> +	dev_dbg(&ts->spi->dev, "%s\n", __func__);
> +
> +	if (!ts->force_end_work && !work_pending(&ts->work) &&
> +	    !freezing(current) && !ts->suspending)
> +		queue_work(ts->workqueue, &ts->work);
> +
> +	return IRQ_HANDLED;
> +}

The kernel has infrastructure for "threaded irqs" nowadays.  What this
driver is doing basically reimplements that concept.  Did you consider
using threaded IRQs directly?

>
> ...
>