RE: TR: interfacing FPGA to AT91RM9200

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you Russell for all those really interesting comments.

I have few more questions:
- I don't understand the reason why you set a name for kobject? Where is
this name used?
- I don't understand kobject_put(&cdev->kobj); when an error is detected.
What is the goal of this function?

Thank you for all your information.

Patrick

-----Message d'origine-----
De : Russell King - ARM Linux [mailto:linux@xxxxxxxxxxxxxxxx] 
Envoyé : mercredi, 10. septembre 2008 22:11
À : Patrick
Cc : linux-arm@xxxxxxxxxxxxxxxxxxxxxx
Objet : Re: TR: interfacing FPGA to AT91RM9200

On Wed, Sep 10, 2008 at 08:47:11PM +0200, Patrick wrote:
> I have made a driver that communicates with an FPGA. The FPGA are
connected
> on the AT91RM9200 memory bus. 
> 
> This is some extract from this driver:

Code posted, so I'll give you a review in the hope that it'll help
you code better for the future. 8)

> int init_module(void) 
> {
> 	int devno;
> 	int i=0;
> 	int tmp[4];
> 	int tmp2 = 0;
> 	struct cdev *romeo_cdev;
> 	
> 	//Initialise the driver structure and communication with userspace
> 	printk("Romeo: Registering FPGA driver...\n");
> 	devno = MKDEV(ROMEO_MAJOR, ROMEO_MINOR);
> 	printk("       devno : %d\n", devno);
> 	romeo_cdev = cdev_alloc();
> 	romeo_cdev->ops = &romeo_fops;
> 	if(cdev_add(romeo_cdev, devno, 1) < 0)
> 	{
> 	    printk("Romeo: Erreur cdev_add\n");
> 	    return -1;
> 	}

First problem with this is that you're making the character device visible
to userspace before you've completed the setup.  That's bad.  Always
complete setup before making interfaces available.

Second problem with the above is that if cdev_add() fails, then you exit
leaving the cdev allocated.  Moreover, you exit using '-1' which is not
a valid return code.  The general principle is that if you call a function
which returns an error code, propagate that error code up.

Thirdly, setting the cdev module pointer would be a good idea, and lastly
giving the associated kobject a name seems to be a good idea.

So:

	int err;

	/*
	 * do all your setup.
	 * - request_mem_region
	 * - ioremap
	 */

	romeo_cdev = cdev_alloc();
	if (!romeo_cdev) {
		pr_err("... failed to allocate cdev\n");
		/* other cleanup - iounmap, release_mem_region */
		return -ENOMEM;
	}

	romeo_cdev->ops = &romeo_fops;
	romeo_cdev->owner = romeo_fops.owner;
	kobject_set_name(&romeo_cdev->kobj, "romeo-somename");

	err = cdev_add(romeo_cdev, devno, 1);
	if (err < 0) {
		pr_err("... failed to add cdev\n");
		kobject_put(&cdev->kobj);
		/* other cleanup - iounmap, release_mem_region */
		return err;
	}

And some people find it easier to do this at the end of the function:

	return 0;

 err_cdev_add:
	kobject_put(&cdev->kobj);
 err_cdev_alloc:
	iounmap();
 err_ioremap:
	release_mem_region();
 err_mem_region:
	return err;

setting 'err' appropriate, using 'goto' the appropriate label.  (We aren't
anti-goto fanatics - we believe goto's have their place, especially when
it improves maintainability and readability!)


-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php


[Index of Archives]     [Linux ARM]     [Linux ARM MSM]     [Linux ARM Kernel]     [Fedora ARM]     [Linux ARM MSM]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux