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    [W:0.040 / U:0.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site