Re: [PATCH 23/23] watchdog: tangox_wdt: Convert to use device managed functions and other improvements
On 10/04/2019 15:37, Guenter Roeck wrote: > On 4/10/19 6:04 AM, Marc Gonzalez wrote: > >> On 09/04/2019 19:24, Guenter Roeck wrote: >> >>> Use device managed functions to simplify error handling, reduce >>> source code size, improve readability, and reduce the likelihood of bugs. >>> Other improvements as listed below. >>> >>> The conversion was done automatically with coccinelle using the >>> following semantic patches. The semantic patches and the scripts >>> used to generate this commit log are available at >>> https://github.com/groeck/coccinelle-patches >>> >>> - Drop assignments to otherwise unused variables >>> - Drop unnecessary braces around conditional return statements >>> - Drop empty remove function >>> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare >>> - Replace stop on remove with call to watchdog_stop_on_unregister() >>> - Use devm_watchdog_register_driver() to register watchdog device >> >> No devm_clk_prepare() in mainline? :-( >> >> https://lore.kernel.org/patchwork/patch/755487/ > > We went through that several times and never succeeded. This was the major > reason why I didn't submit this series earlier since I was hoping for it > to appear at some point. Unfortunately, someone always objected, typically > with comments along the line that it could be misused, or citing individual > examples where the current code in some driver is wrong and should be fixed > instead. > > This isn't really a technical argument: Everything can be misused, and all > code has bugs. Neither is a reason to reject a new useful API. As such, one > has to assume that after refuting such arguments, and even after fixing all > bugs in existing code, the opponents of the new API will come up with other > reasons to reject it. > > At the end, I gave up trying. Feel free to try yourself; I most definitely > won't try it anymore. Using devm_add_action_or_reset() is a bit more clumsy, > but works just as well. Hello Guenter, I am saddened to read about your frustration. I might give it a try, if Dmitry doesn't feel like giving it another shot. Regards.
Re: [PATCH 23/23] watchdog: tangox_wdt: Convert to use device managed functions and other improvements
On 4/10/19 6:04 AM, Marc Gonzalez wrote: On 09/04/2019 19:24, Guenter Roeck wrote: Use device managed functions to simplify error handling, reduce source code size, improve readability, and reduce the likelyhood of bugs. Other improvements as listed below. The conversion was done automatically with coccinelle using the following semantic patches. The semantic patches and the scripts used to generate this commit log are available at https://github.com/groeck/coccinelle-patches - Drop assignments to otherwise unused variables - Drop unnecessary braces around conditional return statements - Drop empty remove function - Use devm_add_action_or_reset() for calls to clk_disable_unprepare - Replace stop on remove with call to watchdog_stop_on_unregister() - Use devm_watchdog_register_driver() to register watchdog device No devm_clk_prepare() in mainline? :-( https://lore.kernel.org/patchwork/patch/755487/ We went through that several times and never succeeded. This was the major reason why I didn't submit this series earlier since I was hoping for it to appear at some point. Unfortunately, someone always objected, typically with comments along the line that it could be misused, or citing individual examples where the current code in some driver is wrong and should be fixed instead. This isn't really a technical argument: Everything can be misused, and all code has bugs. Neither is a reason to reject a new useful API. As such, one has to assume that after refuting such arguments, and even after fixing all bugs in existing code, the opponents of the new API will come up with other reasons to reject it. At the end, I gave up trying. Feel free to try yourself; I most definitely won't try it anymore. Using devm_add_action_or_reset() is a bit more clumsy, but works just as well. Guenter
Re: [PATCH 23/23] watchdog: tangox_wdt: Convert to use device managed functions and other improvements
On 09/04/2019 19:24, Guenter Roeck wrote: > Use device managed functions to simplify error handling, reduce > source code size, improve readability, and reduce the likelyhood of bugs. > Other improvements as listed below. > > The conversion was done automatically with coccinelle using the > following semantic patches. The semantic patches and the scripts > used to generate this commit log are available at > https://github.com/groeck/coccinelle-patches > > - Drop assignments to otherwise unused variables > - Drop unnecessary braces around conditional return statements > - Drop empty remove function > - Use devm_add_action_or_reset() for calls to clk_disable_unprepare > - Replace stop on remove with call to watchdog_stop_on_unregister() > - Use devm_watchdog_register_driver() to register watchdog device No devm_clk_prepare() in mainline? :-( https://lore.kernel.org/patchwork/patch/755487/ Regards.
Re: [PATCH 23/23] watchdog: tangox_wdt: Convert to use device managed functions and other improvements
Guenter Roeck writes:
> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
> Other improvements as listed below.
>
> The conversion was done automatically with coccinelle using the
> following semantic patches. The semantic patches and the scripts
> used to generate this commit log are available at
> https://github.com/groeck/coccinelle-patches
>
> - Drop assignments to otherwise unused variables
> - Drop unnecessary braces around conditional return statements
> - Drop empty remove function
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Replace stop on remove with call to watchdog_stop_on_unregister()
> - Use devm_watchdog_register_driver() to register watchdog device
>
> Cc: Marc Gonzalez
> Cc: Mans Rullgard
> Signed-off-by: Guenter Roeck
> ---
> drivers/watchdog/tangox_wdt.c | 37 ++---
> 1 file changed, 14 insertions(+), 23 deletions(-)
Acked-by: Mans Rullgard
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> index 16611fe0d9d1..1afb0e9d808c 100644
> --- a/drivers/watchdog/tangox_wdt.c
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -108,6 +108,11 @@ static const struct watchdog_ops tangox_wdt_ops = {
> .restart= tangox_wdt_restart,
> };
>
> +static void tangox_clk_disable_unprepare(void *data)
> +{
> + clk_disable_unprepare(data);
> +}
> +
> static int tangox_wdt_probe(struct platform_device *pdev)
> {
> struct tangox_wdt_device *dev;
> @@ -129,12 +134,14 @@ static int tangox_wdt_probe(struct platform_device
> *pdev)
> err = clk_prepare_enable(dev->clk);
> if (err)
> return err;
> + err = devm_add_action_or_reset(>dev,
> +tangox_clk_disable_unprepare, dev->clk);
> + if (err)
> + return err;
>
> dev->clk_rate = clk_get_rate(dev->clk);
> - if (!dev->clk_rate) {
> - err = -EINVAL;
> - goto err;
> - }
> + if (!dev->clk_rate)
> + return -EINVAL;
>
> dev->wdt.parent = >dev;
> dev->wdt.info = _wdt_info;
> @@ -168,31 +175,16 @@ static int tangox_wdt_probe(struct platform_device
> *pdev)
>
> watchdog_set_restart_priority(>wdt, 128);
>
> - err = watchdog_register_device(>wdt);
> + watchdog_stop_on_unregister(>wdt);
> + err = devm_watchdog_register_device(>dev, >wdt);
> if (err)
> - goto err;
> + return err;
>
> platform_set_drvdata(pdev, dev);
>
> dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
>
> return 0;
> -
> - err:
> - clk_disable_unprepare(dev->clk);
> - return err;
> -}
> -
> -static int tangox_wdt_remove(struct platform_device *pdev)
> -{
> - struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
> -
> - tangox_wdt_stop(>wdt);
> - clk_disable_unprepare(dev->clk);
> -
> - watchdog_unregister_device(>wdt);
> -
> - return 0;
> }
>
> static const struct of_device_id tangox_wdt_dt_ids[] = {
> @@ -204,7 +196,6 @@ MODULE_DEVICE_TABLE(of, tangox_wdt_dt_ids);
>
> static struct platform_driver tangox_wdt_driver = {
> .probe = tangox_wdt_probe,
> - .remove = tangox_wdt_remove,
> .driver = {
> .name = "tangox-wdt",
> .of_match_table = tangox_wdt_dt_ids,
> --
> 2.7.4
>
--
Måns Rullgård
[PATCH 23/23] watchdog: tangox_wdt: Convert to use device managed functions and other improvements
Use device managed functions to simplify error handling, reduce
source code size, improve readability, and reduce the likelyhood of bugs.
Other improvements as listed below.
The conversion was done automatically with coccinelle using the
following semantic patches. The semantic patches and the scripts
used to generate this commit log are available at
https://github.com/groeck/coccinelle-patches
- Drop assignments to otherwise unused variables
- Drop unnecessary braces around conditional return statements
- Drop empty remove function
- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace stop on remove with call to watchdog_stop_on_unregister()
- Use devm_watchdog_register_driver() to register watchdog device
Cc: Marc Gonzalez
Cc: Mans Rullgard
Signed-off-by: Guenter Roeck
---
drivers/watchdog/tangox_wdt.c | 37 ++---
1 file changed, 14 insertions(+), 23 deletions(-)
diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index 16611fe0d9d1..1afb0e9d808c 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -108,6 +108,11 @@ static const struct watchdog_ops tangox_wdt_ops = {
.restart= tangox_wdt_restart,
};
+static void tangox_clk_disable_unprepare(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
static int tangox_wdt_probe(struct platform_device *pdev)
{
struct tangox_wdt_device *dev;
@@ -129,12 +134,14 @@ static int tangox_wdt_probe(struct platform_device *pdev)
err = clk_prepare_enable(dev->clk);
if (err)
return err;
+ err = devm_add_action_or_reset(>dev,
+ tangox_clk_disable_unprepare, dev->clk);
+ if (err)
+ return err;
dev->clk_rate = clk_get_rate(dev->clk);
- if (!dev->clk_rate) {
- err = -EINVAL;
- goto err;
- }
+ if (!dev->clk_rate)
+ return -EINVAL;
dev->wdt.parent = >dev;
dev->wdt.info = _wdt_info;
@@ -168,31 +175,16 @@ static int tangox_wdt_probe(struct platform_device *pdev)
watchdog_set_restart_priority(>wdt, 128);
- err = watchdog_register_device(>wdt);
+ watchdog_stop_on_unregister(>wdt);
+ err = devm_watchdog_register_device(>dev, >wdt);
if (err)
- goto err;
+ return err;
platform_set_drvdata(pdev, dev);
dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
return 0;
-
- err:
- clk_disable_unprepare(dev->clk);
- return err;
-}
-
-static int tangox_wdt_remove(struct platform_device *pdev)
-{
- struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
-
- tangox_wdt_stop(>wdt);
- clk_disable_unprepare(dev->clk);
-
- watchdog_unregister_device(>wdt);
-
- return 0;
}
static const struct of_device_id tangox_wdt_dt_ids[] = {
@@ -204,7 +196,6 @@ MODULE_DEVICE_TABLE(of, tangox_wdt_dt_ids);
static struct platform_driver tangox_wdt_driver = {
.probe = tangox_wdt_probe,
- .remove = tangox_wdt_remove,
.driver = {
.name = "tangox-wdt",
.of_match_table = tangox_wdt_dt_ids,
--
2.7.4
