lkml.org 
[lkml]   [2006]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] MIPS RM9K watchdog driver
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;

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

\
 
 \ /
  Last update: 2006-08-12 18:37    [from the cache]
©2003-2011 Jasper Spaans