On Fri, Jan 7, 2011 at 5:33 PM, Jamie Iles <jamie@xxxxxxxxxxxxx> wrote:
(...)
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> new file mode 100644
> index 0000000..e0d0e377
> --- /dev/null
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -0,0 +1,300 @@
(...)
> +static inline int dw_wdt_is_enabled(void)
> +{
> +#define WDOG_CONTROL_REG_WDT_EN_MASK Â Â Â Â Â 0x01
can be moved to top, with other macros.
> + Â Â Â return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
> + Â Â Â Â Â Â Â WDOG_CONTROL_REG_WDT_EN_MASK;
> +}
> +
(...)
> +static void dw_wdt_keepalive(void)
> +{
> +#define WDOG_COUNTER_RESTART_KICK_VALUE Â Â Â Â Â Â0x76
ditto...
> + Â Â Â writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
> + Â Â Â Â Â Â ÂWDOG_COUNTER_RESTART_REG_OFFSET);
> +}
> +
> +static int dw_wdt_open(struct inode *inode, struct file *filp)
> +{
> + Â Â Â /* Make sure we don't get unloaded. */
> + Â Â Â __module_get(THIS_MODULE);
> +
> + Â Â Â spin_lock(&dw_wdt.lock);
> + Â Â Â if (!dw_wdt_is_enabled()) {
> + Â Â Â Â Â Â Â /*
> + Â Â Â Â Â Â Â Â* The watchdog is not currently enabled. Set the timeout to
> + Â Â Â Â Â Â Â Â* the maximum and then start it.
> + Â Â Â Â Â Â Â Â*/
> + Â Â Â Â Â Â Â dw_wdt_set_top(DW_WDT_MAX_TOP);
shouldn't we check return value here??
> + Â Â Â Â Â Â Â writel(WDOG_CONTROL_REG_WDT_EN_MASK,
> + Â Â Â Â Â Â Â Â Â Â Âdw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
> + Â Â Â }
> + Â Â Â spin_unlock(&dw_wdt.lock);
> +
> + Â Â Â return nonseekable_open(inode, filp);
> +}
> +
(...)
> +static const struct dev_pm_ops dw_wdt_pm_ops = {
> +    .suspend    Â= dw_wdt_suspend,
> +    .resume     = dw_wdt_resume,
> +};
> +
> +#define DW_WDT_PM_OPS Â(&dw_wdt_pm_ops)
> +#else /* CONFIG_PM */
> +#define DW_WDT_PM_OPS ÂNULL
> +#endif /* CONFIG_PM */
This can be rewritten as:
static struct platform_driver dw_wdt_driver = {
...
+#ifdef CONFIG_PM
       .pm   = dw_wdt_pm_ops,
#endif
}
> +static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
> +{
> + Â Â Â int ret;
> + Â Â Â struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + Â Â Â if (!mem)
> + Â Â Â Â Â Â Â return -EINVAL;
> +
> + Â Â Â if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"iomem"))
> + Â Â Â Â Â Â Â return -ENOMEM;
> +
> + Â Â Â dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âresource_size(mem));
> + Â Â Â if (!dw_wdt.regs)
> + Â Â Â Â Â Â Â return -ENOMEM;
should release mem_region in case of error.
> +
> + Â Â Â dw_wdt.clk = clk_get(&pdev->dev, NULL);
> + Â Â Â if (IS_ERR_OR_NULL(dw_wdt.clk))
> + Â Â Â Â Â Â Â return -ENODEV;
should release mem_region and free ioremaped space.
Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's
if dw_wdt.clk is !NULL
> + Â Â Â clk_enable(dw_wdt.clk);
> +
> + Â Â Â ret = misc_register(&dw_wdt_miscdev);
> + Â Â Â if (ret)
> + Â Â Â Â Â Â Â goto register_failed;
> +
> + Â Â Â return 0;
> +
> +register_failed:
> + Â Â Â clk_disable(dw_wdt.clk);
> + Â Â Â clk_put(dw_wdt.clk);
also iounmap and release_mem_region...
> +
> + Â Â Â return ret;
> +}
> +
> +static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
> +{
> + Â Â Â clk_disable(dw_wdt.clk);
> + Â Â Â clk_put(dw_wdt.clk);
> +
> + Â Â Â misc_deregister(&dw_wdt_miscdev);
also iounmap and release_mem_region...
> +
> + Â Â Â return 0;
> +}
> +
> +static struct platform_driver dw_wdt_driver = {
> +    .probe     Â= dw_wdt_drv_probe,
> +    .remove     = __devexit_p(dw_wdt_drv_remove),
> +    .driver     = {
> +        .name  = "dw_wdt",
> + Â Â Â Â Â Â Â .owner Â= THIS_MODULE,
> +        .pm   = DW_WDT_PM_OPS,
> + Â Â Â },
> +};
> +
> +static int __init dw_wdt_watchdog_init(void)
> +{
> + Â Â Â spin_lock_init(&dw_wdt.lock);
This can be moved to probe. spin_lock_init should be only called when we have
some device for this driver.
> +
> + Â Â Â return platform_driver_register(&dw_wdt_driver);
> +}
> +
> +static void __exit dw_wdt_watchdog_exit(void)
> +{
> + Â Â Â platform_driver_unregister(&dw_wdt_driver);
> +}
> +
> +module_init(dw_wdt_watchdog_init);
> +module_exit(dw_wdt_watchdog_exit);
should be moved after init & exit routines without any blank lines.
--
viresh
ST Microelectronics
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
