Re: [alsa-devel] [PATCHv7] dmaengine: Add support for BCM2835

On Sun, 2013-11-17 at 16:39 +0100, Florian Meier wrote:

Add support for DMA controller of BCM2835 as used in the Raspberry Pi. Currently it only supports cyclic DMA.

Few comments below.

+++ b/drivers/dma/bcm2835-dma.c @@ -0,0 +1,736 @@

+static int bcm2835_dma_abort(void __iomem *chan_base) +{

• unsigned long int cs;
• long int timeout = 10000;
• cs = readl(chan_base + BCM2835_DMA_CS);
• if (!(cs & BCM2835_DMA_ACTIVE))

• return 0;
  

• /* Write 0 to the active bit - Pause the DMA */
• writel(0, chan_base + BCM2835_DMA_CS);
• /* Wait for any current AXI transfer to complete */
• while ((cs & BCM2835_DMA_ISPAUSED) && --timeout >= 0)

• cs = readl(chan_base + BCM2835_DMA_CS);
  

I actually don't see timeout here. timeout means counter in your case. Might be better to have something like

while (readl(...) & BCM2835_DMA_ISPAUSED && --timeout) cpu_relax();

?

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

if (!timeout)

• return -ETIMEDOUT;
  

• if (!(cs & BCM2835_DMA_ACTIVE))

• return 0;
  

Duplicate code. Perhaps static inline bool is_chan_not_active(unsigned long cs) { return !(cs & BCM2835_DMA_ACTIVE); }

[]

+static irqreturn_t bcm2835_dma_callback(int irq, void *data) +{

[]

• writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);

Since it's duplicate code, perhaps

static inline void set_chan_active(void __iomem *base) { writel(BCM2835_DMA_ACTIVE, base + BCM2835_DMA_CS); }

[]

+static size_t bcm2835_dma_desc_size_pos(struct bcm2835_desc *d, dma_addr_t addr) +{

• unsigned i;

In some cases you use 'unsigned long int' for 'unsigned long', for example, but here 'unsigned' instead of 'unsigned int'. Please, align style with certain choice.

[]

+static enum dma_status bcm2835_dma_tx_status(struct dma_chan *chan,

• dma_cookie_t cookie, struct dma_tx_state *txstate)

+{

[]

• } else {

• txstate->residue = 0;
  

Useless assignment since dmaengine will do this for you in dma_cookie_status.

[]

+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)

+{

[]

• /* Grab configuration */
• if (direction == DMA_DEV_TO_MEM) {

• dev_addr = c->cfg.src_addr;
  

• dev_width = c->cfg.src_addr_width;
  

• sync_type = BCM2835_DMA_S_DREQ;
  

• } else if (direction == DMA_MEM_TO_DEV) {

• dev_addr = c->cfg.dst_addr;
  

• dev_width = c->cfg.dst_addr_width;
  

• sync_type = BCM2835_DMA_D_DREQ;
  

• } else {

• dev_err(chan->device->dev, "%s: bad direction?\n", __func__);
  

• return NULL;
  

• }

You might use following as well

if (!is_slave_direction) { dev_err(...); return NULL; }

if (direction == DMA_DEV_TO_MEM) { ... } else { ... }

?

At least it will be aligned with what you have further in this function.

• /* Bus width translates to the element size (ES) */
• switch (dev_width) {
• case DMA_SLAVE_BUSWIDTH_4_BYTES:

• es = BCM2835_DMA_DATA_TYPE_S32;
  

• break;
  

• default:

• return NULL;
  

• }

So, you use switch-case on hope to extend it later, correct?

[]

+static int bcm2835_dma_terminate_all(struct bcm2835_chan *c) +{

• struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device);
• unsigned long flags;
• int timeout = 1000;
• LIST_HEAD(head);

[]

• if (c->desc) {

• c->desc = NULL;
  

• bcm2835_dma_abort(c->chan_base);
  

• /* Wait for stopping */
  

• while (timeout > 0) {
  

• 	timeout--;
  

while (--timeout)

• 	if (!(readl(c->chan_base + BCM2835_DMA_CS) &
  

• 				BCM2835_DMA_ACTIVE))
  

• 		break;
  

• 	cpu_relax();
  

• }
  

• if (timeout <= 0)
  

if (!timeout)

[]

+static struct dma_chan *bcm2835_dma_xlate(struct of_phandle_args *spec,

• 			   struct of_dma *ofdma)
  

+{

• struct bcm2835_dmadev *d = ofdma->of_dma_data;
• struct dma_chan *chan, *candidate;

+retry:

• candidate = NULL;
• /* Walk the list of channels registered with the current instance and

• * find one that is currently unused */
  

• list_for_each_entry(chan, &d->ddev.channels, device_node)

• if (chan->client_count == 0) {
  

• 	candidate = chan;
  

• 	break;
  

• }
  

• if (!candidate)

• return NULL;
  

• /* dma_get_slave_channel will return NULL if we lost a race between

• * the lookup and the reservation */
  

• chan = dma_get_slave_channel(candidate);
• if (chan) {

Perhaps

if (!chan) goto retry;

• struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
  

• /* Set DREQ from param */
  

• c->dreq = spec->args[0];
  

• return chan;
  

• }
• goto retry;

+}

+static int bcm2835_dma_probe(struct platform_device *pdev) +{

• struct bcm2835_dmadev *od;
• struct resource *res;
• void __iomem *base;
• int rc;
• int i;
• int irq;
• uint32_t chans_available;

Why uint32_t?

[]

• if (pdev->dev.of_node) {

Perhaps

if (!...of_node) { ... return -EINVAL; }

• /* Request DMA channel mask from device tree */
  

• if (of_property_read_u32(pdev->dev.of_node,
  

• 		"brcm,dma-channel-mask",
  

• 		&chans_available)) {
  

• 	dev_err(&pdev->dev, "Failed to get channel mask\n");
  

• 	bcm2835_dma_free(od);
  

• 	return -EINVAL;
  

• }
  

• } else {

• dev_err(&pdev->dev, "Failed to get channel mask. No device tree.\n");
  

• bcm2835_dma_free(od);
  

• return -EINVAL;
  

• }

• dev_dbg(&pdev->dev, "Initialized %i DMA channels\n", i);
• if (pdev->dev.of_node) {

Does it make sense?

• /* Device-tree DMA controller registration */
  

• rc = of_dma_controller_register(pdev->dev.of_node,
  

• 		bcm2835_dma_xlate, od);
  

• if (rc) {
  

• 	dev_err(&pdev->dev, "Failed to register DMA controller\n");
  

• 	bcm2835_dma_free(od);
  

• 	return rc;
  

goto err_no_dma;

• }
  

• }
• rc = dma_async_device_register(&od->ddev);
• if (rc) {

• dev_err(&pdev->dev,
  

• 	"Failed to register slave DMA engine device: %d\n", rc);
  

• bcm2835_dma_free(od);
  

• return rc;
  

goto err_no_dma;

• }
• dev_dbg(&pdev->dev, "Load BCM2835 DMA engine driver\n");

• return rc;

return 0;

err_no_dma: bcm2835_dma_free(od); return rc;

?