| Message ID | 20190419181601.7412-9-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: refactor init_timeout and update users | expand |
On Fri, Apr 19, 2019 at 08:15:53PM +0200, Wolfram Sang wrote: > The core will print out details now. > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/watchdog/imx_sc_wdt.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c > index 86c2722f2a09..6dc24ceb1b2c 100644 > --- a/drivers/watchdog/imx_sc_wdt.c > +++ b/drivers/watchdog/imx_sc_wdt.c This driver isn't in next, and I don't know where to look for it. > @@ -117,10 +117,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev) > imx_sc_wdd->parent = &pdev->dev; > imx_sc_wdd->timeout = DEFAULT_TIMEOUT; > > - ret = watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); > - if (ret) > - dev_warn(&pdev->dev, "Failed to set timeout value, using default\n"); > - > + watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); One side effect is however that ret isn't set any more. So I wonder if a failure in watchdog_init_timeout() really makes the core print the details as expected. Best regards Uwe
On 4/29/19 3:15 AM, Uwe Kleine-König wrote: > On Fri, Apr 19, 2019 at 08:15:53PM +0200, Wolfram Sang wrote: >> The core will print out details now. >> >> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> drivers/watchdog/imx_sc_wdt.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c >> index 86c2722f2a09..6dc24ceb1b2c 100644 >> --- a/drivers/watchdog/imx_sc_wdt.c >> +++ b/drivers/watchdog/imx_sc_wdt.c > > This driver isn't in next, and I don't know where to look for it. > Branch watchdog-next of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git >> @@ -117,10 +117,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev) >> imx_sc_wdd->parent = &pdev->dev; >> imx_sc_wdd->timeout = DEFAULT_TIMEOUT; >> >> - ret = watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); >> - if (ret) >> - dev_warn(&pdev->dev, "Failed to set timeout value, using default\n"); >> - >> + watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); > > One side effect is however that ret isn't set any more. So I wonder if a > failure in watchdog_init_timeout() really makes the core print the > details as expected. > Sorry, I don't understand. The warning is printed in watchdog_init_timeout(). What does that have to do with setting ret here or not ? Thanks, Guenter
Hello, On Mon, Apr 29, 2019 at 05:37:12AM -0700, Guenter Roeck wrote: > On 4/29/19 3:15 AM, Uwe Kleine-König wrote: > > On Fri, Apr 19, 2019 at 08:15:53PM +0200, Wolfram Sang wrote: > > > The core will print out details now. > > > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > --- > > > drivers/watchdog/imx_sc_wdt.c | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c > > > index 86c2722f2a09..6dc24ceb1b2c 100644 > > > --- a/drivers/watchdog/imx_sc_wdt.c > > > +++ b/drivers/watchdog/imx_sc_wdt.c > > > > This driver isn't in next, and I don't know where to look for it. > > > > Branch watchdog-next of > git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git Thanks, found it now. > > > @@ -117,10 +117,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev) > > > imx_sc_wdd->parent = &pdev->dev; > > > imx_sc_wdd->timeout = DEFAULT_TIMEOUT; > > > - ret = watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); > > > - if (ret) > > > - dev_warn(&pdev->dev, "Failed to set timeout value, using default\n"); > > > - > > > + watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); > > > > One side effect is however that ret isn't set any more. So I wonder if a > > failure in watchdog_init_timeout() really makes the core print the > > details as expected. > > > > Sorry, I don't understand. The warning is printed in watchdog_init_timeout(). > What does that have to do with setting ret here or not ? Ah, I thought the warning is done in the caller of the modified function. Maybe this means the commit log can be improved to for example: The function watchdog_init_timeout() itself already emits a more informative warning on failure. So drop the driver specific one. Otherwise my concerns are eliminated. Thanks Uwe
diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c index 86c2722f2a09..6dc24ceb1b2c 100644 --- a/drivers/watchdog/imx_sc_wdt.c +++ b/drivers/watchdog/imx_sc_wdt.c @@ -117,10 +117,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev) imx_sc_wdd->parent = &pdev->dev; imx_sc_wdd->timeout = DEFAULT_TIMEOUT; - ret = watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); - if (ret) - dev_warn(&pdev->dev, "Failed to set timeout value, using default\n"); - + watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev); watchdog_stop_on_reboot(imx_sc_wdd); watchdog_stop_on_unregister(imx_sc_wdd);