> -----Original Message----- > From: Vinod [mailto:vkoul@xxxxxxxxxx] > Sent: 2018年5月29日 18:20 > To: Wen He <wen.he_1@xxxxxxx> > Cc: dmaengine@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; Jiafei Pan > <jiafei.pan@xxxxxxx>; Jiaheng Fan <jiaheng.fan@xxxxxxx> > Subject: Re: [v5 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for > Layerscape SoCs > > 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 > Got it. > > > > + /* > > > > + * 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? > This value is from the datasheet. Should I write comments to here? > > > > +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? > We can't only use vchan lock for all. otherwise enqueue action will be interrupted. Best Regards, Wen > -- > ~Vinod ?韬{.n?????%??檩??w?{.n???Ё?撖)?骅w* jg????????G??⒏ ⒎?:+v???????? ????＂??????
