Re: [PATCH] dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vinod,

On Mon, May 07, 2018 at 07:45:35PM +0530, Vinod Koul wrote:
On Fri, May 04, 2018 at 09:18:19PM +0200, Angelo Dureghello wrote:
Hi Vinod,

thanks for the review,

On Thu, May 03, 2018 at 10:18:30PM +0530, Vinod Koul wrote:
On Wed, Apr 25, 2018 at 10:08:17PM +0200, Angelo Dureghello wrote:
This patch adds dma support for NXP mcf5441x (ColdFire) family.

ColdFire mcf5441x implements an edma hw module similar to the

Is it similar to to edma ?


It is similar to Freescale "edma" but with a different number of
channels, a bit different register set, different interrupt
structure, no channel multiplexer.

ok

one implemented in Vybrid VFxxx controllers, but with a slightly
different register set, more dma channels (64 instead of 32),
a different interrupt mechanism and some other minor differences.

For the above reasons, modfying fsl-edma.c was too complex and
likely too ugly. From here, the decision to create a different
driver, but starting from fsl-edma.

can the common stuff be made into a lib and shared between then two rather
than having a same driver or different drivers?

It should be possible to collect some common code in a kind of
mcf_edma_core.c common module, but in this case i cannot then test
the Vybrid edma after the changes since i miss that hardware.

Sure you should send the patches and folks who care about fsl driver
would look it up and test

Would be maybe possible for you to diff fsl-edma and this mcf-edma,
just to confirm if i can still stay this way, or if moving to a
library becomes mandatory ?

well since you know the IP you would make a better guess on that, best is
to check register sets in drivers

I fixed all the discussed points.

Actaully mcf-edma (ColdFire) has a slightly different register set (due to 64
channels in place of 16 of fsl-edma) and, for the same reason, a different
DMA interrupt structure.
Also, i simplified some parts of the driver considering ColdFire (mcf) 
big endian architecture.

So i would send a rev 2 patch with all the fixes, than eventually in a second
phase i may try to create some common code, but at least we have the ColdFire
DMA. What do you think ?

+// SPDX-License-Identifier: GPL-2.0

Copyright info should be here in c99 style comments


Seems checkpatch.pl, for C files, does not like the C style
initial line comment:

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#87: FILE: drivers/dma/mcf-edma.c:1:
+/* SPDX-License-Identifier: GPL-2.0 */

While c++ type is accepted.

In contrary, in .h files it wants cpp // style and not C style.

SC99 comments style is
// SPDX-License-Identifier: GPL-2.0

Point is the copyright should be added is same formar i.e.,

// Copyright 20018 - foo bar

this line should follow the spdx line

+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

why do you need this, why not use dev_xxx


Well, pr_ style seems to simplify the call a bit, should be allowed
but if you prefer i can move all to dev_ format.

in hindsight dev_ makes better sense, been there done that :)

-- 
~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

Regards,
Angelo
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux