On Sat, Aug 12, 2006 at 06:05:52PM +0200, thomas@xxxxxxxxxxxxxxxxxx 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;
