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
[email protected] +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
