Hi Wolfram,
On Wed, Mar 30, 2016 at 05:28:42PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>
> Add support for watchdogs (RWDT and SWDT) found on RCar Gen3 based SoCs
> from Renesas.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
>
[ ... ]
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
Please also include linux/bitops.h.
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/watchdog.h>
> +
[ ... ]
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, S_IRUGO);
Sure you want this parameter readable ? No problem with me, but it is unusual,
so I figure it is worth asking.
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct rwdt_priv {
> + void __iomem *base;
> + struct watchdog_device wdev;
> + struct clk *clk;
> + unsigned clks_per_sec;
> + u8 cks;
> +};
> +
> +static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned reg)
Please use 'unsigned int' throughout.
> +{
> + if (reg == RWTCNT)
> + val |= 0x5a5a0000;
> + else
> + val |= 0xa5a5a500;
> +
> + writel_relaxed(val, priv->base + reg);
> +}
> +
> +static int rwdt_init_timeout(struct watchdog_device *wdev)
> +{
> + struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + rwdt_write(priv, 65536 - wdev->timeout * priv->clks_per_sec, RWTCNT);
> +
Just wondering, does reading RWTCNT return the remaining timeout ?
If so, you could easily implement WDIOC_GETTIMEOUT.
> + return 0;
> +}
> +
> +static int rwdt_set_timeout(struct watchdog_device *wdev, unsigned new_timeout)
> +{
> + wdev->timeout = new_timeout;
> + rwdt_init_timeout(wdev);
> +
The watchdog core calls the ping function after updating the timeout,
so the call here is unnecessary. On top of that, the watchdog core now also
updates wdev->timeout if WDIOF_SETTIMEOUT is set and there is no set_timeout
function. In other words, you can just drop rwdt_set_timeout() entirely.
Thanks,
Guenter
