[PATCH] rtc: max77686: Remove some dead code

[PATCH] rtc: max77686: Remove some dead code

[Christophe JAILLET]

'ret' is known to be an error pointer here, so it can't be 0.
Remove this dead code.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/rtc/rtc-max77686.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index d51cc12114cb..ce089ed934ad 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -764,8 +764,6 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	if (IS_ERR(info->rtc_dev)) {
 		ret = PTR_ERR(info->rtc_dev);
 		dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
-		if (ret == 0)
-			ret = -EINVAL;
 		goto err_rtc;
 	}
 
-- 
2.30.2

Re: [PATCH] rtc: max77686: Remove some dead code

[Edmundo Carmona Antoranz]

On Fri, May 7, 2021 at 11:43 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>         if (IS_ERR(info->rtc_dev)) {
>                 ret = PTR_ERR(info->rtc_dev);
>                 dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);

Following the recent conversations, I think it might make sense to do
dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);

Is that right?

Re: [PATCH] rtc: max77686: Remove some dead code

[Christophe JAILLET]

Le 08/05/2021 à 16:38, Edmundo Carmona Antoranz a écrit :
> On Fri, May 7, 2021 at 11:43 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>          if (IS_ERR(info->rtc_dev)) {
>>                  ret = PTR_ERR(info->rtc_dev);
>>                  dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
> 
> Following the recent conversations, I think it might make sense to do
> dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
> 
> Is that right?
> 

Yes, it is right, but it should be done in another patch.

Would you like to give it a try?

CJ

Re: [PATCH] rtc: max77686: Remove some dead code

[Edmundo Carmona Antoranz]

On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> >
> > Following the recent conversations, I think it might make sense to do
> > dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
> >
> > Is that right?
> >
>
> Yes, it is right, but it should be done in another patch.
>
> Would you like to give it a try?
>
Sure, I'll have the patch ready to send it when I see yours on next.
> CJ

Re: [PATCH] rtc: max77686: Remove some dead code

[Alexandre Belloni]

On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > >
> > > Following the recent conversations, I think it might make sense to do
> > > dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
> > >
> > > Is that right?
> > >
> >
> > Yes, it is right, but it should be done in another patch.
> >
> > Would you like to give it a try?
> >
> Sure, I'll have the patch ready to send it when I see yours on next.

Does it make sense to print anything at all? Who would use the output?
Is anyone actually going to read it?


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: max77686: Remove some dead code

[Krzysztof Kozlowski]

On 08/05/2021 01:43, Christophe JAILLET wrote:
> 'ret' is known to be an error pointer here, so it can't be 0.
> Remove this dead code.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/rtc/rtc-max77686.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index d51cc12114cb..ce089ed934ad 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -764,8 +764,6 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  	if (IS_ERR(info->rtc_dev)) {
>  		ret = PTR_ERR(info->rtc_dev);
>  		dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
> -		if (ret == 0)
> -			ret = -EINVAL;
>  		goto err_rtc;


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH] rtc: max77686: Remove some dead code

[Krzysztof Kozlowski]

On 09/05/2021 17:06, Alexandre Belloni wrote:
> On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
>> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
>> <christophe.jaillet@wanadoo.fr> wrote:
>>>
>>>>
>>>> Following the recent conversations, I think it might make sense to do
>>>> dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
>>>>
>>>> Is that right?
>>>>
>>>
>>> Yes, it is right, but it should be done in another patch.
>>>
>>> Would you like to give it a try?
>>>
>> Sure, I'll have the patch ready to send it when I see yours on next.
> 
> Does it make sense to print anything at all? Who would use the output?
> Is anyone actually going to read it?

If the RTC core does not print the message, it should be
dev_err_probe().  However the first is recently preferred - RTC core
should do it for all drivers.  I find such error messages useful - helps
easily spotting regressions via dmesg -l err.


Best regards,
Krzysztof

Re: [PATCH] rtc: max77686: Remove some dead code

[Alexandre Belloni]

On 10/05/2021 08:20:52-0400, Krzysztof Kozlowski wrote:
> On 09/05/2021 17:06, Alexandre Belloni wrote:
> > On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
> >> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
> >> <christophe.jaillet@wanadoo.fr> wrote:
> >>>
> >>>>
> >>>> Following the recent conversations, I think it might make sense to do
> >>>> dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
> >>>>
> >>>> Is that right?
> >>>>
> >>>
> >>> Yes, it is right, but it should be done in another patch.
> >>>
> >>> Would you like to give it a try?
> >>>
> >> Sure, I'll have the patch ready to send it when I see yours on next.
> > 
> > Does it make sense to print anything at all? Who would use the output?
> > Is anyone actually going to read it?
> 
> If the RTC core does not print the message, it should be
> dev_err_probe().  However the first is recently preferred - RTC core
> should do it for all drivers.  I find such error messages useful - helps
> easily spotting regressions via dmesg -l err.
> 

The only error path that will not print a message by default (it is
dev_dbg) is when rtc-ops is NULL which I don't expect would regress
anyway.

A better way to remove the dead code would be to switch to
devm_rtc_allocate_device/devm_rtc_register_device. And even better would
be to take that opportunity to set range_min and range_max ;)

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: max77686: Remove some dead code

[Krzysztof Kozlowski]

On 12/05/2021 12:13, Alexandre Belloni wrote:
> On 10/05/2021 08:20:52-0400, Krzysztof Kozlowski wrote:
>> On 09/05/2021 17:06, Alexandre Belloni wrote:
>>> On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
>>>> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
>>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>>
>>>>>>
>>>>>> Following the recent conversations, I think it might make sense to do
>>>>>> dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
>>>>>>
>>>>>> Is that right?
>>>>>>
>>>>>
>>>>> Yes, it is right, but it should be done in another patch.
>>>>>
>>>>> Would you like to give it a try?
>>>>>
>>>> Sure, I'll have the patch ready to send it when I see yours on next.
>>>
>>> Does it make sense to print anything at all? Who would use the output?
>>> Is anyone actually going to read it?
>>
>> If the RTC core does not print the message, it should be
>> dev_err_probe().  However the first is recently preferred - RTC core
>> should do it for all drivers.  I find such error messages useful - helps
>> easily spotting regressions via dmesg -l err.
>>
> 
> The only error path that will not print a message by default (it is
> dev_dbg) is when rtc-ops is NULL which I don't expect would regress
> anyway.

Then the message in the driver is useless and could be removed.

> A better way to remove the dead code would be to switch to
> devm_rtc_allocate_device/devm_rtc_register_device. And even better would
> be to take that opportunity to set range_min and range_max ;)
> 

The driver already uses devm_rtc_device_register() so I think I don't
follow that part.

Best regards,
Krzysztof

Re: [PATCH] rtc: max77686: Remove some dead code

[Alexandre Belloni]

On 12/05/2021 12:24:26-0400, Krzysztof Kozlowski wrote:
> On 12/05/2021 12:13, Alexandre Belloni wrote:
> > On 10/05/2021 08:20:52-0400, Krzysztof Kozlowski wrote:
> >> On 09/05/2021 17:06, Alexandre Belloni wrote:
> >>> On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
> >>>> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
> >>>> <christophe.jaillet@wanadoo.fr> wrote:
> >>>>>
> >>>>>>
> >>>>>> Following the recent conversations, I think it might make sense to do
> >>>>>> dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
> >>>>>>
> >>>>>> Is that right?
> >>>>>>
> >>>>>
> >>>>> Yes, it is right, but it should be done in another patch.
> >>>>>
> >>>>> Would you like to give it a try?
> >>>>>
> >>>> Sure, I'll have the patch ready to send it when I see yours on next.
> >>>
> >>> Does it make sense to print anything at all? Who would use the output?
> >>> Is anyone actually going to read it?
> >>
> >> If the RTC core does not print the message, it should be
> >> dev_err_probe().  However the first is recently preferred - RTC core
> >> should do it for all drivers.  I find such error messages useful - helps
> >> easily spotting regressions via dmesg -l err.
> >>
> > 
> > The only error path that will not print a message by default (it is
> > dev_dbg) is when rtc-ops is NULL which I don't expect would regress
> > anyway.
> 
> Then the message in the driver is useless and could be removed.
> 
> > A better way to remove the dead code would be to switch to
> > devm_rtc_allocate_device/devm_rtc_register_device. And even better would
> > be to take that opportunity to set range_min and range_max ;)
> > 
> 
> The driver already uses devm_rtc_device_register() so I think I don't
> follow that part.

devm_rtc_device_register is different from devm_rtc_register_device.

> 
> Best regards,
> Krzysztof

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: max77686: Remove some dead code

[Christophe JAILLET]

Le 12/05/2021 à 18:13, Alexandre Belloni a écrit :
> On 10/05/2021 08:20:52-0400, Krzysztof Kozlowski wrote:
>> On 09/05/2021 17:06, Alexandre Belloni wrote:
>>> On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
>>>> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
>>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>>
>>>>>>
>>>>>> Following the recent conversations, I think it might make sense to do
>>>>>> dev_err(&pdev->dev, "Failed to register RTC device: %pe\n", info->rtc_dev);
>>>>>>
>>>>>> Is that right?
>>>>>>
>>>>>
>>>>> Yes, it is right, but it should be done in another patch.
>>>>>
>>>>> Would you like to give it a try?
>>>>>
>>>> Sure, I'll have the patch ready to send it when I see yours on next.
>>>
>>> Does it make sense to print anything at all? Who would use the output?
>>> Is anyone actually going to read it?
>>
>> If the RTC core does not print the message, it should be
>> dev_err_probe().  However the first is recently preferred - RTC core
>> should do it for all drivers.  I find such error messages useful - helps
>> easily spotting regressions via dmesg -l err.
>>
> 
> The only error path that will not print a message by default (it is
> dev_dbg) is when rtc-ops is NULL which I don't expect would regress
> anyway.
> 
> A better way to remove the dead code would be to switch to
> devm_rtc_allocate_device/devm_rtc_register_device.

I don't follow you here.
Isn't devm_rtc_device_register = devm_rtc_allocate_device + 
devm_rtc_register_device?

What would be the benefit for switch to the latter?


> And even better would
> be to take that opportunity to set range_min and range_max ;)

Maybe, but this goes beyond my knowledge.
I'll let someone else propose a patch for it.

CJ

Re: [PATCH] rtc: max77686: Remove some dead code

[Alexandre Belloni]

On 12/05/2021 22:02:17+0200, Christophe JAILLET wrote:
> > The only error path that will not print a message by default (it is
> > dev_dbg) is when rtc-ops is NULL which I don't expect would regress
> > anyway.
> > 
> > A better way to remove the dead code would be to switch to
> > devm_rtc_allocate_device/devm_rtc_register_device.
> 
> I don't follow you here.
> Isn't devm_rtc_device_register = devm_rtc_allocate_device +
> devm_rtc_register_device?
> 
> What would be the benefit for switch to the latter?
> 

The immediate benefit is that this solve a possible but very unlikely
race condition around the character device removal when probe ultimately
fails. The other benefit is that I won't have to do it later to handle
the modern features.

> 
> > And even better would
> > be to take that opportunity to set range_min and range_max ;)
> 
> Maybe, but this goes beyond my knowledge.
> I'll let someone else propose a patch for it.
> 
> CJ
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com