RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support

[Lu Jingchang-B35083]

> -----Original Message-----
> From: Lothar WaÃmann [mailto:LW@xxxxxxxxxxxxxxxxxxx]
> Sent: Monday, August 05, 2013 3:54 PM
> To: Lu Jingchang-B35083
> Cc: vinod.koul@xxxxxxxxx; Li Xiaochun-B41219; Wang Huan-B18965; linux-
> kernel@xxxxxxxxxxxxxxx; djbw@xxxxxx; shawn.guo@xxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support

> [...]
> > +	fsl_desc->n_tcds = sg_len;
> > +	for (i = 0; i < sg_len; i++) {
> > +		fsl_desc->tcd[i].vtcd = dma_pool_alloc(fsl_chan->tcd_pool,
> > +					GFP_ATOMIC, &fsl_desc->tcd[i].ptcd);
> >
> Why is this called with GFP_ATOMIC while fsl_desc above is being
> allocated with GFP_KERNEL?
[Lu Jingchang-B35083] 
 I will make these consistently. Thanks.
> 
> > +	for (ch = 0; ch < 32; ch++)
> > +		if (intr & (0x1 << ch))
> > +			break;
> > +
> What, if IRQs for multiple channels are pending at the same time?
> You could handle them all in one go without extra calls of the IRQ
> handler.
[Lu Jingchang-B35083] 
Yes, It will be more efficiently to handle all the irqs once. Thanks.
> 
> > +	writeb(EDMA_CINT_CINT(ch), base_addr + EDMA_CINT);
> > +
> > +	fsl_chan = &fsl_edma->chans[ch];
> > +
> > +	if (!fsl_chan->edesc->iscyclic) {
> > +		list_del(&fsl_chan->edesc->vdesc.node);
> >
> Ain't there any protection needed for the list operation?
[Lu Jingchang-B35083] 
Protection is needed indeed, I will fix this, thanks.
> 
> > +		clk_prepare_enable(fsl_edmamux->clk);
> >
> What, if this fails?
[Lu Jingchang-B35083] 
I will add code to check the return value. Thanks.
> 





Best Regards,
Jingchang
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_