| Message ID | 20230304165653.2179835-2-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers | expand |
On Sat, Mar 04, 2023 at 08:56:53AM -0800, Guenter Roeck wrote: > Use devm_add_action_or_reset() to disable the watchdog when the driver > is removed to simplify the code. With this in place, we can use > devm_watchdog_register_device() to register the watchdog, and the removal > function is no longer necessary. While the cleanup in this driver is good ( Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> ) I wonder if disabling the watchdog at .remove() is right. At least there is an inconsistency among watchdog drivers if the hardware is supposed to stop or not. Best regards Uwe
On 3/6/23 01:15, Uwe Kleine-König wrote: > On Sat, Mar 04, 2023 at 08:56:53AM -0800, Guenter Roeck wrote: >> Use devm_add_action_or_reset() to disable the watchdog when the driver >> is removed to simplify the code. With this in place, we can use >> devm_watchdog_register_device() to register the watchdog, and the removal >> function is no longer necessary. > > While the cleanup in this driver is good ( > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > ) I wonder if disabling the watchdog at .remove() is right. > > At least there is an inconsistency among watchdog drivers if the > hardware is supposed to stop or not. > Yes, it is, and it is one of those endless-argument things. Some driver authors insist that the watchdog be stopped, some insist that it isn't. That is why we have watchdog_stop_on_unregister(). Note I didn't use that here because the watchdog isn't stopped on unregister but just disabled. That is slightly different, and I didn't want to change functionality. Guenter
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index a1fcb79b0b7c..58b262ca4e88 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -623,6 +623,11 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev) return variant; } +static void s3c2410wdt_wdt_disable_action(void *data) +{ + s3c2410wdt_enable(data, false); +} + static int s3c2410wdt_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -724,13 +729,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev) s3c2410wdt_stop(&wdt->wdt_device); } - ret = watchdog_register_device(&wdt->wdt_device); + ret = devm_watchdog_register_device(dev, &wdt->wdt_device); if (ret) return ret; ret = s3c2410wdt_enable(wdt, true); if (ret < 0) - goto err_unregister; + return ret; + + ret = devm_add_action_or_reset(dev, s3c2410wdt_wdt_disable_action, wdt); + if (ret) + return ret; platform_set_drvdata(pdev, wdt); @@ -744,25 +753,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev) (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); return 0; - - err_unregister: - watchdog_unregister_device(&wdt->wdt_device); - - return ret; -} - -static int s3c2410wdt_remove(struct platform_device *dev) -{ - int ret; - struct s3c2410_wdt *wdt = platform_get_drvdata(dev); - - ret = s3c2410wdt_enable(wdt, false); - if (ret < 0) - return ret; - - watchdog_unregister_device(&wdt->wdt_device); - - return 0; } static void s3c2410wdt_shutdown(struct platform_device *dev) @@ -817,7 +807,6 @@ static DEFINE_SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops, static struct platform_driver s3c2410wdt_driver = { .probe = s3c2410wdt_probe, - .remove = s3c2410wdt_remove, .shutdown = s3c2410wdt_shutdown, .id_table = s3c2410_wdt_ids, .driver = {
Use devm_add_action_or_reset() to disable the watchdog when the driver is removed to simplify the code. With this in place, we can use devm_watchdog_register_device() to register the watchdog, and the removal function is no longer necessary. Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-)