On Thu, May 24, 2012 at 08:41:18AM +0800, Richard Zhao wrote:
> On Wed, May 23, 2012 at 09:56:41PM +0300, Felipe Balbi wrote:
> > Hi,
> >
> > I know I have reviewed this, but I just saw something "funny"
> That's ok. Any comments at any time is welcome.
> >
> [snip]
> > > + if (dev->dma_mask) {
> > > + pdev->dev.dma_mask =
> > > + kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> > > + if (!pdev->dev.dma_mask) {
> > > + ret = -ENOMEM;
> > > + goto err;
> > > + }
> > > +
> > > + *pdev->dev.dma_mask = *dev->dma_mask;
> >
> > this would be much better if you would just:
> >
> > pdev->dev.dma_mask = dev->dma_mask;
> >
> > There would be no need to allocate any memory. You can also drop the
> > check for dma_mask up there.
> Right.
> >
> > > + pdev->dev.dma_parms = dev->dma_parms;
> > > + dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask);
> > > + }
> > > +
> > > + ret = platform_device_add_resources(pdev, res, nres);
> > > + if (ret)
> > > + goto err;
> > > +
> > > + ret = platform_device_add_data(pdev, platdata, sizeof(*platdata));
> > > + if (ret)
> > > + goto err;
> > > +
> > > + ret = platform_device_add(pdev);
> > > + if (ret) {
> > > +err:
> > > + kfree(pdev->dev.dma_mask);
> > > + pdev->dev.dma_mask = NULL;
> > > + platform_device_put(pdev);
> > > + return ERR_PTR(ret);
> >
> > this would be better done with several error labels after the return.
> > Something like:
> >
> > [...]
> >
> > return pdev;
> >
> > err2:
> > pdev->dev.dma_mask = NULL;
> >
> > err1:
> > platform_device_put(pdev);
> >
> > err0:
> > return ERR_PTR(ret);
> >
> > or something similar.
> So it looks like below:
> struct platform_device *ci13xxx_add_device(struct device *dev,
> struct resource *res, int nres,
> struct ci13xxx_platform_data *platdata)
> {
> struct platform_device *pdev;
> int id, ret;
>
> id = ci_get_device_id();
> if (id < 0)
> return ERR_PTR(id);
> // There's nothing roll back here. I prefer return directly.
>
> pdev = platform_device_alloc("ci_hdrc", id);
> if (!pdev) {
> ret = -ENOMEM;
> goto put_id;
> }
>
> pdev->dev.parent = dev;
> pdev->dev.dma_mask = dev->dma_mask;
> pdev->dev.dma_parms = dev->dma_parms;
> dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask);
>
> ret = platform_device_add_resources(pdev, res, nres);
> if (ret)
> goto err;
>
> ret = platform_device_add_data(pdev, platdata, sizeof(*platdata));
> if (ret)
> goto err;
>
> ret = platform_device_add(pdev);
> if (ret)
> goto err;
>
> return pdev;
>
> err:
> platform_device_put(pdev);
> put_id:
> ci_put_device_id(id);
> return ERR_PTR(ret);
> }
looks great:
Reviewed-by: Felipe Balbi <balbi@xxxxxx>
--
balbi
Attachment:
signature.asc
Description: Digital signature
