Hello Guenter,
Thank you for your review!
On Thursday, March 02, 2017, Guenter Roeck wrote:
> > +/*
> > + * Renesas RZ/A Series WDT Driver
> > + *
> > + * Copyright (C) 2017 Renesas Electronics America, Inc.
> > + * Copyright (C) 2017 Chris Brandt
> > + *
> > + * This file is subject to the terms and conditions of the GNU
> > +General Public
> > + * License. See the file "COPYING" in the main directory of this
> > +archive
> > + * for more details.
> > + *
> > + *
>
> The above two lines are unnecessary.
OK.
#I'll assume you mean take out just the last sentence (2 lines), not both
sentences (all 3 lines).
> > +/* Watchdog Timer Registers */
> > +#define WTCSR 0
> > +#define WTCSR_MAGIC 0xA500
> > +#define WTSCR_WT (1<<6)
> > +#define WTSCR_TME (1<<5)
>
> BIT()
OK.
> > +#define WTSCR_CKS(i) i
>
> (i)
OK.
> > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
>
> Please use
> #define SOMETHING<tab>value
> throughout and make sure value is aligned.
OK.
> > + rate = clk_get_rate(priv->clk);
> > + if (!rate)
> > + return -ENOENT;
> > +
> > + /* Assume slowest clock rate possible (CKS=7) */
> > + rate /= 16384;
> > +
>
> The rate check should probably be here to avoid situations where rate <
> 16384.
Do I need that if it's technically not possible to have a 'rate' less than 25MHz?
These watchdogs HW are always feed directly from the peripheral clock and there
is no such thing as a 16kHz peripheral block an any Renesas SoC.
> > + priv->wdev.info = &rza_wdt_ident,
> > + priv->wdev.ops = &rza_wdt_ops,
> > + priv->wdev.parent = &pdev->dev;
> > +
> > + /*
> > + * Since the max possible timeout of our 8-bit count register is
> less
> > + * than a second, we must use max_hw_heartbeat_ms.
> > + */
> > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
>
> space before and after /
OK.
#Funny because checkpatch.pl said it didn't like a space on one side but
not the other, so I choose no spaces and it was happy. I'm way below 80
characters for that line so it doesn't matter to me.
> > + dev_info(&pdev->dev, "max hw timeout of %dms\n",
> > + priv->wdev.max_hw_heartbeat_ms);
>
> dev_dbg ?
OK.
#I guess we don't need to see that info on every boot.
> > +
> > + priv->wdev.min_timeout = 1;
> > + priv->wdev.timeout = DEFAULT_TIMEOUT;
> > +
>
> Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get
> the timeout from dt ?
OK. Good idea.
> > + platform_set_drvdata(pdev, priv);
> > + watchdog_set_drvdata(&priv->wdev, priv);
> > +
> > + ret = watchdog_register_device(&priv->wdev);
>
> devm_watchdog_register_device()
OK.
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int rza_wdt_remove(struct platform_device *pdev) {
> > + struct rza_wdt *priv = platform_get_drvdata(pdev);
> > +
> > + watchdog_unregister_device(&priv->wdev);
> > + iounmap(priv->base);
>
> iounmap is unnecessary (and wrong).
Anything mapped with devm_ioremap_resource() automatically gets unmapped
when the drive gets unloaded?
I didn't know that.
I'll wait till the end of the day to see if anyone finds anything else, and
then I'll send out a v5.
Chris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
