Messages in this thread | | | Date | Tue, 12 Sep 2006 05:13:46 +0400 | From | Alexey Dobriyan <> | Subject | Re: [PATCH 2/2] display: Driver ks0108 and cfag12864b |
| |
On Tue, Sep 12, 2006 at 01:57:23AM +0200, Miguel Ojeda wrote: > +int cfag12864b_init(void)
arrrrggg.... I'm in the middle of reading every module_init and every module_exit func, and this starts getting really annoying....
1. module_init function SHOULD be written as
static int __init FOO_init_module(void) { ... }
2. module_exit function SHOULD be written as
static void __exit FOO_exit_module(void) { .. }
3. If one of function during module initialization returns an error, everything done so far MUST be backed out to not cause leaks.
4. module_init and module_exit functions SHOULD NOT spit useless messages upon which system administrator can't act meaningfully.
5. In case of error during initialization, error code SHOULD be propagated as is to upper layers, either via direct assignment/return or via decoding from pointer.
6. Error messages SHOULD start with short unique prefix specific to driver. Module name without .o and .ko is fine.
7. StupidCapitalization MUST NOT be used.
8. Function args SHOULD NOT be prefixed with underscore without reason.
9. Various sorts of operations and methods driver provides to upper layers SHOULD start with short, common to driver prefix and SHOULD be made static where possible:
static struct foobar_operations rtl8139_fops = { ... };
> +{ > + unsigned int i; > + int Result; > + > + printk(PRINTK_PREFIX "Init... "); > + > + Result = alloc_chrdev_region(&FirstDevice, FirstMinor, nDevices, > Name); > + if(Result < 0) { > + printk("ERROR - alloc_chrdev_region\n"); > + return Result; > + } > + Major = MAJOR(FirstDevice); > + > + Devices = kmalloc(nDevices * sizeof(struct cfag12864b), GFP_KERNEL); > + if(Devices == NULL) { > + printk("ERROR - kmalloc\n"); > + return -ENOMEM; > + } > + memset(Devices, 0, nDevices * sizeof(struct cfag12864b)); > + > + for(i=0; i<nDevices; ++i) { > + Devices[i].Minor = FirstMinor+i; > + Devices[i].Device = MKDEV(Major,Devices[i].Minor); > + > + cdev_init(&(Devices[i].CharDevice), &Fops); > + Devices[i].CharDevice.owner = THIS_MODULE; > + Devices[i].CharDevice.ops = &Fops; > + Result = cdev_add(&(Devices[i].CharDevice), > + Devices[i].Device, 1); > + if(Result < 0) { > + printk("ERROR - cdev_add\n"); > + kfree(Devices); > + return Result; > + } > + > + class_device_create( > + display_class,NULL,MKDEV(Major,Devices[i].Minor), > + NULL,"cfag12864b%d",Devices[i].Minor); > + } > + > + > + cfag12864b_Clear(); > + cfag12864b_On();
->open, ->write could be called as soon as cdev_add succeeds. Is hardware functional at that point?
- 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/
| |