Messages in this thread Patch in this message | | | Date | Wed, 21 Mar 2007 09:22:39 +0100 | From | Johannes Weiner <> | Subject | Re: [PATCH 2/7] Initial implementation of the trec driver and include files |
| |
Hi,
On Tue, Mar 20, 2007 at 11:47:35PM -0700, Wink Saville wrote: > --- /dev/null > +++ b/drivers/trec/trec.c > [...] > +struct trec_dev_struct > +{ > + struct cdev cdev; /* Character device > structure */
Your patch has broken lines where there shouldn't be any.
> +#define TREC_DATA_SIZE 0x200 > +struct trec_buffer_struct { > + struct trec_buffer_struct * pNext; > + struct trec_struct * pCur; > + struct trec_struct * pEnd; > + struct trec_struct data[TREC_DATA_SIZE]; > +};
Please don't use camel-case - in general.
> +static int snprint_address(char *b, int bsize, unsigned long address) > +{ > +#ifdef CONFIG_KALLSYMS > + unsigned long offset = 0, symsize; > + const char *symname; > + char *modname; > + char *delim = ":"; > + int n; > + char namebuf[128]; > + > + symname = kallsyms_lookup(address, &symsize, &offset, &modname, > namebuf); > + if (!symname) { > + n = 0; > + } else { > + if (!modname) > + modname = delim = ""; > + n = snprintf(b, bsize, "0x%016lx %s%s%s%s+0x%lx/0x%lx", > + address, delim, modname, delim, symname, offset, > symsize); > + } > + return n; > +#else > + return snprintf(b, bsize, "0x%016lx", address); > + return 0; > +#endif > +}
Would it make sense to #ifdef the whole function? Make it static int (*)(...) if CONFIG_KALLSYMS and otherwise just a static inline int (*)(...) { return 0; }
> [...] > +static int trec_device_init(void) > +{ > + int result; > + dev_t dev_number = 0; > + static struct class *trec_class; > + > + DPK("trec_device_init: E\n"); > + > + if (major) { > + dev_number = MKDEV(major, minor); > + result = register_chrdev_region(dev_number, 1, "trec"); > + DPK("trec_device_init: static major result=%d\n", result); > + } else { > + result = alloc_chrdev_region(&dev_number, minor, 1, "trec"); > + major = MAJOR(dev_number); > + DPK("trec_device_init: dynamic major result=%d\n", result); > + } > + > + if (result < 0) { > + printk(KERN_WARNING "trec: can't get major %d\n", major); > + goto done; > + } > + > + cdev_init(&trec_dev.cdev, &trec_f_ops); > + trec_dev.cdev.owner = THIS_MODULE; > + trec_dev.cdev.ops = &trec_f_ops; > + > + result = cdev_add(&trec_dev.cdev, dev_number, 1); > + if (result) > + { > + DPK("trec_device_init: cdev_add failed\n"); > + goto done;
If you jump to `done' here, unregister_chrdev_region is never called.
You should also declare trec_device_init as __init and trex_device_exit as __exit.
> --- /dev/null > +++ b/include/linux/trec.h > @@ -0,0 +1,34 @@ > +/* > + * Copyright (C) 2007 Saville Software, Inc. > + * > + * This code may be used for any purpose whatsoever, but > + * no warranty of any kind is provided. > + */ > + > +#ifndef _TREC_H > +#define _TREC_H > + > +#include <linux/sched.h> /* For current->pid */ > +#include <asm/processor.h> /* For current_text_addr */ > + > +#define TREC_PC_ADDR ((unsigned long)(current_text_addr())) > +#define TREC_PID (current->pid) > + > +#define TREC0() trec_write(TREC_PC_ADDR, TREC_PID, > 0, 0) > +#define TREC1(__v) trec_write(TREC_PC_ADDR, TREC_PID, (unsigned > long)(__v), 0) > +#define TREC2(__v1, __v2) trec_write(TREC_PC_ADDR, TREC_PID, > (unsigned long)(__v1), (unsigned long)(__v2)) > +#define TREC_RETV(__retv) do { TREC0(); return(__retv); } while (0) > +#define TREC_RET() do { TREC0(); return; } while (0) > + > +#define ZREC0() do { } while (0) > +#define ZREC1(__v) do { } while (0) > +#define ZREC2(__v1, __v2) do { } while (0) > +#define ZREC_RETV(__retv) do { } while (0) > +#define ZREC_RET() do { } while (0) Why not seperate them by an #ifndef? So you don't have to replace TREC? with ZREC? but rather change one #define knob.
=Hannes - 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/
| |