Re: [PATCH 1/4] watchdog: renesas-wdt: add driver
Hi Wolfram,
On Wed, Mar 30, 2016 at 05:28:42PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang
>
> Add support for watchdogs (RWDT and SWDT) found on RCar Gen3 based SoCs
> from Renesas.
>
> Signed-off-by: Wolfram Sang
> ---
>
[ ... ]
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
[ ... ]
> +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 |= 0x5a5a;
> + 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
Re: [PATCH 1/4] watchdog: renesas-wdt: add driver
Hi Guenter, > > +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. No reason, will stick to the usual case. > > +static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned reg) > > Please use 'unsigned int' throughout. Can do. May I ask why? > > + 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. I assume you mean GETTIMELEFT. Tried that, seems to work. > function. In other words, you can just drop rwdt_set_timeout() entirely. Cool news! Works fine. Thanks for the prompt review! Will send V2 in a minute. Wolfram
Re: [PATCH 1/4] watchdog: renesas-wdt: add driver
Hi Wolfram, On 04/01/2016 04:36 AM, Wolfram Sang wrote: Hi Guenter, +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. No reason, will stick to the usual case. +static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned reg) Please use 'unsigned int' throughout. Can do. May I ask why? There are people changing unsigned -> unsigned int with coccinelle scripts. Besides that, checkpatch warns about it. Sure, I know, checkpatch is just a script, I know that story, but I don't want to have to deal with the inevitable follow-up patches. + 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. I assume you mean GETTIMELEFT. Tried that, seems to work. Yes, sorry. Thanks, Guenter function. In other words, you can just drop rwdt_set_timeout() entirely. Cool news! Works fine. Thanks for the prompt review! Will send V2 in a minute. Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
