LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vinod Koul <vinod.koul@intel.com>
To: Rameshwar Sahu <rsahu@apm.com>
Cc: dan.j.williams@intel.com, dmaengine@vger.kernel.org,
	Arnd Bergmann <arnd@arndb.de>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, ddutile@redhat.com,
	jcm@redhat.com, patches@apm.com, Loc Ho <lho@apm.com>
Subject: Re: [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver
Date: Mon, 16 Mar 2015 16:57:30 +0530	[thread overview]
Message-ID: <20150316112730.GN32683@intel.com> (raw)
In-Reply-To: <CAFd313y3avndJ_jDbOSWsbBOYapto_HpDUa+eR7Qy2FO2qLDYg@mail.gmail.com>

On Mon, Mar 16, 2015 at 04:00:22PM +0530, Rameshwar Sahu wrote:

> >> +static struct xgene_dma_desc_sw *xgene_dma_alloc_descriptor(
> >> +                              struct xgene_dma_chan *chan)
> >> +{
> >> +     struct xgene_dma_desc_sw *desc;
> >> +     dma_addr_t phys;
> >> +
> >> +     desc = dma_pool_alloc(chan->desc_pool, GFP_NOWAIT, &phys);
> >> +     if (!desc) {
> >> +             chan_dbg(chan, "Failed to allocate LDs\n");
> > not error?
> 
> Yes it's error only by lacking of dma memory, do I need to use dev_err
> to show the error msg ??
yes

> 
> >
> >> +static void xgene_dma_free_desc_list_reverse(struct xgene_dma_chan *chan,
> >> +                                          struct list_head *list)
> > do we really care about free order?
> 
> Yes it start dellocation of descriptor by tail.
and why by tail is not clear.

> > where are you mapping dma buffers?
> 
>  I didn't get you here. Can you please explain me here what you mean.
> As per my understanding client should map the dma buffer and give the
> physical address and size to this callback prep routines.
not for memcpy, that is true for slave transfers

For mempcy the idea is that drivers will do buffer mapping

> > why are you calling this here, status check shouldnt do this...
> 
> Okay, I will remove it.
> 
> 
> >> +                     spin_unlock_bh(&chan->lock);
> >> +                     return DMA_IN_PROGRESS;
> > residue here is size of transacation.
> 
> We can't calculate here residue size. We don't have any controller
> register which will tell about remaining transaction size.
Okay if you cant calculate residue why do we have this fn?

> 
> >> +             }
> >> +     }
> >> +
> >> +     /* Check if this descriptor is in running queue */
> >> +     list_for_each_entry(desc, &chan->ld_running, node) {
> >> +             if (desc->tx.cookie == cookie) {
> >> +                     /* Cleanup any running and executed descriptors */
> >> +                     xgene_dma_cleanup_descriptors(chan);
> > ditto?
> 
> Okay
> 
> 
> >> +                     spin_unlock_bh(&chan->lock);
> >> +                     return dma_cookie_status(&chan->dma_chan,
> >> +                                              cookie, txstate);
> > and you havent touched txstate so what is the intent here...?
> 
> txstate can filled by caller, so it may be NULL or not NULL, we are
> passing same.
something seems very wrong here. Status should return the current satue of
queried descriptor and if residue value in txstate, you seem to be doing
something else, quotesion is what and why :)

-- 
~Vinod


  reply	other threads:[~2015-03-16 11:31 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-12 11:15 [PATCH v7 0/3] dmaengine: APM X-Gene SoC DMA engine driver support Rameshwar Prasad Sahu
2015-03-12 11:15 ` [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver Rameshwar Prasad Sahu
2015-03-16  9:27   ` Vinod Koul
2015-03-16 10:30     ` Rameshwar Sahu
2015-03-16 11:27       ` Vinod Koul [this message]
2015-03-16 11:54         ` Rameshwar Sahu
2015-03-16 16:26           ` Vinod Koul
2015-03-16 17:31             ` Rameshwar Sahu
2015-03-17  9:33               ` Rameshwar Sahu
2015-03-17 10:19                 ` Vinod Koul
2015-03-17 10:38                   ` Rameshwar Sahu
2015-03-16 11:02     ` Rameshwar Sahu
2015-03-12 11:15 ` [PATCH v7 2/3] arm64: dts: Add APM X-Gene DMA device and DMA clock DTS nodes Rameshwar Prasad Sahu
2015-03-12 11:15 ` [PATCH v7 3/3] Documentation: dma: Add APM X-Gene SoC DMA engine driver documentation Rameshwar Prasad Sahu
2015-03-16  9:29   ` Vinod Koul
2015-03-16 10:31     ` Rameshwar Sahu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150316112730.GN32683@intel.com \
    --to=vinod.koul@intel.com \
    --cc=arnd@arndb.de \
    --cc=dan.j.williams@intel.com \
    --cc=ddutile@redhat.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmaengine@vger.kernel.org \
    --cc=jcm@redhat.com \
    --cc=lho@apm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patches@apm.com \
    --cc=rsahu@apm.com \
    --subject='Re: [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).