dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support

DISCUSSION Angelo Dureghello 1 participant 1 message May 22, 2018 dmaengine

lore

Share & tools

lore

Angelo Dureghello

author

2018-05-22 21:28 UTC #0 lore raw

Hi Vinod,

Quoted 38 lines · Vinod Koul

Quoted 8 lines · Angelo Dureghello
Hi Vinod,
thanks for the review,
Quoted 3 lines · Vinod Koul
Quoted 2 lines · Angelo Dureghello
This patch adds dma support for NXP mcf5441x (ColdFire) family.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
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.
Quoted 6 lines
Quoted 2 lines
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

Quoted 7 lines

Quoted 3 lines
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 ?

Quoted 31 lines

Quoted 8 lines
Quoted 4 lines
Quoted 1 line
+// 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:
Quoted 7 lines
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
Quoted 6 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
why do you need this, why not use dev_xxx

Quoted 11 lines

Quoted 2 lines
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 :)
Signature · 5 lines
~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

Regards,
Angelo
---
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