Use the dev_err_probe function instead of dev_err in the probe function
so that the printed messge includes the return value and also handles
-EPROBE_DEFER nicely.

Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c3287c887c6f..bfa788b3775b 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 		sizeof(lpi2c_imx->adapter.name));
 
 	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
 	lpi2c_imx->num_clks = ret;
 
 	ret = of_property_read_u32(pdev->dev.of_node,
@@ -582,10 +580,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
 			       pdev->name, lpi2c_imx);
-	if (ret) {
-		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
 
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
-- 
2.25.1

On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
> Use the dev_err_probe function instead of dev_err in the probe function
> so that the printed messge includes the return value and also handles
> -EPROBE_DEFER nicely.
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index c3287c887c6f..bfa788b3775b 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  		sizeof(lpi2c_imx->adapter.name));
>  
>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");

you cut on this because the line was going over 100 characters? :)

In theory you shouldn't change the print message when doing such
changes and you can still split it as:

		return dev_err_probe(&pdev->dev, ret,
				     "can't get I2C peripheral clock, ret=%d\n",
				     ret);

and you're even within the 80 characters.

Sorry, I missed it in the previous version, mind resending it?

Andi

Hi, Andi

在 2023/8/5 6:16, Andi Shyti 写道:
> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
>> Use the dev_err_probe function instead of dev_err in the probe function
>> so that the printed messge includes the return value and also handles
>> -EPROBE_DEFER nicely.
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>> ---
>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>> index c3287c887c6f..bfa788b3775b 100644
>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>>  		sizeof(lpi2c_imx->adapter.name));
>>  
>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
>> -	if (ret < 0) {
>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
>> -		return ret;
>> -	}
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
> 
> you cut on this because the line was going over 100 characters? :)
> 
> In theory you shouldn't change the print message when doing such
> changes and you can still split it as:
> 
> 		return dev_err_probe(&pdev->dev, ret,
> 				     "can't get I2C peripheral clock, ret=%d\n",
> 				     ret);
> 
> and you're even within the 80 characters.

Since dev_err_probe always print the second parameter that happens to be the return value,
I remove the "ret=%d" from the original message to avoid a redundant error message.

So is it better to keep the original message unchanged, even though dev_err_probe also prints
the return error value? Or is it better to make this change so that all error messages printed
in the probe function include the return value in a consistent style?

> 
> Sorry, I missed it in the previous version, mind resending it?

Sure, I will resend it in v3.

Thanks.

> 
> Andi

-- 
BR
Liao, Chang

On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
> Hi, Andi
> 
> 在 2023/8/5 6:16, Andi Shyti 写道:
> > On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
> >> Use the dev_err_probe function instead of dev_err in the probe function
> >> so that the printed messge includes the return value and also handles
> >> -EPROBE_DEFER nicely.
> >>
> >> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >> ---
> >>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
> >>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >> index c3287c887c6f..bfa788b3775b 100644
> >> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> >> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> >>  		sizeof(lpi2c_imx->adapter.name));
> >>  
> >>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
> >> -	if (ret < 0) {
> >> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
> >> -		return ret;
> >> -	}
> >> +	if (ret < 0)
> >> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
> > 
> > you cut on this because the line was going over 100 characters? :)
> > 
> > In theory you shouldn't change the print message when doing such
> > changes and you can still split it as:
> > 
> > 		return dev_err_probe(&pdev->dev, ret,
> > 				     "can't get I2C peripheral clock, ret=%d\n",
> > 				     ret);
> > 
> > and you're even within the 80 characters.
> 
> Since dev_err_probe always print the second parameter that happens to be the return value,
> I remove the "ret=%d" from the original message to avoid a redundant error message.
> 
> So is it better to keep the original message unchanged, even though dev_err_probe also prints
> the return error value? Or is it better to make this change so that all error messages printed
> in the probe function include the return value in a consistent style?

yes, you are right! Then please ignore this comment, but...

> >   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> >   		pdev->name, lpi2c_imx);
> > - 	if (ret) {
> > - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> > - 		return ret;
> > - 	}
> > + 	if (ret)
> > + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);

please make it coherent to this second part, as well, where the
error number is printed.

Thank you,
Andi

Hi, Andi

在 2023/8/7 16:17, Andi Shyti 写道:
> On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
>> Hi, Andi
>>
>> 在 2023/8/5 6:16, Andi Shyti 写道:
>>> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
>>>> Use the dev_err_probe function instead of dev_err in the probe function
>>>> so that the printed messge includes the return value and also handles
>>>> -EPROBE_DEFER nicely.
>>>>
>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>> index c3287c887c6f..bfa788b3775b 100644
>>>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>>>>  		sizeof(lpi2c_imx->adapter.name));
>>>>  
>>>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
>>>> -	if (ret < 0) {
>>>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
>>>> -		return ret;
>>>> -	}
>>>> +	if (ret < 0)
>>>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
>>>
>>> you cut on this because the line was going over 100 characters? :)
>>>
>>> In theory you shouldn't change the print message when doing such
>>> changes and you can still split it as:
>>>
>>> 		return dev_err_probe(&pdev->dev, ret,
>>> 				     "can't get I2C peripheral clock, ret=%d\n",
>>> 				     ret);
>>>
>>> and you're even within the 80 characters.
>>
>> Since dev_err_probe always print the second parameter that happens to be the return value,
>> I remove the "ret=%d" from the original message to avoid a redundant error message.
>>
>> So is it better to keep the original message unchanged, even though dev_err_probe also prints
>> the return error value? Or is it better to make this change so that all error messages printed
>> in the probe function include the return value in a consistent style?
> 
> yes, you are right! Then please ignore this comment, but...
> 
>>>   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
>>>   		pdev->name, lpi2c_imx);
>>> - 	if (ret) {
>>> - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
>>> - 		return ret;
>>> - 	}
>>> + 	if (ret)
>>> + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
> 
> please make it coherent to this second part, as well, where the
> error number is printed.

Do you mean to convert it to the following?

    if (ret)
        return dev_err_probe(&pdev->dev, ret, "can't claim irq\n");

I understand that the style of error message printed by dev_err_probe is like
"error [ERRNO]: [customized message]", the [ERRNO] comes from 2nd parameter,
[customized message] comes from 3rd paramter, if the original [customized message]it
also print ERRNO, i intend to remove it in this patch, otherwise, I will just keep it.
In the above code, [customized message] intend to print irq but return value, so it is
better to keep the original message, right?

Thanks.

> 
> Thank you,
> Andi

-- 
BR
Liao, Chang

On Mon, Aug 07, 2023 at 06:44:23PM +0800, Liao, Chang wrote:
> Hi, Andi
> 
> 在 2023/8/7 16:17, Andi Shyti 写道:
> > On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
> >> Hi, Andi
> >>
> >> 在 2023/8/5 6:16, Andi Shyti 写道:
> >>> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
> >>>> Use the dev_err_probe function instead of dev_err in the probe function
> >>>> so that the printed messge includes the return value and also handles
> >>>> -EPROBE_DEFER nicely.
> >>>>
> >>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >>>> ---
> >>>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
> >>>>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >>>> index c3287c887c6f..bfa788b3775b 100644
> >>>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> >>>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> >>>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> >>>>  		sizeof(lpi2c_imx->adapter.name));
> >>>>  
> >>>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
> >>>> -	if (ret < 0) {
> >>>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
> >>>> -		return ret;
> >>>> -	}
> >>>> +	if (ret < 0)
> >>>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
> >>>
> >>> you cut on this because the line was going over 100 characters? :)
> >>>
> >>> In theory you shouldn't change the print message when doing such
> >>> changes and you can still split it as:
> >>>
> >>> 		return dev_err_probe(&pdev->dev, ret,
> >>> 				     "can't get I2C peripheral clock, ret=%d\n",
> >>> 				     ret);
> >>>
> >>> and you're even within the 80 characters.
> >>
> >> Since dev_err_probe always print the second parameter that happens to be the return value,
> >> I remove the "ret=%d" from the original message to avoid a redundant error message.
> >>
> >> So is it better to keep the original message unchanged, even though dev_err_probe also prints
> >> the return error value? Or is it better to make this change so that all error messages printed
> >> in the probe function include the return value in a consistent style?
> > 
> > yes, you are right! Then please ignore this comment, but...
> > 
> >>>   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> >>>   		pdev->name, lpi2c_imx);
> >>> - 	if (ret) {
> >>> - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> >>> - 		return ret;
> >>> - 	}
> >>> + 	if (ret)
> >>> + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
> > 
> > please make it coherent to this second part, as well, where the
> > error number is printed.
> 
> Do you mean to convert it to the following?
> 
>     if (ret)
>         return dev_err_probe(&pdev->dev, ret, "can't claim irq\n");
> 
> I understand that the style of error message printed by dev_err_probe is like
> "error [ERRNO]: [customized message]", the [ERRNO] comes from 2nd parameter,
> [customized message] comes from 3rd paramter, if the original [customized message]it
> also print ERRNO, i intend to remove it in this patch, otherwise, I will just keep it.
> In the above code, [customized message] intend to print irq but return value, so it is
> better to keep the original message, right?

sorry... I just got confused and read wrong the code. Please
ignore my comments on this patch, you are right here. Feel free
to add.

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

Andi

在 2023/8/7 19:55, Andi Shyti 写道:
> On Mon, Aug 07, 2023 at 06:44:23PM +0800, Liao, Chang wrote:
>> Hi, Andi
>>
>> 在 2023/8/7 16:17, Andi Shyti 写道:
>>> On Mon, Aug 07, 2023 at 10:13:30AM +0800, Liao, Chang wrote:
>>>> Hi, Andi
>>>>
>>>> 在 2023/8/5 6:16, Andi Shyti 写道:
>>>>> On Wed, Aug 02, 2023 at 05:57:36PM +0800, Liao Chang wrote:
>>>>>> Use the dev_err_probe function instead of dev_err in the probe function
>>>>>> so that the printed messge includes the return value and also handles
>>>>>> -EPROBE_DEFER nicely.
>>>>>>
>>>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>>>>> ---
>>>>>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 12 ++++--------
>>>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>>>> index c3287c887c6f..bfa788b3775b 100644
>>>>>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>>>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>>>>>> @@ -569,10 +569,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>>>>>>  		sizeof(lpi2c_imx->adapter.name));
>>>>>>  
>>>>>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &lpi2c_imx->clks);
>>>>>> -	if (ret < 0) {
>>>>>> -		dev_err(&pdev->dev, "can't get I2C peripheral clock, ret=%d\n", ret);
>>>>>> -		return ret;
>>>>>> -	}
>>>>>> +	if (ret < 0)
>>>>>> +		return dev_err_probe(&pdev->dev, ret, "can't get I2C peripheral clock\n");
>>>>>
>>>>> you cut on this because the line was going over 100 characters? :)
>>>>>
>>>>> In theory you shouldn't change the print message when doing such
>>>>> changes and you can still split it as:
>>>>>
>>>>> 		return dev_err_probe(&pdev->dev, ret,
>>>>> 				     "can't get I2C peripheral clock, ret=%d\n",
>>>>> 				     ret);
>>>>>
>>>>> and you're even within the 80 characters.
>>>>
>>>> Since dev_err_probe always print the second parameter that happens to be the return value,
>>>> I remove the "ret=%d" from the original message to avoid a redundant error message.
>>>>
>>>> So is it better to keep the original message unchanged, even though dev_err_probe also prints
>>>> the return error value? Or is it better to make this change so that all error messages printed
>>>> in the probe function include the return value in a consistent style?
>>>
>>> yes, you are right! Then please ignore this comment, but...
>>>
>>>>>   	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
>>>>>   		pdev->name, lpi2c_imx);
>>>>> - 	if (ret) {
>>>>> - 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
>>>>> - 		return ret;
>>>>> - 	}
>>>>> + 	if (ret)
>>>>> + 		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", irq);
>>>
>>> please make it coherent to this second part, as well, where the
>>> error number is printed.
>>
>> Do you mean to convert it to the following?
>>
>>     if (ret)
>>         return dev_err_probe(&pdev->dev, ret, "can't claim irq\n");
>>
>> I understand that the style of error message printed by dev_err_probe is like
>> "error [ERRNO]: [customized message]", the [ERRNO] comes from 2nd parameter,
>> [customized message] comes from 3rd paramter, if the original [customized message]it
>> also print ERRNO, i intend to remove it in this patch, otherwise, I will just keep it.
>> In the above code, [customized message] intend to print irq but return value, so it is
>> better to keep the original message, right?
> 
> sorry... I just got confused and read wrong the code. Please
> ignore my comments on this patch, you are right here. Feel free
> to add.

Thanks, I will add a bit more information to explain the changes made in these patches in v3.

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

-- 
BR
Liao, Chang