Messages in this thread | | | Subject | Re: [PATCH v3] dmaengine: add CSR SiRFprimaII DMAC driver | From | Vinod Koul <> | Date | Mon, 17 Oct 2011 21:59:05 +0530 |
| |
On Mon, 2011-10-17 at 22:18 +0800, Barry Song wrote: > 2011/10/17 Vinod Koul <vinod.koul@intel.com>: > >> + > >> + /* Start the DMA transfer */ > >> + writel_relaxed(sdesc->width, sdma->base + SIRFSOC_DMA_WIDTH_0 + cid * 4); > >> + writel_relaxed(cid | (schan->mode << SIRFSOC_DMA_MODE_CTRL_BIT) | > >> + (sdesc->dir << SIRFSOC_DMA_DIR_CTRL_BIT), > >> + sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_CTRL); > >> + writel_relaxed(sdesc->xlen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_XLEN); > >> + writel_relaxed(sdesc->ylen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_YLEN); > >> + writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_INT_EN) | (1 << cid), > >> + sdma->base + SIRFSOC_DMA_INT_EN); > >> + writel(sdesc->addr >> 2, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_ADDR); > >> + > >> + if (sdesc->cyclic) { > >> + writel((1 << cid) | 1 << (cid + 16) | > >> + readl_relaxed(sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL), > >> + sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL); > >> + schan->happened_cyclic = schan->completed_cyclic = 0; > >> + } > > any reason why we have mixed use of writel_relaxes and writel? > > Shouldn't all the DMA register writes be done only using writel? > > Arnd comment this in v2. Yes he certainly did, but for maintainability, it would really help to explain what you are doing here.
> > > >> +} > >> + > >> + > >> +static struct dma_async_tx_descriptor *sirfsoc_dma_prep_interleaved( > >> + struct dma_chan *chan, struct dma_interleaved_template *xt) > >> +{ > >> + struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan); > >> + struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan); > >> + struct sirfsoc_dma_desc *sdesc = NULL; > >> + unsigned long iflags; > >> + int ret; > >> + > >> + if ((xt->dir != MEM_TO_DEV) || (xt->dir != DEV_TO_MEM)) { > >> + ret = -EINVAL; > >> + goto err_dir; > >> + } > >> + > >> + /* Get free descriptor */ > >> + spin_lock_irqsave(&schan->lock, iflags); > >> + if (!list_empty(&schan->free)) { > >> + sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc, > >> + node); > >> + list_del(&sdesc->node); > >> + } > >> + spin_unlock_irqrestore(&schan->lock, iflags); > >> + > >> + if (!sdesc) { > >> + /* try to free completed descriptors */ > >> + sirfsoc_dma_process_completed(sdma); > >> + ret = 0; > >> + goto no_desc; > >> + } > >> + > >> + /* Place descriptor in prepared list */ > >> + spin_lock_irqsave(&schan->lock, iflags); > >> + if ((xt->frame_size == 1) && (xt->numf > 0)) { > > what does this mean? > >> + sdesc->cyclic = 0; > >> + sdesc->xlen = xt->sgl[0].size / 4; > >> + sdesc->width = (xt->sgl[0].size + xt->sgl[0].icg) / 4; > > whats so magical about 4? > > the xlen and dma_width is in 4 byes boundary. xlen =1 means 4 bytes > will be transferred every line. I meant two things - why magical number 4, CSRXXX_DMA_WITH would have been much better - same for frame_size as 1
> > the error path handling in this function is totally non existent!! > > sorry. i must have maken these idiot mistakens due to copying from the > probe of drivers/dma/mpc512x_dma.c. > > >> + > >> +static int __devexit sirfsoc_dma_remove(struct platform_device *op) > >> +{ > >> + struct device *dev = &op->dev; > >> + struct sirfsoc_dma *sdma = dev_get_drvdata(dev); > >> + > >> + dma_async_device_unregister(&sdma->dma); > >> + devm_free_irq(dev, sdma->irq, sdma); > >> + irq_dispose_mapping(sdma->irq); > > who will free mem allocated in probe? > > also due to copying from drivers/dma/mpc512x_dma.c. sorry. Thanks, I will put these in my todo
-- ~Vinod
| |