Re: [PATCH v3 3/3] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Vinod Koul]

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