[v5,2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs

[Vinod Koul <vkoul@kernel.org>]

On 29-05-18, 09:59, Wen He wrote:
> > On 25-05-18, 19:19, Wen He wrote:
> > 
> > > +/**
> > > + * struct fsl_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 point
> > high
> > > + *			    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
> > 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

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

> > > +	/*
> > > +	 * 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 this
> > 
> 
> Maybe 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?

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