| Message ID | a6b23ee8d3ea78f62d3fda0b53aa273718f14c6d.1620452523.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rtc: max77686: Remove some dead code | expand |
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?
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
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
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?
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
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
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 ;)
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
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
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
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 >
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; }
'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(-)