On Sat, May 28, 2016 at 05:47:13PM +0800, Keguang Zhang wrote:
> +/* Loongson 1 DMA Register Definitions */
> +#define DMA_CTRL 0x0
> +
> +/* DMA Control Register Bits */
> +#define DMA_STOP BIT(4)
> +#define DMA_START BIT(3)
> +#define ASK_VALID BIT(2)
> +
> +#define DMA_ADDR_MASK (0xffffffc0)
These should be namespaced properly. Having generic one like DMA_STOP
DMA_ADDR_MASK makes it prone to conflicts with common symbols!
> +bool ls1x_dma_filter(struct dma_chan *chan, void *param)
> +{
> + struct ls1x_dma_chan *dma_chan = to_ls1x_dma_chan(chan);
> + unsigned int chan_id = *(unsigned int *)param;
> +
> + if (chan_id == dma_chan->id)
> + return true;
> + else
> + return false;
> +}
considering this is a new driver do you need a filter? Also where are the
bindings for this driver?
> +static struct ls1x_dma_desc *ls1x_dma_alloc_desc(struct ls1x_dma_chan *dma_chan,
> + int sg_len)
if you splitting up please ensure preceding line is < 80 char, this looks
quite bad
> +
> + switch (direction) {
> + case DMA_MEM_TO_DEV:
> + dev_addr = config->dst_addr;
> + bus_width = config->dst_addr_width;
> + cmd = DMA_RAM2DEV | DMA_INT;
> + break;
empty line here
> + case DMA_DEV_TO_MEM:
> + dev_addr = config->src_addr;
> + bus_width = config->src_addr_width;
> + cmd = DMA_INT;
> + break;
here too
> +static size_t ls1x_dma_desc_residue(struct ls1x_dma_desc *dma_desc,
> + unsigned int next_sg)
> +{
> + struct ls1x_dma_chan *dma_chan = dma_desc->chan;
> + struct dma_slave_config *config = &dma_chan->config;
> + unsigned int i, bus_width, bytes = 0;
> +
> + if (dma_desc->dir == DMA_MEM_TO_DEV)
> + bus_width = config->dst_addr_width;
> + else
> + bus_width = config->src_addr_width;
dma slave config can be reprogrammed for subsequent transaction, you should
read this from descriptor and not channel
> +
> + for (i = next_sg; i < dma_desc->nr_descs; i++)
> + bytes += dma_desc->desc[i]->length * bus_width;
> +
> + return bytes;
Okay how does this tell me "residue"
For not yet processed this tells me all descriptor size, why would I want
that?
> +}
> +
> +static enum dma_status ls1x_dma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct ls1x_dma_chan *dma_chan = to_ls1x_dma_chan(chan);
> + struct ls1x_dma_desc *dma_desc = dma_chan->dma_desc;
> + struct virt_dma_desc *vdesc;
> + enum dma_status status;
> + unsigned int residue = 0;
> + unsigned long flags;
> +
> + status = dma_cookie_status(chan, cookie, txstate);
> + if ((status == DMA_COMPLETE) || !txstate)
> + return status;
> +
> + spin_lock_irqsave(&dma_chan->vchan.lock, flags);
> +
> + vdesc = vchan_find_desc(&dma_chan->vchan, cookie);
> + if (vdesc)
> + /* not yet processed */
> + residue = ls1x_dma_desc_residue(to_ls1x_dma_desc(vdesc), 0);
> + else if (cookie == dma_chan->dma_desc->vdesc.tx.cookie)
> + /* in progress */
> + residue = ls1x_dma_desc_residue(dma_desc, dma_desc->nr_done);
> + else
> + residue = 0;
> +
> + spin_unlock_irqrestore(&dma_chan->vchan.lock, flags);
> +
> + dma_set_residue(txstate, residue);
> +
> + return status;
> +}
> +static int ls1x_dma_remove(struct platform_device *pdev)
> +{
> + struct ls1x_dma *dma = platform_get_drvdata(pdev);
> +
> + dma_async_device_unregister(&dma->dma_dev);
> + clk_disable_unprepare(dma->clk);
You are using devm_request_irq(), now how is the irq quisced at this point
and how do you ensure all your tasklets have finished processing!
> +MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Loongson1 DMA driver");
> +MODULE_LICENSE("GPL");
No ALIAS?
--
~Vinod
