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

[Guenter Roeck <linux@roeck-us.net>]

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;
>   }
>