Re: [PATCH V2] dma: mmp_pdma: support for getting residual bytes

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

On Thu, Jul 11, 2013 at 10:54:29AM +0800, Xiang Wang wrote:
> >> +static u32 mmp_pdma_get_bytes_residue(struct mmp_pdma_chan *chan)
> >> +{
> >> +     u32 reg, orig_pos, cur_pos, ddadr, residue = 0;
> >> +     bool running_desc_found = false;
> >> +     struct mmp_pdma_desc_sw *desc_sw;
> >> +
> >> +     /*
> >> +      * When a phy channel is unavailable, maybe it has been freed, return
> >> +      * last stored value for safe.
> >> +      */
> >> +     if (!chan->phy)
> >> +             return chan->bytes_residue;
> >> +
> >> +     reg = (chan->phy->idx << 4) + DDADR;
> >> +     ddadr = readl_relaxed(chan->phy->base + reg);
> >> +
> >> +     /* iterate over all descriptors to sum up the number of pending bytes */
> > and why?
> >
> > Residue does not mean sum of all pending bytes across all descriptors submitted
> > You need to find the residue of current descriptor only and return
> >
> Here are my thoughts:
> We tell dma engine to transmit x bytes for us and it creates n
> descriptors to transmit which is transparent to us.
> So when we want to find out how many bytes are left, dma engine should
> sum up all pending bytes across all descriptors.
> What's your opinion?
but you may have multiple transactions submmited. Iterating over _all_
descriptors doesnt make sense then. You need to iterate over descriptors for
current transaction only.

> >> +     list_for_each_entry(desc_sw, &chan->chain_running, node) {
> >> +             /* for the case of a running descriptor */
> >> +             if (desc_sw->desc.ddadr == ddadr && !running_desc_found) {
> >> +                     switch (chan->dir) {
> >> +                     case DMA_DEV_TO_MEM:
> >> +                     case DMA_MEM_TO_MEM:
> >> +                             reg = (chan->phy->idx << 4) + DTADR;
> >> +                             cur_pos = readl_relaxed(chan->phy->base + reg);
> >> +                             orig_pos = desc_sw->desc.dtadr;
> >> +                             break;
> >> +
> >> +                     case DMA_MEM_TO_DEV:
> >> +                             reg = (chan->phy->idx << 4) + DSADR;
> >> +                             cur_pos = readl_relaxed(chan->phy->base + reg);
> >> +                             orig_pos = desc_sw->desc.dsadr;
> >> +                             break;
> >> +
> >> +                     default:
> >> +                             cur_pos = 0;
> >> +                             orig_pos = 0;
> > This makes no sense...
> What do you recommend for the default case? Just return 0?
why should direction be anything other than these? This is error!

~Vinod
--