Re: [PATCHv5] dmaengine: Add support for BCM2835

[Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>]

On Fri, 2013-11-15 at 17:28 +0100, Florian Meier wrote:
> Add support for DMA controller of BCM2835 as used in the Raspberry Pi.
> Currently it only supports cyclic DMA.

trivial style notes:

> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
[]
> +/* DMA CS Control and Status bits */
> +#define BCM2835_DMA_ACTIVE	(1 << 0)
> +#define BCM2835_DMA_INT	(1 << 2)
> +#define BCM2835_DMA_ISPAUSED	(1 << 4)  /* Pause requested or not active */
> +#define BCM2835_DMA_ISHELD	(1 << 5)  /* Is held by DREQ flow control */
> +#define BCM2835_DMA_ERR	(1 << 8)
> +#define BCM2835_DMA_ABORT	(1 << 30) /* stop current CB, go to next, WO */
> +#define BCM2835_DMA_RESET	(1 << 31) /* WO, self clearing */

These could use the BIT macro

> +#define BCM2835_DMA_DATA_TYPE_S8 1
> +#define BCM2835_DMA_DATA_TYPE_S16 2
> +#define BCM2835_DMA_DATA_TYPE_S32 4
> +#define BCM2835_DMA_DATA_TYPE_S128 16

Are these sizeof(s8), sizeof(s16), sizeof(s32)?
Is there a S64's?  Are there any s128's?

> +static int bcm2835_dma_abort(void __iomem *dma_chan_base)
> +{
> +	unsigned long int cs;
> +	int rc = 0;

Perhaps better without using an automatic for rc
and using direct returns.

> +
> +	cs = readl(dma_chan_base + BCM2835_DMA_CS);
> +
> +	if (BCM2835_DMA_ACTIVE & cs) {

	if (!(cs & BCM2835_DMA_ACTIVE))
		return 0;

and avoid the indent level and use consistent
(cs & bit) style through the routine instead
of mixing (bit & cs) and (cs & bit)

> +		long int timeout = 10000;

Move timeout to start of routine.

> +		/* write 0 to the active bit - pause the DMA */
> +		writel(0, dma_chan_base + BCM2835_DMA_CS);
> +
> +		/* wait for any current AXI transfer to complete */
> +		while ((cs & BCM2835_DMA_ISPAUSED) && --timeout >= 0)
> +			cs = readl(dma_chan_base + BCM2835_DMA_CS);
> +
> +		if (cs & BCM2835_DMA_ISPAUSED) {
> +			/* we'll un-pause when we set of our next DMA */
> +			rc = -ETIMEDOUT;

	return -ETIMEDOUT;

and avoid another indentation level.

> +
> +		} else if (BCM2835_DMA_ACTIVE & cs) {
> +			/* terminate the control block chain */
> +			writel(0, dma_chan_base + BCM2835_DMA_NEXTCB);
> +
> +			/* abort the whole DMA */
> +			writel(BCM2835_DMA_ABORT | BCM2835_DMA_ACTIVE,
> +			       dma_chan_base + BCM2835_DMA_CS);
> +		}
> +	}
> +
> +	return rc;
> +}

So perhaps this becomes:

static int bcm2835_dma_abort(void __iomem *dma_chan_base)
{
	unsigned long int cs;
	long timeout;

	cs = readl(dma_chan_base + BCM2835_DMA_CS);
+       if (!(cs & BCM2835_DMA_ACTIVE))
		return 0;
	
	/* write 0 to the active bit - pause the DMA */
	writel(0, dma_chan_base + BCM2835_DMA_CS);

	/* wait for any current AXI transfer to complete */
	timeout = 10000;
	while ((cs & BCM2835_DMA_ISPAUSED) && --timeout >= 0)
		cs = readl(dma_chan_base + BCM2835_DMA_CS);

	/* we'll un-pause when we set of our next DMA */
	if (cs & BCM2835_DMA_ISPAUSED)
		return -ETIMEDOUT;

	if (!(cs & BCM2835_DMA_ACTIVE))
		return 0;

	/* terminate the control block chain */
	writel(0, dma_chan_base + BCM2835_DMA_NEXTCB);
	/* abort the whole DMA */
	writel(BCM2835_DMA_ABORT | BCM2835_DMA_ACTIVE,
	       dma_chan_base + BCM2835_DMA_CS);

	return 0;
}

[]

> +static size_t bcm2835_dma_desc_size_pos(struct bcm2835_desc *d, dma_addr_t addr)
> +{
> +	unsigned i;
> +	size_t size;

Please set size to 0 here and not in the for loop
> +
> +	for (size = i = 0; i < d->frames; i++) {

[]

> +static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
> +	struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
> +	size_t period_len, enum dma_transfer_direction direction,
> +	unsigned long flags, void *context)
> +{

> +	/* Allocate memory for control blocks */
> +	d->control_block_size = d->frames * sizeof(struct bcm2835_dma_cb);
> +	d->control_block_base = dma_alloc_coherent(chan->device->dev,
> +			d->control_block_size, &d->control_block_base_phys,
> +			GFP_NOWAIT);
> +
> +	if (!d->control_block_base) {
> +		kfree(d);
> +		dev_err(chan->device->dev,
> +				"%s: Memory allocation error\n", __func__);

Please use dma_zalloc_coherent and the OOM message
isn't necessary as dma_alloc_coherent has a generic
OOM message.

> +		return NULL;
> +	}
> +
> +	memset(d->control_block_base, 0, d->control_block_size);

unnecessary with dma_zalloc_coherent


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html