Messages in this thread | | | Date | Tue, 9 Nov 2010 10:37:37 +0000 | From | Alan Cox <> | Subject | Re: [PATCH] EG20T: Update PCH_UART driver to 2.6.36 |
| |
> +#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 ?
| |