[v1] i2c: at91: Use dev_err_probe() instead of dev_err()

[PATCH RESEND] i2c: at91: Use dev_err_probe() instead of dev_err()

Posted by Yann Sionneau 2 years, 5 months ago

Change
if (IS_ERR(x)) { dev_err(...); return PTR_ERR(x); }
into
return dev_err_probe()

Also, return the correct error instead of hardcoding -ENODEV
This change has also the advantage of handling the -EPROBE_DEFER situation.

Signed-off-by: Yann Sionneau <yann@sionneau.net>
---
 drivers/i2c/busses/i2c-at91-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c
index 05ad3bc3578a..b7bc17b0e5f0 100644
--- a/drivers/i2c/busses/i2c-at91-core.c
+++ b/drivers/i2c/busses/i2c-at91-core.c
@@ -227,10 +227,9 @@ static int at91_twi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, dev);
 
 	dev->clk = devm_clk_get(dev->dev, NULL);
-	if (IS_ERR(dev->clk)) {
-		dev_err(dev->dev, "no clock defined\n");
-		return -ENODEV;
-	}
+	if (IS_ERR(dev->clk))
+		return dev_err_probe(dev->dev, PTR_ERR(dev->clk), "no clock defined\n");
+
 	clk_prepare_enable(dev->clk);
 
 	snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
-- 
2.34.1

Re: [PATCH RESEND] i2c: at91: Use dev_err_probe() instead of dev_err()

Posted by Wolfram Sang 2 years, 5 months ago

On Fri, Aug 25, 2023 at 04:32:34PM +0200, Yann Sionneau wrote:
> Change
> if (IS_ERR(x)) { dev_err(...); return PTR_ERR(x); }
> into
> return dev_err_probe()
> 
> Also, return the correct error instead of hardcoding -ENODEV
> This change has also the advantage of handling the -EPROBE_DEFER situation.
> 
> Signed-off-by: Yann Sionneau <yann@sionneau.net>

Applied to for-next, thanks!

Re: [PATCH RESEND] i2c: at91: Use dev_err_probe() instead of dev_err()

Posted by Andi Shyti 2 years, 5 months ago

Hi Yann,

On Fri, Aug 25, 2023 at 04:32:34PM +0200, Yann Sionneau wrote:
> Change
> if (IS_ERR(x)) { dev_err(...); return PTR_ERR(x); }
> into
> return dev_err_probe()
> 
> Also, return the correct error instead of hardcoding -ENODEV
> This change has also the advantage of handling the -EPROBE_DEFER situation.
> 
> Signed-off-by: Yann Sionneau <yann@sionneau.net>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Thanks,
Andi

Re: [PATCH RESEND] i2c: at91: Use dev_err_probe() instead of dev_err()

Posted by Andi Shyti 2 years, 5 months ago

BTW...

> > Change
> > if (IS_ERR(x)) { dev_err(...); return PTR_ERR(x); }
> > into
> > return dev_err_probe()
> > 
> > Also, return the correct error instead of hardcoding -ENODEV
> > This change has also the advantage of handling the -EPROBE_DEFER situation.
> > 
> > Signed-off-by: Yann Sionneau <yann@sionneau.net>
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

for the next time, please, if you make changes to the patch,
starting from the commit title, commit message, patch body, you
need to increment the versioning and note the change in the
changelog.

Use resend only if the patch is taken and sent as it is without
any change, anywhere.

This should have been [PATCH v2].

Andi

Re: [PATCH RESEND] i2c: at91: Use dev_err_probe() instead of dev_err()

Posted by Nicolas Ferre 2 years, 5 months ago

On 25/08/2023 at 16:32, Yann Sionneau wrote:
> Change
> if (IS_ERR(x)) { dev_err(...); return PTR_ERR(x); }
> into
> return dev_err_probe()
> 
> Also, return the correct error instead of hardcoding -ENODEV
> This change has also the advantage of handling the -EPROBE_DEFER situation.

Is it found using a tool like Coccinelle or you just ran into it and 
figured out it could be good to change?

Regards,
   Nicolas

> Signed-off-by: Yann Sionneau <yann@sionneau.net>
> ---
>   drivers/i2c/busses/i2c-at91-core.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c
> index 05ad3bc3578a..b7bc17b0e5f0 100644
> --- a/drivers/i2c/busses/i2c-at91-core.c
> +++ b/drivers/i2c/busses/i2c-at91-core.c
> @@ -227,10 +227,9 @@ static int at91_twi_probe(struct platform_device *pdev)
>          platform_set_drvdata(pdev, dev);
> 
>          dev->clk = devm_clk_get(dev->dev, NULL);
> -       if (IS_ERR(dev->clk)) {
> -               dev_err(dev->dev, "no clock defined\n");
> -               return -ENODEV;
> -       }
> +       if (IS_ERR(dev->clk))
> +               return dev_err_probe(dev->dev, PTR_ERR(dev->clk), "no clock defined\n");
> +
>          clk_prepare_enable(dev->clk);
> 
>          snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
> --
> 2.34.1
>

Re: [PATCH RESEND] i2c: at91: Use dev_err_probe() instead of dev_err()

Posted by Yann Sionneau 2 years, 5 months ago

Hi,

Le 28/08/2023 à 10:03, Nicolas Ferre a écrit :
> On 25/08/2023 at 16:32, Yann Sionneau wrote:
>> Change
>> if (IS_ERR(x)) { dev_err(...); return PTR_ERR(x); }
>> into
>> return dev_err_probe()
>>
>> Also, return the correct error instead of hardcoding -ENODEV
>> This change has also the advantage of handling the -EPROBE_DEFER 
>> situation.
>
> Is it found using a tool like Coccinelle or you just ran into it and 
> figured out it could be good to change?

I found it by reading the code, I took the time to read the probe 
functions of a few i2c controller driver to try to find improvements.

But I guess one could also find this sort of changes using tools.

Regards,

Yann

>
> Regards,
>   Nicolas
>
>> Signed-off-by: Yann Sionneau <yann@sionneau.net>
>> ---
>>   drivers/i2c/busses/i2c-at91-core.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-at91-core.c 
>> b/drivers/i2c/busses/i2c-at91-core.c
>> index 05ad3bc3578a..b7bc17b0e5f0 100644
>> --- a/drivers/i2c/busses/i2c-at91-core.c
>> +++ b/drivers/i2c/busses/i2c-at91-core.c
>> @@ -227,10 +227,9 @@ static int at91_twi_probe(struct platform_device 
>> *pdev)
>>          platform_set_drvdata(pdev, dev);
>>
>>          dev->clk = devm_clk_get(dev->dev, NULL);
>> -       if (IS_ERR(dev->clk)) {
>> -               dev_err(dev->dev, "no clock defined\n");
>> -               return -ENODEV;
>> -       }
>> +       if (IS_ERR(dev->clk))
>> +               return dev_err_probe(dev->dev, PTR_ERR(dev->clk), "no 
>> clock defined\n");
>> +
>>          clk_prepare_enable(dev->clk);
>>
>>          snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
>> -- 
>> 2.34.1
>>
>