Hi, 

quoted text
> > +static void fsl_dma_set_src(dma_addr_t addr,
> > +				struct dma_async_tx_descriptor 
> *tx, int index)
> > +{
> 
> What is index supposed to mean?  It's not used, or documented 
> anywhere than
> I can see.

I've also got more document here. Hi, Dan, could you give me some
explanation about this API? :)

quoted text
> 
> > +		else {
> > +			/* Run the link descriptor callback function */
> > +			if (desc->async_tx.callback) {
> > +				
> spin_unlock_irqrestore(&fsl_chan->desc_lock,
> > +								flags);
> > +				dev_dbg(fsl_chan->device->dev,
> > +					"link descriptor %p 
> callback\n", desc);
> > +				desc->async_tx.callback(
> > +						
> desc->async_tx.callback_param);
> > +				
> spin_lock_irqsave(&fsl_chan->desc_lock, flags);
> 
> After dropping the lock, you can no longer assume that your 
> iterator is
> still valid; you need to work off of the list head.
> 

list_for_each_entry_safe() is used here. I think the safe should be ok.
:P

quoted text
> > +	/* Find the first un-transfer desciptor */
> > +	for (ld_node = fsl_chan->ld_queue.next;
> > +		(ld_node != &fsl_chan->ld_queue)
> > +			&& (DMA_SUCCESS == dma_async_is_complete(
> > +					
> to_fsl_desc(ld_node)->async_tx.cookie,
> > +					fsl_chan->completed_cookie,
> > +					fsl_chan->common.cookie));
> > +		ld_node = ld_node->next);
> 
> Call fsl_dma_is_complete directly, don't waste time going through the
> virtual call.
> 
> And you have a recursive lock usage here; fsl_dma_is_complete calls
> fsl_chan_ld_cleanup, which acquires desc_lock, but you 
> already have it.
> 
> Couldn't you just call fsl_chan_ld_cleanup, and then check 
> what's at the
> head of the list?
> 

I'll split interrupt and poll here.

quoted text
> > +static irqreturn_t fsl_dma_do_interrupt(int irq, void *data)
> > +{
> > +	struct fsl_dma_device *fdev = (struct fsl_dma_device *)data;
> > +	struct fsl_dma_chan *fsl_chan = NULL;
> > +	u32 gsr;
> > +	int ch_nr;
> > +	struct dma_chan *int_chan;
> > +
> > +	gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? 
> in_be32(fdev->reg_base)
> > +			: in_le32(fdev->reg_base);
> > +	ch_nr = (32 - ffs(gsr)) / 8;
> > +
> > +	list_for_each_entry(int_chan, &fdev->common.channels, 
> device_node)
> > +		if (to_fsl_chan(int_chan)->id == ch_nr)
> > +			fsl_chan = to_fsl_chan(int_chan);
> 
> Why not use an array of channels?

The list is used in dma engine core file. And it's possible that there
are not all channel listed in dts and array.

quoted text
> > +
> > +	return fsl_chan ? fsl_dma_chan_do_interrupt(irq, 
> fsl_chan) : IRQ_NONE;
> > +
> > +}
> > +
> > +static void dma_do_tasklet(unsigned long unused)
> > +{
> > +	struct fsl_desc_sw *desc, *_desc;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&recy_ln_lock, flags);
> > +	list_for_each_entry_safe(desc, _desc, &recy_ln_chain, node) {
> > +		struct fsl_dma_chan *fsl_chan =
> > +					
> to_fsl_chan(desc->async_tx.chan);
> > +		/* Run the link descriptor callback function */
> > +		if (desc->async_tx.callback) {
> > +			spin_unlock_irqrestore(&recy_ln_lock, flags);
> > +			dev_dbg(fsl_chan->device->dev,
> > +				"dma_tasklet: link descriptor 
> %p callback\n",
> > +				desc);
> > +			desc->async_tx.callback(
> > +					desc->async_tx.callback_param);
> > +			spin_lock_irqsave(&recy_ln_lock, flags);
> > +		}
> > +		/* Recycle it! */
> > +		list_del(&desc->node);
> 
> You should remove it from the list before dropping the lock, 
> as otherwise
> something else could come along and remove it again.

All right!

quoted text
> 
> > +	if (strcmp(match->compatible, "fsl,mpc8540-dma-channel") == 0)
> > +		new_fsl_chan->feature = FSL_DMA_IP_86XX | 
> FSL_DMA_BIG_ENDIAN;
> 
> Shouldn't it be 85XX, to be consistent?
> 
> > +	else if (strcmp(match->compatible, 
> "fsl,mpc8349-dma-channel") == 0)
> > +		new_fsl_chan->feature = FSL_DMA_IP_83XX | 
> FSL_DMA_LITTLE_ENDIAN;
> 
> You could have the features be part of the match struct, so 
> you don't have
> to do extra strcmps.
> 

Can I use the data field of struct of_device_id?

quoted text
> 
> > +static struct of_device_id of_fsl_dma_ids[] = {
> > +	{ .compatible = "fsl,dma", },
> > +};
> 
> Why do we need to bind to the parent node at all?

Yes, the MPC83xx should get interrupt source from DMA device register.

quoted text
> 
> > +/* There is no asm instructions for 64 bits reverse loads 
> and stores */
> > +static u64 in_le64(const u64 __iomem *addr)
> > +{
> > +	return le64_to_cpu(in_be64(addr));
> > +}
> > +
> > +static void out_le64(u64 __iomem *addr, u64 val)
> > +{
> > +	out_be64(addr, cpu_to_le64(val));
> > +}
> > +#endif
> 
> You can use asm instructions for this, as such:

Aha, they are just copied from io.h.

quoted text
> 
> static u64 in_le64(const u64 __iomem *addr)
> {
> 	return ((u64)in_le32((u32 *)addr + 1) << 32) | 
> (in_le32((u32 *)addr));
> }
> 
> 
> static void out_le64(u64 __iomem *addr, u64 val)
> {
> 	out_le32((u32 *)addr, (u32)val);
> 	out_le32((u32 *)addr + 1, val >> 32);
> }
> 

And I agree with your other comments.

Thanks a lot!
- zw
-
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/