Re: [PATCH] watchdog: add driver for the MEN 16z069 IP-Core
On Fri, Jul 13, 2018 at 06:50:25AM -0700, Guenter Roeck wrote:
[...]
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> Alphabetic order, please
Done.
>
> > +struct men_z069_drv {
> > + struct watchdog_device wdt;
> > + void __iomem *base;
> > + struct resource *mem;
> > +};
> > +
> > +#define MEN_Z069_WTR 0x10
> > +#define MEN_Z069_WTR_WDEN BIT(15)
> > +#define MEN_Z069_WTR_WDET_MASK 0x7fff
> > +#define MEN_Z069_WVR 0x14
> > +
> > +#define MEN_Z069_TIMER_FREQ 500 /* 500 Hz */
> > +#define MEN_Z069_WDT_COUNTER_MIN 1
> > +#define MEN_Z069_WDT_COUNTER_MAX 0x7fff
> > +
> Looks like you are sometimes using tabs and sometimes not.
> Please align all values with tabs, or if you really dislike that don't use
> tabs at all.
Made it all tabs.
[...]
> > + if (val < MEN_Z069_WDT_COUNTER_MIN || val > MEN_Z069_WDT_COUNTER_MAX)
> > + return -EINVAL;
> > +
>
> This is unnecessary. As long as the limits are provided, they are validated
> by the watchdog core.
Good point removed the check.
>
> Note that it is wrng to set ->timeout and then return an error.
> The watchdog core will then report a bad current timeout to user space.
Yeah the return isn't needed anymore.
>
> > + reg = readw(drv->base + MEN_Z069_WVR);
> > + ena = reg & MEN_Z069_WTR_WDEN;
> > + reg = ena | (val & MEN_Z069_WTR_WDET_MASK);
>
> Masking val is unnecessary here. val is already guaranteed to be
> smaller than the mask.
>
Yep, removed the mask.
> I would suggest to use
> .max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ;
> or define MAX_TIMEOUT as (MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ) and
> use it.
Went down the 1st route.
[...]
> > + drv->base = devm_ioremap(&dev->dev, mem->start, resource_size(mem));
> > + if (drv->base == NULL)
> > + goto release_mem;
>
> The proper errr to return here is -ENOMEM, or use devm_ioremap_resource()
> and return whatever error it reports.
Done.
>
> > +
> > + drv->mem = mem;
> > +
> > + watchdog_init_timeout(&men_z069_wdt, 30, &dev->dev);
>
> Please make '30' a define. Unless you know for sure that this will never be
> used
> in a devicetree system, I would suggest to set the default timeout in struct
> watchdog_device and pass 0 as argument here; this way the core picks the
> default
> timeout if set in devicetree.
Well it sits on a self describing bus so no device-tree needed here. Anyways
made it 0.
>
> > + watchdog_set_nowayout(&men_z069_wdt, nowayout);
> > + watchdog_set_drvdata(&men_z069_wdt, drv);
> > + men_z069_wdt.parent = &dev->dev;
> > + drv->wdt = men_z069_wdt;
>
> This is unusual. I would suggest to drop men_z069_wdt and set the necessary
> fields
> in drv->wdt directly.
Well as I'm not really working on watchdogs often I just copy&pasted
it from mena21_wdt.c which was my first driver some years ago. So this
might be the reason for this oddity. Anyways fixed it.
>
> > + mcb_set_drvdata(dev, drv);
> > +
> > + /* Set initial timeout to 65.5s and disable the watchdog */
> > + writew(MEN_Z069_WDT_COUNTER_MAX, drv->base + MEN_Z069_WTR);
> > +
>
> Hmm - above default is set to 30 seconds.
>
> Another possibility would be to detect the current watchdog state
> (and possibly timeout) and inform the watchdog core that the watchdog
> is running. This way there is no gap in watchdog coverage if the
> watchdog was already enabled in BIOS/ROMMON.
This is a leftover from debugging the Qemu emulation of the device I
guess, removed it now.
[...]
> > +};
> > +module_mcb_driver(men_z069_driver);
> > +
> > +
> Double empty line.
Gone.
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] watchdog: add driver for the MEN 16z069 IP-Core
On 07/13/2018 05:58 AM, Johannes Thumshirn wrote:
Add a driver for the MEN 16z069 Watchdog and Reset Controller IP-Core.
Signed-off-by: Johannes Thumshirn
---
MAINTAINERS | 6 ++
drivers/watchdog/Kconfig | 10 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/menz69_wdt.c | 175 ++
4 files changed, 192 insertions(+)
create mode 100644 drivers/watchdog/menz69_wdt.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 07d1576fc766..67a76f740294 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9298,6 +9298,12 @@ F: drivers/leds/leds-menf21bmc.c
F:drivers/hwmon/menf21bmc_hwmon.c
F:Documentation/hwmon/menf21bmc
+MEN Z069 WATCHDOG DRIVER
+M: Johannes Thumshirn
+L: linux-watch...@vger.kernel.org
+S: Maintained
+F: drivers/watchdog/menz069_wdt.c
+
MESON AO CEC DRIVER FOR AMLOGIC SOCS
M:Neil Armstrong
L:linux-me...@lists.freedesktop.org
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd92763..df55d65bbb1c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -161,6 +161,16 @@ config MENF21BMC_WATCHDOG
This driver can also be built as a module. If so the module
will be called menf21bmc_wdt.
+config MENZ069_WATCHDOG
+ tristate "MEN 16Z069 Watchdog"
+ depends on MCB || COMPILE_TEST
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support for the MEN 16Z069 Watchdog.
+
+ This driver can also be built as a module. If so the module
+ will be called menz069_wdt.
+
config TANGOX_WATCHDOG
tristate "Sigma Designs SMP86xx/SMP87xx watchdog"
select WATCHDOG_CORE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 1d3c6b094fe5..bf92e7bf9ce0 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -215,4 +215,5 @@ obj-$(CONFIG_MAX77620_WATCHDOG) += max77620_wdt.o
obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o
obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
+obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
new file mode 100644
index ..9dccd8aabdb9
--- /dev/null
+++ b/drivers/watchdog/menz69_wdt.c
@@ -0,0 +1,175 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Watchdog driver for the MEN z069 IP-Core
+ *
+ * Copyright (C) 2018 Johannes Thumshirn
+ */
+#include
+#include
+#include
+#include
+#include
+
Alphabetic order, please
+struct men_z069_drv {
+ struct watchdog_device wdt;
+ void __iomem *base;
+ struct resource *mem;
+};
+
+#define MEN_Z069_WTR 0x10
+#define MEN_Z069_WTR_WDEN BIT(15)
+#define MEN_Z069_WTR_WDET_MASK 0x7fff
+#define MEN_Z069_WVR 0x14
+
+#define MEN_Z069_TIMER_FREQ 500 /* 500 Hz */
+#define MEN_Z069_WDT_COUNTER_MIN 1
+#define MEN_Z069_WDT_COUNTER_MAX 0x7fff
+
Looks like you are sometimes using tabs and sometimes not.
Please align all values with tabs, or if you really dislike that don't use
tabs at all.
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static int men_z069_wdt_start(struct watchdog_device *wdt)
+{
+ struct men_z069_drv *drv = watchdog_get_drvdata(wdt);
+ u16 val;
+
+ val = readw(drv->base + MEN_Z069_WTR);
+ val |= MEN_Z069_WTR_WDEN;
+ writew(val, drv->base + MEN_Z069_WTR);
+
+ return 0;
+}
+
+static int men_z069_wdt_stop(struct watchdog_device *wdt)
+{
+ struct men_z069_drv *drv = watchdog_get_drvdata(wdt);
+ u16 val;
+
+ val = readw(drv->base + MEN_Z069_WTR);
+ val &= ~MEN_Z069_WTR_WDEN;
+ writew(val, drv->base + MEN_Z069_WTR);
+
+ return 0;
+}
+
+static int men_z069_wdt_ping(struct watchdog_device *wdt)
+{
+ struct men_z069_drv *drv = watchdog_get_drvdata(wdt);
+ u16 val;
+
+ /* The watchdog trigger value toggles between 0x and 0x */
+ val = readw(drv->base + MEN_Z069_WVR);
+ val ^= 0x;
+ writew(val, drv->base + MEN_Z069_WVR);
+
+ return 0;
+}
+
+static int men_z069_wdt_set_timeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ struct men_z069_drv *drv = watchdog_get_drvdata(wdt);
+ u16 reg, val, ena;
+
+ wdt->timeout = timeout;
+ val = timeout * MEN_Z069_TIMER_FREQ;
+
+ if (val < MEN_Z069_WDT_COUNTER_MIN || val > MEN_Z069_WDT_COUNTER_MAX)
+ return -EINVAL;
+
This is unnecessary. As long as the limits are provided, they are validated
by the watchdog core.
Note that it is wrng to set ->timeout and then return an error.
The watchdog core will then report a bad current timeout to user space
