Re: [PATCH v3 3/4] USB: Chipidea: add unified ci13xxx_{add,remove}_device for platform drivers

[Felipe Balbi <balbi@xxxxxx>]

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