[PATCH 05/62] watchdog: bcm2835_wdt: Convert to use device managed functions and other improvements

[PATCH 05/62] watchdog: bcm2835_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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
- Replace of_iomap() with platform_get_resource() followed by
  devm_ioremap_resource()
- Replace &pdev->dev with dev if 'struct device *dev' is a declared
  variable
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Lee Jones <lee@kernel.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list at broadcom.com
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/bcm2835_wdt.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
index 4e0adb6c88f0..496f6c106bb1 100644
--- a/drivers/watchdog/bcm2835_wdt.c
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -172,8 +172,8 @@ static void bcm2835_power_off(void)
 
 static int bcm2835_wdt_probe(struct platform_device *pdev)
 {
+	struct resource *res;
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 	struct bcm2835_wdt *wdt;
 	int err;
 
@@ -184,16 +184,15 @@ static int bcm2835_wdt_probe(struct platform_device *pdev)
 
 	spin_lock_init(&wdt->lock);
 
-	wdt->base = of_iomap(np, 0);
-	if (!wdt->base) {
-		dev_err(dev, "Failed to remap watchdog regs");
-		return -ENODEV;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(wdt->base))
+		return PTR_ERR(wdt->base);
 
 	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
 	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
 	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
-	bcm2835_wdt_wdd.parent = &pdev->dev;
+	bcm2835_wdt_wdd.parent = dev;
 	if (bcm2835_wdt_is_running(wdt)) {
 		/*
 		 * The currently active timeout value (set by the
@@ -208,10 +207,10 @@ static int bcm2835_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_restart_priority(&bcm2835_wdt_wdd, 128);
 
-	err = watchdog_register_device(&bcm2835_wdt_wdd);
+	watchdog_stop_on_reboot(&bcm2835_wdt_wdd);
+	err = devm_watchdog_register_device(dev, &bcm2835_wdt_wdd);
 	if (err) {
 		dev_err(dev, "Failed to register watchdog device");
-		iounmap(wdt->base);
 		return err;
 	}
 
@@ -224,21 +223,12 @@ static int bcm2835_wdt_probe(struct platform_device *pdev)
 
 static int bcm2835_wdt_remove(struct platform_device *pdev)
 {
-	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
-
 	if (pm_power_off == bcm2835_power_off)
 		pm_power_off = NULL;
-	watchdog_unregister_device(&bcm2835_wdt_wdd);
-	iounmap(wdt->base);
 
 	return 0;
 }
 
-static void bcm2835_wdt_shutdown(struct platform_device *pdev)
-{
-	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
-}
-
 static const struct of_device_id bcm2835_wdt_of_match[] = {
 	{ .compatible = "brcm,bcm2835-pm-wdt", },
 	{},
@@ -248,7 +238,6 @@ MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
 static struct platform_driver bcm2835_wdt_driver = {
 	.probe		= bcm2835_wdt_probe,
 	.remove		= bcm2835_wdt_remove,
-	.shutdown	= bcm2835_wdt_shutdown,
 	.driver = {
 		.name =		"bcm2835-wdt",
 		.of_match_table = bcm2835_wdt_of_match,
-- 
2.7.4

[PATCH 05/62] watchdog: bcm2835_wdt: Convert to use device managed functions and other improvements

[Eric Anholt]

Guenter Roeck <linux@roeck-us.net> 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
> - Replace of_iomap() with platform_get_resource() followed by
>   devm_ioremap_resource()

Every time I see this pattern I wish we had a
devm_ioremap_platform_resource().

> - Replace &pdev->dev with dev if 'struct device *dev' is a declared
>   variable
> - Use devm_watchdog_register_driver() to register watchdog device
> - Replace shutdown function with call to watchdog_stop_on_reboot()

I'm trusting you here that this last change is right.  All the rest of
it looks good:

Acked-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170113/817a1030/attachment.sig>

[PATCH 10/62] watchdog: coh901327_wdt: Convert to use device managed functions

[Guenter Roeck]

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

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Use devm_clk_get() if the device parameter is not NULL
- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'val = e; return val;' with 'return e;'
- Replace 'if (e) { return expr; }' with 'if (e) return expr;'
- Replace request_irq, request_threaded_irq, and request_any_context_irq
  with their device managed equivalent
- Replace &pdev->dev with dev if 'struct device *dev' is a declared
  variable
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/coh901327_wdt.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index 38dd60f0cfcc..8b6f7f35c479 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -241,11 +241,7 @@ static struct watchdog_device coh901327_wdt = {
 
 static int __exit coh901327_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&coh901327_wdt);
 	coh901327_disable();
-	free_irq(irq, pdev);
-	clk_disable_unprepare(clk);
-	clk_put(clk);
 	return 0;
 }
 
@@ -263,7 +259,7 @@ static int __init coh901327_probe(struct platform_device *pdev)
 	if (IS_ERR(virtbase))
 		return PTR_ERR(virtbase);
 
-	clk = clk_get(dev, NULL);
+	clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(clk)) {
 		ret = PTR_ERR(clk);
 		dev_err(dev, "could not get clock\n");
@@ -272,8 +268,13 @@ static int __init coh901327_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(clk);
 	if (ret) {
 		dev_err(dev, "could not prepare and enable clock\n");
-		goto out_no_clk_enable;
+		return ret;
 	}
+	ret = devm_add_action_or_reset(dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       clk);
+	if (ret)
+		return ret;
 
 	val = readw(virtbase + U300_WDOG_SR);
 	switch (val) {
@@ -309,31 +310,21 @@ static int __init coh901327_probe(struct platform_device *pdev)
 	writew(U300_WDOG_SR_RESET_STATUS_RESET, virtbase + U300_WDOG_SR);
 
 	irq = platform_get_irq(pdev, 0);
-	if (request_irq(irq, coh901327_interrupt, 0,
-			DRV_NAME " Bark", pdev)) {
-		ret = -EIO;
-		goto out_no_irq;
-	}
+	if (devm_request_irq(dev, irq, coh901327_interrupt, 0,
+			     DRV_NAME " Bark", pdev))
+		return -EIO;
 
 	ret = watchdog_init_timeout(&coh901327_wdt, margin, dev);
 	if (ret < 0)
 		coh901327_wdt.timeout = 60;
 
 	coh901327_wdt.parent = dev;
-	ret = watchdog_register_device(&coh901327_wdt);
+	ret = devm_watchdog_register_device(dev, &coh901327_wdt);
 	if (ret)
-		goto out_no_wdog;
+		return ret;
 
 	dev_info(dev, "initialized. timer margin=%d sec\n", margin);
 	return 0;
-
-out_no_wdog:
-	free_irq(irq, pdev);
-out_no_irq:
-	clk_disable_unprepare(clk);
-out_no_clk_enable:
-	clk_put(clk);
-	return ret;
 }
 
 #ifdef CONFIG_PM
-- 
2.7.4

[PATCH 10/62] watchdog: coh901327_wdt: Convert to use device managed functions

[Linus Walleij]

On Wed, Jan 11, 2017 at 12:34 AM, Guenter Roeck <linux@roeck-us.net> wrote:

> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
>
> 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
>
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Use devm_clk_get() if the device parameter is not NULL
> - Replace 'goto l; ... l: return e;' with 'return e;'
> - Replace 'val = e; return val;' with 'return e;'
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Replace request_irq, request_threaded_irq, and request_any_context_irq
>   with their device managed equivalent
> - Replace &pdev->dev with dev if 'struct device *dev' is a declared
>   variable
> - Use devm_watchdog_register_driver() to register watchdog device
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 16/62] watchdog: digicolor_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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

- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'val = e; return val;' with 'return e;'
- Drop assignments to otherwise unused variables
- Replace 'if (e) { return expr; }' with 'if (e) return expr;'
- Drop remove function
- Replace of_iomap() with platform_get_resource() followed by
  devm_ioremap_resource()
- Drop platform_set_drvdata()
- Replace &pdev->dev with dev if 'struct device *dev' is a declared
  variable
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/digicolor_wdt.c | 48 ++++++++++------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/drivers/watchdog/digicolor_wdt.c b/drivers/watchdog/digicolor_wdt.c
index dfe72944822d..870694d9ebc7 100644
--- a/drivers/watchdog/digicolor_wdt.c
+++ b/drivers/watchdog/digicolor_wdt.c
@@ -119,62 +119,40 @@ static struct watchdog_device dc_wdt_wdd = {
 
 static int dc_wdt_probe(struct platform_device *pdev)
 {
+	struct resource *res;
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 	struct dc_wdt *wdt;
 	int ret;
 
 	wdt = devm_kzalloc(dev, sizeof(struct dc_wdt), GFP_KERNEL);
 	if (!wdt)
 		return -ENOMEM;
-	platform_set_drvdata(pdev, wdt);
 
-	wdt->base = of_iomap(np, 0);
-	if (!wdt->base) {
-		dev_err(dev, "Failed to remap watchdog regs");
-		return -ENODEV;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(wdt->base))
+		return PTR_ERR(wdt->base);
 
-	wdt->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(wdt->clk)) {
-		ret = PTR_ERR(wdt->clk);
-		goto err_iounmap;
-	}
+	wdt->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(wdt->clk))
+		return PTR_ERR(wdt->clk);
 	dc_wdt_wdd.max_timeout = U32_MAX / clk_get_rate(wdt->clk);
 	dc_wdt_wdd.timeout = dc_wdt_wdd.max_timeout;
-	dc_wdt_wdd.parent = &pdev->dev;
+	dc_wdt_wdd.parent = dev;
 
 	spin_lock_init(&wdt->lock);
 
 	watchdog_set_drvdata(&dc_wdt_wdd, wdt);
 	watchdog_set_restart_priority(&dc_wdt_wdd, 128);
 	watchdog_init_timeout(&dc_wdt_wdd, timeout, dev);
-	ret = watchdog_register_device(&dc_wdt_wdd);
+	watchdog_stop_on_reboot(&dc_wdt_wdd);
+	ret = devm_watchdog_register_device(dev, &dc_wdt_wdd);
 	if (ret) {
 		dev_err(dev, "Failed to register watchdog device");
-		goto err_iounmap;
+		return ret;
 	}
 
 	return 0;
-
-err_iounmap:
-	iounmap(wdt->base);
-	return ret;
-}
-
-static int dc_wdt_remove(struct platform_device *pdev)
-{
-	struct dc_wdt *wdt = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&dc_wdt_wdd);
-	iounmap(wdt->base);
-
-	return 0;
-}
-
-static void dc_wdt_shutdown(struct platform_device *pdev)
-{
-	dc_wdt_stop(&dc_wdt_wdd);
 }
 
 static const struct of_device_id dc_wdt_of_match[] = {
@@ -185,8 +163,6 @@ MODULE_DEVICE_TABLE(of, dc_wdt_of_match);
 
 static struct platform_driver dc_wdt_driver = {
 	.probe		= dc_wdt_probe,
-	.remove		= dc_wdt_remove,
-	.shutdown	= dc_wdt_shutdown,
 	.driver = {
 		.name =		"digicolor-wdt",
 		.of_match_table = dc_wdt_of_match,
-- 
2.7.4

[PATCH 16/62] watchdog: digicolor_wdt: Convert to use device managed functions and other improvements

[Baruch Siach]

Hi Guenter,

On Tue, Jan 10, 2017 at 03:34:33PM -0800, 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
> 
> - Replace 'goto l; ... l: return e;' with 'return e;'
> - Replace 'val = e; return val;' with 'return e;'
> - Drop assignments to otherwise unused variables
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Drop remove function
> - Replace of_iomap() with platform_get_resource() followed by
>   devm_ioremap_resource()
> - Drop platform_set_drvdata()
> - Replace &pdev->dev with dev if 'struct device *dev' is a declared
>   variable
> - Use devm_watchdog_register_driver() to register watchdog device
> - Replace shutdown function with call to watchdog_stop_on_reboot()
> 
> Cc: Baruch Siach <baruch@tkos.co.il>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Baruch Siach <baruch@tkos.co.il>
Tested-by: Baruch Siach <baruch@tkos.co.il>

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[PATCH 27/62] watchdog: lpc18xx_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'val = e; return val;' with 'return e;'
- Replace 'if (e) return e; return 0;' with 'return e;'
- Drop assignments to otherwise unused variables
- Drop remove function
- Drop platform_set_drvdata()
- Replace &pdev->dev with dev if 'struct device *dev' is a declared
  variable
- Call del_timer() using devm_add_action()
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Joachim Eastwood <manabian@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/lpc18xx_wdt.c | 58 ++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/drivers/watchdog/lpc18xx_wdt.c b/drivers/watchdog/lpc18xx_wdt.c
index 3b8bb59adf02..554030628caa 100644
--- a/drivers/watchdog/lpc18xx_wdt.c
+++ b/drivers/watchdog/lpc18xx_wdt.c
@@ -231,19 +231,28 @@ static int lpc18xx_wdt_probe(struct platform_device *pdev)
 		dev_err(dev, "could not prepare or enable sys clock\n");
 		return ret;
 	}
+	ret = devm_add_action_or_reset(dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       lpc18xx_wdt->reg_clk);
+	if (ret)
+		return ret;
 
 	ret = clk_prepare_enable(lpc18xx_wdt->wdt_clk);
 	if (ret) {
 		dev_err(dev, "could not prepare or enable wdt clock\n");
-		goto disable_reg_clk;
+		return ret;
 	}
+	ret = devm_add_action_or_reset(dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       lpc18xx_wdt->wdt_clk);
+	if (ret)
+		return ret;
 
 	/* We use the clock rate to calculate timeouts */
 	lpc18xx_wdt->clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk);
 	if (lpc18xx_wdt->clk_rate == 0) {
 		dev_err(dev, "failed to get clock rate\n");
-		ret = -EINVAL;
-		goto disable_wdt_clk;
+		return -EINVAL;
 	}
 
 	lpc18xx_wdt->wdt_dev.info = &lpc18xx_wdt_info;
@@ -269,44 +278,17 @@ static int lpc18xx_wdt_probe(struct platform_device *pdev)
 
 	setup_timer(&lpc18xx_wdt->timer, lpc18xx_wdt_timer_feed,
 		    (unsigned long)&lpc18xx_wdt->wdt_dev);
+	ret = devm_add_action(dev, (void(*)(void *))del_timer,
+			       &lpc18xx_wdt->timer);
+	if (ret)
+		return ret;
 
 	watchdog_set_nowayout(&lpc18xx_wdt->wdt_dev, nowayout);
 	watchdog_set_restart_priority(&lpc18xx_wdt->wdt_dev, 128);
 
-	platform_set_drvdata(pdev, lpc18xx_wdt);
-
-	ret = watchdog_register_device(&lpc18xx_wdt->wdt_dev);
-	if (ret)
-		goto disable_wdt_clk;
-
-	return 0;
-
-disable_wdt_clk:
-	clk_disable_unprepare(lpc18xx_wdt->wdt_clk);
-disable_reg_clk:
-	clk_disable_unprepare(lpc18xx_wdt->reg_clk);
-	return ret;
-}
-
-static void lpc18xx_wdt_shutdown(struct platform_device *pdev)
-{
-	struct lpc18xx_wdt_dev *lpc18xx_wdt = platform_get_drvdata(pdev);
-
-	lpc18xx_wdt_stop(&lpc18xx_wdt->wdt_dev);
-}
-
-static int lpc18xx_wdt_remove(struct platform_device *pdev)
-{
-	struct lpc18xx_wdt_dev *lpc18xx_wdt = platform_get_drvdata(pdev);
-
-	dev_warn(&pdev->dev, "I quit now, hardware will probably reboot!\n");
-	del_timer(&lpc18xx_wdt->timer);
-
-	watchdog_unregister_device(&lpc18xx_wdt->wdt_dev);
-	clk_disable_unprepare(lpc18xx_wdt->wdt_clk);
-	clk_disable_unprepare(lpc18xx_wdt->reg_clk);
-
-	return 0;
+	watchdog_stop_on_reboot(&lpc18xx_wdt->wdt_dev);
+	return devm_watchdog_register_device(dev,
+					     &lpc18xx_wdt->wdt_dev);
 }
 
 static const struct of_device_id lpc18xx_wdt_match[] = {
@@ -321,8 +303,6 @@ static struct platform_driver lpc18xx_wdt_driver = {
 		.of_match_table	= lpc18xx_wdt_match,
 	},
 	.probe = lpc18xx_wdt_probe,
-	.remove = lpc18xx_wdt_remove,
-	.shutdown = lpc18xx_wdt_shutdown,
 };
 module_platform_driver(lpc18xx_wdt_driver);
 
-- 
2.7.4

[PATCH 32/62] watchdog: meson_gxbb_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Check return value from clk_prepare_enable()
- Replace 'val = e; return val;' with 'return e;'
- Replace 'if (e) return e; return 0;' with 'return e;'
- Drop assignments to otherwise unused variables
- Replace 'if (e) { return expr; }' with 'if (e) return expr;'
- Drop remove function
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/meson_gxbb_wdt.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
index 45d47664a00a..913d8a644460 100644
--- a/drivers/watchdog/meson_gxbb_wdt.c
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -203,7 +203,14 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(data->clk))
 		return PTR_ERR(data->clk);
 
-	clk_prepare_enable(data->clk);
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       data->clk);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, data);
 
@@ -224,37 +231,12 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
 
 	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
 
-	ret = watchdog_register_device(&data->wdt_dev);
-	if (ret) {
-		clk_disable_unprepare(data->clk);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int meson_gxbb_wdt_remove(struct platform_device *pdev)
-{
-	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&data->wdt_dev);
-
-	clk_disable_unprepare(data->clk);
-
-	return 0;
-}
-
-static void meson_gxbb_wdt_shutdown(struct platform_device *pdev)
-{
-	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
-
-	meson_gxbb_wdt_stop(&data->wdt_dev);
+	watchdog_stop_on_reboot(&data->wdt_dev);
+	return devm_watchdog_register_device(&pdev->dev, &data->wdt_dev);
 }
 
 static struct platform_driver meson_gxbb_wdt_driver = {
 	.probe	= meson_gxbb_wdt_probe,
-	.remove	= meson_gxbb_wdt_remove,
-	.shutdown = meson_gxbb_wdt_shutdown,
 	.driver = {
 		.name = "meson-gxbb-wdt",
 		.pm = &meson_gxbb_wdt_pm_ops,
-- 
2.7.4

[PATCH 32/62] watchdog: meson_gxbb_wdt: Convert to use device managed functions and other improvements

[Neil Armstrong]

On 01/11/2017 01:44 AM, 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
> 
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Check return value from clk_prepare_enable()
> - Replace 'val = e; return val;' with 'return e;'
> - Replace 'if (e) return e; return 0;' with 'return e;'
> - Drop assignments to otherwise unused variables
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Drop remove function
> - Use devm_watchdog_register_driver() to register watchdog device
> - Replace shutdown function with call to watchdog_stop_on_reboot()
> 
> Cc: Carlo Caione <carlo@caione.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/meson_gxbb_wdt.c | 38 ++++++++++----------------------------
>  1 file changed, 10 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> index 45d47664a00a..913d8a644460 100644
> --- a/drivers/watchdog/meson_gxbb_wdt.c
> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> @@ -203,7 +203,14 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(data->clk))
>  		return PTR_ERR(data->clk);
>  
> -	clk_prepare_enable(data->clk);
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		return ret;
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       (void(*)(void *))clk_disable_unprepare,
> +				       data->clk);
> +	if (ret)
> +		return ret;
>  
>  	platform_set_drvdata(pdev, data);
>  
> @@ -224,37 +231,12 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>  
>  	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>  
> -	ret = watchdog_register_device(&data->wdt_dev);
> -	if (ret) {
> -		clk_disable_unprepare(data->clk);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int meson_gxbb_wdt_remove(struct platform_device *pdev)
> -{
> -	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
> -
> -	watchdog_unregister_device(&data->wdt_dev);
> -
> -	clk_disable_unprepare(data->clk);
> -
> -	return 0;
> -}
> -
> -static void meson_gxbb_wdt_shutdown(struct platform_device *pdev)
> -{
> -	struct meson_gxbb_wdt *data = platform_get_drvdata(pdev);
> -
> -	meson_gxbb_wdt_stop(&data->wdt_dev);
> +	watchdog_stop_on_reboot(&data->wdt_dev);
> +	return devm_watchdog_register_device(&pdev->dev, &data->wdt_dev);
>  }
>  
>  static struct platform_driver meson_gxbb_wdt_driver = {
>  	.probe	= meson_gxbb_wdt_probe,
> -	.remove	= meson_gxbb_wdt_remove,
> -	.shutdown = meson_gxbb_wdt_shutdown,
>  	.driver = {
>  		.name = "meson-gxbb-wdt",
>  		.pm = &meson_gxbb_wdt_pm_ops,
> 

Was on my todo list, glad you did this !

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH 32/62] watchdog: meson_gxbb_wdt: Convert to use device managed functions and other improvements

[Kevin Hilman]

Guenter Roeck <linux@roeck-us.net> 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
>
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Check return value from clk_prepare_enable()
> - Replace 'val = e; return val;' with 'return e;'
> - Replace 'if (e) return e; return 0;' with 'return e;'
> - Drop assignments to otherwise unused variables
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Drop remove function
> - Use devm_watchdog_register_driver() to register watchdog device
> - Replace shutdown function with call to watchdog_stop_on_reboot()
>
> Cc: Carlo Caione <carlo@caione.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Nice, thanks for the cleanup!

Acked-by: Kevin Hilman <khilman@baylibre.com>

[PATCH 33/62] watchdog: meson_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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 remove function
- Drop platform_set_drvdata()
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/meson_wdt.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/watchdog/meson_wdt.c b/drivers/watchdog/meson_wdt.c
index 56ea1caf71c3..491b9bf13d84 100644
--- a/drivers/watchdog/meson_wdt.c
+++ b/drivers/watchdog/meson_wdt.c
@@ -201,38 +201,19 @@ static int meson_wdt_probe(struct platform_device *pdev)
 
 	meson_wdt_stop(&meson_wdt->wdt_dev);
 
-	err = watchdog_register_device(&meson_wdt->wdt_dev);
+	watchdog_stop_on_reboot(&meson_wdt->wdt_dev);
+	err = devm_watchdog_register_device(&pdev->dev, &meson_wdt->wdt_dev);
 	if (err)
 		return err;
 
-	platform_set_drvdata(pdev, meson_wdt);
-
 	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
 		 meson_wdt->wdt_dev.timeout, nowayout);
 
 	return 0;
 }
 
-static int meson_wdt_remove(struct platform_device *pdev)
-{
-	struct meson_wdt_dev *meson_wdt = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&meson_wdt->wdt_dev);
-
-	return 0;
-}
-
-static void meson_wdt_shutdown(struct platform_device *pdev)
-{
-	struct meson_wdt_dev *meson_wdt = platform_get_drvdata(pdev);
-
-	meson_wdt_stop(&meson_wdt->wdt_dev);
-}
-
 static struct platform_driver meson_wdt_driver = {
 	.probe		= meson_wdt_probe,
-	.remove		= meson_wdt_remove,
-	.shutdown	= meson_wdt_shutdown,
 	.driver		= {
 		.name		= DRV_NAME,
 		.of_match_table	= meson_wdt_dt_ids,
-- 
2.7.4

[PATCH 33/62] watchdog: meson_wdt: Convert to use device managed functions and other improvements

[Kevin Hilman]

Guenter Roeck <linux@roeck-us.net> 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 remove function
> - Drop platform_set_drvdata()
> - Use devm_watchdog_register_driver() to register watchdog device
> - Replace shutdown function with call to watchdog_stop_on_reboot()
>
> Cc: Carlo Caione <carlo@caione.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Kevin Hilman <khilman@baylibre.com>

[PATCH 37/62] watchdog: mtk_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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 remove function
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/mtk_wdt.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 7ed417a765c7..db2e70ed368b 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -192,7 +192,8 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 
 	mtk_wdt_stop(&mtk_wdt->wdt_dev);
 
-	err = watchdog_register_device(&mtk_wdt->wdt_dev);
+	watchdog_stop_on_reboot(&mtk_wdt->wdt_dev);
+	err = devm_watchdog_register_device(&pdev->dev, &mtk_wdt->wdt_dev);
 	if (unlikely(err))
 		return err;
 
@@ -202,23 +203,6 @@ static int mtk_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void mtk_wdt_shutdown(struct platform_device *pdev)
-{
-	struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
-
-	if (watchdog_active(&mtk_wdt->wdt_dev))
-		mtk_wdt_stop(&mtk_wdt->wdt_dev);
-}
-
-static int mtk_wdt_remove(struct platform_device *pdev)
-{
-	struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&mtk_wdt->wdt_dev);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int mtk_wdt_suspend(struct device *dev)
 {
@@ -256,8 +240,6 @@ static const struct dev_pm_ops mtk_wdt_pm_ops = {
 
 static struct platform_driver mtk_wdt_driver = {
 	.probe		= mtk_wdt_probe,
-	.remove		= mtk_wdt_remove,
-	.shutdown	= mtk_wdt_shutdown,
 	.driver		= {
 		.name		= DRV_NAME,
 		.pm		= &mtk_wdt_pm_ops,
-- 
2.7.4

[PATCH 39/62] watchdog: of_xilinx_wdt: Convert to use device managed functions

[Guenter Roeck]

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

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace 'goto l; ... l: return e;' with 'return e;'
- Drop assignments to otherwise unused variables
- Drop remove function
- Drop platform_set_drvdata()
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Michal Simek <michal.simek@xilinx.com>
Cc: "S?ren Brinkmann" <soren.brinkmann@xilinx.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/of_xilinx_wdt.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index fae7fe929ea3..277de711c31f 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -210,38 +210,27 @@ static int xwdt_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "unable to enable clock\n");
 		return rc;
 	}
+	rc = devm_add_action_or_reset(&pdev->dev,
+				      (void(*)(void *))clk_disable_unprepare,
+				      xdev->clk);
+	if (rc)
+		return rc;
 
 	rc = xwdt_selftest(xdev);
 	if (rc == XWT_TIMER_FAILED) {
 		dev_err(&pdev->dev, "SelfTest routine error\n");
-		goto err_clk_disable;
+		return rc;
 	}
 
-	rc = watchdog_register_device(xilinx_wdt_wdd);
+	rc = devm_watchdog_register_device(&pdev->dev, xilinx_wdt_wdd);
 	if (rc) {
 		dev_err(&pdev->dev, "Cannot register watchdog (err=%d)\n", rc);
-		goto err_clk_disable;
+		return rc;
 	}
 
 	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
 		 xdev->base, xilinx_wdt_wdd->timeout);
 
-	platform_set_drvdata(pdev, xdev);
-
-	return 0;
-err_clk_disable:
-	clk_disable_unprepare(xdev->clk);
-
-	return rc;
-}
-
-static int xwdt_remove(struct platform_device *pdev)
-{
-	struct xwdt_device *xdev = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&xdev->xilinx_wdt_wdd);
-	clk_disable_unprepare(xdev->clk);
-
 	return 0;
 }
 
@@ -255,7 +244,6 @@ MODULE_DEVICE_TABLE(of, xwdt_of_match);
 
 static struct platform_driver xwdt_driver = {
 	.probe       = xwdt_probe,
-	.remove      = xwdt_remove,
 	.driver = {
 		.name  = WATCHDOG_NAME,
 		.of_match_table = xwdt_of_match,
-- 
2.7.4

[PATCH 44/62] watchdog: pnx4008_wdt: Convert to use device managed functions

[Guenter Roeck]

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

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace 'goto l; ... l: return e;' with 'return e;'
- Drop remove function
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Vladimir Zapolskiy <vz@mleia.com>
Cc: Sylvain Lemieux <slemieux.tyco@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/pnx4008_wdt.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 0529aed158a4..a0cda8748c9b 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -202,6 +202,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(wdt_clk);
 	if (ret)
 		return ret;
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       wdt_clk);
+	if (ret)
+		return ret;
 
 	pnx4008_wdd.bootstatus = (readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
 			WDIOF_CARDRESET : 0;
@@ -211,28 +216,15 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
 
 	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
 
-	ret = watchdog_register_device(&pnx4008_wdd);
+	ret = devm_watchdog_register_device(&pdev->dev, &pnx4008_wdd);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "cannot register watchdog device\n");
-		goto disable_clk;
+		return ret;
 	}
 
 	dev_info(&pdev->dev, "heartbeat %d sec\n", pnx4008_wdd.timeout);
 
 	return 0;
-
-disable_clk:
-	clk_disable_unprepare(wdt_clk);
-	return ret;
-}
-
-static int pnx4008_wdt_remove(struct platform_device *pdev)
-{
-	watchdog_unregister_device(&pnx4008_wdd);
-
-	clk_disable_unprepare(wdt_clk);
-
-	return 0;
 }
 
 #ifdef CONFIG_OF
@@ -249,7 +241,6 @@ static struct platform_driver platform_wdt_driver = {
 		.of_match_table = of_match_ptr(pnx4008_wdt_match),
 	},
 	.probe = pnx4008_wdt_probe,
-	.remove = pnx4008_wdt_remove,
 };
 
 module_platform_driver(platform_wdt_driver);
-- 
2.7.4

[PATCH 44/62] watchdog: pnx4008_wdt: Convert to use device managed functions

[Vladimir Zapolskiy]

Hi Guenter,

On 01/11/2017 02:44 AM, Guenter Roeck wrote:
> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
> 
> 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
> 
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Replace 'goto l; ... l: return e;' with 'return e;'
> - Drop remove function
> - Use devm_watchdog_register_driver() to register watchdog device
> 
> Cc: Vladimir Zapolskiy <vz@mleia.com>

Acked-by: Vladimir Zapolskiy <vz@mleia.com>

Thank you for the cleanup.

> Cc: Sylvain Lemieux <slemieux.tyco@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

--
With best wishes,
Vladimir

[PATCH 52/62] watchdog: sirfsoc_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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

- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Barry Song <baohua@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/sirfsoc_wdt.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/sirfsoc_wdt.c b/drivers/watchdog/sirfsoc_wdt.c
index 3050a0031479..fb88e04d18ae 100644
--- a/drivers/watchdog/sirfsoc_wdt.c
+++ b/drivers/watchdog/sirfsoc_wdt.c
@@ -161,7 +161,8 @@ static int sirfsoc_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(&sirfsoc_wdd, nowayout);
 	sirfsoc_wdd.parent = &pdev->dev;
 
-	ret = watchdog_register_device(&sirfsoc_wdd);
+	watchdog_stop_on_reboot(&sirfsoc_wdd);
+	ret = devm_watchdog_register_device(&pdev->dev, &sirfsoc_wdd);
 	if (ret)
 		return ret;
 
@@ -170,16 +171,9 @@ static int sirfsoc_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void sirfsoc_wdt_shutdown(struct platform_device *pdev)
-{
-	struct watchdog_device *wdd = platform_get_drvdata(pdev);
-
-	sirfsoc_wdt_disable(wdd);
-}
-
 static int sirfsoc_wdt_remove(struct platform_device *pdev)
 {
-	sirfsoc_wdt_shutdown(pdev);
+	sirfsoc_wdt_disable(platform_get_drvdata(pdev));
 	return 0;
 }
 
@@ -221,7 +215,6 @@ static struct platform_driver sirfsoc_wdt_driver = {
 	},
 	.probe = sirfsoc_wdt_probe,
 	.remove = sirfsoc_wdt_remove,
-	.shutdown = sirfsoc_wdt_shutdown,
 };
 module_platform_driver(sirfsoc_wdt_driver);
 
-- 
2.7.4

[PATCH 53/62] watchdog: st_lpc_wdt: Convert to use device managed functions

[Guenter Roeck]

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

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Patrice Chotard <patrice.chotard@st.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/st_lpc_wdt.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/st_lpc_wdt.c b/drivers/watchdog/st_lpc_wdt.c
index e6100e447dd8..d561fbf1df85 100644
--- a/drivers/watchdog/st_lpc_wdt.c
+++ b/drivers/watchdog/st_lpc_wdt.c
@@ -215,6 +215,11 @@ static int st_wdog_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Unable to enable clock\n");
 		return ret;
 	}
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       clk);
+	if (ret)
+		return ret;
 
 	watchdog_set_drvdata(&st_wdog_dev, st_wdog);
 	watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT);
@@ -223,14 +228,12 @@ static int st_wdog_probe(struct platform_device *pdev)
 	ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n");
-		clk_disable_unprepare(clk);
 		return ret;
 	}
 
-	ret = watchdog_register_device(&st_wdog_dev);
+	ret = devm_watchdog_register_device(&pdev->dev, &st_wdog_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Unable to register watchdog\n");
-		clk_disable_unprepare(clk);
 		return ret;
 	}
 
@@ -247,8 +250,6 @@ static int st_wdog_remove(struct platform_device *pdev)
 	struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev);
 
 	st_wdog_setup(st_wdog, false);
-	watchdog_unregister_device(&st_wdog_dev);
-	clk_disable_unprepare(st_wdog->clk);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 55/62] watchdog: sunxi_wdt: Convert to use device managed functions and other improvements

[Guenter Roeck]

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 remove function
- Drop platform_set_drvdata()
- Use devm_watchdog_register_driver() to register watchdog device
- Replace shutdown function with call to watchdog_stop_on_reboot()

Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/sunxi_wdt.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index 953bb7b7446f..9728fa32c357 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -242,8 +242,6 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
 	if (!sunxi_wdt)
 		return -EINVAL;
 
-	platform_set_drvdata(pdev, sunxi_wdt);
-
 	device = of_match_device(sunxi_wdt_dt_ids, &pdev->dev);
 	if (!device)
 		return -ENODEV;
@@ -270,7 +268,8 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
 
 	sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
 
-	err = watchdog_register_device(&sunxi_wdt->wdt_dev);
+	watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev);
+	err = devm_watchdog_register_device(&pdev->dev, &sunxi_wdt->wdt_dev);
 	if (unlikely(err))
 		return err;
 
@@ -280,27 +279,8 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int sunxi_wdt_remove(struct platform_device *pdev)
-{
-	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
-
-	watchdog_unregister_device(&sunxi_wdt->wdt_dev);
-	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, NULL);
-
-	return 0;
-}
-
-static void sunxi_wdt_shutdown(struct platform_device *pdev)
-{
-	struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
-
-	sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
-}
-
 static struct platform_driver sunxi_wdt_driver = {
 	.probe		= sunxi_wdt_probe,
-	.remove		= sunxi_wdt_remove,
-	.shutdown	= sunxi_wdt_shutdown,
 	.driver		= {
 		.name		= DRV_NAME,
 		.of_match_table	= sunxi_wdt_dt_ids,
-- 
2.7.4

[PATCH 55/62] watchdog: sunxi_wdt: Convert to use device managed functions and other improvements

[Maxime Ripard]

On Tue, Jan 10, 2017 at 06:09:09PM -0800, 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 remove function
> - Drop platform_set_drvdata()
> - Use devm_watchdog_register_driver() to register watchdog device
> - Replace shutdown function with call to watchdog_stop_on_reboot()
> 
> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170111/51fbbf9a/attachment-0001.sig>

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

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

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

- Use devm_add_action_or_reset() for calls to clk_disable_unprepare
- Replace 'goto l; ... l: return e;' with 'return e;'
- Replace 'val = e; return val;' with 'return e;'
- Replace 'if (e) { return expr; }' with 'if (e) return expr;'
- Use devm_watchdog_register_driver() to register watchdog device

Cc: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/tangox_wdt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index d5fcce062920..7688e1b35867 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -134,12 +134,15 @@ 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(&pdev->dev,
+				       (void(*)(void *))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 = &pdev->dev;
 	dev->wdt.info = &tangox_wdt_info;
@@ -173,19 +176,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_restart_priority(&dev->wdt, 128);
 
-	err = watchdog_register_device(&dev->wdt);
+	err = devm_watchdog_register_device(&pdev->dev, &dev->wdt);
 	if (err)
-		goto err;
+		return err;
 
 	platform_set_drvdata(pdev, dev);
 
 	dev_info(&pdev->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)
@@ -193,9 +192,6 @@ static int tangox_wdt_remove(struct platform_device *pdev)
 	struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
 
 	tangox_wdt_stop(&dev->wdt);
-	clk_disable_unprepare(dev->clk);
-
-	watchdog_unregister_device(&dev->wdt);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 03:09, Guenter Roeck wrote:

> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
> 
> 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
> 
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Replace 'goto l; ... l: return e;' with 'return e;'
> - Replace 'val = e; return val;' with 'return e;'
> - Replace 'if (e) { return expr; }' with 'if (e) return expr;'
> - Use devm_watchdog_register_driver() to register watchdog device
> 
> Cc: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/tangox_wdt.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> index d5fcce062920..7688e1b35867 100644
> --- a/drivers/watchdog/tangox_wdt.c
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -134,12 +134,15 @@ 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(&pdev->dev,
> +				       (void(*)(void *))clk_disable_unprepare,
> +				       dev->clk);
> +	if (err)
> +		return err;

Hello Guenter,

I would rather avoid the function pointer cast.
How about defining an auxiliary function for the cleanup action?

clk_disable_unprepare() is static inline, so gcc will have to
define an auxiliary function either way. What do you think?

Regards.


diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
index 202c4b9cc921..1a4f6d245a83 100644
--- a/drivers/watchdog/tangox_wdt.c
+++ b/drivers/watchdog/tangox_wdt.c
@@ -114,6 +114,11 @@ static int tangox_wdt_restart(struct notifier_block *nb, unsigned long action,
        return NOTIFY_DONE;
 }
 
+static void cleanup(void *clk)
+{
+       clk_disable_unprepare(clk);
+}
+
 static int tangox_wdt_probe(struct platform_device *pdev)
 {
        struct tangox_wdt_device *dev;
@@ -138,6 +143,10 @@ static int tangox_wdt_probe(struct platform_device *pdev)
        if (err)
                return err;
 
+       err = devm_add_action_or_reset(&pdev->dev, cleanup, dev->clk);
+       if (err)
+               return err;
+
        dev->clk_rate = clk_get_rate(dev->clk);
        if (!dev->clk_rate) {
                err = -EINVAL;

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 01:07 AM, Marc Gonzalez wrote:

>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>> +				       (void(*)(void *))clk_disable_unprepare,
>> +				       dev->clk);
>> +	if (err)
>> +		return err;
>
> Hello Guenter,
>
> I would rather avoid the function pointer cast.
> How about defining an auxiliary function for the cleanup action?
>
> clk_disable_unprepare() is static inline, so gcc will have to
> define an auxiliary function either way. What do you think?
>

Not really. It would just make it more complicated to replace the
call with devm_clk_prepare_enable(), should it ever find its way
into the light of day.

Guenter

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 11:52, Guenter Roeck wrote:

> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> 
>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>>> +				       (void(*)(void *))clk_disable_unprepare,
>>> +				       dev->clk);
>>> +	if (err)
>>> +		return err;
>>
>> Hello Guenter,
>>
>> I would rather avoid the function pointer cast.
>> How about defining an auxiliary function for the cleanup action?
>>
>> clk_disable_unprepare() is static inline, so gcc will have to
>> define an auxiliary function either way. What do you think?
> 
> Not really. It would just make it more complicated to replace the
> call with devm_clk_prepare_enable(), should it ever find its way
> into the light of day.

More complicated, because the cleanup function will have to be deleted later?
The compiler will warn if someone forgets to do that.

In my opinion, it's not a good idea to rely on the fact that casting
void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
on most platforms. (It has undefined behavior, strictly speaking.)

Do you really dislike the portable solution I suggested? :-(

Regards.

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
> On 11/01/2017 11:52, Guenter Roeck wrote:
>
>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>
>>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>>>> +				       (void(*)(void *))clk_disable_unprepare,
>>>> +				       dev->clk);
>>>> +	if (err)
>>>> +		return err;
>>>
>>> Hello Guenter,
>>>
>>> I would rather avoid the function pointer cast.
>>> How about defining an auxiliary function for the cleanup action?
>>>
>>> clk_disable_unprepare() is static inline, so gcc will have to
>>> define an auxiliary function either way. What do you think?
>>
>> Not really. It would just make it more complicated to replace the
>> call with devm_clk_prepare_enable(), should it ever find its way
>> into the light of day.
>
> More complicated, because the cleanup function will have to be deleted later?
> The compiler will warn if someone forgets to do that.
>
> In my opinion, it's not a good idea to rely on the fact that casting
> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> on most platforms. (It has undefined behavior, strictly speaking.)
>
I do hear that you object to this code.

However, I must admit that you completely lost me here. It is a cast from
one function pointer to another, passed as argument to another function,
with a secondary cast of its argument from a typed pointer to a void pointer.
I don't think C permits for "undefined behavior, strictly speaking".
Besides, that same mechanism is already used elsewhere, which is how I
got the idea. Are you claiming that there are situations where it won't
work ?

> Do you really dislike the portable solution I suggested? :-(
>
It is not more portable than the above. It is more expensive and adds more
code.

Thanks,
Guenter

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Måns Rullgård]

Guenter Roeck <linux@roeck-us.net> writes:

> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
>>>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>>>>> +				       (void(*)(void *))clk_disable_unprepare,
>>>>> +				       dev->clk);
>>>>> +	if (err)
>>>>> +		return err;
>>>>
>>>> Hello Guenter,
>>>>
>>>> I would rather avoid the function pointer cast.
>>>> How about defining an auxiliary function for the cleanup action?
>>>>
>>>> clk_disable_unprepare() is static inline, so gcc will have to
>>>> define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>>
> I do hear that you object to this code.
>
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another, passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".
> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?

A pointer to void is interchangeable with any other pointer type.  That
doesn't necessarily imply that pointers to functions taking arguments of
different pointer types (as we have here) are always compatible.  I'd
have to read the C standard carefully to see if there's any such
promise, and I have other things to do right now.  I am, however, not
aware of any ABI (certainly none used by Linux) where it would pose a
problem.

-- 
M?ns Rullg?rd

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 15:25, Guenter Roeck wrote:
> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
>>>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>>>>> +				       (void(*)(void *))clk_disable_unprepare,
>>>>> +				       dev->clk);
>>>>> +	if (err)
>>>>> +		return err;
>>>>
>>>> Hello Guenter,
>>>>
>>>> I would rather avoid the function pointer cast.
>>>> How about defining an auxiliary function for the cleanup action?
>>>>
>>>> clk_disable_unprepare() is static inline, so gcc will have to
>>>> define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>
> I do hear that you object to this code.
> 
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another,

Perhaps you are used to work at the assembly level, where pointers are
just addresses, and all pointers are interchangeable.

At a slightly higher level (C abstract machine), it is not so.

> passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".

The C standard leaves quite a lot of behavior undefined, e.g.

char *foo = "hello";
foo[1] = 'a'; // UB

char buf[4];
*(int *)&buf = 0xdeadbeef; // UB

1 << 64; // UB

> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?

If this technique is already used elsewhere in the kernel, then I'll
crawl back under my rock (and weep).

I can see two issues with the code you propose.

First is the same for all casts: silencing potential warnings,
e.g. if the prototype of clk_disable_unprepare ever changed.
(Though casts are required for vararg function arguments.)

Second is just theory and not a real-world concern.

>> Do you really dislike the portable solution I suggested? :-(
>
> It is not more portable than the above. It is more expensive and adds more
> code.

Maybe I am mistaken. Can you tell me why adding an auxiliary function
is more expensive? (In CPU cycles?)

clk_disable_unprepare() is static inline, so an auxiliary function
exists either way (implicit or explicit).

Regards.

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On Wed, Jan 11, 2017 at 04:28:12PM +0100, Marc Gonzalez wrote:
> On 11/01/2017 15:25, Guenter Roeck wrote:
> > On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
> >> On 11/01/2017 11:52, Guenter Roeck wrote:
> >>
> >>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> >>>
> >>>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
> >>>>> +				       (void(*)(void *))clk_disable_unprepare,
> >>>>> +				       dev->clk);
> >>>>> +	if (err)
> >>>>> +		return err;
> >>>>
> >>>> Hello Guenter,
> >>>>
> >>>> I would rather avoid the function pointer cast.
> >>>> How about defining an auxiliary function for the cleanup action?
> >>>>
> >>>> clk_disable_unprepare() is static inline, so gcc will have to
> >>>> define an auxiliary function either way. What do you think?
> >>>
> >>> Not really. It would just make it more complicated to replace the
> >>> call with devm_clk_prepare_enable(), should it ever find its way
> >>> into the light of day.
> >>
> >> More complicated, because the cleanup function will have to be deleted later?
> >> The compiler will warn if someone forgets to do that.
> >>
> >> In my opinion, it's not a good idea to rely on the fact that casting
> >> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> >> on most platforms. (It has undefined behavior, strictly speaking.)
> >
> > I do hear that you object to this code.
> > 
> > However, I must admit that you completely lost me here. It is a cast from
> > one function pointer to another,
> 
> Perhaps you are used to work at the assembly level, where pointers are
> just addresses, and all pointers are interchangeable.
> 
> At a slightly higher level (C abstract machine), it is not so.
> 
> > passed as argument to another function,
> > with a secondary cast of its argument from a typed pointer to a void pointer.
> > I don't think C permits for "undefined behavior, strictly speaking".
> 
> The C standard leaves quite a lot of behavior undefined, e.g.
> 
> char *foo = "hello";
> foo[1] = 'a'; // UB
> 
> char buf[4];
> *(int *)&buf = 0xdeadbeef; // UB
> 
> 1 << 64; // UB
> 
Ah, yes, I stand corrected.

However, some other unrelated undefined behavior does not mean that this
specific behavior is undefined.

So far we have a claim that a cast to a void * may somehow be different
to a cast to a different pointer, if used as function argument, and that
the behavior with such a cast may be undefined. In other words, you claim
that a function implemented as, say,

   void func(int *var) {}

might result in undefined behavior if some header file declares it as

    void func(void *);

and it is called as

    int var;

    func(&var);

That seems really far fetched to me.

I do get the message that you do not like this kind of cast. But that doesn't
mean it is not correct.

> > Besides, that same mechanism is already used elsewhere, which is how I
> > got the idea. Are you claiming that there are situations where it won't
> > work ?
> 
> If this technique is already used elsewhere in the kernel, then I'll
> crawl back under my rock (and weep).
> 

git grep "(void(\*)(void \*))"

and variants thereof:

git grep "(void(\*)"

> I can see two issues with the code you propose.
> 
> First is the same for all casts: silencing potential warnings,
> e.g. if the prototype of clk_disable_unprepare ever changed.
> (Though casts are required for vararg function arguments.)
> 
Understood. However, one should really hope that anyone changing
an API has a look at all its callers and does not just pray that
there are no problems.

> Second is just theory and not a real-world concern.
> 
> >> Do you really dislike the portable solution I suggested? :-(
> >
> > It is not more portable than the above. It is more expensive and adds more
> > code.
> 
> Maybe I am mistaken. Can you tell me why adding an auxiliary function
> is more expensive? (In CPU cycles?)
> 
In terms of code required. The idea here is to simplify the code, not
to make it more complex. The auxiliary function needs to be declared
and maintained in each affected file. I do find it easier and better
(and safer, for that matter) to let the C compiler handle it.

Guenter

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 11/01/2017 18:51, Guenter Roeck wrote:

> However, some other unrelated undefined behavior does not mean that this
> specific behavior is undefined.

True :-)

Let me just give two additional examples of UB that /have/ bitten
Linux kernel devs.

int i;
for (i = 1; i > 0; ++i)
	/* do_something(); */

=> optimized into an infinite loop

and

void func(struct foo *p) {
	int n = p->field;
	if (!p) return;

=> null-pointer check optimized away

> So far we have a claim that a cast to a void * may somehow be different
> to a cast to a different pointer, if used as function argument, and that
> the behavior with such a cast may be undefined. In other words, you claim
> that a function implemented as, say,
> 
>    void func(int *var) {}
> 
> might result in undefined behavior if some header file declares it as
> 
>     void func(void *);
> 
> and it is called as
> 
>     int var;
> 
>     func(&var);
> 
> That seems really far fetched to me.

Thanks for giving me an opportunity to play the language lawyer :-)

C99 6.3.2.3 sub-clause 8 states:

"A pointer to a function of one type may be converted to a pointer to a function of another
type and back again; the result shall compare equal to the original pointer. If a converted
pointer is used to call a function whose type is not compatible with the pointed-to type,
the behavior is undefined."

So, the behavior is undefined, not when you cast clk_disable_unprepare,
but when clk_disable_unprepare is later called through the devres->action
function pointer.

However, I agree that it will work as expected on typical platforms
(where all pointers are the same size, and the calling convention
treats all pointers the same).

> I do get the message that you do not like this kind of cast. But that doesn't
> mean it is not correct.

If it's already widely used in the kernel, it seems there is no point
fighting it ;-)

Regards.

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Uwe Kleine-König]

On Thu, Jan 12, 2017 at 10:44:07AM +0100, Marc Gonzalez wrote:
> On 11/01/2017 18:51, Guenter Roeck wrote:
> 
> > However, some other unrelated undefined behavior does not mean that this
> > specific behavior is undefined.
> 
> True :-)
> 
> Let me just give two additional examples of UB that /have/ bitten
> Linux kernel devs.
> 
> int i;
> for (i = 1; i > 0; ++i)
> 	/* do_something(); */
> 
> => optimized into an infinite loop
> 
> and
> 
> void func(struct foo *p) {
> 	int n = p->field;
> 	if (!p) return;
> 
> => null-pointer check optimized away
> 
> > So far we have a claim that a cast to a void * may somehow be different
> > to a cast to a different pointer, if used as function argument, and that
> > the behavior with such a cast may be undefined. In other words, you claim
> > that a function implemented as, say,
> > 
> >    void func(int *var) {}
> > 
> > might result in undefined behavior if some header file declares it as
> > 
> >     void func(void *);
> > 
> > and it is called as
> > 
> >     int var;
> > 
> >     func(&var);
> > 
> > That seems really far fetched to me.
> 
> Thanks for giving me an opportunity to play the language lawyer :-)
> 
> C99 6.3.2.3 sub-clause 8 states:
> 
> "A pointer to a function of one type may be converted to a pointer to a function of another
> type and back again; the result shall compare equal to the original pointer. If a converted
> pointer is used to call a function whose type is not compatible with the pointed-to type,
> the behavior is undefined."
> 
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.
> 
> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).
> 
> > I do get the message that you do not like this kind of cast. But that doesn't
> > mean it is not correct.
> 
> If it's already widely used in the kernel, it seems there is no point
> fighting it ;-)

I'd say +.5 here (where +1 is an ack). My approach would be to push
devm_clk_prepare_enable and use that. It cannot be that hard, can it?

It looks prettier, is well defined, easier to fit into 80 chars per
line. I wonder why not everybody jubilates on this new function.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

>> So far we have a claim that a cast to a void * may somehow be different
>> to a cast to a different pointer, if used as function argument, and that
>> the behavior with such a cast may be undefined. In other words, you claim
>> that a function implemented as, say,
>> 
>>    void func(int *var) {}
>> 
>> might result in undefined behavior if some header file declares it as
>> 
>>     void func(void *);
>> 
>> and it is called as
>> 
>>     int var;
>> 
>>     func(&var);
>> 
>> That seems really far fetched to me.
>
> Thanks for giving me an opportunity to play the language lawyer :-)
>
> C99 6.3.2.3 sub-clause 8 states:
>
> "A pointer to a function of one type may be converted to a pointer to
> a function of another type and back again; the result shall compare
> equal to the original pointer. If a converted pointer is used to call
> a function whose type is not compatible with the pointed-to type, the
> behavior is undefined."
>
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.

Only if the function types are incompatible.  C99 6.7.5.3 subclause 15:

  For two function types to be compatible, both shall specify compatible
  return types.  Moreover, the parameter type lists, if both are
  present, shall agree in the number of parameters and in use of the
  ellipsis terminator; corresponding parameters shall have compatible
  types.

The question then is whether pointer to void and pointer to struct clk
are compatible types.  6.7.5.1 subclause 2:

  For two pointer types to be compatible, both shall be identically
  qualified and both shall be pointers to compatible types.

6.2.5 subclause 27:

  A pointer to void shall have the same representation and alignment
  requirements as a pointer to a character type. 39)

  39) The same representation and alignment requirements are meant to
      imply interchangeability as arguments to functions, return values
      from functions, and members of unions.

6.3.2.3 subclause 1:

  A pointer to void may be converted to or from a pointer to any
  incomplete or object type.

>From what I can tell, the standard stops just short of declaring pointer
to void compatible with other pointer types.

> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).

Yes, I don't see how it could possibly go wrong.

-- 
M?ns Rullg?rd

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Marc Gonzalez]

On 12/01/2017 12:24, M?ns Rullg?rd wrote:

> Marc Gonzalez writes:
> 
>>> So far we have a claim that a cast to a void * may somehow be different
>>> to a cast to a different pointer, if used as function argument, and that
>>> the behavior with such a cast may be undefined. In other words, you claim
>>> that a function implemented as, say,
>>>
>>>    void func(int *var) {}
>>>
>>> might result in undefined behavior if some header file declares it as
>>>
>>>     void func(void *);
>>>
>>> and it is called as
>>>
>>>     int var;
>>>
>>>     func(&var);
>>>
>>> That seems really far fetched to me.
>>
>> Thanks for giving me an opportunity to play the language lawyer :-)
>>
>> C99 6.3.2.3 sub-clause 8 states:
>>
>> "A pointer to a function of one type may be converted to a pointer to
>> a function of another type and back again; the result shall compare
>> equal to the original pointer. If a converted pointer is used to call
>> a function whose type is not compatible with the pointed-to type, the
>> behavior is undefined."
>>
>> So, the behavior is undefined, not when you cast clk_disable_unprepare,
>> but when clk_disable_unprepare is later called through the devres->action
>> function pointer.
> 
> Only if the function types are incompatible.  C99 6.7.5.3 subclause 15:
> 
>   For two function types to be compatible, both shall specify compatible
>   return types.  Moreover, the parameter type lists, if both are
>   present, shall agree in the number of parameters and in use of the
>   ellipsis terminator; corresponding parameters shall have compatible
>   types.
> 
> The question then is whether pointer to void and pointer to struct clk
> are compatible types.

6.2.7 Compatible type and composite type
sub-clause 1

"Two types have compatible type if their types are the same. Additional rules for
determining whether two types are compatible are described in 6.7.2 for type specifiers,
in 6.7.3 for type qualifiers, and in 6.7.5 for declarators."

6.7.5.1 Pointer declarators
sub-clause 2

"For two pointer types to be compatible, both shall be identically qualified and both shall
be pointers to compatible types."

I don't think void and struct clk are compatible types.
AFAIU, conversion and compatibility are two separate subjects.

Regards.

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Uwe Kleine-König]

On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
> On 11/01/2017 11:52, Guenter Roeck wrote:
> 
> > On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> > 
> >>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
> >>> +				       (void(*)(void *))clk_disable_unprepare,
> >>> +				       dev->clk);
> >>> +	if (err)
> >>> +		return err;

This looks wrong. There is no clk_unprepare_disable when
devm_add_action_or_reset fails.

> >>
> >> Hello Guenter,
> >>
> >> I would rather avoid the function pointer cast.
> >> How about defining an auxiliary function for the cleanup action?
> >>
> >> clk_disable_unprepare() is static inline, so gcc will have to
> >> define an auxiliary function either way. What do you think?
> > 
> > Not really. It would just make it more complicated to replace the
> > call with devm_clk_prepare_enable(), should it ever find its way
> > into the light of day.
> 
> More complicated, because the cleanup function will have to be deleted later?
> The compiler will warn if someone forgets to do that.
> 
> In my opinion, it's not a good idea to rely on the fact that casting
> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> on most platforms. (It has undefined behavior, strictly speaking.)

I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
there couldn't be found a better solution.

If in the end it looks like the following that would be good I think:

	clk = devm_clk_get(...);
	if (IS_ERR(clk))
		...

	ret = devm_clk_prepare_enable(clk)
	if (ret)
		return ret;

	...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Vladimir Zapolskiy]

Hello Uwe,

On 01/11/2017 04:39 PM, Uwe Kleine-K?nig wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
>>>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>>>>> +				       (void(*)(void *))clk_disable_unprepare,
>>>>> +				       dev->clk);
>>>>> +	if (err)
>>>>> +		return err;
> 
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.

actually there is a call to clk_disable_unprepare() on error path, you may
take a look at devm_add_action_or_reset() implementation.

Your comment is valid for devm_add_action() function though, but it's not
the case here.

--
With best wishes,
Vladimir

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On Wed, Jan 11, 2017 at 03:39:17PM +0100, Uwe Kleine-K?nig wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
> > On 11/01/2017 11:52, Guenter Roeck wrote:
> > 
> > > On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> > > 
> > >>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
> > >>> +				       (void(*)(void *))clk_disable_unprepare,
> > >>> +				       dev->clk);
> > >>> +	if (err)
> > >>> +		return err;
> 
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.
> 
That is what the _or_reset part of devm_add_action_or_reset() is for.

> > >>
> > >> Hello Guenter,
> > >>
> > >> I would rather avoid the function pointer cast.
> > >> How about defining an auxiliary function for the cleanup action?
> > >>
> > >> clk_disable_unprepare() is static inline, so gcc will have to
> > >> define an auxiliary function either way. What do you think?
> > > 
> > > Not really. It would just make it more complicated to replace the
> > > call with devm_clk_prepare_enable(), should it ever find its way
> > > into the light of day.
> > 
> > More complicated, because the cleanup function will have to be deleted later?
> > The compiler will warn if someone forgets to do that.
> > 
> > In my opinion, it's not a good idea to rely on the fact that casting
> > void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
> > on most platforms. (It has undefined behavior, strictly speaking.)
> 
> I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
> there couldn't be found a better solution.
> 
> If in the end it looks like the following that would be good I think:
> 
> 	clk = devm_clk_get(...);
> 	if (IS_ERR(clk))
> 		...
> 
> 	ret = devm_clk_prepare_enable(clk)
> 	if (ret)
> 		return ret;
> 
Yes, Dmitry tried to introduce devm_clk_prepare_enable() some 5 years ago,
but the effort stalled.

My take is that it will be easy to write another coccinelle script to convert
to devm_clk_prepare_enable() once that is available, but I didn't see the point
of waiting for that, especially since it may never happen.

Guenter

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 06:39 AM, Uwe Kleine-K?nig wrote:
> On Wed, Jan 11, 2017 at 01:31:47PM +0100, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
>>>>> @@ -134,12 +134,15 @@ 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(&pdev->dev,
>>>>> +				       (void(*)(void *))clk_disable_unprepare,
>>>>> +				       dev->clk);
>>>>> +	if (err)
>>>>> +		return err;
>
> This looks wrong. There is no clk_unprepare_disable when
> devm_add_action_or_reset fails.
>
>>>>
>>>> Hello Guenter,
>>>>
>>>> I would rather avoid the function pointer cast.
>>>> How about defining an auxiliary function for the cleanup action?
>>>>
>>>> clk_disable_unprepare() is static inline, so gcc will have to
>>>> define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>
> I would expect it to work on all (Linux) platforms. Anyhow, I wonder if
> there couldn't be found a better solution.
>
> If in the end it looks like the following that would be good I think:
>
> 	clk = devm_clk_get(...);
> 	if (IS_ERR(clk))
> 		...
>
> 	ret = devm_clk_prepare_enable(clk)
> 	if (ret)
> 		return ret;
>

It turns out that at least one static analyzer complains about different
parameter pointer types in situations like this, and at least one embedded
compiler manages to create function names with embedded parameter type
(eg it appends an 'i' to the function name for each integer parameter).

With that, I consider the typecast to be too risky after all. It may work
for all of today's Linux architectures and compilers, but who knows if I
get flooded with static analyzer warnings, and who knows if gcc version
18.0 or binutils 35.0 makes it truly incompatible (following the logic of
"we can, therefore we do"). Since I also dislike the stub function solution,
at least in this situation, I'll drop all patches touching clk_prepare_enable(),
and wait for devm_clk_prepare_enable() to be available.

Guenter

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Andy Shevchenko]

On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
> Not really. It would just make it more complicated to replace the
> call with devm_clk_prepare_enable(), should it ever find its way
> into the light of day.
Actually what is the status to the patch series which brings devm_clk
stuff like prepare_enable()? It was submitted 2(?) years ago.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions

[Guenter Roeck]

On 01/11/2017 04:12 PM, Andy Shevchenko wrote:
> On Wed, Jan 11, 2017 at 12:52 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>> Not really. It would just make it more complicated to replace the
>> call with devm_clk_prepare_enable(), should it ever find its way
>> into the light of day.
> Actually what is the status to the patch series which brings devm_clk
> stuff like prepare_enable()? It was submitted 2(?) years ago.
>

It stalled.

Guenter