LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Haavard Skinnemoen <hskinnemoen@atmel.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux@maxim.org.za, linux@bohmer.net, coldwell@redhat.com,
	marc.pignat@hevs.ch, david-b@pacbell.net,
	linux-kernel@vger.kernel.org, alan@lxorguk.ukuu.org.uk
Subject: Re: [PATCH -mm v4 7/9] atmel_serial: Add DMA support
Date: Mon, 28 Jan 2008 10:59:09 +0100	[thread overview]
Message-ID: <20080128105909.0fa8a431@dhcp-252-066.norway.atmel.com> (raw)
In-Reply-To: <20080126220200.368742e7.akpm@linux-foundation.org>

On Sat, 26 Jan 2008 22:02:00 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 24 Jan 2008 13:41:49 +0100 Haavard Skinnemoen <hskinnemoen@atmel.com> 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

  reply	other threads:[~2008-01-28  9:59 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-24 12:41 [PATCH -mm v4 0/9] atmel_serial cleanups and improvements Haavard Skinnemoen
2008-01-24 12:41 ` [PATCH -mm v4 1/9] MAINTAINERS: Add myself as maintainer of the atmel_serial driver Haavard Skinnemoen
2008-01-24 12:41   ` [PATCH -mm v4 2/9] atmel_serial: Clean up the code Haavard Skinnemoen
2008-01-24 12:41     ` [PATCH -mm v4 3/9] atmel_serial: Use cpu_relax() when busy-waiting Haavard Skinnemoen
2008-01-24 12:41       ` [PATCH -mm v4 4/9] atmel_serial: Use existing console options only if BRG is running Haavard Skinnemoen
2008-01-24 12:41         ` [PATCH -mm v4 5/9] atmel_serial: Fix bugs in probe() error path and remove() Haavard Skinnemoen
2008-01-24 12:41           ` [PATCH -mm v4 6/9] atmel_serial: Split the interrupt handler Haavard Skinnemoen
2008-01-24 12:41             ` [PATCH -mm v4 7/9] atmel_serial: Add DMA support Haavard Skinnemoen
2008-01-24 12:41               ` [PATCH -mm v4 8/9] atmel_serial: Use container_of instead of direct cast Haavard Skinnemoen
2008-01-24 12:41                 ` [PATCH -mm v4 9/9] atmel_serial: Show tty name in /proc/interrupts Haavard Skinnemoen
2008-01-27  6:02               ` [PATCH -mm v4 7/9] atmel_serial: Add DMA support Andrew Morton
2008-01-28  9:59                 ` Haavard Skinnemoen [this message]
2008-01-28 10:20                   ` Andrew Morton
2008-01-28 11:41                     ` Haavard Skinnemoen
2008-01-28 11:48                       ` [PATCH -mm] atmel_serial dma: Misc fixes and cleanups Haavard Skinnemoen
2008-01-24 13:32 ` [PATCH -mm v4 0/9] atmel_serial cleanups and improvements Marc Pignat
2008-01-24 15:07   ` Haavard Skinnemoen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080128105909.0fa8a431@dhcp-252-066.norway.atmel.com \
    --to=hskinnemoen@atmel.com \
    --cc=akpm@linux-foundation.org \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=coldwell@redhat.com \
    --cc=david-b@pacbell.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@bohmer.net \
    --cc=linux@maxim.org.za \
    --cc=marc.pignat@hevs.ch \
    --subject='Re: [PATCH -mm v4 7/9] atmel_serial: Add DMA support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).