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
> 
> No.
> PCH DMA have been already included at 2.6.36.
> You can see "pch_dma.c" in drivers/dma.

So that means pch_dma is always in the kernel source tree as of 2.6.36
- so the ifdef can go away ?

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

We are gradually going through converting them all and I'm trying to
make sure no new ones creep in that don't use tty_port_tty_get.

Basically it does this

	tty_port_tty_get()

will return either a tty pointer with a reference (ie the tty will not
be deleted while you hold it), or will return NULL if the physical port
is no longer connected to a tty through close/hangup. It does all the
locking internally to ensure this is done safely.

	tty_kref_put()

drops the reference it obtained and after that point if the tty
reference count hits zero it may be deleted.

It ensures this cannot occur

		CPU1			CPU2

interrupt:
	tty = port->port.tty
					close
					port->port.tty = NULL
					kfree tty
	write to tty



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

As far as possible the selection should be done at runtime, especially
for PC class devices. Linux distributors want to build a single kernel
for everything so having it runtime configured helps a lot.


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

Sorry my fault. I mean CMSPAR

CMSPAR if set selects mark/space parity. If your hardware doesn't
support this then do

	termios->c_cflag &= ~CMSPAR;


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

dma_alloc_coherent takes a device pointer which should be the device
which is doing the DMA.

> 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().

I will take another look next time the patch is submitted. However if
you are relying on disabling an IRQ source to avoid the interrupt
running in parallel with another routine remember on a multithreaded
CPU and on some hardware that it is possible for this to occur


		CPU1			CPU2
	Interrupt arrives
					Disable interrupt
	Interrupt routine runs		Other code runs


Alan