lkml.org 
[lkml]   [2009]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Add watchdog driver for w90p910
Hi Wan,

I reviewed your code and have some comments and questions.

...

> +struct w90p910_wdt {
> + struct resource *res;
> + struct clk *wdt_clock;
> + struct platform_device *pdev;
> + unsigned int open_lock;
> + unsigned int wdt_irq;
> + void __iomem *wdt_base;
> + char expect_close;
> + spinlock_t wdt_lock;
> +};

Where is the spinlock initialized?

...

> +static irqreturn_t w90p910_wdt_irq(int irq, void *dev_id)
> +{
> + w90p910_wdt_keepalive();
> + return IRQ_HANDLED;
> +}

How does the interrupt work? what does the watchdog do when it times out?
(Is there a datasheet available somewhere?)

...

> +static int w90p910_wdt_settimeout(int new_time_level)
> +{
> + unsigned int val;
> +
> + if ((new_time_level < 0) || (new_time_level > WDT_MAX_TIME_LEVEL))
> + return -EINVAL;
> +
> + val = __raw_readl(w90p910_wdt->wdt_base + REG_WTCR);
> + val &= ~WTIS;
> + val |= new_time_level;

Shouldn't this be val |= (new_time_level << 0x04); ?
The read-write cyclus to REG_WTCR should also be guarded with a spinlock.
Can you incorporate the call to w90p910_wdt_start() in the ioctl code into this function?

...

> +static ssize_t w90p910_wdt_write(struct file *file, const char *data,
> + size_t len, loff_t *ppos)

should be: ..., const char __user *data, ...

...

> +static int __devinit w90p910wdt_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + w90p910_wdt = kzalloc(sizeof(struct w90p910_wdt), GFP_KERNEL);
> + if (!w90p910_wdt)
> + return -ENOMEM;
> +
> + w90p910_wdt->pdev = pdev;
> +
> + w90p910_wdt->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (w90p910_wdt->res == NULL) {
> + dev_err(&pdev->dev, "no memory resource specified\n");
> + ret = -ENOENT;
> + goto err_get;
> + }
> +
> + if (!request_mem_region(w90p910_wdt->res->start,
> + resource_size(w90p910_wdt->res), pdev->name)) {
> + dev_err(&pdev->dev, "failed to get memory region\n");
> + ret = -ENOENT;
> + goto err_req;

This should be: goto err_get;

...

> +
> +static int __devexit w90p910wdt_remove(struct platform_device *pdev)
> +{

The misc_deregister(&w90p910wdt_miscdev); should go first. We don't want userspace interactivity when we clean up all reservations.

> + free_irq(w90p910_wdt->wdt_irq, NULL);
> +
> + clk_disable(w90p910_wdt->wdt_clock);
> + clk_put(w90p910_wdt->wdt_clock);
> +
> + iounmap(w90p910_wdt->wdt_base);
> +
> + release_mem_region(w90p910_wdt->res->start,
> + resource_size(w90p910_wdt->res));
> +
> + kfree(w90p910_wdt);
> +
> + misc_deregister(&w90p910wdt_miscdev);

see comment about misc_deregister above.

For the rest the code looks good to me.

Kind regards,
Wim.



\
 
 \ /
  Last update: 2009-08-04 22:13    [W:0.074 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site