'ret' is known to be an error pointer here, so it can't be 0.
Remove this dead code.
Signed-off-by: Christophe JAILLET <[email protected]>
---
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
On Fri, May 7, 2021 at 11:43 PM Christophe JAILLET
<[email protected]> 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?
Le 08/05/2021 à 16:38, Edmundo Carmona Antoranz a écrit :
> On Fri, May 7, 2021 at 11:43 PM Christophe JAILLET
> <[email protected]> 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
On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
<[email protected]> 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
On 08/05/2021 18:06:03-0600, Edmundo Carmona Antoranz wrote:
> On Sat, May 8, 2021 at 10:59 AM Christophe JAILLET
> <[email protected]> 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
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
>> <[email protected]> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
Krzysztof
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
> >> <[email protected]> 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
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
>>>> <[email protected]> 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
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
> >>>> <[email protected]> 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
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
>>>> <[email protected]> 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
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