| Message ID | 1343752762-16861-6-git-send-email-damien.cassou@lifl.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: > > From: Damien Cassou <damien.cassou@lifl.fr> > > The various devm_ functions allocate memory that is released when a driver > detaches. This patch uses these functions for data that is allocated in > the probe function of a platform device and is only freed in the remove > function. > > Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> > > --- > drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- > 1 file changed, 7 insertions(+), 20 deletions(-) > > diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c > index c6c016a..00fe4f0 100644 > --- a/drivers/video/exynos/exynos_dp_core.c > +++ b/drivers/video/exynos/exynos_dp_core.c > @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > > dp->dev = &pdev->dev; > > - dp->clock = clk_get(&pdev->dev, "dp"); > + dp->clock = devm_clk_get(&pdev->dev, "dp"); > if (IS_ERR(dp->clock)) { > dev_err(&pdev->dev, "failed to get clock\n"); > return PTR_ERR(dp->clock); > @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > clk_enable(dp->clock); > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!res) { > - dev_err(&pdev->dev, "failed to get registers\n"); > - ret = -EINVAL; > - goto err_clock; > - } Why do you remove this return check? If there is no reason, please, do it as follows: res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { dev_err(&pdev->dev, "failed to get registers\n"); - ret = -EINVAL; - goto err_clock; + return -EINVAL; } Best regards, Jingoo Han > > dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); > if (!dp->reg_base) { > dev_err(&pdev->dev, "failed to ioremap\n"); > - ret = -ENOMEM; > - goto err_clock; > + return -ENOMEM; > } > > dp->irq = platform_get_irq(pdev, 0); > if (!dp->irq) { > dev_err(&pdev->dev, "failed to get irq\n"); > - ret = -ENODEV; > - goto err_clock; > + return -ENODEV; > } > > ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, > "exynos-dp", dp); > if (ret) { > dev_err(&pdev->dev, "failed to request irq\n"); > - goto err_clock; > + return ret; > } > > dp->video_info = pdata->video_info; > @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > ret = exynos_dp_detect_hpd(dp); > if (ret) { > dev_err(&pdev->dev, "unable to detect hpd\n"); > - goto err_clock; > + return ret; > } > > exynos_dp_handle_edid(dp); > @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > dp->video_info->link_rate); > if (ret) { > dev_err(&pdev->dev, "unable to do link train\n"); > - goto err_clock; > + return ret; > } > > exynos_dp_enable_scramble(dp, 1); > @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > ret = exynos_dp_config_video(dp, dp->video_info); > if (ret) { > dev_err(&pdev->dev, "unable to config video\n"); > - goto err_clock; > + return ret; > } > > platform_set_drvdata(pdev, dp); > > return 0; > - > -err_clock: > - clk_put(dp->clock); > - > - return ret; > } > > static int __devexit exynos_dp_remove(struct platform_device *pdev) > @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) > pdata->phy_exit(); > > clk_disable(dp->clock); > - clk_put(dp->clock); > > return 0; > } -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote: > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: >> >> From: Damien Cassou <damien.cassou@lifl.fr> >> >> The various devm_ functions allocate memory that is released when a driver >> detaches. This patch uses these functions for data that is allocated in >> the probe function of a platform device and is only freed in the remove >> function. >> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> >> >> --- >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- >> 1 file changed, 7 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c >> index c6c016a..00fe4f0 100644 >> --- a/drivers/video/exynos/exynos_dp_core.c >> +++ b/drivers/video/exynos/exynos_dp_core.c >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> >> dp->dev = &pdev->dev; >> >> - dp->clock = clk_get(&pdev->dev, "dp"); >> + dp->clock = devm_clk_get(&pdev->dev, "dp"); >> if (IS_ERR(dp->clock)) { >> dev_err(&pdev->dev, "failed to get clock\n"); >> return PTR_ERR(dp->clock); >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> clk_enable(dp->clock); >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - if (!res) { >> - dev_err(&pdev->dev, "failed to get registers\n"); >> - ret = -EINVAL; >> - goto err_clock; >> - } > > Why do you remove this return check? > If there is no reason, please, do it as follows: > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > dev_err(&pdev->dev, "failed to get registers\n"); > - ret = -EINVAL; > - goto err_clock; > + return -EINVAL; > } > > devm_request_and_ioremap function checks the validity of res. Hence this check above is redundant and can be removed. Damien, This patch only adds devm_clk_get() function. Hence you could make the subject line more specific. > Best regards, > Jingoo Han > > >> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); >> if (!dp->reg_base) { >> dev_err(&pdev->dev, "failed to ioremap\n"); >> - ret = -ENOMEM; >> - goto err_clock; >> + return -ENOMEM; >> } >> >> dp->irq = platform_get_irq(pdev, 0); >> if (!dp->irq) { >> dev_err(&pdev->dev, "failed to get irq\n"); >> - ret = -ENODEV; >> - goto err_clock; >> + return -ENODEV; >> } >> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, >> "exynos-dp", dp); >> if (ret) { >> dev_err(&pdev->dev, "failed to request irq\n"); >> - goto err_clock; >> + return ret; >> } >> >> dp->video_info = pdata->video_info; >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> ret = exynos_dp_detect_hpd(dp); >> if (ret) { >> dev_err(&pdev->dev, "unable to detect hpd\n"); >> - goto err_clock; >> + return ret; >> } >> >> exynos_dp_handle_edid(dp); >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> dp->video_info->link_rate); >> if (ret) { >> dev_err(&pdev->dev, "unable to do link train\n"); >> - goto err_clock; >> + return ret; >> } >> >> exynos_dp_enable_scramble(dp, 1); >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> ret = exynos_dp_config_video(dp, dp->video_info); >> if (ret) { >> dev_err(&pdev->dev, "unable to config video\n"); >> - goto err_clock; >> + return ret; >> } >> >> platform_set_drvdata(pdev, dp); >> >> return 0; >> - >> -err_clock: >> - clk_put(dp->clock); >> - >> - return ret; >> } >> >> static int __devexit exynos_dp_remove(struct platform_device *pdev) >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) >> pdata->phy_exit(); >> >> clk_disable(dp->clock); >> - clk_put(dp->clock); >> >> return 0; >> } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote: > > On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote: > > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: > >> > >> From: Damien Cassou <damien.cassou@lifl.fr> > >> > >> The various devm_ functions allocate memory that is released when a driver > >> detaches. This patch uses these functions for data that is allocated in > >> the probe function of a platform device and is only freed in the remove > >> function. > >> > >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> > >> > >> --- > >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- > >> 1 file changed, 7 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c > >> index c6c016a..00fe4f0 100644 > >> --- a/drivers/video/exynos/exynos_dp_core.c > >> +++ b/drivers/video/exynos/exynos_dp_core.c > >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> > >> dp->dev = &pdev->dev; > >> > >> - dp->clock = clk_get(&pdev->dev, "dp"); > >> + dp->clock = devm_clk_get(&pdev->dev, "dp"); > >> if (IS_ERR(dp->clock)) { > >> dev_err(&pdev->dev, "failed to get clock\n"); > >> return PTR_ERR(dp->clock); > >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> clk_enable(dp->clock); > >> > >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> - if (!res) { > >> - dev_err(&pdev->dev, "failed to get registers\n"); > >> - ret = -EINVAL; > >> - goto err_clock; > >> - } > > > > Why do you remove this return check? > > If there is no reason, please, do it as follows: > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > if (!res) { > > dev_err(&pdev->dev, "failed to get registers\n"); > > - ret = -EINVAL; > > - goto err_clock; > > + return -EINVAL; > > } > > > > > > devm_request_and_ioremap function checks the validity of res. Hence > this check above is redundant and can be removed. I don't think so. Even though function called next checks the NULL value, for robustness, the return value of platform_get_resource() should be checked. It is possible that devm_request_and_ioremap() can be changed in the future, as request_mem_region() & ioremap() were changed to devm_request_and_ioremap(). Best regards, Jingoo Han > > Damien, > This patch only adds devm_clk_get() function. Hence you could make the > subject line more specific. > > > > > > Best regards, > > Jingoo Han > > > > > >> > >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); > >> if (!dp->reg_base) { > >> dev_err(&pdev->dev, "failed to ioremap\n"); > >> - ret = -ENOMEM; > >> - goto err_clock; > >> + return -ENOMEM; > >> } > >> > >> dp->irq = platform_get_irq(pdev, 0); > >> if (!dp->irq) { > >> dev_err(&pdev->dev, "failed to get irq\n"); > >> - ret = -ENODEV; > >> - goto err_clock; > >> + return -ENODEV; > >> } > >> > >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, > >> "exynos-dp", dp); > >> if (ret) { > >> dev_err(&pdev->dev, "failed to request irq\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> dp->video_info = pdata->video_info; > >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> ret = exynos_dp_detect_hpd(dp); > >> if (ret) { > >> dev_err(&pdev->dev, "unable to detect hpd\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> exynos_dp_handle_edid(dp); > >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> dp->video_info->link_rate); > >> if (ret) { > >> dev_err(&pdev->dev, "unable to do link train\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> exynos_dp_enable_scramble(dp, 1); > >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> ret = exynos_dp_config_video(dp, dp->video_info); > >> if (ret) { > >> dev_err(&pdev->dev, "unable to config video\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> platform_set_drvdata(pdev, dp); > >> > >> return 0; > >> - > >> -err_clock: > >> - clk_put(dp->clock); > >> - > >> - return ret; > >> } > >> > >> static int __devexit exynos_dp_remove(struct platform_device *pdev) > >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) > >> pdata->phy_exit(); > >> > >> clk_disable(dp->clock); > >> - clk_put(dp->clock); > >> > >> return 0; > >> } > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > > -- > With warm regards, > Sachin -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote: > On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote: >> >> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote: >> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: >> >> >> >> From: Damien Cassou <damien.cassou@lifl.fr> >> >> >> >> The various devm_ functions allocate memory that is released when a driver >> >> detaches. This patch uses these functions for data that is allocated in >> >> the probe function of a platform device and is only freed in the remove >> >> function. >> >> >> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> >> >> >> >> --- >> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- >> >> 1 file changed, 7 insertions(+), 20 deletions(-) >> >> >> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c >> >> index c6c016a..00fe4f0 100644 >> >> --- a/drivers/video/exynos/exynos_dp_core.c >> >> +++ b/drivers/video/exynos/exynos_dp_core.c >> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> >> >> >> dp->dev = &pdev->dev; >> >> >> >> - dp->clock = clk_get(&pdev->dev, "dp"); >> >> + dp->clock = devm_clk_get(&pdev->dev, "dp"); >> >> if (IS_ERR(dp->clock)) { >> >> dev_err(&pdev->dev, "failed to get clock\n"); >> >> return PTR_ERR(dp->clock); >> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> >> clk_enable(dp->clock); >> >> >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> >> - if (!res) { >> >> - dev_err(&pdev->dev, "failed to get registers\n"); >> >> - ret = -EINVAL; >> >> - goto err_clock; >> >> - } >> > >> > Why do you remove this return check? >> > If there is no reason, please, do it as follows: >> > >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> > if (!res) { >> > dev_err(&pdev->dev, "failed to get registers\n"); >> > - ret = -EINVAL; >> > - goto err_clock; >> > + return -EINVAL; >> > } >> > >> > >> >> devm_request_and_ioremap function checks the validity of res. Hence >> this check above is redundant and can be removed. > > > I don't think so. > Even though function called next checks the NULL value, > for robustness, the return value of platform_get_resource() should be > checked. > > It is possible that devm_request_and_ioremap() can be changed in the future, > as request_mem_region() & ioremap() were changed to devm_request_and_ioremap(). They are not changed. They still exist. devm_request_and_ioremap() is an additional function provided for device managed resources. > > > Best regards, > Jingoo Han > > >> >> Damien, >> This patch only adds devm_clk_get() function. Hence you could make the >> subject line more specific. >> >> >> >> >> > Best regards, >> > Jingoo Han >> > >> > >> >> >> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); >> >> if (!dp->reg_base) { >> >> dev_err(&pdev->dev, "failed to ioremap\n"); >> >> - ret = -ENOMEM; >> >> - goto err_clock; >> >> + return -ENOMEM; >> >> } >> >> >> >> dp->irq = platform_get_irq(pdev, 0); >> >> if (!dp->irq) { >> >> dev_err(&pdev->dev, "failed to get irq\n"); >> >> - ret = -ENODEV; >> >> - goto err_clock; >> >> + return -ENODEV; >> >> } >> >> >> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, >> >> "exynos-dp", dp); >> >> if (ret) { >> >> dev_err(&pdev->dev, "failed to request irq\n"); >> >> - goto err_clock; >> >> + return ret; >> >> } >> >> >> >> dp->video_info = pdata->video_info; >> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> >> ret = exynos_dp_detect_hpd(dp); >> >> if (ret) { >> >> dev_err(&pdev->dev, "unable to detect hpd\n"); >> >> - goto err_clock; >> >> + return ret; >> >> } >> >> >> >> exynos_dp_handle_edid(dp); >> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> >> dp->video_info->link_rate); >> >> if (ret) { >> >> dev_err(&pdev->dev, "unable to do link train\n"); >> >> - goto err_clock; >> >> + return ret; >> >> } >> >> >> >> exynos_dp_enable_scramble(dp, 1); >> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >> >> ret = exynos_dp_config_video(dp, dp->video_info); >> >> if (ret) { >> >> dev_err(&pdev->dev, "unable to config video\n"); >> >> - goto err_clock; >> >> + return ret; >> >> } >> >> >> >> platform_set_drvdata(pdev, dp); >> >> >> >> return 0; >> >> - >> >> -err_clock: >> >> - clk_put(dp->clock); >> >> - >> >> - return ret; >> >> } >> >> >> >> static int __devexit exynos_dp_remove(struct platform_device *pdev) >> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) >> >> pdata->phy_exit(); >> >> >> >> clk_disable(dp->clock); >> >> - clk_put(dp->clock); >> >> >> >> return 0; >> >> } >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> > the body of a message to majordomo@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > Please read the FAQ at http://www.tux.org/lkml/ >> >> >> >> -- >> With warm regards, >> Sachin >
On Wednesday, August 01, 2012 1:38 PM Sachin Kamat wrote: > > On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote: > > On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote: > >> > >> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote: > >> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: > >> >> > >> >> From: Damien Cassou <damien.cassou@lifl.fr> > >> >> > >> >> The various devm_ functions allocate memory that is released when a driver > >> >> detaches. This patch uses these functions for data that is allocated in > >> >> the probe function of a platform device and is only freed in the remove > >> >> function. > >> >> > >> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> > >> >> > >> >> --- > >> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- > >> >> 1 file changed, 7 insertions(+), 20 deletions(-) > >> >> > >> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c > >> >> index c6c016a..00fe4f0 100644 > >> >> --- a/drivers/video/exynos/exynos_dp_core.c > >> >> +++ b/drivers/video/exynos/exynos_dp_core.c > >> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> >> > >> >> dp->dev = &pdev->dev; > >> >> > >> >> - dp->clock = clk_get(&pdev->dev, "dp"); > >> >> + dp->clock = devm_clk_get(&pdev->dev, "dp"); > >> >> if (IS_ERR(dp->clock)) { > >> >> dev_err(&pdev->dev, "failed to get clock\n"); > >> >> return PTR_ERR(dp->clock); > >> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> >> clk_enable(dp->clock); > >> >> > >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> >> - if (!res) { > >> >> - dev_err(&pdev->dev, "failed to get registers\n"); > >> >> - ret = -EINVAL; > >> >> - goto err_clock; > >> >> - } > >> > > >> > Why do you remove this return check? > >> > If there is no reason, please, do it as follows: > >> > > >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> > if (!res) { > >> > dev_err(&pdev->dev, "failed to get registers\n"); > >> > - ret = -EINVAL; > >> > - goto err_clock; > >> > + return -EINVAL; > >> > } > >> > > >> > > >> > >> devm_request_and_ioremap function checks the validity of res. Hence > >> this check above is redundant and can be removed. > > > > > > I don't think so. > > Even though function called next checks the NULL value, > > for robustness, the return value of platform_get_resource() should be > > checked. > > > > It is possible that devm_request_and_ioremap() can be changed in the future, > > as request_mem_region() & ioremap() were changed to devm_request_and_ioremap(). > > They are not changed. They still exist. devm_request_and_ioremap() is > an additional function provided for device managed resources. OK, I see. I accept it. Anyway it is simpler. > > > > > > > > Best regards, > > Jingoo Han > > > > > >> > >> Damien, > >> This patch only adds devm_clk_get() function. Hence you could make the > >> subject line more specific. > >> > >> > >> > >> > >> > Best regards, > >> > Jingoo Han > >> > > >> > > >> >> > >> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); > >> >> if (!dp->reg_base) { > >> >> dev_err(&pdev->dev, "failed to ioremap\n"); > >> >> - ret = -ENOMEM; > >> >> - goto err_clock; > >> >> + return -ENOMEM; > >> >> } > >> >> > >> >> dp->irq = platform_get_irq(pdev, 0); > >> >> if (!dp->irq) { > >> >> dev_err(&pdev->dev, "failed to get irq\n"); > >> >> - ret = -ENODEV; > >> >> - goto err_clock; > >> >> + return -ENODEV; > >> >> } > >> >> > >> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, > >> >> "exynos-dp", dp); > >> >> if (ret) { > >> >> dev_err(&pdev->dev, "failed to request irq\n"); > >> >> - goto err_clock; > >> >> + return ret; > >> >> } > >> >> > >> >> dp->video_info = pdata->video_info; > >> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> >> ret = exynos_dp_detect_hpd(dp); > >> >> if (ret) { > >> >> dev_err(&pdev->dev, "unable to detect hpd\n"); > >> >> - goto err_clock; > >> >> + return ret; > >> >> } > >> >> > >> >> exynos_dp_handle_edid(dp); > >> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> >> dp->video_info->link_rate); > >> >> if (ret) { > >> >> dev_err(&pdev->dev, "unable to do link train\n"); > >> >> - goto err_clock; > >> >> + return ret; > >> >> } > >> >> > >> >> exynos_dp_enable_scramble(dp, 1); > >> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> >> ret = exynos_dp_config_video(dp, dp->video_info); > >> >> if (ret) { > >> >> dev_err(&pdev->dev, "unable to config video\n"); > >> >> - goto err_clock; > >> >> + return ret; > >> >> } > >> >> > >> >> platform_set_drvdata(pdev, dp); > >> >> > >> >> return 0; > >> >> - > >> >> -err_clock: > >> >> - clk_put(dp->clock); > >> >> - > >> >> - return ret; > >> >> } > >> >> > >> >> static int __devexit exynos_dp_remove(struct platform_device *pdev) > >> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) > >> >> pdata->phy_exit(); > >> >> > >> >> clk_disable(dp->clock); > >> >> - clk_put(dp->clock); > >> >> > >> >> return 0; > >> >> } > >> > > >> > -- > >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > >> > the body of a message to majordomo@vger.kernel.org > >> > More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > Please read the FAQ at http://www.tux.org/lkml/ > >> > >> > >> > >> -- > >> With warm regards, > >> Sachin > > > > > > -- > With warm regards, > Sachin -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, August 01, 2012 1:00 PM Sachin Kamat wrote: > > On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote: > > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: > >> > >> From: Damien Cassou <damien.cassou@lifl.fr> > >> > >> The various devm_ functions allocate memory that is released when a driver > >> detaches. This patch uses these functions for data that is allocated in > >> the probe function of a platform device and is only freed in the remove > >> function. > >> > >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> > >> > >> --- > >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- > >> 1 file changed, 7 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c > >> index c6c016a..00fe4f0 100644 > >> --- a/drivers/video/exynos/exynos_dp_core.c > >> +++ b/drivers/video/exynos/exynos_dp_core.c > >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> > >> dp->dev = &pdev->dev; > >> > >> - dp->clock = clk_get(&pdev->dev, "dp"); > >> + dp->clock = devm_clk_get(&pdev->dev, "dp"); > >> if (IS_ERR(dp->clock)) { > >> dev_err(&pdev->dev, "failed to get clock\n"); > >> return PTR_ERR(dp->clock); > >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> clk_enable(dp->clock); > >> > >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> - if (!res) { > >> - dev_err(&pdev->dev, "failed to get registers\n"); > >> - ret = -EINVAL; > >> - goto err_clock; > >> - } > > > > Why do you remove this return check? > > If there is no reason, please, do it as follows: > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > if (!res) { > > dev_err(&pdev->dev, "failed to get registers\n"); > > - ret = -EINVAL; > > - goto err_clock; > > + return -EINVAL; > > } > > > > > > devm_request_and_ioremap function checks the validity of res. Hence > this check above is redundant and can be removed. > > Damien, > This patch only adds devm_clk_get() function. Hence you could make the > subject line more specific. To Damien, As Sachin Kamat mentioned, please change the subject more specific. For example, video: exynos_dp: use devm_clk_get function Best regards, Jingoo Han > > > > > > Best regards, > > Jingoo Han > > > > > >> > >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); > >> if (!dp->reg_base) { > >> dev_err(&pdev->dev, "failed to ioremap\n"); > >> - ret = -ENOMEM; > >> - goto err_clock; > >> + return -ENOMEM; > >> } > >> > >> dp->irq = platform_get_irq(pdev, 0); > >> if (!dp->irq) { > >> dev_err(&pdev->dev, "failed to get irq\n"); > >> - ret = -ENODEV; > >> - goto err_clock; > >> + return -ENODEV; > >> } > >> > >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, > >> "exynos-dp", dp); > >> if (ret) { > >> dev_err(&pdev->dev, "failed to request irq\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> dp->video_info = pdata->video_info; > >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> ret = exynos_dp_detect_hpd(dp); > >> if (ret) { > >> dev_err(&pdev->dev, "unable to detect hpd\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> exynos_dp_handle_edid(dp); > >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> dp->video_info->link_rate); > >> if (ret) { > >> dev_err(&pdev->dev, "unable to do link train\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> exynos_dp_enable_scramble(dp, 1); > >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) > >> ret = exynos_dp_config_video(dp, dp->video_info); > >> if (ret) { > >> dev_err(&pdev->dev, "unable to config video\n"); > >> - goto err_clock; > >> + return ret; > >> } > >> > >> platform_set_drvdata(pdev, dp); > >> > >> return 0; > >> - > >> -err_clock: > >> - clk_put(dp->clock); > >> - > >> - return ret; > >> } > >> > >> static int __devexit exynos_dp_remove(struct platform_device *pdev) > >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) > >> pdata->phy_exit(); > >> > >> clk_disable(dp->clock); > >> - clk_put(dp->clock); > >> > >> return 0; > >> } > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > > -- > With warm regards, > Sachin > -- > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 1 Aug 2012, Jingoo Han wrote: > On Wednesday, August 01, 2012 1:38 PM Sachin Kamat wrote: >> >> On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote: >>> On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote: >>>> >>>> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote: >>>>> On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote: >>>>>> >>>>>> From: Damien Cassou <damien.cassou@lifl.fr> >>>>>> >>>>>> The various devm_ functions allocate memory that is released when a driver >>>>>> detaches. This patch uses these functions for data that is allocated in >>>>>> the probe function of a platform device and is only freed in the remove >>>>>> function. >>>>>> >>>>>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr> >>>>>> >>>>>> --- >>>>>> drivers/video/exynos/exynos_dp_core.c | 27 +++++++-------------------- >>>>>> 1 file changed, 7 insertions(+), 20 deletions(-) >>>>>> >>>>>> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c >>>>>> index c6c016a..00fe4f0 100644 >>>>>> --- a/drivers/video/exynos/exynos_dp_core.c >>>>>> +++ b/drivers/video/exynos/exynos_dp_core.c >>>>>> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >>>>>> >>>>>> dp->dev = &pdev->dev; >>>>>> >>>>>> - dp->clock = clk_get(&pdev->dev, "dp"); >>>>>> + dp->clock = devm_clk_get(&pdev->dev, "dp"); >>>>>> if (IS_ERR(dp->clock)) { >>>>>> dev_err(&pdev->dev, "failed to get clock\n"); >>>>>> return PTR_ERR(dp->clock); >>>>>> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) >>>>>> clk_enable(dp->clock); >>>>>> >>>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> - if (!res) { >>>>>> - dev_err(&pdev->dev, "failed to get registers\n"); >>>>>> - ret = -EINVAL; >>>>>> - goto err_clock; >>>>>> - } >>>>> >>>>> Why do you remove this return check? >>>>> If there is no reason, please, do it as follows: >>>>> >>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> if (!res) { >>>>> dev_err(&pdev->dev, "failed to get registers\n"); >>>>> - ret = -EINVAL; >>>>> - goto err_clock; >>>>> + return -EINVAL; >>>>> } >>>>> >>>>> >>>> >>>> devm_request_and_ioremap function checks the validity of res. Hence >>>> this check above is redundant and can be removed. >>> >>> >>> I don't think so. >>> Even though function called next checks the NULL value, >>> for robustness, the return value of platform_get_resource() should be >>> checked. >>> >>> It is possible that devm_request_and_ioremap() can be changed in the future, >>> as request_mem_region() & ioremap() were changed to devm_request_and_ioremap(). >> >> They are not changed. They still exist. devm_request_and_ioremap() is >> an additional function provided for device managed resources. > > > OK, I see. I accept it. > Anyway it is simpler. This thread contains a discussion about the issue http://lkml.org/lkml/2012/1/28/10 Look for the comments by Wolfram Sang, who implemented devm_request_and_ioremap, and who suggests that the NULL test be removed. I rather agree with the desire to be safe and uniform, but these initialization functions are really large, and with error handling code (although not in this case) there is always the danger of jumping to the wrong place, and thus making more of a mess. It would be nice if the platform_get_resource could be merged with devm_request_and_ioremap, but I think that I looked once and there were not enough calls that were similar enough to make that compelling. julia -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 1, 2012 at 7:08 AM, Jingoo Han <jg1.han@samsung.com> wrote: > To Damien, > As Sachin Kamat mentioned, please change the subject more specific. For example, > > video: exynos_dp: use devm_clk_get function ok, done in a new thread titled: [PATCH v2] video: exynos_dp: use devm_clk_get function Thank you for the feedback
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c index c6c016a..00fe4f0 100644 --- a/drivers/video/exynos/exynos_dp_core.c +++ b/drivers/video/exynos/exynos_dp_core.c @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) dp->dev = &pdev->dev; - dp->clock = clk_get(&pdev->dev, "dp"); + dp->clock = devm_clk_get(&pdev->dev, "dp"); if (IS_ERR(dp->clock)) { dev_err(&pdev->dev, "failed to get clock\n"); return PTR_ERR(dp->clock); @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) clk_enable(dp->clock); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - dev_err(&pdev->dev, "failed to get registers\n"); - ret = -EINVAL; - goto err_clock; - } dp->reg_base = devm_request_and_ioremap(&pdev->dev, res); if (!dp->reg_base) { dev_err(&pdev->dev, "failed to ioremap\n"); - ret = -ENOMEM; - goto err_clock; + return -ENOMEM; } dp->irq = platform_get_irq(pdev, 0); if (!dp->irq) { dev_err(&pdev->dev, "failed to get irq\n"); - ret = -ENODEV; - goto err_clock; + return -ENODEV; } ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0, "exynos-dp", dp); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto err_clock; + return ret; } dp->video_info = pdata->video_info; @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) ret = exynos_dp_detect_hpd(dp); if (ret) { dev_err(&pdev->dev, "unable to detect hpd\n"); - goto err_clock; + return ret; } exynos_dp_handle_edid(dp); @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) dp->video_info->link_rate); if (ret) { dev_err(&pdev->dev, "unable to do link train\n"); - goto err_clock; + return ret; } exynos_dp_enable_scramble(dp, 1); @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev) ret = exynos_dp_config_video(dp, dp->video_info); if (ret) { dev_err(&pdev->dev, "unable to config video\n"); - goto err_clock; + return ret; } platform_set_drvdata(pdev, dp); return 0; - -err_clock: - clk_put(dp->clock); - - return ret; } static int __devexit exynos_dp_remove(struct platform_device *pdev) @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev) pdata->phy_exit(); clk_disable(dp->clock); - clk_put(dp->clock); return 0; }