2022-08-17 06:12:32

by zhaoxiao

Subject: [PATCH] pwm: rockchip: Convert to use dev_err_probe()

It's fine to call dev_err_probe() in ->probe() when error code is known.
Convert the driver to use dev_err_probe().

Signed-off-by: zhaoxiao <[email protected]>
---
drivers/pwm/pwm-rockchip.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index f3647b317152..bd7ab37aaf88 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -331,15 +331,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pc->pclk)) {
ret = PTR_ERR(pc->pclk);
if (ret != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
- return ret;
+ return dev_err_probe(&pdev->dev, ret, "Can't get APB clk: %d\n");
}

ret = clk_prepare_enable(pc->clk);
- if (ret) {
- dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret);
- return ret;
- }
+ if (ret)
+ dev_err_probe(&pdev->dev, ret, "Can't prepare enable PWM clk: %d\n");

ret = clk_prepare_enable(pc->pclk);
if (ret) {
--
2.20.1

2022-08-17 07:00:23

by Christophe JAILLET

Subject: Re: [PATCH] pwm: rockchip: Convert to use dev_err_probe()

Le 17/08/2022 à 07:59, zhaoxiao a écrit :
> It's fine to call dev_err_probe() in ->probe() when error code is known.
> Convert the driver to use dev_err_probe().
>
> Signed-off-by: zhaoxiao <[email protected]>
> ---
> drivers/pwm/pwm-rockchip.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f3647b317152..bd7ab37aaf88 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -331,15 +331,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> if (IS_ERR(pc->pclk)) {
> ret = PTR_ERR(pc->pclk);
> if (ret != -EPROBE_DEFER)
> - dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
> - return ret;
> + return dev_err_probe(&pdev->dev, ret, "Can't get APB clk: %d\n");

Hi,

Previously, if (ret != -EPROBE_DEFER), we were returning. Now we just
ignore it and continue.
If done on purpose, you should explain why in the commit log.

My guess is that the test should also be removed. dev_err_probe()
handles that for us.


You could use PTR_ERR(pc->pclk) directly. There is no need to assign it
to ret. This would simplify even further the code.


You removed the last 'ret' parameter, which is fine, but left the %d in
the message.
Compiling trigger the issue.
Please, ALWAYS, at least compile test your changes, even when things
look obvious.


Look at the code around line 316 to see how it is done.

> }
>
> ret = clk_prepare_enable(pc->clk);
> - if (ret) {
> - dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret);
> - return ret;
> - }
> + if (ret)
> + dev_err_probe(&pdev->dev, ret, "Can't prepare enable PWM clk: %d\n");

'return' before dev_err_probe() missing?

You removed the last 'ret' parameter, but left the %d in the message.
(same comment about compiling as above)

>
> ret = clk_prepare_enable(pc->pclk);
> if (ret) {


Why just converting 2 dev_err() and leaving the other one in the probe
untouched?

CJ