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
- Follow-Ups:
- Re: TR: interfacing FPGA to AT91RM9200
- From: Russell King - ARM Linux
- Re: TR: interfacing FPGA to AT91RM9200
- References:
- Re: TR: interfacing FPGA to AT91RM9200
- From: Russell King - ARM Linux
- Re: TR: interfacing FPGA to AT91RM9200
- Prev by Date: how to pass pointers to packed data?
- Next by Date: Re: TR: interfacing FPGA to AT91RM9200
- Previous by thread: Re: TR: interfacing FPGA to AT91RM9200
- Next by thread: Re: TR: interfacing FPGA to AT91RM9200
- Index(es):