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

[Anatolij Gustschin <agust@denx.de>]

On Thu, 21 Jan 2010 10:22:27 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Tue, Jan 19, 2010 at 1:24 PM, Anatolij Gustschin <agust@denx.de> wrote:
> > From: Piotr Ziecik <kosmo@semihalf.com>
> >
> > 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.

> > ...
> > =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 +++++++++++++++++++++++++++=
++++++++++++++++++
> > =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
> > +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
> > +static void __exit mpc_dma_remove(struct of_device *op)
>=20
> __devexit

Ok.

> > +{
> > + =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().

> > +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,
> > + =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.

> > +module_exit(mpc_dma_exit);
> > +
> > +/* MODULE API */
>=20
> Meaningless comment.

Ok, will remove it.

Thanks,
Anatolij