Re: [PATCH] EG20T: Update PCH_UART driver to 2.6.36

[Alan Cox <alan@linux.intel.com>]

> +#ifdef CONFIG_PCH_DMA
> + #include <linux/dmaengine.h>
> + #include <linux/pch_dma.h>
> +#endif

The PCH DMA will have been included in the kernel anyway before this
merge so you can remove the ifdefs and include it regardless


> +#if !defined(PORT_PCH_256FIFO) || !defined(PORT_PCH_64FIFO)
> + #undef PORT_PCH_256FIFO
> + #undef PORT_PCH_64FIFO
> + #define PORT_PCH_256FIFO	(PORT_MAX_8250+1) /* PCH UART with
> 256 byte
> +						     FIFO */
> + #define PORT_PCH_64FIFO	(PORT_MAX_8250+2) /* PCH UART with
> 64 byte
> +						     FIFO */
> +#endif

FIFO config really all wants to be done at runtime.



> +static inline void wr(void __iomem *addr, unsigned int value)
> +{
> +#if PCH_UART_DMA_REG_BOUNDARY == 4
> +	iowrite32(value, addr);
> +#else
> +	iowrite8(value, (void *)addr);

Why the cast ?

> +#endif

Again you've got magic ifdefs with no explanation ?
> +}
> +
> +static void pch_uart_hal_request(struct pci_dev *pdev, int fifosize,
> +				 int base_baud)
> +{
> +	struct eg20t_port *priv = pci_get_drvdata(pdev);
> +
> +	priv->trigger_level = 1;
> +	priv->fcr = 0;
> +}
> +
> +static int __get_msr(struct eg20t_port *priv, void __iomem *base)
> +{

Please avoid __ names for functions

> +	unsigned int msr;
> +
> +	msr = rr(base + PCH_UART_MSR);
> +	priv->dmsr |= msr & PCH_UART_MSR_DELTA;
> +
> +	return (int)msr;

Why - if it is unsigned then why not return unsigned values ?


> +static int pch_uart_hal_enable_interrupt(struct eg20t_port *priv,
> +					 unsigned int flag)
> +{
> +	void __iomem *base;
> +	int ret;
> +
> +	base = priv->membase;
> +	ret = __pch_uart_hal_enable_interrupt(base, flag);
> +
> +	return ret;

Why not just

	"return __pch_uart_hal_enable_interrupt(priv->membase, flag);"

in fact why not just remove all these wrappers entirely ?


> +static int push_rx(struct eg20t_port *priv, const unsigned char *buf,
> +		   int size)
> +{
> +	struct uart_port *port;
> +	struct tty_struct *tty;
> +	int sz, i, j;
> +	int loop;
> +	int pushed;
> +
> +	port = &priv->port;
> +	tty = port->state->port.tty;

tty ports are refcounted

	tty = tty_port_tty_get(...)

and when finished tty_kref_put(tty);

Also tty may be NULL, if the port closed as you were doing this, so
check the return from tty_port_tty_get and act accordingly.


> +static int handle_error(struct eg20t_port *priv, int err)
> +{
> +	int ret = 0;
> +	return ret;
> +}

Why ?????

> +
> +#ifndef CONFIG_PCH_DMA

DMA should be runtime configuration - vendors need to build generic
kernels so need things like DMA switching to be done on load not on
compile




> +static void pch_uart_set_mctrl(struct uart_port *port, unsigned int
> mctrl) +{
> +}

Seems to be unimplemented ?


> +static void pch_uart_set_termios(struct uart_port *port,
> +				 struct ktermios *termios, struct
> ktermios *old) +{
> +	int baud;
> +	unsigned int parity, bits, stb;
> +	struct eg20t_port *priv;
> +	unsigned long flags;
> +
> +	priv = container_of(port, struct eg20t_port, port);
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS5:
> +		bits = PCH_UART_HAL_5BIT;
> +		break;
> +	case CS6:
> +		bits = PCH_UART_HAL_6BIT;
> +		break;
> +	case CS7:
> +		bits = PCH_UART_HAL_7BIT;
> +		break;
> +	default:		/* CS8 */
> +		bits = PCH_UART_HAL_8BIT;
> +		break;
> +	}
> +	if (termios->c_cflag & CSTOPB)
> +		stb = PCH_UART_HAL_STB2;
> +	else
> +		stb = PCH_UART_HAL_STB1;
> +
> +	if (termios->c_cflag & PARENB) {
> +		if (!(termios->c_cflag & PARODD))
> +			parity = PCH_UART_HAL_PARITY_ODD;
> +		else
> +			parity = PCH_UART_HAL_PARITY_EVEN;
> +
> +	} else {
> +		parity = PCH_UART_HAL_PARITY_NONE;
> +	}

If you don't support CPARMRK then you should clear that bit in
termios->c_flag so the caller knows it couldn't be set.


> +	baud = uart_get_baud_rate(port, termios, old, 0,
> port->uartclk / 16); +
> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	uart_update_timeout(port, termios->c_cflag, baud);
> +	pch_uart_hal_set_line(priv, baud, parity, bits, stb);

Baud rate should also be written back here

       /* Don't rewrite B0 */
        if (tty_termios_baud_rate(termios))
                tty_termios_encode_baud_rate(termios, baud, baud);

> +	txbuf = __get_free_page(GFP_KERNEL|GFP_DMA);
> +	if (!txbuf)
> +		goto init_port_error_end;
> +
> +	rxbuf = __get_free_page(GFP_KERNEL|GFP_DMA);
> +	if (!rxbuf)
> +		goto init_port_free_txbuf;

No - for bus masterable DMA buffers use the dma_alloc_coherent
interfaces with the correct device pointer, otherwise it will break on
a system with an IOMMU.

I assume the correct device in this case would be the DMA controller ?


The big thing I don't understand here is the locking model - what stops
interrupts and other things interfering with each other. For almost all
of the calls coming from the serial layer the port lock protects them
but I see no protection on the IRQ side at all ?