Re: [PATCH] Add watchdog driver for w90p910

[Wim Van Sebroeck]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/