Re: [PATCH v3] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

2011/10/18 Vinod Koul <vinod.koul@xxxxxxxxx>:
> On Mon, 2011-10-17 at 22:18 +0800, Barry Song wrote:
>> 2011/10/17 Vinod Koul <vinod.koul@xxxxxxxxx>:
>> >> +
>> >> + Â Â /* 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

you will find many drivers haven't error handlers in drivers/dma.....
we have missed while reviewing before.
people might copy from a same source, then make same mistakens.

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/