* [PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver
2017-01-04 3:25 [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Guenter Roeck
@ 2017-01-04 3:25 ` Guenter Roeck
2017-01-09 19:38 ` Linus Walleij
2017-01-04 3:25 ` [PATCH 3/4] watchdog: coh901327_wdt: Use devm_ioremap_resource to map resources Guenter Roeck
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2017-01-04 3:25 UTC (permalink / raw)
To: Linus Walleij
Cc: Wim Van Sebroeck, linux-arm-kernel, linux-watchdog, linux-kernel,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver
2017-01-04 3:25 ` [PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver Guenter Roeck
@ 2017-01-09 19:38 ` Linus Walleij
0 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2017-01-09 19:38 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, linux-arm-kernel, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] watchdog: coh901327_wdt: Use devm_ioremap_resource to map resources
2017-01-04 3:25 [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Guenter Roeck
2017-01-04 3:25 ` [PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver Guenter Roeck
@ 2017-01-04 3:25 ` Guenter Roeck
2017-01-09 19:39 ` Linus Walleij
2017-01-04 3:25 ` [PATCH 4/4] watchdog: coh901327_wdt: Use dev variable instead of pdev->dev Guenter Roeck
2017-01-09 19:38 ` [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Linus Walleij
3 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2017-01-04 3:25 UTC (permalink / raw)
To: Linus Walleij
Cc: Wim Van Sebroeck, linux-arm-kernel, linux-watchdog, linux-kernel,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] watchdog: coh901327_wdt: Use dev variable instead of pdev->dev
2017-01-04 3:25 [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Guenter Roeck
2017-01-04 3:25 ` [PATCH 2/4] watchdog: coh901327_wdt: Keep clock enabled after loading driver Guenter Roeck
2017-01-04 3:25 ` [PATCH 3/4] watchdog: coh901327_wdt: Use devm_ioremap_resource to map resources Guenter Roeck
@ 2017-01-04 3:25 ` Guenter Roeck
2017-01-09 19:39 ` Linus Walleij
2017-01-09 19:38 ` [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Linus Walleij
3 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2017-01-04 3:25 UTC (permalink / raw)
To: Linus Walleij
Cc: Wim Van Sebroeck, linux-arm-kernel, linux-watchdog, linux-kernel,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function
2017-01-04 3:25 [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Guenter Roeck
` (2 preceding siblings ...)
2017-01-04 3:25 ` [PATCH 4/4] watchdog: coh901327_wdt: Use dev variable instead of pdev->dev Guenter Roeck
@ 2017-01-09 19:38 ` Linus Walleij
2017-01-09 20:17 ` Guenter Roeck
3 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2017-01-09 19:38 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, linux-arm-kernel, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function
2017-01-09 19:38 ` [PATCH 1/4] watchdog: coh901327_wdt: Simplify error handling in probe function Linus Walleij
@ 2017-01-09 20:17 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2017-01-09 20:17 UTC (permalink / raw)
To: Linus Walleij
Cc: Wim Van Sebroeck, linux-arm-kernel, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread