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);
}
Thanks
Richard
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
