Re: [PATCH 07/11] dma: Add MPC512x DMA driver

On Thu, 21 Jan 2010 10:22:27 -0700
Grant Likely [off-list ref] wrote:

On Tue, Jan 19, 2010 at 1:24 PM, Anatolij Gustschin [off-list ref] wrote:

quoted

From: Piotr Ziecik <redacted>

Adds initial version of MPC512x DMA driver.
Only memory to memory transfers are currenly supported.

=20
Comments below on brief review.  I've not looked at the code in-depth.

quoted

...
=C2=A0drivers/dma/Kconfig =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A07 +
=C2=A0drivers/dma/Makefile =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A01 +
=C2=A0drivers/dma/mpc512x_dma.c | =C2=A0636 +++++++++++++++++++++++++++=

++++++++++++++++++

quoted

=C2=A0drivers/dma/mpc512x_dma.h | =C2=A0192 ++++++++++++++

=20
Unless the stuff in the .h file is used by other .c files, it really
belongs in mpc512x_dma.c

Ok, will be moved to .c file in next version.

=20

quoted

+static int __init mpc_dma_probe(struct of_device *op,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=

 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 const struc=
t of_device_id *match)

=20
__devinit
=20

quoted

+static void __exit mpc_dma_remove(struct of_device *op)

=20
__devexit

Ok.

quoted

+{
+ =C2=A0 =C2=A0 =C2=A0 struct device *dev =3D &op->dev;
+ =C2=A0 =C2=A0 =C2=A0 struct mpc_dma *mdma =3D dev_get_drvdata(dev);
+
+ =C2=A0 =C2=A0 =C2=A0 devm_free_irq(dev, mdma->irq, mdma);
+}

=20
No unregistration of the dma device?  No unmapping?  No kfree()?

I will add unregistration of the dma device. Unmapping and
freeing should be done automatically on driver detach, mapping
and allocation is done by devm_ioremap() and devm_kzalloc().

quoted

+static struct of_platform_driver mpc_dma_driver =3D {
+ =C2=A0 =C2=A0 =C2=A0 .match_table =C2=A0 =C2=A0=3D mpc_dma_match,
+ =C2=A0 =C2=A0 =C2=A0 .probe =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D mpc=

_dma_probe,

quoted

+ =C2=A0 =C2=A0 =C2=A0 .remove =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D __exit_p=

(mpc_dma_remove),

=20
__devexit_p()

Ok.

quoted

+module_exit(mpc_dma_exit);
+
+/* MODULE API */

=20
Meaningless comment.

Ok, will remove it.

Thanks,
Anatolij