Messages in this thread | | | Date | Fri, 12 Nov 2010 10:07:28 +0000 | From | Alan Cox <> | Subject | Re: [PATCH v2] EG20T: Update PCH_UART driver to 2.6.36 |
| |
On Fri, 12 Nov 2010 11:55:42 +0900 Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com> wrote:
> Hi Alan, > > I have modified for your indications. > (As to details, please refer previous my response mail) > Please check below.
Thanks.. More comments below
I have looked at the locking and agree it looks right.
+#include <linux/interrupt.h> +#include <linux/io.h> + +#ifdef CONFIG_PCH_DMA +#include <linux/dmaengine.h> +#include <linux/pch_dma.h> +#endif
You do not need this ifdef. Even if you are not using the DMA feature it will still compile. DMA/non-DMA needs to be runtime. See below
+enum { + PCH_UART_HAL_INVALID_PARAM = EINVAL, + PCH_UART_HAL_NOT_INITIALIZED = 256, + PCH_UART_HAL_NOT_REQUESTED, + PCH_UART_HAL_BASE_NOT_SET, + PCH_UART_HAL_INVALID_BAUD, + PCH_UART_HAL_INVALID_PARITY, + PCH_UART_HAL_INVALID_WLS, + PCH_UART_HAL_INVALID_FIFO_CLR, + PCH_UART_HAL_INVALID_DMAMODE, + PCH_UART_HAL_INVALID_FIFOSIZE, + PCH_UART_HAL_INVALID_TRIGGER, + PCH_UART_HAL_INVALID_STB, + PCH_UART_HAL_READ_ERROR, +};
These should be replaced with ordinary Linux error codes in the functions.
+struct eg20t_port { + struct uart_port port; + int port_type; + void __iomem *membase; + resource_size_t mapbase; + unsigned int iobase; + struct pci_dev *pdev; + int fifo_size; + int base_baud; + int start_tx; + int start_rx; + int tx_empty; + int int_dis_flag; + int trigger; + int trigger_level; + struct pch_uart_buffer rxbuf; + unsigned int dmsr; + unsigned int fcr; +#ifdef CONFIG_PCH_DMA + struct dma_async_tx_descriptor *desc_tx; + struct dma_async_tx_descriptor *desc_rx; + struct pch_dma_slave param_tx; + struct pch_dma_slave param_rx; + struct dma_chan *chan_tx; + struct dma_chan *chan_rx; + struct scatterlist sg_tx; + struct scatterlist sg_rx; + int tx_dma_use; + void *rx_buf_virt; + dma_addr_t rx_buf_dma; +#endif +};
+static unsigned int get_msr(struct eg20t_port *priv, void __iomem *base) +{ + unsigned int msr = ioread8(base + PCH_UART_MSR); + priv->dmsr |= msr & PCH_UART_MSR_DELTA; + + return msr;
msr should be a u8 ? Is there a reason priv->dmsr is unsigned int ?
+static void pch_uart_hal_enable_interrupt(struct eg20t_port *priv, + unsigned int flag) +{ + unsigned int ier = ioread8(priv->membase + PCH_UART_IER); + ier |= flag & PCH_UART_IER_MASK; + iowrite8(ier, priv->membase + PCH_UART_IER); +}
Same for ier and fcr ?
+static int pch_uart_hal_read(struct eg20t_port *priv, unsigned char *buf, + int rx_size) +{ + int i; + unsigned int rbr, lsr; + + lsr = ioread8(priv->membase + PCH_UART_LSR); + for (i = 0, lsr = ioread8(priv->membase + PCH_UART_LSR); + i < rx_size && lsr & PCH_UART_LSR_DR; + lsr = ioread8(priv->membase + PCH_UART_LSR)) { + rbr = ioread8(priv->membase + PCH_UART_RBR); + buf[i++] = (unsigned char)rbr; + }
Same again here - which would also lose the cast
+static int pch_uart_hal_get_line_status(struct eg20t_port *priv) +{ + unsigned int lsr; + int ret; + + lsr = ioread8(priv->membase + PCH_UART_LSR); + ret = (int)lsr;
And here
+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 = tty_port_tty_get(&port->state->port); + if (!tty) { + pr_info("%s:tty is busy now", __func__); + return -EBUSY; + }
Don't log this. It is a perfectly normal situation - no pr_info needed (pr_debug if testing perhaps)
+ + for (pushed = 0, i = 0, loop = 1; (pushed < size) && loop; + pushed += sz, i++) { + sz = tty_insert_flip_string(tty, &buf[pushed], size - pushed); + for (j = 0; (j < 100000) && (sz == 0); j++) { + tty_flip_buffer_push(tty); + sz = tty_insert_flip_string(tty, &buf[pushed], + size - pushed); + } + if (sz == 0) + loop = 0; + }
This should not be needed. tty_insert_flip_string tries to insert as much as it can. Except when tty->low_latency is set, which requires the call is not from an interrupt handler the flip_buffer_push will queue data to the line discipline it will not make space.
+ room = tty_buffer_request_room(tty, size); + + if (room < size) + dev_warn(port->dev, "Rx overrun: dropping %u bytes\n", + size - room); + if (!room) + return room; + + for (i = 0; i < room; i++) { + tty_insert_flip_char(tty, ((u8 *)sg_virt(&priv->sg_rx))[i], + TTY_NORMAL); + }
You can replace all of that with a single call to tty_insert_flip_string ?
+ struct tty_struct *tty = tty_port_tty_get(&port->state->port); + if (!tty) { + pr_info("%s:tty is busy now", __func__);
Again don't log this.
+static void pch_uart_send_xchar(struct uart_port *port, char ch) +{ +}
??
+ ret = pch_uart_hal_set_fifo(priv, PCH_UART_HAL_DMA_MODE0, + fifo_size, priv->trigger);
Is this missing check intentional ? + + ret = request_irq(priv->port.irq, pch_uart_interrupt, IRQF_SHARED, + KBUILD_MODNAME, priv); + + if (ret < 0) + return ret;
+static int pch_uart_verify_port(struct uart_port *port, + struct serial_struct *serinfo) +{ + return 0; +}
As you don't support low latency you want to do
static int pch_uart_verify_port(struct uart_port *port, struct serial_struct *serinfo) { if (serinfo->flags & UPF_LOW_LATENCY) return -EOPNOTSUPP; return 0; }
(A few other drivers also miss this)
What you could also consider doing is
static int pch_uart_verify_port(struct uart_port *port, struct serial_struct *serinfo) { if (serinfo->flags & UPF_LOW_LATENCY) use_pio_modes; /* Avoid tty->low_latency being set as we do not support it */ serinfo->flags &= ~UPF_LOW_LATENCY; } else use_dma_modes; return 0; }
I think that would make "setserial /dev/ttyPCH0 low_latency" select PIO and the reverse "setserial /dev/ttyPCH0 ^low_latency" select DMA mode but it would need testing to make sure that it is all that is needed.
| |