On 26-09-19, 12:18, Green Wan wrote:
> Hi Vinod,
>
> Thanks for the comments. Check my reply below.
>
> On Wed, Sep 25, 2019 at 5:21 AM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
>
> > Hi Green,
> >
> > On 20-09-19, 17:01, Green Wan wrote:
> >
> > Please make sure threading is *not* broken in your patch series. Atm
> > they are all over place in my mailbox!
> >
> > K, I'll check. Just simply git send to the list retrieved from "
> get_maintainer.pl".
Well I guess you used each patch on git-send, you should pass on the
whole series so that it threads as well
To test: you can send to yourself and check if threading is fine or not.
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index d0caa09a479e..c5f0662c9106 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -14594,6 +14594,7 @@ F: drivers/media/mmc/siano/
> > > SIFIVE PDMA DRIVER
> > > M: Green Wan <green.wan@xxxxxxxxxx>
> > > S: Maintained
> > > +F: drivers/dma/sf-pdma/
> > > F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
> >
> > What is this generated against, only one line?
> >
> > against patch v3 1/3. I split the maintainer modification into patch 1/3
> and 3/3 to make "checkpatch.pl --strict" zero warning for both of them. And
> to give info more specifically, I can add
Ah, Can you please add these changes in a separate patch at the end
please
> > > + chan->dma_dev_addr,
> > > + chan->dma_dev_size,
> > > + chan->dma_dir, 0);
> > > + chan->dma_dir = DMA_NONE;
> > > +}
> > > +
> > > +static int sf_pdma_slave_config(struct dma_chan *dchan,
> > > + struct dma_slave_config *cfg)
> > > +{
> > > + struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> > > +
> > > + memcpy(&chan->cfg, cfg, sizeof(*cfg));
> > > + sf_pdma_unprep_slave_dma(chan);
> >
> > Why unprep?
> >
>
> I think the original idea from ./drivers/dma/fsl-edma* is to make sure the
We should fix that too!
> > > +static enum dma_status
> > > +sf_pdma_tx_status(struct dma_chan *dchan,
> > > + dma_cookie_t cookie,
> > > + struct dma_tx_state *txstate)
> > > +{
> > > + struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> > > + enum dma_status status;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&chan->lock, flags);
> > > + if (chan->xfer_err) {
> > > + chan->status = DMA_ERROR;
> > > + spin_unlock_irqrestore(&chan->lock, flags);
> > > + return chan->status;
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&chan->lock, flags);
> > > +
> > > + status = dma_cookie_status(dchan, cookie, txstate);
> > > +
> > > + if (status == DMA_COMPLETE)
> > > + return status;
> > > +
> > > + if (!txstate)
> > > + return chan->status;
> >
> > why not return status? Is that expected to be different than status?
> >
> >
> Depends on the value set by dma_cookie_status(). At the moment, the value
> of chan->status should be DMA_IN_PROGRESS till changed by
> sf_pdma_desc_residue() or set to DMA_ERROR by err ISR. The value could be
> different between status and chan->status.
In case !txstate there is no sf_pdma_desc_residue() so it doesnt make
sense to me to have return different things here!
--
~Vinod
