[v5,2/2] dmaengine: fsl-edma: add ColdFire mcf5441x edma support

[Angelo Dureghello <angelo@sysam.it>]

Hi Vinod,

fixed mostly all, but sorry, i have still two questions before
proceeding,

On Thu, Jun 28, 2018 at 11:53:41AM +0530, Vinod wrote:
> On 22-06-18, 11:44, Angelo Dureghello wrote:
> >  obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
> >  obj-$(CONFIG_FSL_DMA) += fsldma.o
> >  obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o
> > +obj-$(CONFIG_MCF_EDMA) += mcf-edma.o fsl-edma-common.o
> 
> that makes kernel have two copies of common.o one in thsi driver and one
> in previous one why not do:
> 
> CONFIG_FSL_COMMON += fsl-edma-common.o
> CONFIG_FSL_EDMA += fsl-edma.o
> CONFIG_MCF_EDMA += mcf-edma.o
> 
> and you select CONFIG_FSL_COMMON in both FSL and MCF Kconfig?
> 
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2013-2014 Freescale Semiconductor, Inc
> > +// Copyright (c) 2017 Sysam, Angelo Dureghello  <angelo@sysam.it>
> > +/*
> > + * drivers/dma/mcf-edma.c
> > + *
> > + * Driver for the Freescale ColdFire 64-ch eDMA implementation,
> > + * derived from drivers/dma/fsl-edma.c.
> > + *
> > + * This program is free software; you can redistribute  it and/or modify it
> > + * under  the terms of  the GNU General  Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + */
> 
> again, no need for text
> 

It is not clear to me now how the initial header should be (i guess for 
all the 3 c files at this point).

Do you want just something as :

// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2013-2014 Freescale Semiconductor, Inc
// Copyright (c) 2017 Sysam, Angelo Dureghello  <angelo@sysam.it>

And nothing else ?

Majority of the files in the dma folder has also generally a line with 
the file name and path, a brief driver explaination and the reduced GPL
licence text, and, as imx-sdma.c often copyrights at the end. So what is 
the current rule ?


> > +static irqreturn_t mcf_edma_tx_handler(int irq, void *dev_id)
> > +{
> > +	struct fsl_edma_engine *mcf_edma = dev_id;
> > +	struct edma_regs *regs = &mcf_edma->regs;
> > +	unsigned int ch;
> > +	struct fsl_edma_chan *mcf_chan;
> > +	u64 intmap;
> > +
> > +	intmap = ioread32(regs->inth);
> > +	intmap <<= 32;
> > +	intmap |= ioread32(regs->intl);
> > +	if (!intmap)
> > +		return IRQ_NONE;
> > +
> > +	for (ch = 0; ch < mcf_edma->n_chans; ch++) {
> > +		if (intmap & (0x1 << ch)) {
> 
>                 intmap & BIT(ch)
> 
> > +static irqreturn_t mcf_edma_err_handler(int irq, void *dev_id)
> > +{
> > +	struct fsl_edma_engine *mcf_edma = dev_id;
> > +	struct edma_regs *regs = &mcf_edma->regs;
> > +	unsigned int err, ch;
> > +
> > +	err = ioread32(regs->errl);
> > +	if (!err)
> > +		return IRQ_NONE;
> > +
> > +	for (ch = 0; ch < (EDMA_CHANNELS / 2); ch++) {
> > +		if (err & (0x1 << ch)) {
> 
> here as well
> 
> > +static int mcf_edma_remove(struct platform_device *pdev)
> > +{
> > +	struct fsl_edma_engine *mcf_edma = platform_get_drvdata(pdev);
> > +
> > +	fsl_edma_cleanup_vchan(&mcf_edma->dma_dev);
> > +	dma_async_device_unregister(&mcf_edma->dma_dev);
> 
> at this point your irqs are still registered and running. You vchan
> tasklet maybe still pending to be eecuted and can be scheduled again
> 
> > +static int __init mcf_edma_init(void)
> > +{
> > +	return platform_driver_register(&mcf_edma_driver);
> > +}
> > +subsys_initcall(mcf_edma_init);
> 
> why subsys_initcall?
> 

I find subsys_initcall in several dma drivers, my understanding is that
it initializes the driver before other drivers can use it.
It also sets the driver as built in only.
This seems ok for my case.

Regards,
Angelo

> -- 
> ~Vinod
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html