LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alexey Dobriyan <adobriyan@gmail.com>
To: Miguel Ojeda <maxextreme@gmail.com>
Cc: akpm@osdl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] display: Driver ks0108 and cfag12864b
Date: Tue, 12 Sep 2006 05:13:46 +0400	[thread overview]
Message-ID: <20060912011346.GB5192@martell.zuzino.mipt.ru> (raw)
In-Reply-To: <653402b90609111657r1fa861e0gf4d71508df60a5ec@mail.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?


  reply	other threads:[~2006-09-12  1:13 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-09-11 23:27 [PATCH 2.6.17.13] " Miguel Ojeda
2006-09-11 23:54 ` [PATCH 1/2] " Miguel Ojeda
2006-09-11 23:57 ` [PATCH 2/2] " Miguel Ojeda
2006-09-12  1:13   ` Alexey Dobriyan [this message]
2006-09-12  2:37     ` Miguel Ojeda
2006-09-12  3:51 ` [PATCH 2.6.17.13] " Greg KH
2006-09-12  5:50   ` [PATCH V2] " Miguel Ojeda
2006-09-12  6:09   ` [PATCH 2.6.17.13] " Miguel Ojeda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20060912011346.GB5192@martell.zuzino.mipt.ru \
    --to=adobriyan@gmail.com \
    --cc=akpm@osdl.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maxextreme@gmail.com \
    --subject='Re: [PATCH 2/2] display: Driver ks0108 and cfag12864b' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).