linux-mips
[Top] [All Lists]

Re: [PATCH] MIPS RM9K watchdog driver

To: thomas@koeller.dyndns.org
Subject: Re: [PATCH] MIPS RM9K watchdog driver
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Sat, 12 Aug 2006 20:33:59 +0400
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>, wim@iguana.be, linux-kernel@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org
Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:date:from:to:cc:subject:message-id:references:mime-version:content-type:content-disposition:in-reply-to:user-agent; b=IUxcGctV+6etoBIkiVUF+0iDK/2aFSHgwKa7CnUjfnUCp3mY0LQ4J/HEDT7yuTA0dwO0WE/38TGHbOemzNTkexjRfjscPMeMbE9G6aGK99LHUF/F76HAA8eBNyrdQJ5AOi7pC2qyv8bzBSJX+il+PVeUs3orWBjICcmY44Lo0oY=
In-reply-to: <200608121805.52358.thomas@koeller.dyndns.org>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <200608121805.52358.thomas@koeller.dyndns.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.11
On Sat, Aug 12, 2006 at 06:05:52PM +0200, thomas@koeller.dyndns.org wrote:
> This is a driver for the on-chip watchdog device found on some
> MIPS RM9000 processors.

> +/* Module arguments */
> +static int timeout = MAX_TIMEOUT_SECONDS;
> +module_param(timeout, int, 444);
                              ^^^
*cough* ITYM, 0444

> +static unsigned long resetaddr = 0xbffdc200;
> +module_param(resetaddr, ulong, 444);

0444

> +static unsigned long flagaddr = 0xbffdc104;
> +module_param(flagaddr, ulong, 444);

0444

> +static int powercycle = 0;
> +module_param(powercycle, bool, 444);

0444

> +module_param(nowayout, bool, 444);

0444

> +static struct file_operations fops =
> +{

On one line, please.

> +static struct miscdevice miscdev =
> +{

Ditto.

> +static struct device_driver wdt_gpi_driver =
> +{

> +     .shutdown       = NULL,
> +     .suspend        = NULL,
> +     .resume         = NULL

If you don't provide, don't write it at all and C99 compiler will DTRT.

> +static struct notifier_block wdt_gpi_shutdown =
> +{
> +     wdt_gpi_notify
> +};

C99 initializer needed.

> +static int __init wdt_gpi_probe(struct device *dev)
> +{
> +     int res;
> +     struct platform_device * const pdv = to_platform_device(dev);
> +     const struct resource
> +             * const rr = wdt_gpi_get_resource(pdv, WDT_RESOURCE_REGS,
> +                                               IORESOURCE_MEM),
> +             * const ri = wdt_gpi_get_resource(pdv, WDT_RESOURCE_IRQ,
> +                                               IORESOURCE_IRQ),
> +             * const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,
> +                                               0);

Looks horrible. First, gcc almost certainly won't do anything useful
with const qualifiers. Second, only short initializers are tolerated.
That way you won't even hit 80-chars border.

        struct resource *rr, *ri, *rc;

        rr = wdt_gpi_get_resource(...);
        ri = wdt_gpi_get_resource(...);
        rc = wdt_gpi_get_resource(...);

> +     if (unlikely(!rr || !ri || !rc))
> +             return -ENXIO;

Probing is hardly critical path, so code clarity wins and "unlikely"
should be removed here and below. And give better names to these
variables. ;-)

> +static int wdt_gpi_release(struct inode *i, struct file *f)

It's usually "struct inode *inode" and "struct file *file".

> +     switch (cmd) {
> +             case WDIOC_GETSUPPORT:

One tab please

        switch (cmd) {
        case WDIOC_GETSUPPORT:
                ...
        }

> +                     wdinfo.options = nowayout ?
> +                             WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING :
> +                             WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | 
> WDIOF_MAGICCLOSE;
> +                     res = __copy_to_user((void __user *)arg, &wdinfo, size) 
> ?
> +                             -EFAULT : size;
> +                     break;
> +
> +             case WDIOC_GETSTATUS:
> +                     break;
> +             
> +             case WDIOC_GETBOOTSTATUS:
> +                     stat = (*(volatile char *) flagaddr & 0x01)
> +                             ? WDIOF_CARDRESET : 0;
> +                     res = __copy_to_user((void __user *)arg, &stat, size) ?
> +                             -EFAULT : size;
> +                     break;
> +
> +             case WDIOC_SETOPTIONS:
> +                     break;
> +
> +             case WDIOC_KEEPALIVE:
> +                     wdt_gpi_set_timeout(timeout);
> +                     res = size;
> +                     break;
> +
> +             case WDIOC_SETTIMEOUT:
> +                     {
> +                             int val;
> +                             if (unlikely(__copy_from_user(&val, (const void 
> __user *) arg,
> +                                                  size))) {
> +                                     res = -EFAULT;
> +                                     break;
> +                             }
> +
> +                             if (val > 32)
> +                                     val = 32;
> +                             timeout = val;
> +                             wdt_gpi_set_timeout(val);
> +                             res = size;
> +                             printk("%s: timeout set to %u seconds\n",
> +                                    wdt_gpi_name, timeout);
> +                     }
> +                     break;
> +
> +             case WDIOC_GETTIMEOUT:
> +                     res = __copy_to_user((void __user *) arg, &timeout, 
> size) ?
> +                             -EFAULT : size;
> +                     break;
> +     }
> +     
> +     return res;


<Prev in Thread] Current Thread [Next in Thread>