Hi,
I know I have reviewed this, but I just saw something "funny"
On Wed, May 23, 2012 at 11:45:17PM +0800, Richard Zhao wrote:
> Platform drivers do the similar things to add/remove ci13xxx device, so
> create a unified one.
>
> Signed-off-by: Richard Zhao <richard.zhao@xxxxxxxxxxxxx>
> Reviewed-by: Felipe Balbi <balbi@xxxxxx>
> ---
> drivers/usb/chipidea/ci13xxx_msm.c | 34 ++++-----------------
> drivers/usb/chipidea/ci13xxx_pci.c | 34 ++++------------------
> drivers/usb/chipidea/core.c | 56 ++++++++++++++++++++++++++++++++++++
> include/linux/usb/chipidea.h | 7 ++++
> 4 files changed, 76 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci13xxx_msm.c b/drivers/usb/chipidea/ci13xxx_msm.c
> index 23f7067..a1aa719 100644
> --- a/drivers/usb/chipidea/ci13xxx_msm.c
> +++ b/drivers/usb/chipidea/ci13xxx_msm.c
> @@ -58,50 +58,30 @@ static struct ci13xxx_platform_data ci13xxx_msm_platdata = {
> static int __devinit ci13xxx_msm_probe(struct platform_device *pdev)
> {
> struct platform_device *plat_ci;
> - int ret;
>
> dev_dbg(&pdev->dev, "ci13xxx_msm_probe\n");
>
> - plat_ci = platform_device_alloc("ci_hdrc", -1);
> - if (!plat_ci) {
> - dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> - return -ENOMEM;
> + plat_ci = ci13xxx_add_device(&pdev->dev,
> + pdev->resource, pdev->num_resources,
> + &ci13xxx_msm_platdata);
> + if (IS_ERR(plat_ci)) {
> + dev_err(&pdev->dev, "ci13xxx_add_device failed!\n");
> + return PTR_ERR(plat_ci);
> }
>
> - ret = platform_device_add_resources(plat_ci, pdev->resource,
> - pdev->num_resources);
> - if (ret) {
> - dev_err(&pdev->dev, "can't add resources to platform device\n");
> - goto put_platform;
> - }
> -
> - ret = platform_device_add_data(plat_ci, &ci13xxx_msm_platdata,
> - sizeof(ci13xxx_msm_platdata));
> - if (ret)
> - goto put_platform;
> -
> - ret = platform_device_add(plat_ci);
> - if (ret)
> - goto put_platform;
> -
> platform_set_drvdata(pdev, plat_ci);
>
> pm_runtime_no_callbacks(&pdev->dev);
> pm_runtime_enable(&pdev->dev);
>
> return 0;
> -
> -put_platform:
> - platform_device_put(plat_ci);
> -
> - return ret;
> }
>
> static int __devexit ci13xx_msm_remove(struct platform_device *pdev)
> {
> struct platform_device *plat_ci = platform_get_drvdata(pdev);
>
> - platform_device_unregister(plat_ci);
> + ci13xxx_remove_device(plat_ci);
>
> return 0;
> }
> diff --git a/drivers/usb/chipidea/ci13xxx_pci.c b/drivers/usb/chipidea/ci13xxx_pci.c
> index cdcac3a..918e149 100644
> --- a/drivers/usb/chipidea/ci13xxx_pci.c
> +++ b/drivers/usb/chipidea/ci13xxx_pci.c
> @@ -75,13 +75,6 @@ static int __devinit ci13xxx_pci_probe(struct pci_dev *pdev,
> pci_set_master(pdev);
> pci_try_set_mwi(pdev);
>
> - plat_ci = platform_device_alloc("ci_hdrc", -1);
> - if (!plat_ci) {
> - dev_err(&pdev->dev, "can't allocate ci_hdrc platform device\n");
> - retval = -ENOMEM;
> - goto disable_device;
> - }
> -
> memset(res, 0, sizeof(res));
> res[0].start = pci_resource_start(pdev, 0);
> res[0].end = pci_resource_end(pdev, 0);
> @@ -89,32 +82,17 @@ static int __devinit ci13xxx_pci_probe(struct pci_dev *pdev,
> res[1].start = pdev->irq;
> res[1].flags = IORESOURCE_IRQ;
>
> - retval = platform_device_add_resources(plat_ci, res, nres);
> - if (retval) {
> - dev_err(&pdev->dev, "can't add resources to platform device\n");
> - goto put_platform;
> + plat_ci = ci13xxx_add_device(&pdev->dev, res, nres, platdata);
> + if (IS_ERR(plat_ci)) {
> + dev_err(&pdev->dev, "ci13xxx_add_device failed!\n");
> + retval = PTR_ERR(plat_ci);
> + goto disable_device;
> }
>
> - retval = platform_device_add_data(plat_ci, platdata, sizeof(*platdata));
> - if (retval)
> - goto put_platform;
> -
> - dma_set_coherent_mask(&plat_ci->dev, pdev->dev.coherent_dma_mask);
> - plat_ci->dev.dma_mask = pdev->dev.dma_mask;
> - plat_ci->dev.dma_parms = pdev->dev.dma_parms;
> - plat_ci->dev.parent = &pdev->dev;
> -
> pci_set_drvdata(pdev, plat_ci);
>
> - retval = platform_device_add(plat_ci);
> - if (retval)
> - goto put_platform;
> -
> return 0;
>
> - put_platform:
> - pci_set_drvdata(pdev, NULL);
> - platform_device_put(plat_ci);
> disable_device:
> pci_disable_device(pdev);
> done:
> @@ -133,7 +111,7 @@ static void __devexit ci13xxx_pci_remove(struct pci_dev *pdev)
> {
> struct platform_device *plat_ci = pci_get_drvdata(pdev);
>
> - platform_device_unregister(plat_ci);
> + ci13xxx_remove_device(plat_ci);
> pci_set_drvdata(pdev, NULL);
> pci_disable_device(pdev);
> }
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 9a883bd..5fccb9c 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -332,6 +332,62 @@ static irqreturn_t ci_irq(int irq, void *data)
> return ci->role == CI_ROLE_END ? ret : ci_role(ci)->irq(ci);
> }
>
> +struct platform_device *ci13xxx_add_device(struct device *dev,
> + struct resource *res, int nres,
> + struct ci13xxx_platform_data *platdata)
> +{
> + struct platform_device *pdev;
> + int ret;
> +
> + /* FIXME: find a way to choose id */
> + pdev = platform_device_alloc("ci_hdrc", -1);
> + if (!pdev)
> + return ERR_PTR(-ENOMEM);
> +
> + pdev->dev.parent = dev;
> +
> + 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.
> + 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.
--
balbi
Attachment:
signature.asc
Description: Digital signature
