[PATCH -next] remoteproc: stm32: Clean up redundant dev_err_probe()

[PATCH -next] remoteproc: stm32: Clean up redundant dev_err_probe()

[Chen Jiahao]

Referring to platform_get_irq()'s definition, the return value has
already been checked if ret < 0, and printed via dev_err_probe().
Calling dev_err_probe() one more time outside platform_get_irq()
is obviously redundant.

Removing dev_err_probe() outside platform_get_irq() to clean up
above problem.

Signed-off-by: Chen Jiahao <chenjiahao16@huawei.com>
---
 drivers/remoteproc/stm32_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 98234b44f038..a09eeb83ea5c 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -714,7 +714,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev,
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq == -EPROBE_DEFER)
-		return dev_err_probe(dev, irq, "failed to get interrupt\n");
+		return irq;
 
 	if (irq > 0) {
 		err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0,
-- 
2.34.1

Re: [PATCH -next] remoteproc: stm32: Clean up redundant dev_err_probe()

[Arnaud POULIQUEN]

Hi,

On 8/2/23 11:51, Chen Jiahao wrote:
> Referring to platform_get_irq()'s definition, the return value has
> already been checked if ret < 0, and printed via dev_err_probe().
> Calling dev_err_probe() one more time outside platform_get_irq()
> is obviously redundant.
> 
> Removing dev_err_probe() outside platform_get_irq() to clean up
> above problem.
> 
> Signed-off-by: Chen Jiahao <chenjiahao16@huawei.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 98234b44f038..a09eeb83ea5c 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -714,7 +714,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev,
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq == -EPROBE_DEFER)
> -		return dev_err_probe(dev, irq, "failed to get interrupt\n");
> +		return irq;

The IRQ is optional so using platform_get_irq_optional seems a better option to me.

-  	irq = platform_get_irq(pdev, 0);
+  	irq = platform_get_irq_optional(pdev, 0);
>  	if (irq == -EPROBE_DEFER)
>		return dev_err_probe(dev, irq, "failed to get interrupt\n");

Thanks,
Arnaud

>  
>  	if (irq > 0) {
>  		err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0,

Re: [PATCH -next] remoteproc: stm32: Clean up redundant dev_err_probe()

[chenjiahao (C)]

On 2023/8/16 20:46, Arnaud POULIQUEN wrote:
> Hi,
>
> On 8/2/23 11:51, Chen Jiahao wrote:
>> Referring to platform_get_irq()'s definition, the return value has
>> already been checked if ret < 0, and printed via dev_err_probe().
>> Calling dev_err_probe() one more time outside platform_get_irq()
>> is obviously redundant.
>>
>> Removing dev_err_probe() outside platform_get_irq() to clean up
>> above problem.
>>
>> Signed-off-by: Chen Jiahao <chenjiahao16@huawei.com>
>> ---
>>   drivers/remoteproc/stm32_rproc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index 98234b44f038..a09eeb83ea5c 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -714,7 +714,7 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev,
>>   
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq == -EPROBE_DEFER)
>> -		return dev_err_probe(dev, irq, "failed to get interrupt\n");
>> +		return irq;
> The IRQ is optional so using platform_get_irq_optional seems a better option to me.
>
> -  	irq = platform_get_irq(pdev, 0);
> +  	irq = platform_get_irq_optional(pdev, 0);

Hi,

Thanks for your reminding.

It seems that platform_get_irq is nothing more than platform_get_irq_optional,
except wrapping a dev_err_probe for error code checking. So using the former
one should also be OK.

Or have I missed anywhere else?

Best Regards,
Jiahao

>>   	if (irq == -EPROBE_DEFER)
>> 		return dev_err_probe(dev, irq, "failed to get interrupt\n");
> Thanks,
> Arnaud
>
>>   
>>   	if (irq > 0) {
>>   		err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0,

Re: [PATCH -next] remoteproc: stm32: Clean up redundant dev_err_probe()

[Arnaud POULIQUEN]

On 8/17/23 04:33, chenjiahao (C) wrote:
> 
> On 2023/8/16 20:46, Arnaud POULIQUEN wrote:
>> Hi,
>>
>> On 8/2/23 11:51, Chen Jiahao wrote:
>>> Referring to platform_get_irq()'s definition, the return value has
>>> already been checked if ret < 0, and printed via dev_err_probe().
>>> Calling dev_err_probe() one more time outside platform_get_irq()
>>> is obviously redundant.
>>>
>>> Removing dev_err_probe() outside platform_get_irq() to clean up
>>> above problem.
>>>
>>> Signed-off-by: Chen Jiahao <chenjiahao16@huawei.com>
>>> ---
>>>   drivers/remoteproc/stm32_rproc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>>> index 98234b44f038..a09eeb83ea5c 100644
>>> --- a/drivers/remoteproc/stm32_rproc.c
>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>> @@ -714,7 +714,7 @@ static int stm32_rproc_parse_dt(struct platform_device
>>> *pdev,
>>>         irq = platform_get_irq(pdev, 0);
>>>       if (irq == -EPROBE_DEFER)
>>> -        return dev_err_probe(dev, irq, "failed to get interrupt\n");
>>> +        return irq;
>> The IRQ is optional so using platform_get_irq_optional seems a better option
>> to me.
>>
>> -      irq = platform_get_irq(pdev, 0);
>> +      irq = platform_get_irq_optional(pdev, 0);
> 
> Hi,
> 
> Thanks for your reminding.
> 
> It seems that platform_get_irq is nothing more than platform_get_irq_optional,
> except wrapping a dev_err_probe for error code checking. So using the former
> one should also be OK.
> 
> Or have I missed anywhere else?

The main difference is that platform_get_irq prints an error message, while
platform_get_irq just returns without print.


The IRQ is optional, It would be better to only include traces that is necessary
and useful for debugging purposes.

Best Regards,
Arnaud


> 
> Best Regards,
> Jiahao
> 
>>>       if (irq == -EPROBE_DEFER)
>>>         return dev_err_probe(dev, irq, "failed to get interrupt\n");
>> Thanks,
>> Arnaud
>>
>>>         if (irq > 0) {
>>>           err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0,

Re: [PATCH -next] remoteproc: stm32: Clean up redundant dev_err_probe()

[chenjiahao (C)]

On 2023/8/17 15:02, Arnaud POULIQUEN wrote:
>
> On 8/17/23 04:33, chenjiahao (C) wrote:
>> On 2023/8/16 20:46, Arnaud POULIQUEN wrote:
>>> Hi,
>>>
>>> On 8/2/23 11:51, Chen Jiahao wrote:
>>>> Referring to platform_get_irq()'s definition, the return value has
>>>> already been checked if ret < 0, and printed via dev_err_probe().
>>>> Calling dev_err_probe() one more time outside platform_get_irq()
>>>> is obviously redundant.
>>>>
>>>> Removing dev_err_probe() outside platform_get_irq() to clean up
>>>> above problem.
>>>>
>>>> Signed-off-by: Chen Jiahao <chenjiahao16@huawei.com>
>>>> ---
>>>>    drivers/remoteproc/stm32_rproc.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>>>> index 98234b44f038..a09eeb83ea5c 100644
>>>> --- a/drivers/remoteproc/stm32_rproc.c
>>>> +++ b/drivers/remoteproc/stm32_rproc.c
>>>> @@ -714,7 +714,7 @@ static int stm32_rproc_parse_dt(struct platform_device
>>>> *pdev,
>>>>          irq = platform_get_irq(pdev, 0);
>>>>        if (irq == -EPROBE_DEFER)
>>>> -        return dev_err_probe(dev, irq, "failed to get interrupt\n");
>>>> +        return irq;
>>> The IRQ is optional so using platform_get_irq_optional seems a better option
>>> to me.
>>>
>>> -      irq = platform_get_irq(pdev, 0);
>>> +      irq = platform_get_irq_optional(pdev, 0);
>> Hi,
>>
>> Thanks for your reminding.
>>
>> It seems that platform_get_irq is nothing more than platform_get_irq_optional,
>> except wrapping a dev_err_probe for error code checking. So using the former
>> one should also be OK.
>>
>> Or have I missed anywhere else?
> The main difference is that platform_get_irq prints an error message, while
> platform_get_irq just returns without print.
>
>
> The IRQ is optional, It would be better to only include traces that is necessary
> and useful for debugging purposes.

Make sense. I will update a v2 patch soon.

Thanks

>
> Best Regards,
> Arnaud
>
>
>> Best Regards,
>> Jiahao
>>
>>>>        if (irq == -EPROBE_DEFER)
>>>>          return dev_err_probe(dev, irq, "failed to get interrupt\n");
>>> Thanks,
>>> Arnaud
>>>
>>>>          if (irq > 0) {
>>>>            err = devm_request_irq(dev, irq, stm32_rproc_wdg, 0,