Re: [PATCH v2 2/2] dmaengine: usb-dmac: Add Renesas USB DMA Controller (USB-DMAC) driver

[Vinod Koul <vinod.koul@intel.com>]

On Wed, Mar 11, 2015 at 02:39:54PM +0900, Yoshihiro Shimoda wrote:
> +static struct dma_async_tx_descriptor *
> +usb_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		       unsigned int sg_len, enum dma_transfer_direction dir,
> +		       unsigned long dma_flags, void *context)
> +{
> +	struct usb_dmac_chan *uchan = to_usb_dmac_chan(chan);
> +	struct usb_dmac_desc *desc;
> +
> +	/* A client driver will use dmaengine_prep_slave_single() */
> +	if (sg_len != 1) {
and why is that?

> +static enum dma_status usb_dmac_tx_status(struct dma_chan *chan,
> +					  dma_cookie_t cookie,
> +					  struct dma_tx_state *txstate)
> +{
> +	struct usb_dmac_chan *uchan = to_usb_dmac_chan(chan);
> +	enum dma_status status;
> +	unsigned long flags;
> +	unsigned int residue;
> +
> +	status = dma_cookie_status(chan, cookie, txstate);
> +	/* a client driver will get residue after DMA_COMPLETE */
can you explain this comment?

> +	if (!txstate)
> +		return status;
> +
> +	spin_lock_irqsave(&uchan->vc.lock, flags);
> +	residue = uchan->residue;
how is this related to the cookie for which request is made?
> +static void usb_dmac_chan_tasklet(unsigned long data)
> +{
> +	struct usb_dmac_chan *chan = (struct usb_dmac_chan *)data;
> +	struct usb_dmac_desc *desc = chan->desc;
> +
> +	spin_lock_irq(&chan->vc.lock);
> +
> +	/* This driver assumes a transfer finishes if desc is not NULL */
> +	if (desc) {
> +		chan->residue = usb_dmac_chan_get_last_residue(chan, desc);
why should this be for channel and not the descriptor which is completed, so
you overwrite this before user has chance to query ?

Looking at this, so you issue a transaction of 100bytes, will you get
completion even if 100bytes have not been transfered, If so how does dma
decide theat transfer is complete even if 100bytes are not transferred yet?

> +		vchan_cookie_complete(&desc->vd);
> +	}
> +
> +	/* (Re)start the next transfer if this driver has a next desc */
> +	usb_dmac_chan_start_xfer(chan);
why not do this in irq :)


> +#ifdef CONFIG_PM
> +static int usb_dmac_runtime_suspend(struct device *dev)
> +{
> +	struct usb_dmac *dmac = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < dmac->n_channels; ++i)
> +		usb_dmac_chan_halt(&dmac->channels[i]);
> +
> +	return 0;
> +}
> +
> +static int usb_dmac_runtime_resume(struct device *dev)
> +{
> +	struct usb_dmac *dmac = dev_get_drvdata(dev);
> +
> +	return usb_dmac_init(dmac);
> +}
> +#endif
> +
> +static const struct dev_pm_ops usb_dmac_pm = {
> +	SET_RUNTIME_PM_OPS(usb_dmac_runtime_suspend, usb_dmac_runtime_resume,
since you are using SET_RUNTIME_PM_OPS, you dont need to wrap this with
ifdef CONFIG_PM

> +
> +static void usb_dmac_chan_remove(struct usb_dmac_chan *uchan)
> +{
> +	tasklet_kill(&uchan->task);
that part is good, but how about disabling irq? you can still get insterrupt

-- 
~Vinod