LinuxLists.cc - [PATCH 00/22] watchdog: Expand use of device managed functions (series 3 of 3)

2019-04-10 18:45:30

Subject: [PATCH 00/22] watchdog: Expand use of device managed functions (series 3 of 3)

Use device managed functions and other changes to simplify error handling,
reduce source code size, improve readability, and reduce the likelyhood
of bugs.

The changes made in this series can be summarized to

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Use devm_watchdog_register_driver() to register watchdog device
- Replace 'of_clk_get(np, 0)' with 'devm_clk_get(dev, NULL)'
- Drop assignments to otherwise unused variables
- Drop unnecessary braces around conditional return statements
- Drop empty remove functions
- Replace shutdown function with call to watchdog_stop_on_reboot()
- Replace stop on remove with call to watchdog_stop_on_unregister()
- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'ret = e; return ret;' with 'return e;'.
- Use local variable 'struct device *dev' consistently
- Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly
- Drop unnecessary calls to platform_set_drvdata()

Conversions were performed automatically with coccinelle using a number
of semantic patches. The semantic patches and the scripts used to generate
commit logs are available at https://github.com/groeck/coccinelle-patches.
All patches were compile tested and manually reviewed.

This is the third of three series of similar patches for watchdog drivers.
More patches may follow over time.

2019-04-10 18:45:38

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 02/22] watchdog: tqmx86_wdt: Use 'dev' instead of dereferencing it repeatedly

2019-04-10 18:45:44

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 04/22] watchdog: ts72xx_wdt: Use 'dev' instead of dereferencing it repeatedly

2019-04-10 18:46:02

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 16/22] watchdog: pic32-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 empty remove function
- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly
- Use devm_watchdog_register_driver() to register watchdog device

Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/pic32-wdt.c | 58 ++++++++++++++++++--------------------------
1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/watchdog/pic32-wdt.c b/drivers/watchdog/pic32-wdt.c
index 455ec38363a6..540500940cc0 100644
--- a/drivers/watchdog/pic32-wdt.c
+++ b/drivers/watchdog/pic32-wdt.c
@@ -166,13 +166,19 @@ static const struct of_device_id pic32_wdt_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, pic32_wdt_dt_ids);

+static void pic32_clk_disable_unprepare(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
static int pic32_wdt_drv_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
int ret;
struct watchdog_device *wdd = &pic32_wdd;
struct pic32_wdt *wdt;

- wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;

@@ -180,73 +186,57 @@ static int pic32_wdt_drv_probe(struct platform_device *pdev)
if (IS_ERR(wdt->regs))
return PTR_ERR(wdt->regs);

- wdt->rst_base = devm_ioremap(&pdev->dev, PIC32_BASE_RESET, 0x10);
+ wdt->rst_base = devm_ioremap(dev, PIC32_BASE_RESET, 0x10);
if (!wdt->rst_base)
return -ENOMEM;

- wdt->clk = devm_clk_get(&pdev->dev, NULL);
+ wdt->clk = devm_clk_get(dev, NULL);
if (IS_ERR(wdt->clk)) {
- dev_err(&pdev->dev, "clk not found\n");
+ dev_err(dev, "clk not found\n");
return PTR_ERR(wdt->clk);
}

ret = clk_prepare_enable(wdt->clk);
if (ret) {
- dev_err(&pdev->dev, "clk enable failed\n");
+ dev_err(dev, "clk enable failed\n");
return ret;
}
+ ret = devm_add_action_or_reset(dev, pic32_clk_disable_unprepare,
+ wdt->clk);
+ if (ret)
+ return ret;

if (pic32_wdt_is_win_enabled(wdt)) {
- dev_err(&pdev->dev, "windowed-clear mode is not supported.\n");
- ret = -ENODEV;
- goto out_disable_clk;
+ dev_err(dev, "windowed-clear mode is not supported.\n");
+ return -ENODEV;
}

- wdd->timeout = pic32_wdt_get_timeout_secs(wdt, &pdev->dev);
+ wdd->timeout = pic32_wdt_get_timeout_secs(wdt, dev);
if (!wdd->timeout) {
- dev_err(&pdev->dev,
- "failed to read watchdog register timeout\n");
- ret = -EINVAL;
- goto out_disable_clk;
+ dev_err(dev, "failed to read watchdog register timeout\n");
+ return -EINVAL;
}

- dev_info(&pdev->dev, "timeout %d\n", wdd->timeout);
+ dev_info(dev, "timeout %d\n", wdd->timeout);

wdd->bootstatus = pic32_wdt_bootstatus(wdt) ? WDIOF_CARDRESET : 0;

watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
watchdog_set_drvdata(wdd, wdt);

- ret = watchdog_register_device(wdd);
+ ret = devm_watchdog_register_device(dev, wdd);
if (ret) {
- dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
- goto out_disable_clk;
+ dev_err(dev, "watchdog register failed, err %d\n", ret);
+ return ret;
}

platform_set_drvdata(pdev, wdd);

return 0;
-
-out_disable_clk:
- clk_disable_unprepare(wdt->clk);
-
- return ret;
-}
-
-static int pic32_wdt_drv_remove(struct platform_device *pdev)
-{
- struct watchdog_device *wdd = platform_get_drvdata(pdev);
- struct pic32_wdt *wdt = watchdog_get_drvdata(wdd);
-
- watchdog_unregister_device(wdd);
- clk_disable_unprepare(wdt->clk);
-
- return 0;
}

static struct platform_driver pic32_wdt_driver = {
.probe = pic32_wdt_drv_probe,
- .remove = pic32_wdt_drv_remove,
.driver = {
.name = "pic32-wdt",
.of_match_table = of_match_ptr(pic32_wdt_dt_ids),
--
2.7.4

2019-04-10 18:46:10

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 17/22] watchdog: loongson1_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
- Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Keguang Zhang <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/loongson1_wdt.c | 48 ++++++++++++++++++----------------------
1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/watchdog/loongson1_wdt.c b/drivers/watchdog/loongson1_wdt.c
index 1119634b5c87..d8075e2affa7 100644
--- a/drivers/watchdog/loongson1_wdt.c
+++ b/drivers/watchdog/loongson1_wdt.c
@@ -83,14 +83,20 @@ static const struct watchdog_ops ls1x_wdt_ops = {
.set_timeout = ls1x_wdt_set_timeout,
};

+static void ls1x_clk_disable_unprepare(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
static int ls1x_wdt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct ls1x_wdt_drvdata *drvdata;
struct watchdog_device *ls1x_wdt;
unsigned long clk_rate;
int err;

- drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
if (!drvdata)
return -ENOMEM;

@@ -98,21 +104,23 @@ static int ls1x_wdt_probe(struct platform_device *pdev)
if (IS_ERR(drvdata->base))
return PTR_ERR(drvdata->base);

- drvdata->clk = devm_clk_get(&pdev->dev, pdev->name);
+ drvdata->clk = devm_clk_get(dev, pdev->name);
if (IS_ERR(drvdata->clk))
return PTR_ERR(drvdata->clk);

err = clk_prepare_enable(drvdata->clk);
if (err) {
- dev_err(&pdev->dev, "clk enable failed\n");
+ dev_err(dev, "clk enable failed\n");
return err;
}
+ err = devm_add_action_or_reset(dev, ls1x_clk_disable_unprepare,
+ drvdata->clk);
+ if (err)
+ return err;

clk_rate = clk_get_rate(drvdata->clk);
- if (!clk_rate) {
- err = -EINVAL;
- goto err0;
- }
+ if (!clk_rate)
+ return -EINVAL;
drvdata->clk_rate = clk_rate;

ls1x_wdt = &drvdata->wdt;
@@ -121,41 +129,27 @@ static int ls1x_wdt_probe(struct platform_device *pdev)
ls1x_wdt->timeout = DEFAULT_HEARTBEAT;
ls1x_wdt->min_timeout = 1;
ls1x_wdt->max_hw_heartbeat_ms = U32_MAX / clk_rate * 1000;
- ls1x_wdt->parent = &pdev->dev;
+ ls1x_wdt->parent = dev;

- watchdog_init_timeout(ls1x_wdt, heartbeat, &pdev->dev);
+ watchdog_init_timeout(ls1x_wdt, heartbeat, dev);
watchdog_set_nowayout(ls1x_wdt, nowayout);
watchdog_set_drvdata(ls1x_wdt, drvdata);

- err = watchdog_register_device(&drvdata->wdt);
+ err = devm_watchdog_register_device(dev, &drvdata->wdt);
if (err) {
- dev_err(&pdev->dev, "failed to register watchdog device\n");
- goto err0;
+ dev_err(dev, "failed to register watchdog device\n");
+ return err;
}

platform_set_drvdata(pdev, drvdata);

- dev_info(&pdev->dev, "Loongson1 Watchdog driver registered\n");
-
- return 0;
-err0:
- clk_disable_unprepare(drvdata->clk);
- return err;
-}
-
-static int ls1x_wdt_remove(struct platform_device *pdev)
-{
- struct ls1x_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
- watchdog_unregister_device(&drvdata->wdt);
- clk_disable_unprepare(drvdata->clk);
+ dev_info(dev, "Loongson1 Watchdog driver registered\n");

return 0;
}

static struct platform_driver ls1x_wdt_driver = {
.probe = ls1x_wdt_probe,
- .remove = ls1x_wdt_remove,
.driver = {
.name = "ls1x-wdt",
},
--
2.7.4

2019-04-10 18:46:33

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 20/22] watchdog: jz4740_wdt: Use 'dev' instead of dereferencing it repeatedly

Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly. Also, there is no call to dev_get_drvdata()
or platform_get_drvdata() in the driver, so drop the unnecessary
call to platform_set_drvdata().

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

Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/jz4740_wdt.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index aa9d3523addd..d1bc7cbd4f2b 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -163,11 +163,12 @@ MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);

static int jz4740_wdt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct jz4740_wdt_drvdata *drvdata;
struct watchdog_device *jz4740_wdt;
int ret;

- drvdata = devm_kzalloc(&pdev->dev, sizeof(struct jz4740_wdt_drvdata),
+ drvdata = devm_kzalloc(dev, sizeof(struct jz4740_wdt_drvdata),
GFP_KERNEL);
if (!drvdata)
return -ENOMEM;
@@ -181,7 +182,7 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
jz4740_wdt->timeout = heartbeat;
jz4740_wdt->min_timeout = 1;
jz4740_wdt->max_timeout = MAX_HEARTBEAT;
- jz4740_wdt->parent = &pdev->dev;
+ jz4740_wdt->parent = dev;
watchdog_set_nowayout(jz4740_wdt, nowayout);
watchdog_set_drvdata(jz4740_wdt, drvdata);

@@ -189,18 +190,16 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
if (IS_ERR(drvdata->base))
return PTR_ERR(drvdata->base);

- drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
+ drvdata->rtc_clk = devm_clk_get(dev, "rtc");
if (IS_ERR(drvdata->rtc_clk)) {
- dev_err(&pdev->dev, "cannot find RTC clock\n");
+ dev_err(dev, "cannot find RTC clock\n");
return PTR_ERR(drvdata->rtc_clk);
}

- ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
+ ret = devm_watchdog_register_device(dev, &drvdata->wdt);
if (ret < 0)
return ret;

- platform_set_drvdata(pdev, drvdata);
-
return 0;
}

--
2.7.4

2019-04-10 18:46:34

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 21/22] watchdog: mpc8xxx_wdt: Convert to use device managed functions and other improvements

2019-04-10 18:47:05

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 10/22] watchdog: imx_sc_wdt: Use 'dev' instead of dereferencing it repeatedly

2019-04-10 18:47:10

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 13/22] watchdog: stm32_iwdg: 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 empty remove function
- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Maxime Coquelin <[email protected]>
Cc: Alexandre Torgue <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/stm32_iwdg.c | 65 +++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index 309563e002b8..68756a40eb36 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -138,38 +138,52 @@ static int stm32_iwdg_set_timeout(struct watchdog_device *wdd,
return 0;
}

+static void stm32_clk_disable_unprepare(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
static int stm32_iwdg_clk_init(struct platform_device *pdev,
struct stm32_iwdg *wdt)
{
+ struct device *dev = &pdev->dev;
u32 ret;

- wdt->clk_lsi = devm_clk_get(&pdev->dev, "lsi");
+ wdt->clk_lsi = devm_clk_get(dev, "lsi");
if (IS_ERR(wdt->clk_lsi)) {
- dev_err(&pdev->dev, "Unable to get lsi clock\n");
+ dev_err(dev, "Unable to get lsi clock\n");
return PTR_ERR(wdt->clk_lsi);
}

/* optional peripheral clock */
if (wdt->has_pclk) {
- wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk");
+ wdt->clk_pclk = devm_clk_get(dev, "pclk");
if (IS_ERR(wdt->clk_pclk)) {
- dev_err(&pdev->dev, "Unable to get pclk clock\n");
+ dev_err(dev, "Unable to get pclk clock\n");
return PTR_ERR(wdt->clk_pclk);
}

ret = clk_prepare_enable(wdt->clk_pclk);
if (ret) {
- dev_err(&pdev->dev, "Unable to prepare pclk clock\n");
+ dev_err(dev, "Unable to prepare pclk clock\n");
return ret;
}
+ ret = devm_add_action_or_reset(dev,
+ stm32_clk_disable_unprepare,
+ wdt->clk_pclk);
+ if (ret)
+ return ret;
}

ret = clk_prepare_enable(wdt->clk_lsi);
if (ret) {
- dev_err(&pdev->dev, "Unable to prepare lsi clock\n");
- clk_disable_unprepare(wdt->clk_pclk);
+ dev_err(dev, "Unable to prepare lsi clock\n");
return ret;
}
+ ret = devm_add_action_or_reset(dev, stm32_clk_disable_unprepare,
+ wdt->clk_lsi);
+ if (ret)
+ return ret;

wdt->rate = clk_get_rate(wdt->clk_lsi);

@@ -199,16 +213,17 @@ MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);

static int stm32_iwdg_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct watchdog_device *wdd;
const struct of_device_id *match;
struct stm32_iwdg *wdt;
int ret;

- match = of_match_device(stm32_iwdg_of_match, &pdev->dev);
+ match = of_match_device(stm32_iwdg_of_match, dev);
if (!match)
return -ENODEV;

- wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;

@@ -217,7 +232,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
/* This is the timer base. */
wdt->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(wdt->regs)) {
- dev_err(&pdev->dev, "Could not get resource\n");
+ dev_err(dev, "Could not get resource\n");
return PTR_ERR(wdt->regs);
}

@@ -231,46 +246,28 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
wdd->ops = &stm32_iwdg_ops;
wdd->min_timeout = ((RLR_MIN + 1) * 256) / wdt->rate;
wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * 256 * 1000) / wdt->rate;
- wdd->parent = &pdev->dev;
+ wdd->parent = dev;

watchdog_set_drvdata(wdd, wdt);
watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);

- ret = watchdog_init_timeout(wdd, 0, &pdev->dev);
+ ret = watchdog_init_timeout(wdd, 0, dev);
if (ret)
- dev_warn(&pdev->dev,
- "unable to set timeout value, using default\n");
+ dev_warn(dev, "unable to set timeout value, using default\n");

- ret = watchdog_register_device(wdd);
+ ret = devm_watchdog_register_device(dev, wdd);
if (ret) {
- dev_err(&pdev->dev, "failed to register watchdog device\n");
- goto err;
+ dev_err(dev, "failed to register watchdog device\n");
+ return ret;
}

platform_set_drvdata(pdev, wdt);

return 0;
-err:
- clk_disable_unprepare(wdt->clk_lsi);
- clk_disable_unprepare(wdt->clk_pclk);
-
- return ret;
-}
-
-static int stm32_iwdg_remove(struct platform_device *pdev)
-{
- struct stm32_iwdg *wdt = platform_get_drvdata(pdev);
-
- watchdog_unregister_device(&wdt->wdd);
- clk_disable_unprepare(wdt->clk_lsi);
- clk_disable_unprepare(wdt->clk_pclk);
-
- return 0;
}

static struct platform_driver stm32_iwdg_driver = {
.probe = stm32_iwdg_probe,
- .remove = stm32_iwdg_remove,
.driver = {
.name = "iwdg",
.of_match_table = of_match_ptr(stm32_iwdg_of_match),
--
2.7.4

2019-04-10 18:47:20

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 18/22] watchdog: mt7621_wdt: Use 'dev' instead of dereferencing it repeatedly

2019-04-10 18:47:26

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 11/22] watchdog: sbsa_gwdt: Convert to use device managed functions and other improvements

2019-04-10 18:47:47

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 06/22] watchdog: uniphier_wdt: drop platform_set_drvdata

There is no call to dev_get_drvdata() or platform_get_drvdata() in the
driver. Drop the unnecessary call to platform_set_drvdata().

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

Cc: Masahiro Yamada <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/uniphier_wdt.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/watchdog/uniphier_wdt.c b/drivers/watchdog/uniphier_wdt.c
index e20a7a459d69..8e9242c23022 100644
--- a/drivers/watchdog/uniphier_wdt.c
+++ b/drivers/watchdog/uniphier_wdt.c
@@ -191,8 +191,6 @@ static int uniphier_wdt_probe(struct platform_device *pdev)
if (!wdev)
return -ENOMEM;

- platform_set_drvdata(pdev, wdev);
-
parent = of_get_parent(dev->of_node); /* parent should be syscon node */
regmap = syscon_node_to_regmap(parent);
of_node_put(parent);
--
2.7.4

2019-04-10 18:47:51

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 05/22] watchdog: twl4030_wdt: Convert to use device managed functions and other improvements

2019-04-10 18:48:02

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 22/22] watchdog: pnx4008_wdt: Convert to use device managed functions and other improvements

2019-04-10 18:59:38

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 03/22] watchdog: ts4800_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 empty remove function
- Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly
- Use devm_watchdog_register_driver() to register watchdog device

Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/ts4800_wdt.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/watchdog/ts4800_wdt.c b/drivers/watchdog/ts4800_wdt.c
index 89843b16b04a..9dc6d7f45806 100644
--- a/drivers/watchdog/ts4800_wdt.c
+++ b/drivers/watchdog/ts4800_wdt.c
@@ -108,7 +108,8 @@ static const struct watchdog_info ts4800_wdt_info = {

static int ts4800_wdt_probe(struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
struct device_node *syscon_np;
struct watchdog_device *wdd;
struct ts4800_wdt *wdt;
@@ -117,18 +118,18 @@ static int ts4800_wdt_probe(struct platform_device *pdev)

syscon_np = of_parse_phandle(np, "syscon", 0);
if (!syscon_np) {
- dev_err(&pdev->dev, "no syscon property\n");
+ dev_err(dev, "no syscon property\n");
return -ENODEV;
}

ret = of_property_read_u32_index(np, "syscon", 1, &reg);
if (ret < 0) {
- dev_err(&pdev->dev, "no offset in syscon\n");
+ dev_err(dev, "no offset in syscon\n");
return ret;
}

/* allocate memory for watchdog struct */
- wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;

@@ -137,13 +138,13 @@ static int ts4800_wdt_probe(struct platform_device *pdev)
wdt->regmap = syscon_node_to_regmap(syscon_np);
of_node_put(syscon_np);
if (IS_ERR(wdt->regmap)) {
- dev_err(&pdev->dev, "cannot get parent's regmap\n");
+ dev_err(dev, "cannot get parent's regmap\n");
return PTR_ERR(wdt->regmap);
}

/* Initialize struct watchdog_device */
wdd = &wdt->wdd;
- wdd->parent = &pdev->dev;
+ wdd->parent = dev;
wdd->info = &ts4800_wdt_info;
wdd->ops = &ts4800_wdt_ops;
wdd->min_timeout = ts4800_wdt_map[0].timeout;
@@ -151,7 +152,7 @@ static int ts4800_wdt_probe(struct platform_device *pdev)

watchdog_set_drvdata(wdd, wdt);
watchdog_set_nowayout(wdd, nowayout);
- watchdog_init_timeout(wdd, 0, &pdev->dev);
+ watchdog_init_timeout(wdd, 0, dev);

/*
* As this watchdog supports only a few values, ts4800_wdt_set_timeout
@@ -169,31 +170,20 @@ static int ts4800_wdt_probe(struct platform_device *pdev)
*/
ts4800_wdt_stop(wdd);

- ret = watchdog_register_device(wdd);
+ ret = devm_watchdog_register_device(dev, wdd);
if (ret) {
- dev_err(&pdev->dev,
- "failed to register watchdog device\n");
+ dev_err(dev, "failed to register watchdog device\n");
return ret;
}

platform_set_drvdata(pdev, wdt);

- dev_info(&pdev->dev,
- "initialized (timeout = %d sec, nowayout = %d)\n",
+ dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
wdd->timeout, nowayout);

return 0;
}

-static int ts4800_wdt_remove(struct platform_device *pdev)
-{
- struct ts4800_wdt *wdt = platform_get_drvdata(pdev);
-
- watchdog_unregister_device(&wdt->wdd);
-
- return 0;
-}
-
static const struct of_device_id ts4800_wdt_of_match[] = {
{ .compatible = "technologic,ts4800-wdt", },
{ },
@@ -202,7 +192,6 @@ MODULE_DEVICE_TABLE(of, ts4800_wdt_of_match);

static struct platform_driver ts4800_wdt_driver = {
.probe = ts4800_wdt_probe,
- .remove = ts4800_wdt_remove,
.driver = {
.name = "ts4800_wdt",
.of_match_table = ts4800_wdt_of_match,
--
2.7.4

2019-04-10 18:59:41

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 01/22] watchdog: tegra_wdt: Use watchdog_stop_on_unregister and other improvements

Use watchdog_stop_on_unregister() in probe instead of calling
tegra_wdt_stop() in the remove function. Also introduce local variable
'struct device *dev' and use it instead of dereferencing it repeatedly.
Finally, drop the now empty remove function.

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

Cc: Thierry Reding <[email protected]>
Cc: Jonathan Hunter <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/tegra_wdt.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
index fc3cf5edf6c7..a58b000acc4f 100644
--- a/drivers/watchdog/tegra_wdt.c
+++ b/drivers/watchdog/tegra_wdt.c
@@ -181,6 +181,7 @@ static const struct watchdog_ops tegra_wdt_ops = {

static int tegra_wdt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct watchdog_device *wdd;
struct tegra_wdt *wdt;
void __iomem *regs;
@@ -195,7 +196,7 @@ static int tegra_wdt_probe(struct platform_device *pdev)
* Allocate our watchdog driver data, which has the
* struct watchdog_device nested within it.
*/
- wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;

@@ -210,39 +211,27 @@ static int tegra_wdt_probe(struct platform_device *pdev)
wdd->ops = &tegra_wdt_ops;
wdd->min_timeout = MIN_WDT_TIMEOUT;
wdd->max_timeout = MAX_WDT_TIMEOUT;
- wdd->parent = &pdev->dev;
+ wdd->parent = dev;

watchdog_set_drvdata(wdd, wdt);

watchdog_set_nowayout(wdd, nowayout);

- ret = devm_watchdog_register_device(&pdev->dev, wdd);
+ watchdog_stop_on_unregister(wdd);
+ ret = devm_watchdog_register_device(dev, wdd);
if (ret) {
- dev_err(&pdev->dev,
- "failed to register watchdog device\n");
+ dev_err(dev, "failed to register watchdog device\n");
return ret;
}

platform_set_drvdata(pdev, wdt);

- dev_info(&pdev->dev,
- "initialized (heartbeat = %d sec, nowayout = %d)\n",
+ dev_info(dev, "initialized (heartbeat = %d sec, nowayout = %d)\n",
heartbeat, nowayout);

return 0;
}

-static int tegra_wdt_remove(struct platform_device *pdev)
-{
- struct tegra_wdt *wdt = platform_get_drvdata(pdev);
-
- tegra_wdt_stop(&wdt->wdd);
-
- dev_info(&pdev->dev, "removed wdt\n");
-
- return 0;
-}
-
#ifdef CONFIG_PM_SLEEP
static int tegra_wdt_runtime_suspend(struct device *dev)
{
@@ -278,7 +267,6 @@ static const struct dev_pm_ops tegra_wdt_pm_ops = {

static struct platform_driver tegra_wdt_driver = {
.probe = tegra_wdt_probe,
- .remove = tegra_wdt_remove,
.driver = {
.name = "tegra-wdt",
.pm = &tegra_wdt_pm_ops,
--
2.7.4

2019-04-10 18:59:44

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 07/22] watchdog: wdat_wdt: Use 'dev' instead of dereferencing it repeatedly

Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly.

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

Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/wdat_wdt.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 387892fe63b4..430ee4e9b185 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -308,6 +308,7 @@ static const struct watchdog_ops wdat_wdt_ops = {

static int wdat_wdt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
const struct acpi_wdat_entry *entries;
const struct acpi_table_wdat *tbl;
struct wdat_wdt *wdat;
@@ -321,11 +322,11 @@ static int wdat_wdt_probe(struct platform_device *pdev)
if (ACPI_FAILURE(status))
return -ENODEV;

- wdat = devm_kzalloc(&pdev->dev, sizeof(*wdat), GFP_KERNEL);
+ wdat = devm_kzalloc(dev, sizeof(*wdat), GFP_KERNEL);
if (!wdat)
return -ENOMEM;

- regs = devm_kcalloc(&pdev->dev, pdev->num_resources, sizeof(*regs),
+ regs = devm_kcalloc(dev, pdev->num_resources, sizeof(*regs),
GFP_KERNEL);
if (!regs)
return -ENOMEM;
@@ -350,15 +351,15 @@ static int wdat_wdt_probe(struct platform_device *pdev)

res = &pdev->resource[i];
if (resource_type(res) == IORESOURCE_MEM) {
- reg = devm_ioremap_resource(&pdev->dev, res);
+ reg = devm_ioremap_resource(dev, res);
if (IS_ERR(reg))
return PTR_ERR(reg);
} else if (resource_type(res) == IORESOURCE_IO) {
- reg = devm_ioport_map(&pdev->dev, res->start, 1);
+ reg = devm_ioport_map(dev, res->start, 1);
if (!reg)
return -ENOMEM;
} else {
- dev_err(&pdev->dev, "Unsupported resource\n");
+ dev_err(dev, "Unsupported resource\n");
return -EINVAL;
}

@@ -376,12 +377,11 @@ static int wdat_wdt_probe(struct platform_device *pdev)

action = entries[i].action;
if (action >= MAX_WDAT_ACTIONS) {
- dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
- action);
+ dev_dbg(dev, "Skipping unknown action: %u\n", action);
continue;
}

- instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
+ instr = devm_kzalloc(dev, sizeof(*instr), GFP_KERNEL);
if (!instr)
return -ENOMEM;

@@ -398,7 +398,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
r.flags = IORESOURCE_IO;
} else {
- dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
+ dev_dbg(dev, "Unsupported address space: %d\n",
gas->space_id);
continue;
}
@@ -413,14 +413,15 @@ static int wdat_wdt_probe(struct platform_device *pdev)
}

if (!instr->reg) {
- dev_err(&pdev->dev, "I/O resource not found\n");
+ dev_err(dev, "I/O resource not found\n");
return -EINVAL;
}

instructions = wdat->instructions[action];
if (!instructions) {
- instructions = devm_kzalloc(&pdev->dev,
- sizeof(*instructions), GFP_KERNEL);
+ instructions = devm_kzalloc(dev,
+ sizeof(*instructions),
+ GFP_KERNEL);
if (!instructions)
return -ENOMEM;

@@ -441,7 +442,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, wdat);

watchdog_set_nowayout(&wdat->wdd, nowayout);
- return devm_watchdog_register_device(&pdev->dev, &wdat->wdd);
+ return devm_watchdog_register_device(dev, &wdat->wdd);
}

#ifdef CONFIG_PM_SLEEP
--
2.7.4

2019-04-10 19:00:15

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 12/22] watchdog: zx2967_wdt: Convert to use device managed functions and other improvements

2019-04-10 19:00:25

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 09/22] watchdog: xen_wdt: Use 'dev' instead of dereferencing it repeatedly

2019-04-10 19:00:30

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 19/22] watchdog: rt2880_wdt: Convert to use device managed functions and other improvements

2019-04-10 19:01:10

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 15/22] watchdog: pic32-dmt: 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 empty remove function
- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly
- Use devm_watchdog_register_driver() to register watchdog device

Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/pic32-dmt.c | 46 ++++++++++++++++++--------------------------
1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/watchdog/pic32-dmt.c b/drivers/watchdog/pic32-dmt.c
index 052751c224fd..9a3c53e03c60 100644
--- a/drivers/watchdog/pic32-dmt.c
+++ b/drivers/watchdog/pic32-dmt.c
@@ -168,13 +168,19 @@ static struct watchdog_device pic32_dmt_wdd = {
.ops = &pic32_dmt_fops,
};

+static void pic32_clk_disable_unprepare(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
static int pic32_dmt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
int ret;
struct pic32_dmt *dmt;
struct watchdog_device *wdd = &pic32_dmt_wdd;

- dmt = devm_kzalloc(&pdev->dev, sizeof(*dmt), GFP_KERNEL);
+ dmt = devm_kzalloc(dev, sizeof(*dmt), GFP_KERNEL);
if (!dmt)
return -ENOMEM;

@@ -182,54 +188,41 @@ static int pic32_dmt_probe(struct platform_device *pdev)
if (IS_ERR(dmt->regs))
return PTR_ERR(dmt->regs);

- dmt->clk = devm_clk_get(&pdev->dev, NULL);
+ dmt->clk = devm_clk_get(dev, NULL);
if (IS_ERR(dmt->clk)) {
- dev_err(&pdev->dev, "clk not found\n");
+ dev_err(dev, "clk not found\n");
return PTR_ERR(dmt->clk);
}

ret = clk_prepare_enable(dmt->clk);
if (ret)
return ret;
+ ret = devm_add_action_or_reset(dev, pic32_clk_disable_unprepare,
+ dmt->clk);
+ if (ret)
+ return ret;

wdd->timeout = pic32_dmt_get_timeout_secs(dmt);
if (!wdd->timeout) {
- dev_err(&pdev->dev,
- "failed to read watchdog register timeout\n");
- ret = -EINVAL;
- goto out_disable_clk;
+ dev_err(dev, "failed to read watchdog register timeout\n");
+ return -EINVAL;
}

- dev_info(&pdev->dev, "timeout %d\n", wdd->timeout);
+ dev_info(dev, "timeout %d\n", wdd->timeout);

wdd->bootstatus = pic32_dmt_bootstatus(dmt) ? WDIOF_CARDRESET : 0;

watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
watchdog_set_drvdata(wdd, dmt);

- ret = watchdog_register_device(wdd);
+ ret = devm_watchdog_register_device(dev, wdd);
if (ret) {
- dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
- goto out_disable_clk;
+ dev_err(dev, "watchdog register failed, err %d\n", ret);
+ return ret;
}

platform_set_drvdata(pdev, wdd);
return 0;
-
-out_disable_clk:
- clk_disable_unprepare(dmt->clk);
- return ret;
-}
-
-static int pic32_dmt_remove(struct platform_device *pdev)
-{
- struct watchdog_device *wdd = platform_get_drvdata(pdev);
- struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
-
- watchdog_unregister_device(wdd);
- clk_disable_unprepare(dmt->clk);
-
- return 0;
}

static const struct of_device_id pic32_dmt_of_ids[] = {
@@ -240,7 +233,6 @@ MODULE_DEVICE_TABLE(of, pic32_dmt_of_ids);

static struct platform_driver pic32_dmt_driver = {
.probe = pic32_dmt_probe,
- .remove = pic32_dmt_remove,
.driver = {
.name = "pic32-dmt",
.of_match_table = of_match_ptr(pic32_dmt_of_ids),
--
2.7.4

2019-04-10 19:01:12

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 14/22] watchdog: ux500_wdt: Convert to use device managed functions and other improvements

2019-04-10 19:01:28

by Guenter Roeck

[permalink] [raw]

Subject: [PATCH 08/22] watchdog: wm831x_wdt: Use 'dev' instead of dereferencing it repeatedly

Introduce local variable 'struct device *dev' and use it instead of
dereferencing it repeatedly.

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

Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/wm831x_wdt.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/wm831x_wdt.c b/drivers/watchdog/wm831x_wdt.c
index 116c2f47b463..9b6565a3fab4 100644
--- a/drivers/watchdog/wm831x_wdt.c
+++ b/drivers/watchdog/wm831x_wdt.c
@@ -180,8 +180,9 @@ static const struct watchdog_ops wm831x_wdt_ops = {

static int wm831x_wdt_probe(struct platform_device *pdev)
{
- struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
- struct wm831x_pdata *chip_pdata = dev_get_platdata(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct wm831x *wm831x = dev_get_drvdata(dev->parent);
+ struct wm831x_pdata *chip_pdata = dev_get_platdata(dev->parent);
struct wm831x_watchdog_pdata *pdata;
struct wm831x_wdt_drvdata *driver_data;
struct watchdog_device *wm831x_wdt;
@@ -198,8 +199,7 @@ static int wm831x_wdt_probe(struct platform_device *pdev)
if (reg & WM831X_WDOG_DEBUG)
dev_warn(wm831x->dev, "Watchdog is paused\n");

- driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
- GFP_KERNEL);
+ driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
if (!driver_data)
return -ENOMEM;

@@ -210,7 +210,7 @@ static int wm831x_wdt_probe(struct platform_device *pdev)

wm831x_wdt->info = &wm831x_wdt_info;
wm831x_wdt->ops = &wm831x_wdt_ops;
- wm831x_wdt->parent = &pdev->dev;
+ wm831x_wdt->parent = dev;
watchdog_set_nowayout(wm831x_wdt, nowayout);
watchdog_set_drvdata(wm831x_wdt, driver_data);

@@ -240,10 +240,9 @@ static int wm831x_wdt_probe(struct platform_device *pdev)
reg |= pdata->software << WM831X_WDOG_RST_SRC_SHIFT;

if (pdata->update_gpio) {
- ret = devm_gpio_request_one(&pdev->dev,
- pdata->update_gpio,
- GPIOF_OUT_INIT_LOW,
- "Watchdog update");
+ ret = devm_gpio_request_one(dev, pdata->update_gpio,
+ GPIOF_OUT_INIT_LOW,
+ "Watchdog update");
if (ret < 0) {
dev_err(wm831x->dev,
"Failed to request update GPIO: %d\n",
@@ -268,7 +267,7 @@ static int wm831x_wdt_probe(struct platform_device *pdev)
}
}

- ret = devm_watchdog_register_device(&pdev->dev, &driver_data->wdt);
+ ret = devm_watchdog_register_device(dev, &driver_data->wdt);
if (ret != 0) {
dev_err(wm831x->dev, "watchdog_register_device() failed: %d\n",
ret);
--
2.7.4

2019-04-10 19:03:13

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 20/22] watchdog: jz4740_wdt: Use 'dev' instead of dereferencing it repeatedly

On Wed, 2019-04-10 at 09:28 -0700, Guenter Roeck wrote:
> Introduce local variable 'struct device *dev' and use it instead of
> dereferencing it repeatedly. Also, there is no call to dev_get_drvdata()
> or platform_get_drvdata() in the driver, so drop the unnecessary
> call to platform_set_drvdata().

Dropping platform_set_drvdata seems to me like it should
be a separate patch.

And are you sure no other function uses a get_drvdata call?
Maybe something in watchdog_dev.c? Possibly:

#ifdef CONFIG_WATCHDOG_SYSFS
static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct watchdog_device *wdd = dev_get_drvdata(dev);

return sprintf(buf, "%d\n", !!test_bit(WDOG_NO_WAY_OUT, &wdd->status));
}
static DEVICE_ATTR_RO(nowayout);

> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
[]
> @@ -189,18 +190,16 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
[]
> - platform_set_drvdata(pdev, drvdata);
> -
> return 0;
> }
>

2019-04-10 19:08:23

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 20/22] watchdog: jz4740_wdt: Use 'dev' instead of dereferencing it repeatedly

Hi Joe,

On Wed, Apr 10, 2019 at 09:52:09AM -0700, Joe Perches wrote:
> On Wed, 2019-04-10 at 09:28 -0700, Guenter Roeck wrote:
> > Introduce local variable 'struct device *dev' and use it instead of
> > dereferencing it repeatedly. Also, there is no call to dev_get_drvdata()
> > or platform_get_drvdata() in the driver, so drop the unnecessary
> > call to platform_set_drvdata().
>
> Dropping platform_set_drvdata seems to me like it should
> be a separate patch.
>

Bundling all changes into one patch per driver already resulted in more than
60 patches total in this series. Splitting that into three sets of patches
over three days already earned me automated replies telling me that I am now
considered to be a spammer. One logical change per patch would have resulted
in hundreds of patches. I don't think that would have scaled well.

I considered other splits, such as one coccinelle rule per patch, affecting
multiple drivers, but that would have had the same result since it would have
needed dozens of Cc: lines per patch. Ultimately, I decided to go with one patch
per file.

> And are you sure no other function uses a get_drvdata call?
> Maybe something in watchdog_dev.c? Possibly:
>
> #ifdef CONFIG_WATCHDOG_SYSFS
> static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct watchdog_device *wdd = dev_get_drvdata(dev);
>

'dev' in nowayout_show() points to the watchdog device, not to the platform
device. Its drvdata is set in drivers/base/core.c:device_create_groups_vargs().
Not all watchdog drivers are platform drivers, and the watchdog core can not
depend on a watchdog device even having a parent device, much less make
assumptions about its drvdata.

Guenter

2019-04-10 19:12:41

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 18/22] watchdog: mt7621_wdt: Use 'dev' instead of dereferencing it repeatedly

On Wed, 2019-04-10 at 09:27 -0700, Guenter Roeck wrote:
> Introduce local variable 'struct device *dev' and use it instead of
> dereferencing it repeatedly.
>
> 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

Interesting collection. It would be useful to specify which
particular script generated or enabled this patch.

Just scanning briefly, it might have been this one:
https://github.com/groeck/coccinelle-patches/blob/master/common/deref.cocci
But it looks like some manual bit might have been required too.

And trivially:

> diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
[]
> @@ -133,18 +133,19 @@ static struct watchdog_device mt7621_wdt_dev = {
[]
> watchdog_init_timeout(&mt7621_wdt_dev, mt7621_wdt_dev.max_timeout,
> - &pdev->dev);
> + dev);

This could be on one line.

2019-04-10 19:55:04

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 18/22] watchdog: mt7621_wdt: Use 'dev' instead of dereferencing it repeatedly

On Wed, Apr 10, 2019 at 11:46:24AM -0700, Joe Perches wrote:
> On Wed, 2019-04-10 at 09:27 -0700, Guenter Roeck wrote:
> > Introduce local variable 'struct device *dev' and use it instead of
> > dereferencing it repeatedly.
> >
> > 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
>
> Interesting collection. It would be useful to specify which
> particular script generated or enabled this patch.
>

It is pdev-addvar.cocci, rule 'new'. deref.cocci wasn't used for the
watchdog patches. The script to apply the various rules is watchdog/make.sh.

Pointing to the actual scripts used is a good idea. I'll see if I can add
this for subsequent series. After all, the commit log is also auto-generated,
so this should be straightforward.

> Just scanning briefly, it might have been this one:
> https://github.com/groeck/coccinelle-patches/blob/master/common/deref.cocci
> But it looks like some manual bit might have been required too.

Not for this one. There were a couple of situations where I had to manually
split long lines to avoid checkpatch warnings, and I manually updated a few
of the commit logs, but not in this patch.

>
> And trivially:
>
> > diff --git a/drivers/watchdog/mt7621_wdt.c b/drivers/watchdog/mt7621_wdt.c
> []
> > @@ -133,18 +133,19 @@ static struct watchdog_device mt7621_wdt_dev = {
> []
> > watchdog_init_timeout(&mt7621_wdt_dev, mt7621_wdt_dev.max_timeout,
> > - &pdev->dev);
> > + dev);
>
> This could be on one line.
>
Coccinelle isn't perfect. The rule doesn't modify the entire argument list,
only the last argument, so coccinelle missed that it could have merged the
two lines into one.

A checkpatch rule suggesting that multiple extension lines can be merged
might be useful to help finding such situations. Just a thought.

Thanks,
Guenter

2019-04-10 22:11:03

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 18/22] watchdog: mt7621_wdt: Use 'dev' instead of dereferencing it repeatedly

On Wed, 2019-04-10 at 12:54 -0700, Guenter Roeck wrote:
> A checkpatch rule suggesting that multiple extension lines can be merged
> might be useful to help finding such situations. Just a thought.

You are welcome to try to write that one.
It's likely not a trivial task to make it sensible.

cheers, Joe

2019-04-10 22:16:48

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 18/22] watchdog: mt7621_wdt: Use 'dev' instead of dereferencing it repeatedly

Hi Joe,

On Wed, Apr 10, 2019 at 03:10:09PM -0700, Joe Perches wrote:
> On Wed, 2019-04-10 at 12:54 -0700, Guenter Roeck wrote:
> > A checkpatch rule suggesting that multiple extension lines can be merged
> > might be useful to help finding such situations. Just a thought.
>
> You are welcome to try to write that one.
> It's likely not a trivial task to make it sensible.
>

No chance - if _you_ think it is difficult, it would be absolutely hopeless
for me, and I am not going to touch it.

Cheers,
Guenter

2019-04-11 08:49:24

by Shawn Guo

[permalink] [raw]

Subject: Re: [PATCH 10/22] watchdog: imx_sc_wdt: Use 'dev' instead of dereferencing it repeatedly

On Wed, Apr 10, 2019 at 09:27:51AM -0700, Guenter Roeck wrote:
> Introduce local variable 'struct device *dev' and use it instead of
> dereferencing it repeatedly.
>
> 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
>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Pengutronix Kernel Team <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: NXP Linux Team <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>

Acked-by: Shawn Guo <[email protected]>

2019-04-11 08:54:21

by Shawn Guo

[permalink] [raw]

Subject: Re: [PATCH 12/22] watchdog: zx2967_wdt: Convert to use device managed functions and other improvements

On Wed, Apr 10, 2019 at 09:27:53AM -0700, 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 empty remove function
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Use local variable 'struct device *dev' consistently
> - Use devm_watchdog_register_driver() to register watchdog device
>
> Cc: Jun Nie <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>

Acked-by: Shawn Guo <[email protected]>

2019-04-23 13:29:36

by Christophe Leroy

[permalink] [raw]

Subject: Re: [PATCH 21/22] watchdog: mpc8xxx_wdt: Convert to use device managed functions and other improvements

Le 10/04/2019 à 18:28, Guenter Roeck a écrit :
> 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 empty remove function
> This also drops the inaccurate message on remove; the driver won't be
> removed if the hardware watchdog is running, no matter if the watchdog
> device is open or not.
> - Use local variable 'struct device *dev' consistently
> - Use devm_watchdog_register_driver() to register watchdog device
>
> Signed-off-by: Guenter Roeck <[email protected]>

Reviewed-by: Christophe Leroy <[email protected]>

> ---
> drivers/watchdog/mpc8xxx_wdt.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
> index 3b5b446b690c..9b6d6a5a27ad 100644
> --- a/drivers/watchdog/mpc8xxx_wdt.c
> +++ b/drivers/watchdog/mpc8xxx_wdt.c
> @@ -204,9 +204,10 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
> if (ddata->wdd.timeout < ddata->wdd.min_timeout)
> ddata->wdd.timeout = ddata->wdd.min_timeout;
>
> - ret = watchdog_register_device(&ddata->wdd);
> + ret = devm_watchdog_register_device(dev, &ddata->wdd);
> if (ret) {
> - dev_err(dev, "cannot register watchdog device (err=%d)\n", ret);
> + dev_err(dev, "cannot register watchdog device (err=%d)\n",
> + ret);
> return ret;
> }
>
> @@ -218,17 +219,6 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
> return 0;
> }
>
> -static int mpc8xxx_wdt_remove(struct platform_device *ofdev)
> -{
> - struct mpc8xxx_wdt_ddata *ddata = platform_get_drvdata(ofdev);
> -
> - dev_crit(&ofdev->dev, "Watchdog removed, expect the %s soon!\n",
> - reset ? "reset" : "machine check exception");
> - watchdog_unregister_device(&ddata->wdd);
> -
> - return 0;
> -}
> -
> static const struct of_device_id mpc8xxx_wdt_match[] = {
> {
> .compatible = "mpc83xx_wdt",
> @@ -259,7 +249,6 @@ MODULE_DEVICE_TABLE(of, mpc8xxx_wdt_match);
>
> static struct platform_driver mpc8xxx_wdt_driver = {
> .probe = mpc8xxx_wdt_probe,
> - .remove = mpc8xxx_wdt_remove,
> .driver = {
> .name = "mpc8xxx_wdt",
> .of_match_table = mpc8xxx_wdt_match,
>