Date:	Thu, 11 Nov 2010 16:00:30 +0900
From:	"Tomoya MORINAGA" <tomoya-linux@....okisemi.com>
To:	"Alan Cox" <alan@...ux.intel.com>
Cc:	"Greg Kroah-Hartman" <gregkh@...e.de>,
	"Ben Dooks" <ben-linux@...ff.org>,
	"Kukjin Kim" <kgene.kim@...sung.com>,
	"Mike Frysinger" <vapier@...too.org>,
	"Feng Tang" <feng.tang@...el.com>,
	"Tobias Klauser" <tklauser@...tanz.ch>,
	"LKML" <linux-kernel@...r.kernel.org>, <yong.y.wang@...el.com>,
	<qi.wang@...el.com>, <kok.howg.ewe@...el.com>,
	<andrew.chih.howe.khor@...el.com>
Subject: Re: [PATCH] EG20T: Update PCH_UART driver to 2.6.36

On Tuesday, November 09, 2010 7:37 PM,  Alan Cox wrote:

> 
> > +#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

No.
PCH DMA have been already included at 2.6.36.
You can see "pch_dma.c" in drivers/dma.

> 
> 
> > +#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.

I will modify.

> 
> 
> 
> > +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 ?

I will delete the cast.

> 
> > +#endif
> 
> Again you've got magic ifdefs with no explanation ?

I will delete this.

> > +}
> > +
> > +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

I will modify.

> 
> > + 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 ?

I will modify.

> 
> 
> > +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 ?

I agree.
I will delete thease wrappers.

> 
> 
> > +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.

I have a question.
Though I can't find any accepted UART driver uses these functions(tty_port.../tty_kref...),
What's for ?

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

I will modify.

> > +
> > +#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
> 
Do you mean we souldn't use both below?
#ifndef CONFIG_PCH_DMA
#ifdef CONFIG_PCH_DMA

Many UART drivers accepted by upstream use the above macro, right ?

> 
> 
> 
> > +static void pch_uart_set_mctrl(struct uart_port *port, unsigned int
> > mctrl) +{
> > +}
> 
> Seems to be unimplemented ?

I will implement.

> 
> 
> > +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.

Sorry, I don't know CPARMRK.
What's CPARMRK ?

> 
> 
> > + 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);
> 

I will add above.


> > + 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 will use dma_alloc_coherent.

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

Sorry, I can't understand your saying "correct device".
What does "correct device" mean ?

> 
> 
> 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 ?
> 

Do you mean UART Rx/Tx interrupt Enable/Disable ?
If yes, the control hava been already implemented.
For example, in handle_tx (called from IRQ handler),
you can see pch_uart_hal_disable_interrupt().


---
Thanks,

Tomoya MORINAGA
OKI SEMICONDUCTOR CO., LTD.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists