[PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function

[PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function

[Guenter Roeck]

Checking if there is no error followed by a goto if there is one is
confusing. Reverse the logic.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/coh901327_wdt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index a099b77fc0b9..dc97b2fd6c49 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -360,12 +360,10 @@ static int __init coh901327_probe(struct platform_device *pdev)
 
 	coh901327_wdt.parent = &pdev->dev;
 	ret = watchdog_register_device(&coh901327_wdt);
-	if (ret == 0)
-		dev_info(&pdev->dev,
-			 "initialized. timer margin=%d sec\n", margin);
-	else
+	if (ret)
 		goto out_no_wdog;
 
+	dev_info(&pdev->dev, "initialized. timer margin=%d sec\n", margin);
 	return 0;
 
 out_no_wdog:
-- 
2.7.4

[PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver

[Guenter Roeck]

Enabling the clock before accessing chip registers and disabling it
afterwards does not really make sense and only adds complexity to
the driver. In addition to that, a comment int the driver suggests
that it does not serve a useful purpose either.

"The watchdog block is of course always clocked, the
 clk_enable()/clk_disable() calls are mainly for performing reference
 counting higher up in the clock hierarchy."

Just keep the clock enabled instead.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/coh901327_wdt.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index dc97b2fd6c49..1385a920df4f 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -74,11 +74,6 @@ static int irq;
 static void __iomem *virtbase;
 static struct device *parent;
 
-/*
- * The watchdog block is of course always clocked, the
- * clk_enable()/clk_disable() calls are mainly for performing reference
- * counting higher up in the clock hierarchy.
- */
 static struct clk *clk;
 
 /*
@@ -90,7 +85,6 @@ static void coh901327_enable(u16 timeout)
 	unsigned long freq;
 	unsigned long delay_ns;
 
-	clk_enable(clk);
 	/* Restart timer if it is disabled */
 	val = readw(virtbase + U300_WDOG_D2R);
 	if (val == U300_WDOG_D2R_DISABLE_STATUS_DISABLED)
@@ -118,7 +112,6 @@ static void coh901327_enable(u16 timeout)
 	 */
 	(void) readw(virtbase + U300_WDOG_CR);
 	val = readw(virtbase + U300_WDOG_D2R);
-	clk_disable(clk);
 	if (val != U300_WDOG_D2R_DISABLE_STATUS_ENABLED)
 		dev_err(parent,
 			"%s(): watchdog not enabled! D2R value %04x\n",
@@ -129,7 +122,6 @@ static void coh901327_disable(void)
 {
 	u16 val;
 
-	clk_enable(clk);
 	/* Disable the watchdog interrupt if it is active */
 	writew(0x0000U, virtbase + U300_WDOG_IMR);
 	/* If the watchdog is currently enabled, attempt to disable it */
@@ -144,7 +136,6 @@ static void coh901327_disable(void)
 		       virtbase + U300_WDOG_D2R);
 	}
 	val = readw(virtbase + U300_WDOG_D2R);
-	clk_disable(clk);
 	if (val != U300_WDOG_D2R_DISABLE_STATUS_DISABLED)
 		dev_err(parent,
 			"%s(): watchdog not disabled! D2R value %04x\n",
@@ -165,11 +156,9 @@ static int coh901327_stop(struct watchdog_device *wdt_dev)
 
 static int coh901327_ping(struct watchdog_device *wdd)
 {
-	clk_enable(clk);
 	/* Feed the watchdog */
 	writew(U300_WDOG_FR_FEED_RESTART_TIMER,
 	       virtbase + U300_WDOG_FR);
-	clk_disable(clk);
 	return 0;
 }
 
@@ -177,13 +166,11 @@ static int coh901327_settimeout(struct watchdog_device *wdt_dev,
 				unsigned int time)
 {
 	wdt_dev->timeout = time;
-	clk_enable(clk);
 	/* Set new timeout value */
 	writew(time * 100, virtbase + U300_WDOG_TR);
 	/* Feed the dog */
 	writew(U300_WDOG_FR_FEED_RESTART_TIMER,
 	       virtbase + U300_WDOG_FR);
-	clk_disable(clk);
 	return 0;
 }
 
@@ -191,13 +178,11 @@ static unsigned int coh901327_gettimeleft(struct watchdog_device *wdt_dev)
 {
 	u16 val;
 
-	clk_enable(clk);
 	/* Read repeatedly until the value is stable! */
 	val = readw(virtbase + U300_WDOG_CR);
 	while (val & U300_WDOG_CR_VALID_IND)
 		val = readw(virtbase + U300_WDOG_CR);
 	val &= U300_WDOG_CR_COUNT_VALUE_MASK;
-	clk_disable(clk);
 	if (val != 0)
 		val /= 100;
 
@@ -221,13 +206,11 @@ static irqreturn_t coh901327_interrupt(int irq, void *data)
 	 * to prevent a watchdog reset by feeding the watchdog at this
 	 * point.
 	 */
-	clk_enable(clk);
 	val = readw(virtbase + U300_WDOG_IER);
 	if (val == U300_WDOG_IER_WILL_BARK_IRQ_EVENT_IND)
 		writew(U300_WDOG_IER_WILL_BARK_IRQ_ACK_ENABLE,
 		       virtbase + U300_WDOG_IER);
 	writew(0x0000U, virtbase + U300_WDOG_IMR);
-	clk_disable(clk);
 	dev_crit(parent, "watchdog is barking!\n");
 	return IRQ_HANDLED;
 }
@@ -263,7 +246,7 @@ static int __exit coh901327_remove(struct platform_device *pdev)
 	watchdog_unregister_device(&coh901327_wdt);
 	coh901327_disable();
 	free_irq(irq, pdev);
-	clk_unprepare(clk);
+	clk_disable_unprepare(clk);
 	clk_put(clk);
 	iounmap(virtbase);
 	release_mem_region(phybase, physize);
@@ -352,8 +335,6 @@ static int __init coh901327_probe(struct platform_device *pdev)
 		goto out_no_irq;
 	}
 
-	clk_disable(clk);
-
 	ret = watchdog_init_timeout(&coh901327_wdt, margin, &pdev->dev);
 	if (ret < 0)
 		coh901327_wdt.timeout = 60;
-- 
2.7.4

Re: [PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver

[Linus Walleij]

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

> Enabling the clock before accessing chip registers and disabling it
> afterwards does not really make sense and only adds complexity to
> the driver. In addition to that, a comment int the driver suggests
> that it does not serve a useful purpose either.
>
> "The watchdog block is of course always clocked, the
>  clk_enable()/clk_disable() calls are mainly for performing reference
>  counting higher up in the clock hierarchy."
>
> Just keep the clock enabled instead.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

Yours,
Linus Walleij

[PATCH 3/4] watchdog: coh901327_wdt: Use devm_ioremap_resource to map resources

[Guenter Roeck]

Map resources using devm_ioremap_resource() to simplify error handling.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/coh901327_wdt.c | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index 1385a920df4f..986222efe174 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -68,8 +68,6 @@
 
 /* Default timeout in seconds = 1 minute */
 static unsigned int margin = 60;
-static resource_size_t phybase;
-static resource_size_t physize;
 static int irq;
 static void __iomem *virtbase;
 static struct device *parent;
@@ -248,8 +246,6 @@ static int __exit coh901327_remove(struct platform_device *pdev)
 	free_irq(irq, pdev);
 	clk_disable_unprepare(clk);
 	clk_put(clk);
-	iounmap(virtbase);
-	release_mem_region(phybase, physize);
 	return 0;
 }
 
@@ -259,30 +255,18 @@ static int __init coh901327_probe(struct platform_device *pdev)
 	u16 val;
 	struct resource *res;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENOENT;
-
 	parent = &pdev->dev;
-	physize = resource_size(res);
-	phybase = res->start;
-
-	if (request_mem_region(phybase, physize, DRV_NAME) == NULL) {
-		ret = -EBUSY;
-		goto out;
-	}
 
-	virtbase = ioremap(phybase, physize);
-	if (!virtbase) {
-		ret = -ENOMEM;
-		goto out_no_remap;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	virtbase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(virtbase))
+		return PTR_ERR(virtbase);
 
 	clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
 		ret = PTR_ERR(clk);
 		dev_err(&pdev->dev, "could not get clock\n");
-		goto out_no_clk;
+		return ret;
 	}
 	ret = clk_prepare_enable(clk);
 	if (ret) {
@@ -353,11 +337,6 @@ static int __init coh901327_probe(struct platform_device *pdev)
 	clk_disable_unprepare(clk);
 out_no_clk_enable:
 	clk_put(clk);
-out_no_clk:
-	iounmap(virtbase);
-out_no_remap:
-	release_mem_region(phybase, SZ_4K);
-out:
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH 3/4] watchdog: coh901327_wdt: Use devm_ioremap_resource to map resources

[Linus Walleij]

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

> Map resources using devm_ioremap_resource() to simplify error handling.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

Yours,
Linus Walleij

[PATCH 4/4] watchdog: coh901327_wdt: Use dev variable instead of pdev->dev

[Guenter Roeck]

Use a local dev variable instead of dereferencing pdev->dev several
times in the probe function to make the code easier to read.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/coh901327_wdt.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index 986222efe174..38dd60f0cfcc 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -251,60 +251,56 @@ static int __exit coh901327_remove(struct platform_device *pdev)
 
 static int __init coh901327_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	int ret;
 	u16 val;
 	struct resource *res;
 
-	parent = &pdev->dev;
+	parent = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	virtbase = devm_ioremap_resource(&pdev->dev, res);
+	virtbase = devm_ioremap_resource(dev, res);
 	if (IS_ERR(virtbase))
 		return PTR_ERR(virtbase);
 
-	clk = clk_get(&pdev->dev, NULL);
+	clk = clk_get(dev, NULL);
 	if (IS_ERR(clk)) {
 		ret = PTR_ERR(clk);
-		dev_err(&pdev->dev, "could not get clock\n");
+		dev_err(dev, "could not get clock\n");
 		return ret;
 	}
 	ret = clk_prepare_enable(clk);
 	if (ret) {
-		dev_err(&pdev->dev, "could not prepare and enable clock\n");
+		dev_err(dev, "could not prepare and enable clock\n");
 		goto out_no_clk_enable;
 	}
 
 	val = readw(virtbase + U300_WDOG_SR);
 	switch (val) {
 	case U300_WDOG_SR_STATUS_TIMED_OUT:
-		dev_info(&pdev->dev,
-			"watchdog timed out since last chip reset!\n");
+		dev_info(dev, "watchdog timed out since last chip reset!\n");
 		coh901327_wdt.bootstatus |= WDIOF_CARDRESET;
 		/* Status will be cleared below */
 		break;
 	case U300_WDOG_SR_STATUS_NORMAL:
-		dev_info(&pdev->dev,
-			"in normal status, no timeouts have occurred.\n");
+		dev_info(dev, "in normal status, no timeouts have occurred.\n");
 		break;
 	default:
-		dev_info(&pdev->dev,
-			"contains an illegal status code (%08x)\n", val);
+		dev_info(dev, "contains an illegal status code (%08x)\n", val);
 		break;
 	}
 
 	val = readw(virtbase + U300_WDOG_D2R);
 	switch (val) {
 	case U300_WDOG_D2R_DISABLE_STATUS_DISABLED:
-		dev_info(&pdev->dev, "currently disabled.\n");
+		dev_info(dev, "currently disabled.\n");
 		break;
 	case U300_WDOG_D2R_DISABLE_STATUS_ENABLED:
-		dev_info(&pdev->dev,
-			 "currently enabled! (disabling it now)\n");
+		dev_info(dev, "currently enabled! (disabling it now)\n");
 		coh901327_disable();
 		break;
 	default:
-		dev_err(&pdev->dev,
-			"contains an illegal enable/disable code (%08x)\n",
+		dev_err(dev, "contains an illegal enable/disable code (%08x)\n",
 			val);
 		break;
 	}
@@ -319,16 +315,16 @@ static int __init coh901327_probe(struct platform_device *pdev)
 		goto out_no_irq;
 	}
 
-	ret = watchdog_init_timeout(&coh901327_wdt, margin, &pdev->dev);
+	ret = watchdog_init_timeout(&coh901327_wdt, margin, dev);
 	if (ret < 0)
 		coh901327_wdt.timeout = 60;
 
-	coh901327_wdt.parent = &pdev->dev;
+	coh901327_wdt.parent = dev;
 	ret = watchdog_register_device(&coh901327_wdt);
 	if (ret)
 		goto out_no_wdog;
 
-	dev_info(&pdev->dev, "initialized. timer margin=%d sec\n", margin);
+	dev_info(dev, "initialized. timer margin=%d sec\n", margin);
 	return 0;
 
 out_no_wdog:
-- 
2.7.4

Re: [PATCH 4/4] watchdog: coh901327_wdt: Use dev variable instead of pdev->dev

[Linus Walleij]

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

> Use a local dev variable instead of dereferencing pdev->dev several
> times in the probe function to make the code easier to read.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

Yours,
Linus Walleij

Re: [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function

[Linus Walleij]

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

> Checking if there is no error followed by a goto if there is one is
> confusing. Reverse the logic.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

Yours,
Linus Walleij

Re: [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function

[Guenter Roeck]

On Mon, Jan 09, 2017 at 08:38:03PM +0100, Linus Walleij wrote:
> On Wed, Jan 4, 2017 at 4:25 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Checking if there is no error followed by a goto if there is one is
> > confusing. Reverse the logic.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
Thanks!

Guenter