[RFC 4/5] bcm2835-dma: add support for slave_sg transfer mode

[Stephen Warren]

On 10/28/2014 06:04 PM, Piotr Król wrote:
> Add bcm2835_dma_prep_slave_sg to suport bcm2835-mmc DMA transfers, what
> can improve throughput and system CPU load.

I expect that if I were to apply patches 1..3 but not patches 4 or 5,
the kernel would no longer work?

When applying a patch series, the kernel must be fully functional after
each and every patch (compile, boot, run OK). This simplifies tracking
down any problems that get introduced, since "git bisect" or the manual
equivalent won't hit commits where the code isn't even expected to work.

Also note that typically the patches in this series would be split up
and applied to different trees; 1,2 to the MMC tree, 3,5 to the bcm2835
tree, and 4 to the DMA tree. Each of those branches should be expected
to work on its own. Sometimes series with lots of dependencies can be
applied all at once, or dependencies merged in before dependent patches
are merged. However, life is simpler if this isn't required.

It would be good to have the original author of this driver (Florian
Meier <florian.meier at koalo.de>) and/or someone from Broadcom or the RPi
Foundation review the changes, since I know nothing about this HW module.

> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c

> - * Author:      Florian Meier <florian.meier at koalo.de>
> - *              Copyright 2013
> + * Author:	Florian Meier <florian.meier at koalo.de>
> + *		Gellert Weisz <gellert at raspberrypi.org>
...
> +MODULE_AUTHOR("Gellert Weisz <gellert at raspberrypi.org>");

Signed-off-by for Gellert's part of this patch is missing.

> @@ -140,9 +150,11 @@ static inline struct bcm2835_desc *to_bcm2835_dma_desc(
>  	return container_of(t, struct bcm2835_desc, vd.tx);
>  }
>  
> +
>  static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)

That's a gratuitous whitespace change. There are some others too.

> @@ -232,12 +250,15 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
>  static int bcm2835_dma_alloc_chan_resources(struct dma_chan *chan)
>  {
>  	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> +	int ret;
>  
>  	dev_dbg(c->vc.chan.device->dev,
>  			"Allocating DMA channel %d\n", c->ch);
>  
> -	return request_irq(c->irq_number,
> +	ret = request_irq(c->irq_number,
>  			bcm2835_dma_callback, 0, "DMA IRQ", c);
> +
> +	return ret;
>  }

That change seems pointless.

> @@ -415,9 +435,9 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
>  			control_block->info |= sync_type;
>  
>  		/* Setup DREQ channel */
> -		if (c->dreq != 0)
> +		if (c->cfg.slave_id != 0)
>  			control_block->info |=
> -				BCM2835_DMA_PER_MAP(c->dreq);
> +				BCM2835_DMA_PER_MAP(c->cfg.slave_id);

Are existing cyclic clients definitely setting cfg.slave_id rather than
(or as well as) dreq?