dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
Vinod Koul
maintainerPATCH v5 2/6 dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs
Quoted 18 lines · Wen He
Quoted 18 lines · Wen He
Quoted 12 lines
+/**
+ * structfsl_qdma_format- This is the struct holding describing compound
+ * descriptor format with qDMA.
+ * @status: This field which describes command status and
+ * enqueue status notification.
+ * @cfg: This field which describes frame offset and frame
+ * format.
+ * @addr_lo: This field which indicating the start of the buffer
+ * holding the compound descriptor of the lower 32-bits
+ * address in memory 40-bit address.
+ * @addr_hi: This field's the same as above field, but pointhigh
Quoted 4 lines
+ * 8-bits in memory 40-bit address.
+ * @__reserved1: Reserved field.
+ * @cfg8b_w1: This field which describes compound descriptor
+ * command queue origin produced by qDMA and
Quoted 7 lines
Quoted 5 lines
dynamic
you may remove 'This field which describes'... in above lines, give reader no information :)
Ok, so I remove 'this filed which describes' but keep the second half, right?
right
Quoted 10 lines
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ for (i = 0; i < queue_num; i++) { + if (queue_size[i] > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX + || queue_size[i] < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN) { + dev_err(&pdev->dev, "Get wrong queue-sizes.\n"); + return NULL;the indents here are bad for reading..
So I add a empty line in here?
No, indent of trailing condition is same as code block, that causes confusion, they should have different indent
Quoted 11 lines
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ /* + * Clear the command queue interrupt detect register for all queues. + */ + qdma_writel(fsl_qdma, 0xffffffff, block + FSL_QDMA_BCQIDR(0));bunch of writes with
0xffffffff, can you explain why? Also helps to make a macro for thisMaybe I missed that I should defined the value to the macro and add comment. Right?
that would help, but why are you writing 0xffffffff at all these places?
Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+static void fsl_qdma_issue_pending(struct dma_chan *chan) { + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan); + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue; + unsigned long flags; + + spin_lock_irqsave(&fsl_queue->queue_lock, flags);Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ spin_lock(&fsl_chan->vchan.lock); + if (vchan_issue_pending(&fsl_chan->vchan)) + fsl_qdma_enqueue_desc(fsl_chan); + spin_unlock(&fsl_chan->vchan.lock); + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);why do we need two locks, and since you are doing vchan why should you add your own lock on top
Quoted 3 lines
Yes, we need two locks.
As you know, the QDMA support multiple virtualized blocks for multi-core support. so we need to make sure that muliti-core access issues.
but why cant you use vchan lock for all?