On Tue, Sep 23, 2014 at 05:10:02PM -0500, Andy Gross wrote:

<snip>

quoted

quoted

+			break;
+		default:
+			achan->slave.src_maxburst = 0;
+			achan->slave.dst_maxburst = 0;

Why clear these for error cases

With the return I shouldn't need to.  I'll fix this.

quoted

quoted

+			ret = -EINVAL;
+			break;
+		}
+
+		if (!ret)
+			writel(achan->blk_size,
+				adev->regs + HI_CRCI_CTL(achan->id, adev->ee));

and why do we write to HW on this. Shouldn't this be done when you program
the descriptor?

It could be deferred to later.  The main point is that I don't see the
slave_config happening every transaction.  I was only modifying it now instead
of making a register write for every transaction.

Well wouldn't it cause a problem if you write to hardware and a transaction
is already in progress.
So it makese sense to write every time at transaction start with latest
value programmed.

quoted

quoted

+static enum dma_status adm_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
+	struct dma_tx_state *txstate)
+{
+	struct adm_chan *achan = to_adm_chan(chan);
+	struct virt_dma_desc *vd;
+	enum dma_status ret;
+	unsigned long flags;
+	size_t residue = 0;
+
+	ret = dma_cookie_status(chan, cookie, txstate);
+

Last arg can be null to this, so before you do residue calcluation and block
interrupts would make sense to return from here if arg is NULL

Good catch.  Will fix.

quoted

quoted

+	spin_lock_irqsave(&achan->vc.lock, flags);
+
+	vd = vchan_find_desc(&achan->vc, cookie);
+	if (vd)
+		residue = container_of(vd, struct adm_async_desc, vd)->length;
+	else if (achan->curr_txd && achan->curr_txd->vd.tx.cookie == cookie)
+		residue = achan->curr_txd->length;

so this is current cookie, so you need to read from HW on current position

There is no way to get current position unfortunately without relying on
unreliable debug registers.

In that case would it make sense to return complete txn length?

-- 
~Vinod