Messages in this thread | | | Date | Mon, 16 Jul 2018 09:25:23 +0200 | From | Johannes Thumshirn <> | Subject | 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 <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/mcb.h> > > +#include <linux/watchdog.h> > > +#include <linux/io.h> > > + > 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 jthumshirn@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
| |