Re: [PATCH -mm v4 7/9] atmel_serial: Add DMA support

[Haavard Skinnemoen]

On Sat, 26 Jan 2008 22:02:00 -0800
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Thu, 24 Jan 2008 13:41:49 +0100 Haavard Skinnemoen <hskinnemoen@xxxxxxxxx> wrote:
> > +#define PDC_RX_BUF(port)	&(port)->pdc_rx[(port)->pdc_rx_idx]
> > +#define PDC_RX_SWITCH(port)	(port)->pdc_rx_idx = !(port)->pdc_rx_idx
> 
> These macros refer to their arg more than one time and hance are dangerous.
> Think of the effects of PDC_RX_BUF(foo++).

They're also unused...I'll remove them.

> Generally, please don't use macros for anything which can be coded as a
> regular C function.

Agree.

> > +/*
> > + * Called from tasklet with ENDTX and TXBUFE interrupts disabled.
> > + */
> > +static void atmel_tx_dma(struct uart_port *port)
> > +{
> > +	struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
> > +	struct circ_buf *xmit = &port->info->xmit;
> > +	struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
> > +	int count;
> > +
> > +	xmit->tail += pdc->ofs;
> > +	if (xmit->tail >= SERIAL_XMIT_SIZE)
> > +		xmit->tail -= SERIAL_XMIT_SIZE;
> 
> Maybe this should be a uart_circ_whatever() helper rather than open-coded.

Hmm. uart_circ_get_multiple() or something?

Also, we should probably be using UART_XMIT_SIZE here, not
SERIAL_XMIT_SIZE. Why do we have two such definitions (which both
happen to be PAGE_SIZE)?

> > +	port->icount.tx += pdc->ofs;
> > +	pdc->ofs = 0;
> > +
> > +	if (!uart_circ_empty(xmit)) {
> 
> ho-hum.  The generic uart buffer-handling code does ringbuffers the wrong
> way.  Maybe it has to handle non-power-of-two buffer sizes.

Hmm...I don't understand. What does it do wrong?

> > +		/* more to transmit - setup next transfer */
> > +
> > +		/* disable PDC transmit */
> > +		UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
> > +		dma_sync_single_for_device(port->dev,
> > +					   pdc->dma_addr,
> > +					   pdc->dma_size,
> > +					   DMA_TO_DEVICE);
> > +
> > +		if (xmit->tail < xmit->head)
> > +			count = xmit->head - xmit->tail;
> > +		else
> > +			count = SERIAL_XMIT_SIZE - xmit->tail;
> 
> Doesn't uart_circ_chars_pending() do this?

Hmm...no. I think we really want something CIRC_CNT_TO_END()-ish.

> All those uart_circ_*() macros reference their arg more than once and ... 
> you know the deal.

Yeah. Would you like a patch that inline-ifies <linux/circ.h>?

> > +		if (head >= pdc->dma_size)
> > +			head = pdc->dma_size;
> 
> min()?

Absolutely.

> > +		if (likely(head != tail)) {
> > +			dma_sync_single_for_cpu(port->dev, pdc->dma_addr,
> > +					pdc->dma_size, DMA_FROM_DEVICE);
> > +
> > +			count = head - tail;
> 
> No wraparound issues here?

No. "head" is the current offset that the DMA controller is reading
from, while "tail" is the offset we stopped at the last time around.
head will only wrap around when we recycle the DMA buffer, and when
that happens, we explicitly set tail to 0.

Haavard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/