Re: [PATCH 2/2] display: Driver ks0108 and cfag12864b

[Alexey Dobriyan <adobriyan@gmail.com>]

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/