Re: [PATCH] MIPS RM9K watchdog driver

[Alexey Dobriyan]

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;

-
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/