On Tue, Dec 19, 2017 at 06:28:01AM +0000, Wen He wrote:
> > > +#include <asm/cacheflush.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/dmapool.h>
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_dma.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/spinlock.h>
> >
> > Do you need all these?
> >
>
> Don't need all these, Is that ok?
Okay please remove the ones not needed
> > BIT() or GENMASK() pls for register definations, here and other places in the
> > driver
> >
>
> Okay, does this means, I should be defined BIT() or GENMASK() to here?
Yes please
> > this looks terribly similar to fsl_qdma_ccdf, if you have union then why not add
> > this too to it?
> >
>
> This struct defined description two different dma message format that Compound Command Descriptor Format and qDMA Compound S/G Format,
> if add this too to union, it could be ambiguous.
at lower level do we care about the format, we fill and submit to HW
> > > +static u32 qdma_readl(struct fsl_qdma_engine *qdma, void __iomem
> > > +*addr) {
> > > + if (qdma->big_endian)
> > > + return ioread32be(addr);
> > > + else
> > > + return ioread32(addr);
> > > +}
> >
> > IIRC there are macros which which take care of this, can you check other FSL
> > drivers please
> >
>
> Okay, but I search the FSL drivers and no found. Do you know where are them ?
See DMA_IN/OUT in dma/fsldma.h
> > > + sg_entry_total = dst_sg_entry_block + src_sg_entry_block;
> > > + if (sg_entry_total) {
> > > + sg_block = kzalloc(sizeof(*sg_block) *
> > > + sg_entry_total,
> > > + GFP_KERNEL);
> > > + if (!sg_block)
> > > + return NULL;
> >
> > okay you are leaking here the pool allocation need to be rolled back
> >
>
> okay, I got it, Is that ok?
>
> if (!sg_block) {
> dma_pool_free(queue->comp_pool,
> comp_temp->virt_addr,
> comp_temp->bus_addr);
> return NULL;
Looks okay
> > > +
> > > + dma_free_coherent(&pdev->dev, sizeof(struct fsl_qdma_ccdf) *
> > > + status->n_cq, status->cq, status->bus_addr);
> >
> > at this point you can still get irq and tasklets can be still scheduled, how do
> > you ensure they are stopped
> >
>
> May be I missed, will and below to funtions call In fsl_qdma_remove() in next version.
> Is that ok?
sure
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
