[PATCH v1 0/5] watchdog: mpc8xxx: several cleanups

[PATCH v1 0/5] watchdog: mpc8xxx: several cleanups

[Uwe Kleine-König]

Hello,

while working with an MPC5125 processor and Guenter's latest watchdog series
I cleaned up the mpc8xxx a bit. Here is the result.

Best regards
Uwe

Uwe Kleine-König (5):
  watchdog: mpc8xxx: remove dead code
  watchdog: mpc8xxx: simplify registration
  watchdog: mpc8xxx: make use of of_device_get_match_data
  watchdog: mpc8xxx: use dynamic memory for device specific data
  watchdog: mpc8xxx: allow to compile for MPC512x

 drivers/watchdog/Kconfig       |   2 +-
 drivers/watchdog/mpc8xxx_wdt.c | 150 +++++++++++++++++------------------------
 2 files changed, 61 insertions(+), 91 deletions(-)

-- 
2.4.6

[PATCH v1 1/5] watchdog: mpc8xxx: remove dead code

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/mpc8xxx_wdt.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
index 689381a24887..8ad42b83f995 100644
--- a/drivers/watchdog/mpc8xxx_wdt.c
+++ b/drivers/watchdog/mpc8xxx_wdt.c
@@ -68,12 +68,6 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-/*
- * We always prescale, but if someone really doesn't want to they can set this
- * to 0
- */
-static int prescale = 1;
-
 static DEFINE_SPINLOCK(wdt_spinlock);
 
 static void mpc8xxx_wdt_keepalive(void)
@@ -101,11 +95,9 @@ static void mpc8xxx_wdt_timer_ping(unsigned long arg)
 
 static int mpc8xxx_wdt_start(struct watchdog_device *w)
 {
-	u32 tmp = SWCRR_SWEN;
+	u32 tmp = SWCRR_SWEN | SWCRR_SWPR;
 
 	/* Good, fire up the show */
-	if (prescale)
-		tmp |= SWCRR_SWPR;
 	if (reset)
 		tmp |= SWCRR_SWRI;
 
@@ -179,10 +171,7 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	}
 
 	/* Calculate the timeout in seconds */
-	if (prescale)
-		timeout_sec = (timeout * wdt_type->prescaler) / freq;
-	else
-		timeout_sec = timeout / freq;
+	timeout_sec = (timeout * wdt_type->prescaler) / freq;
 
 	mpc8xxx_wdt_dev.timeout = timeout_sec;
 #ifdef MODULE
-- 
2.4.6

Re: [PATCH v1 1/5] watchdog: mpc8xxx: remove dead code

[Guenter Roeck]

On 08/07/2015 03:07 AM, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

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

> ---
>   drivers/watchdog/mpc8xxx_wdt.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
> index 689381a24887..8ad42b83f995 100644
> --- a/drivers/watchdog/mpc8xxx_wdt.c
> +++ b/drivers/watchdog/mpc8xxx_wdt.c
> @@ -68,12 +68,6 @@ module_param(nowayout, bool, 0);
>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>   		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> -/*
> - * We always prescale, but if someone really doesn't want to they can set this
> - * to 0
> - */
> -static int prescale = 1;
> -
>   static DEFINE_SPINLOCK(wdt_spinlock);
>
>   static void mpc8xxx_wdt_keepalive(void)
> @@ -101,11 +95,9 @@ static void mpc8xxx_wdt_timer_ping(unsigned long arg)
>
>   static int mpc8xxx_wdt_start(struct watchdog_device *w)
>   {
> -	u32 tmp = SWCRR_SWEN;
> +	u32 tmp = SWCRR_SWEN | SWCRR_SWPR;
>
>   	/* Good, fire up the show */
> -	if (prescale)
> -		tmp |= SWCRR_SWPR;
>   	if (reset)
>   		tmp |= SWCRR_SWRI;
>
> @@ -179,10 +171,7 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   	}
>
>   	/* Calculate the timeout in seconds */
> -	if (prescale)
> -		timeout_sec = (timeout * wdt_type->prescaler) / freq;
> -	else
> -		timeout_sec = timeout / freq;
> +	timeout_sec = (timeout * wdt_type->prescaler) / freq;
>
>   	mpc8xxx_wdt_dev.timeout = timeout_sec;
>   #ifdef MODULE
>

[PATCH v1 2/5] watchdog: mpc8xxx: simplify registration

[Uwe Kleine-König]

Since commit ef90174f8210 ("watchdog: watchdog_core: Add watchdog
registration deferral mechanism") there is no need to delay the call to
watchdog_register_device any more. So simplify the registration code
accordingly.

Resetting wd_base to NULL can the also be dropped because nothing
depends on it being NULL to signal probe failure any more. (The matching
wd_base = NULL in .remove was missing, too.)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/mpc8xxx_wdt.c | 38 +++++++-------------------------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
index 8ad42b83f995..e0acda3282d9 100644
--- a/drivers/watchdog/mpc8xxx_wdt.c
+++ b/drivers/watchdog/mpc8xxx_wdt.c
@@ -51,7 +51,6 @@ struct mpc8xxx_wdt_type {
 };
 
 static struct mpc8xxx_wdt __iomem *wd_base;
-static int mpc8xxx_wdt_init_late(void);
 
 static u16 timeout = 0xffff;
 module_param(timeout, ushort, 0);
@@ -174,11 +173,14 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	timeout_sec = (timeout * wdt_type->prescaler) / freq;
 
 	mpc8xxx_wdt_dev.timeout = timeout_sec;
-#ifdef MODULE
-	ret = mpc8xxx_wdt_init_late();
-	if (ret)
+
+	watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
+
+	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
+	if (ret) {
+		pr_err("cannot register watchdog device (err=%d)\n", ret);
 		goto err_unmap;
-#endif
+	}
 
 	pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d (%d seconds)\n",
 		reset ? "reset" : "interrupt", timeout, timeout_sec);
@@ -193,7 +195,6 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	return 0;
 err_unmap:
 	iounmap(wd_base);
-	wd_base = NULL;
 	return ret;
 }
 
@@ -242,31 +243,6 @@ static struct platform_driver mpc8xxx_wdt_driver = {
 	},
 };
 
-/*
- * We do wdt initialization in two steps: arch_initcall probes the wdt
- * very early to start pinging the watchdog (misc devices are not yet
- * available), and later module_init() just registers the misc device.
- */
-static int mpc8xxx_wdt_init_late(void)
-{
-	int ret;
-
-	if (!wd_base)
-		return -ENODEV;
-
-	watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
-
-	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
-	if (ret) {
-		pr_err("cannot register watchdog device (err=%d)\n", ret);
-		return ret;
-	}
-	return 0;
-}
-#ifndef MODULE
-module_init(mpc8xxx_wdt_init_late);
-#endif
-
 static int __init mpc8xxx_wdt_init(void)
 {
 	return platform_driver_register(&mpc8xxx_wdt_driver);
-- 
2.4.6

Re: [PATCH v1 2/5] watchdog: mpc8xxx: simplify registration

[Guenter Roeck]

On 08/07/2015 03:07 AM, Uwe Kleine-König wrote:
> Since commit ef90174f8210 ("watchdog: watchdog_core: Add watchdog
> registration deferral mechanism") there is no need to delay the call to
> watchdog_register_device any more. So simplify the registration code
> accordingly.
>
> Resetting wd_base to NULL can the also be dropped because nothing
> depends on it being NULL to signal probe failure any more. (The matching
> wd_base = NULL in .remove was missing, too.)
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

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

> ---
>   drivers/watchdog/mpc8xxx_wdt.c | 38 +++++++-------------------------------
>   1 file changed, 7 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
> index 8ad42b83f995..e0acda3282d9 100644
> --- a/drivers/watchdog/mpc8xxx_wdt.c
> +++ b/drivers/watchdog/mpc8xxx_wdt.c
> @@ -51,7 +51,6 @@ struct mpc8xxx_wdt_type {
>   };
>
>   static struct mpc8xxx_wdt __iomem *wd_base;
> -static int mpc8xxx_wdt_init_late(void);
>
>   static u16 timeout = 0xffff;
>   module_param(timeout, ushort, 0);
> @@ -174,11 +173,14 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   	timeout_sec = (timeout * wdt_type->prescaler) / freq;
>
>   	mpc8xxx_wdt_dev.timeout = timeout_sec;
> -#ifdef MODULE
> -	ret = mpc8xxx_wdt_init_late();
> -	if (ret)
> +
> +	watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
> +
> +	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
> +	if (ret) {
> +		pr_err("cannot register watchdog device (err=%d)\n", ret);
>   		goto err_unmap;
> -#endif
> +	}
>
>   	pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d (%d seconds)\n",
>   		reset ? "reset" : "interrupt", timeout, timeout_sec);
> @@ -193,7 +195,6 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   	return 0;
>   err_unmap:
>   	iounmap(wd_base);
> -	wd_base = NULL;
>   	return ret;
>   }
>
> @@ -242,31 +243,6 @@ static struct platform_driver mpc8xxx_wdt_driver = {
>   	},
>   };
>
> -/*
> - * We do wdt initialization in two steps: arch_initcall probes the wdt
> - * very early to start pinging the watchdog (misc devices are not yet
> - * available), and later module_init() just registers the misc device.
> - */
> -static int mpc8xxx_wdt_init_late(void)
> -{
> -	int ret;
> -
> -	if (!wd_base)
> -		return -ENODEV;
> -
> -	watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
> -
> -	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
> -	if (ret) {
> -		pr_err("cannot register watchdog device (err=%d)\n", ret);
> -		return ret;
> -	}
> -	return 0;
> -}
> -#ifndef MODULE
> -module_init(mpc8xxx_wdt_init_late);
> -#endif
> -
>   static int __init mpc8xxx_wdt_init(void)
>   {
>   	return platform_driver_register(&mpc8xxx_wdt_driver);
>

[PATCH v1 3/5] watchdog: mpc8xxx: make use of of_device_get_match_data

[Uwe Kleine-König]

This function is new in v4.2-rc1 and makes a forward declaration of the
match table superfluous which can so be removed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/mpc8xxx_wdt.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
index e0acda3282d9..a6790fcfa69b 100644
--- a/drivers/watchdog/mpc8xxx_wdt.c
+++ b/drivers/watchdog/mpc8xxx_wdt.c
@@ -139,34 +139,31 @@ static struct watchdog_device mpc8xxx_wdt_dev = {
 	.ops = &mpc8xxx_wdt_ops,
 };
 
-static const struct of_device_id mpc8xxx_wdt_match[];
 static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 {
 	int ret;
-	const struct of_device_id *match;
-	struct device_node *np = ofdev->dev.of_node;
+	struct resource *res;
 	const struct mpc8xxx_wdt_type *wdt_type;
 	u32 freq = fsl_get_sys_freq();
 	bool enabled;
 	unsigned int timeout_sec;
 
-	match = of_match_device(mpc8xxx_wdt_match, &ofdev->dev);
-	if (!match)
+	wdt_type = of_device_get_match_data(&ofdev->dev);
+	if (!wdt_type)
 		return -EINVAL;
-	wdt_type = match->data;
 
 	if (!freq || freq == -1)
 		return -EINVAL;
 
-	wd_base = of_iomap(np, 0);
-	if (!wd_base)
-		return -ENOMEM;
+	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+	wd_base = devm_ioremap_resource(&ofdev->dev, res);
+	if (IS_ERR(wd_base))
+		return PTR_ERR(wd_base);
 
 	enabled = in_be32(&wd_base->swcrr) & SWCRR_SWEN;
 	if (!enabled && wdt_type->hw_enabled) {
 		pr_info("could not be enabled in software\n");
-		ret = -ENOSYS;
-		goto err_unmap;
+		return -ENOSYS;
 	}
 
 	/* Calculate the timeout in seconds */
@@ -179,7 +176,7 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
 	if (ret) {
 		pr_err("cannot register watchdog device (err=%d)\n", ret);
-		goto err_unmap;
+		return ret;
 	}
 
 	pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d (%d seconds)\n",
@@ -193,9 +190,6 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	if (enabled)
 		mod_timer(&wdt_timer, jiffies);
 	return 0;
-err_unmap:
-	iounmap(wd_base);
-	return ret;
 }
 
 static int mpc8xxx_wdt_remove(struct platform_device *ofdev)
@@ -204,7 +198,6 @@ static int mpc8xxx_wdt_remove(struct platform_device *ofdev)
 		reset ? "reset" : "machine check exception");
 	del_timer_sync(&wdt_timer);
 	watchdog_unregister_device(&mpc8xxx_wdt_dev);
-	iounmap(wd_base);
 
 	return 0;
 }
-- 
2.4.6

Re: [PATCH v1 3/5] watchdog: mpc8xxx: make use of of_device_get_match_data

[Guenter Roeck]

Hi Uwe,

On 08/07/2015 03:07 AM, Uwe Kleine-König wrote:
> This function is new in v4.2-rc1 and makes a forward declaration of the
> match table superfluous which can so be removed.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/watchdog/mpc8xxx_wdt.c | 25 +++++++++----------------
>   1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
> index e0acda3282d9..a6790fcfa69b 100644
> --- a/drivers/watchdog/mpc8xxx_wdt.c
> +++ b/drivers/watchdog/mpc8xxx_wdt.c
> @@ -139,34 +139,31 @@ static struct watchdog_device mpc8xxx_wdt_dev = {
>   	.ops = &mpc8xxx_wdt_ops,
>   };
>
> -static const struct of_device_id mpc8xxx_wdt_match[];
>   static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   {
>   	int ret;
> -	const struct of_device_id *match;
> -	struct device_node *np = ofdev->dev.of_node;
> +	struct resource *res;
>   	const struct mpc8xxx_wdt_type *wdt_type;
>   	u32 freq = fsl_get_sys_freq();
>   	bool enabled;
>   	unsigned int timeout_sec;
>
> -	match = of_match_device(mpc8xxx_wdt_match, &ofdev->dev);
> -	if (!match)
> +	wdt_type = of_device_get_match_data(&ofdev->dev);
> +	if (!wdt_type)
>   		return -EINVAL;
> -	wdt_type = match->data;
>
>   	if (!freq || freq == -1)
>   		return -EINVAL;
>
> -	wd_base = of_iomap(np, 0);
> -	if (!wd_base)
> -		return -ENOMEM;
> +	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> +	wd_base = devm_ioremap_resource(&ofdev->dev, res);
> +	if (IS_ERR(wd_base))
> +		return PTR_ERR(wd_base);
>
>   	enabled = in_be32(&wd_base->swcrr) & SWCRR_SWEN;
>   	if (!enabled && wdt_type->hw_enabled) {
>   		pr_info("could not be enabled in software\n");
> -		ret = -ENOSYS;
> -		goto err_unmap;
> +		return -ENOSYS;
>   	}

checkpatch says:

WARNING: ENOSYS means 'invalid syscall nr' and nothing else
#65: FILE: drivers/watchdog/mpc8xxx_wdt.c:166:
+		return -ENOSYS;

Maybe just use -ENODEV.

Thanks,
Guenter

Re: [PATCH v1 3/5] watchdog: mpc8xxx: make use of of_device_get_match_data

[Uwe Kleine-König]

Hello Guenter,

On Fri, Aug 07, 2015 at 07:56:51AM -0700, Guenter Roeck wrote:
> >  	enabled = in_be32(&wd_base->swcrr) & SWCRR_SWEN;
> >  	if (!enabled && wdt_type->hw_enabled) {
> >  		pr_info("could not be enabled in software\n");
> >-		ret = -ENOSYS;
> >-		goto err_unmap;
> >+		return -ENOSYS;
> >  	}
> 
> checkpatch says:
> 
> WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> #65: FILE: drivers/watchdog/mpc8xxx_wdt.c:166:
> +		return -ENOSYS;
> 
> Maybe just use -ENODEV.
That would be a separate patch tough because I'm just changing the code
flow and ENOSYS was there already before.

Best regards
Uwe

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

Re: [PATCH v1 3/5] watchdog: mpc8xxx: make use of of_device_get_match_data

[Guenter Roeck]

Hi Uwe,

On 08/10/2015 12:24 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Fri, Aug 07, 2015 at 07:56:51AM -0700, Guenter Roeck wrote:
>>>   	enabled = in_be32(&wd_base->swcrr) & SWCRR_SWEN;
>>>   	if (!enabled && wdt_type->hw_enabled) {
>>>   		pr_info("could not be enabled in software\n");
>>> -		ret = -ENOSYS;
>>> -		goto err_unmap;
>>> +		return -ENOSYS;
>>>   	}
>>
>> checkpatch says:
>>
>> WARNING: ENOSYS means 'invalid syscall nr' and nothing else
>> #65: FILE: drivers/watchdog/mpc8xxx_wdt.c:166:
>> +		return -ENOSYS;
>>
>> Maybe just use -ENODEV.
> That would be a separate patch tough because I'm just changing the code
> flow and ENOSYS was there already before.
>
Since you are at it, can you add a patch to do that ?

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] watchdog: mpc8xxx: make use of of_device_get_match_data

[Uwe Kleine-König]

On Fri, Aug 07, 2015 at 12:07:45PM +0200, Uwe Kleine-König wrote:
> This function is new in v4.2-rc1 and makes a forward declaration of the
> match table superfluous which can so be removed.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
This patch is broken, it is supposed to do less, I'll look into this
later.


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

Re: [PATCH v1 3/5] watchdog: mpc8xxx: make use of of_device_get_match_data

[Guenter Roeck]

On 08/10/2015 12:25 AM, Uwe Kleine-König wrote:
> On Fri, Aug 07, 2015 at 12:07:45PM +0200, Uwe Kleine-König wrote:
>> This function is new in v4.2-rc1 and makes a forward declaration of the
>> match table superfluous which can so be removed.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> This patch is broken, it is supposed to do less, I'll look into this
> later.
>
>
Ok, thanks for the heads-up.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/5] watchdog: mpc8xxx: use dynamic memory for device specific data

[Uwe Kleine-König]

Instead of relying on global static memory dynamically allocate the
needed data. This has the benefit of some saved bytes if the driver is
not in use and making it possible to bind more than one device (even
though this has no known use case).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/mpc8xxx_wdt.c | 82 ++++++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 35 deletions(-)

diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
index a6790fcfa69b..a36ced9c95e4 100644
--- a/drivers/watchdog/mpc8xxx_wdt.c
+++ b/drivers/watchdog/mpc8xxx_wdt.c
@@ -50,7 +50,12 @@ struct mpc8xxx_wdt_type {
 	bool hw_enabled;
 };
 
-static struct mpc8xxx_wdt __iomem *wd_base;
+struct mpc8xxx_wdt_ddata {
+	struct mpc8xxx_wdt __iomem *base;
+	struct watchdog_device wdd;
+	struct timer_list timer;
+	spinlock_t lock;
+};
 
 static u16 timeout = 0xffff;
 module_param(timeout, ushort, 0);
@@ -67,33 +72,28 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static DEFINE_SPINLOCK(wdt_spinlock);
-
-static void mpc8xxx_wdt_keepalive(void)
+static void mpc8xxx_wdt_keepalive(struct mpc8xxx_wdt_ddata *ddata)
 {
 	/* Ping the WDT */
-	spin_lock(&wdt_spinlock);
-	out_be16(&wd_base->swsrr, 0x556c);
-	out_be16(&wd_base->swsrr, 0xaa39);
-	spin_unlock(&wdt_spinlock);
+	spin_lock(&ddata->lock);
+	out_be16(&ddata->base->swsrr, 0x556c);
+	out_be16(&ddata->base->swsrr, 0xaa39);
+	spin_unlock(&ddata->lock);
 }
 
-static struct watchdog_device mpc8xxx_wdt_dev;
-static void mpc8xxx_wdt_timer_ping(unsigned long arg);
-static DEFINE_TIMER(wdt_timer, mpc8xxx_wdt_timer_ping, 0,
-		(unsigned long)&mpc8xxx_wdt_dev);
-
 static void mpc8xxx_wdt_timer_ping(unsigned long arg)
 {
-	struct watchdog_device *w = (struct watchdog_device *)arg;
+	struct mpc8xxx_wdt_ddata *ddata = (void *)arg;
 
-	mpc8xxx_wdt_keepalive();
+	mpc8xxx_wdt_keepalive(ddata);
 	/* We're pinging it twice faster than needed, just to be sure. */
-	mod_timer(&wdt_timer, jiffies + HZ * w->timeout / 2);
+	mod_timer(&ddata->timer, jiffies + HZ * ddata->wdd.timeout / 2);
 }
 
 static int mpc8xxx_wdt_start(struct watchdog_device *w)
 {
+	struct mpc8xxx_wdt_ddata *ddata = container_of(w, struct mpc8xxx_wdt_ddata, wdd);
+
 	u32 tmp = SWCRR_SWEN | SWCRR_SWPR;
 
 	/* Good, fire up the show */
@@ -102,22 +102,24 @@ static int mpc8xxx_wdt_start(struct watchdog_device *w)
 
 	tmp |= timeout << 16;
 
-	out_be32(&wd_base->swcrr, tmp);
+	out_be32(&ddata->base->swcrr, tmp);
 
-	del_timer_sync(&wdt_timer);
+	del_timer_sync(&ddata->timer);
 
 	return 0;
 }
 
 static int mpc8xxx_wdt_ping(struct watchdog_device *w)
 {
-	mpc8xxx_wdt_keepalive();
+	struct mpc8xxx_wdt_ddata *ddata = container_of(w, struct mpc8xxx_wdt_ddata, wdd);
+	mpc8xxx_wdt_keepalive(ddata);
 	return 0;
 }
 
 static int mpc8xxx_wdt_stop(struct watchdog_device *w)
 {
-	mod_timer(&wdt_timer, jiffies);
+	struct mpc8xxx_wdt_ddata *ddata = container_of(w, struct mpc8xxx_wdt_ddata, wdd);
+	mod_timer(&ddata->timer, jiffies);
 	return 0;
 }
 
@@ -134,16 +136,12 @@ static struct watchdog_ops mpc8xxx_wdt_ops = {
 	.stop = mpc8xxx_wdt_stop,
 };
 
-static struct watchdog_device mpc8xxx_wdt_dev = {
-	.info = &mpc8xxx_wdt_info,
-	.ops = &mpc8xxx_wdt_ops,
-};
-
 static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 {
 	int ret;
 	struct resource *res;
 	const struct mpc8xxx_wdt_type *wdt_type;
+	struct mpc8xxx_wdt_ddata *ddata;
 	u32 freq = fsl_get_sys_freq();
 	bool enabled;
 	unsigned int timeout_sec;
@@ -155,25 +153,35 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	if (!freq || freq == -1)
 		return -EINVAL;
 
+	ddata = devm_kzalloc(&ofdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
 	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
-	wd_base = devm_ioremap_resource(&ofdev->dev, res);
-	if (IS_ERR(wd_base))
-		return PTR_ERR(wd_base);
+	ddata->base = devm_ioremap_resource(&ofdev->dev, res);
+	if (IS_ERR(ddata->base))
+		return PTR_ERR(ddata->base);
 
-	enabled = in_be32(&wd_base->swcrr) & SWCRR_SWEN;
+	enabled = in_be32(&ddata->base->swcrr) & SWCRR_SWEN;
 	if (!enabled && wdt_type->hw_enabled) {
 		pr_info("could not be enabled in software\n");
 		return -ENOSYS;
 	}
 
+	spin_lock_init(&ddata->lock);
+	setup_timer(&ddata->timer, mpc8xxx_wdt_timer_ping, (unsigned long)ddata);
+
+	ddata->wdd.info = &mpc8xxx_wdt_info,
+	ddata->wdd.ops = &mpc8xxx_wdt_ops,
+
 	/* Calculate the timeout in seconds */
 	timeout_sec = (timeout * wdt_type->prescaler) / freq;
 
-	mpc8xxx_wdt_dev.timeout = timeout_sec;
+	ddata->wdd.timeout = timeout_sec;
 
-	watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
+	watchdog_set_nowayout(&ddata->wdd, nowayout);
 
-	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
+	ret = watchdog_register_device(&ddata->wdd);
 	if (ret) {
 		pr_err("cannot register watchdog device (err=%d)\n", ret);
 		return ret;
@@ -188,16 +196,20 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
 	 * userspace handles it.
 	 */
 	if (enabled)
-		mod_timer(&wdt_timer, jiffies);
+		mod_timer(&ddata->timer, jiffies);
+
+	platform_set_drvdata(ofdev, ddata);
 	return 0;
 }
 
 static int mpc8xxx_wdt_remove(struct platform_device *ofdev)
 {
+	struct mpc8xxx_wdt_ddata *ddata = platform_get_drvdata(ofdev);
+
 	pr_crit("Watchdog removed, expect the %s soon!\n",
 		reset ? "reset" : "machine check exception");
-	del_timer_sync(&wdt_timer);
-	watchdog_unregister_device(&mpc8xxx_wdt_dev);
+	del_timer_sync(&ddata->timer);
+	watchdog_unregister_device(&ddata->wdd);
 
 	return 0;
 }
-- 
2.4.6

Re: [PATCH v1 4/5] watchdog: mpc8xxx: use dynamic memory for device specific data

[Guenter Roeck]

Hi Uwe,

On 08/07/2015 03:07 AM, Uwe Kleine-König wrote:
> Instead of relying on global static memory dynamically allocate the
> needed data. This has the benefit of some saved bytes if the driver is
> not in use and making it possible to bind more than one device (even
> though this has no known use case).
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/watchdog/mpc8xxx_wdt.c | 82 ++++++++++++++++++++++++------------------
>   1 file changed, 47 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
> index a6790fcfa69b..a36ced9c95e4 100644
> --- a/drivers/watchdog/mpc8xxx_wdt.c
> +++ b/drivers/watchdog/mpc8xxx_wdt.c
> @@ -50,7 +50,12 @@ struct mpc8xxx_wdt_type {
>   	bool hw_enabled;
>   };
>
> -static struct mpc8xxx_wdt __iomem *wd_base;
> +struct mpc8xxx_wdt_ddata {
> +	struct mpc8xxx_wdt __iomem *base;
> +	struct watchdog_device wdd;
> +	struct timer_list timer;
> +	spinlock_t lock;
> +};
>
>   static u16 timeout = 0xffff;
>   module_param(timeout, ushort, 0);
> @@ -67,33 +72,28 @@ module_param(nowayout, bool, 0);
>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>   		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> -static DEFINE_SPINLOCK(wdt_spinlock);
> -
> -static void mpc8xxx_wdt_keepalive(void)
> +static void mpc8xxx_wdt_keepalive(struct mpc8xxx_wdt_ddata *ddata)
>   {
>   	/* Ping the WDT */
> -	spin_lock(&wdt_spinlock);
> -	out_be16(&wd_base->swsrr, 0x556c);
> -	out_be16(&wd_base->swsrr, 0xaa39);
> -	spin_unlock(&wdt_spinlock);
> +	spin_lock(&ddata->lock);
> +	out_be16(&ddata->base->swsrr, 0x556c);
> +	out_be16(&ddata->base->swsrr, 0xaa39);
> +	spin_unlock(&ddata->lock);
>   }
>
> -static struct watchdog_device mpc8xxx_wdt_dev;
> -static void mpc8xxx_wdt_timer_ping(unsigned long arg);
> -static DEFINE_TIMER(wdt_timer, mpc8xxx_wdt_timer_ping, 0,
> -		(unsigned long)&mpc8xxx_wdt_dev);
> -
>   static void mpc8xxx_wdt_timer_ping(unsigned long arg)
>   {
> -	struct watchdog_device *w = (struct watchdog_device *)arg;
> +	struct mpc8xxx_wdt_ddata *ddata = (void *)arg;
>
> -	mpc8xxx_wdt_keepalive();
> +	mpc8xxx_wdt_keepalive(ddata);
>   	/* We're pinging it twice faster than needed, just to be sure. */
> -	mod_timer(&wdt_timer, jiffies + HZ * w->timeout / 2);
> +	mod_timer(&ddata->timer, jiffies + HZ * ddata->wdd.timeout / 2);
>   }
>
>   static int mpc8xxx_wdt_start(struct watchdog_device *w)
>   {
> +	struct mpc8xxx_wdt_ddata *ddata = container_of(w, struct mpc8xxx_wdt_ddata, wdd);
> +
>   	u32 tmp = SWCRR_SWEN | SWCRR_SWPR;
>
>   	/* Good, fire up the show */
> @@ -102,22 +102,24 @@ static int mpc8xxx_wdt_start(struct watchdog_device *w)
>
>   	tmp |= timeout << 16;
>
> -	out_be32(&wd_base->swcrr, tmp);
> +	out_be32(&ddata->base->swcrr, tmp);
>
> -	del_timer_sync(&wdt_timer);
> +	del_timer_sync(&ddata->timer);
>
>   	return 0;
>   }
>
>   static int mpc8xxx_wdt_ping(struct watchdog_device *w)
>   {
> -	mpc8xxx_wdt_keepalive();
> +	struct mpc8xxx_wdt_ddata *ddata = container_of(w, struct mpc8xxx_wdt_ddata, wdd);

This causes a bunch of checkpatch warnings, line too long and missing blank line.

It would be great if you can fix those up.

Thanks,
Guenter

> +	mpc8xxx_wdt_keepalive(ddata);
>   	return 0;
>   }
>
>   static int mpc8xxx_wdt_stop(struct watchdog_device *w)
>   {
> -	mod_timer(&wdt_timer, jiffies);
> +	struct mpc8xxx_wdt_ddata *ddata = container_of(w, struct mpc8xxx_wdt_ddata, wdd);
> +	mod_timer(&ddata->timer, jiffies);
>   	return 0;
>   }
>
> @@ -134,16 +136,12 @@ static struct watchdog_ops mpc8xxx_wdt_ops = {
>   	.stop = mpc8xxx_wdt_stop,
>   };
>
> -static struct watchdog_device mpc8xxx_wdt_dev = {
> -	.info = &mpc8xxx_wdt_info,
> -	.ops = &mpc8xxx_wdt_ops,
> -};
> -
>   static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   {
>   	int ret;
>   	struct resource *res;
>   	const struct mpc8xxx_wdt_type *wdt_type;
> +	struct mpc8xxx_wdt_ddata *ddata;
>   	u32 freq = fsl_get_sys_freq();
>   	bool enabled;
>   	unsigned int timeout_sec;
> @@ -155,25 +153,35 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   	if (!freq || freq == -1)
>   		return -EINVAL;
>
> +	ddata = devm_kzalloc(&ofdev->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
>   	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> -	wd_base = devm_ioremap_resource(&ofdev->dev, res);
> -	if (IS_ERR(wd_base))
> -		return PTR_ERR(wd_base);
> +	ddata->base = devm_ioremap_resource(&ofdev->dev, res);
> +	if (IS_ERR(ddata->base))
> +		return PTR_ERR(ddata->base);
>
> -	enabled = in_be32(&wd_base->swcrr) & SWCRR_SWEN;
> +	enabled = in_be32(&ddata->base->swcrr) & SWCRR_SWEN;
>   	if (!enabled && wdt_type->hw_enabled) {
>   		pr_info("could not be enabled in software\n");
>   		return -ENOSYS;
>   	}
>
> +	spin_lock_init(&ddata->lock);
> +	setup_timer(&ddata->timer, mpc8xxx_wdt_timer_ping, (unsigned long)ddata);
> +
> +	ddata->wdd.info = &mpc8xxx_wdt_info,
> +	ddata->wdd.ops = &mpc8xxx_wdt_ops,
> +
>   	/* Calculate the timeout in seconds */
>   	timeout_sec = (timeout * wdt_type->prescaler) / freq;
>
> -	mpc8xxx_wdt_dev.timeout = timeout_sec;
> +	ddata->wdd.timeout = timeout_sec;
>
> -	watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
> +	watchdog_set_nowayout(&ddata->wdd, nowayout);
>
> -	ret = watchdog_register_device(&mpc8xxx_wdt_dev);
> +	ret = watchdog_register_device(&ddata->wdd);
>   	if (ret) {
>   		pr_err("cannot register watchdog device (err=%d)\n", ret);
>   		return ret;
> @@ -188,16 +196,20 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>   	 * userspace handles it.
>   	 */
>   	if (enabled)
> -		mod_timer(&wdt_timer, jiffies);
> +		mod_timer(&ddata->timer, jiffies);
> +
> +	platform_set_drvdata(ofdev, ddata);
>   	return 0;
>   }
>
>   static int mpc8xxx_wdt_remove(struct platform_device *ofdev)
>   {
> +	struct mpc8xxx_wdt_ddata *ddata = platform_get_drvdata(ofdev);
> +
>   	pr_crit("Watchdog removed, expect the %s soon!\n",
>   		reset ? "reset" : "machine check exception");
> -	del_timer_sync(&wdt_timer);
> -	watchdog_unregister_device(&mpc8xxx_wdt_dev);
> +	del_timer_sync(&ddata->timer);
> +	watchdog_unregister_device(&ddata->wdd);
>
>   	return 0;
>   }
>

[PATCH v1 5/5] watchdog: mpc8xxx: allow to compile for MPC512x

[Uwe Kleine-König]

The MPC5125 processor features a watchdog compiler that is identical to
the MPC8610 one. So allow to enable the driver for MPC512x kernel
configurations.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..8c43ae9bde2b 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1333,7 +1333,7 @@ config MPC5200_WDT
 
 config 8xxx_WDT
 	tristate "MPC8xxx Platform Watchdog Timer"
-	depends on PPC_8xx || PPC_83xx || PPC_86xx
+	depends on PPC_8xx || PPC_83xx || PPC_86xx || PPC_MPC512x
 	select WATCHDOG_CORE
 	help
 	  This driver is for a SoC level watchdog that exists on some
-- 
2.4.6

Re: [PATCH v1 5/5] watchdog: mpc8xxx: allow to compile for MPC512x

[Guenter Roeck]

Hi Uwe,

On 08/07/2015 03:07 AM, Uwe Kleine-König wrote:
> The MPC5125 processor features a watchdog compiler that is identical to

compiler ?

Guenter

> the MPC8610 one. So allow to enable the driver for MPC512x kernel
> configurations.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/watchdog/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..8c43ae9bde2b 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1333,7 +1333,7 @@ config MPC5200_WDT
>
>   config 8xxx_WDT
>   	tristate "MPC8xxx Platform Watchdog Timer"
> -	depends on PPC_8xx || PPC_83xx || PPC_86xx
> +	depends on PPC_8xx || PPC_83xx || PPC_86xx || PPC_MPC512x
>   	select WATCHDOG_CORE
>   	help
>   	  This driver is for a SoC level watchdog that exists on some
>