Skip to contentSkip to conversation

dmaengine: fsl-edma: add ColdFire mcf5441x edma support

PATCH New Angelo Dureghello 1 participant 1 message Jun 30, 2018 dmaengine

lore
Share & tools
lore

Angelo Dureghello

author
#0 lore raw

PATCH v5 2/2 dmaengine: fsl-edma: add ColdFire mcf5441x edma support

Hi Vinod,

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

Quoted 15 lines · Vinod
Quoted 4 lines · Angelo Dureghello

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?

Quoted 17 lines
Quoted 4 lines
(quoted diff) +4 -0
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2013-2014 Freescale Semiconductor, Inc
+// Copyright (c) 2017 Sysam, Angelo Dureghello  <[email protected]>
+/*

Review

Quoted 10 lines
(quoted diff) +10 -0
+ * 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 <[email protected]>

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 ?

Quoted 44 lines
Quoted 6 lines
(quoted diff) +6 -0
+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;

Review

Quoted 10 lines
(quoted diff) +10 -0
+	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)

Review

Quoted 12 lines
(quoted diff) +12 -0
+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

Review

Quoted 6 lines
(quoted diff) +6 -0
+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

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+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

Quoted 2 lines
Signature · 1 line
~Vinod

---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html