Messages in this thread | | | Date | Mon, 30 Aug 2010 18:24:35 +0200 | From | "Hans J. Koch" <> | Subject | Re: UIO and Fedora 13 (kernel 33.6) |
| |
On Mon, Aug 30, 2010 at 12:49:10PM +0200, Armin Steinhoff wrote: > Hi, > > I'm writing an UIO driver for a plain PC/104 board (ISA bus). > After insmod <my_driver_mod> I don't see an entry of uio0 in /dev > and also no entries in /sys/class. There are no error messages at > module load.
Are you sure your probe() function is called? After a successfull uio_register_device() there is both a /dev/uioX and a directory /sys/class/uio/uioX/.
> > The same happens after loading the module uio.ko and uio_pdrv.ko ... > no entries at all, no error messages.
uio_pdrv needs platform data set up somewhere, did you do that? See docs in Documentation/DocBook/ for more details.
> > In the attachment is the kernel part of the driver. What could be > the problem?
Some comments below.
> > --Armin > > a-steinhoff-at-web-de
> /* > * UIO CAN L2 > * > * (C) Armin Steinhoff <as@steinhoff-automation.com> > * (C) 2007 Hans J. Koch <hjk@linutronix.de> > * Original code (C) 2005 Benedikt Spranger <b.spranger@linutronix.de> > * > * Licensed under GPL version 2 only. > * > */ > > #include <linux/device.h> > #include <linux/module.h> > #include <linux/uio_driver.h> > #include <linux/platform_device.h> > #include <linux/moduleparam.h> > #include <asm/io.h>
#include <linux/io.h>, please.
> > #define DEBUG 1
What's that used for?
> #define DRIVER_MAJOR 240
not needed.
> #define INT_QUEUE_SIZE 64
What's that used for?
> > static struct uio_info *info;
Are you sure you can have only one instance of this driver?
> static unsigned char int_x[2], * int_q[2];
No space between * and variable name. There are more cases below. Please run your patch through checkpatch.pl and fix the issues.
> static void __iomem *isr[2]; > > static unsigned int irq = 11; > module_param (irq, uint, 11); > MODULE_PARM_DESC (irq, "IRQ (default 11)"); > > static unsigned long base_addr = 0xd00000; > module_param (base_addr, ulong, 0xd00000); > MODULE_PARM_DESC (base_addr, "Base address (default 0xD0000)"); > > static unsigned long base_len; > module_param (base_len, ulong, 0x1); > MODULE_PARM_DESC (base_len, "Base length (default 0x1)"); > > static struct platform_device * uio_jand_device; > static int jand_probe(struct device *dev); > static int jand_remove(struct device *dev);
These declarations are not neeeded...
> > static struct device_driver uio_jand_driver = > { > .name = "uio_jand", > .bus = &platform_bus_type, > .probe = jand_probe, > .remove = jand_remove, > };
...if you move this struct to the end of the file.
> > static irqreturn_t int_handler(int irq, struct uio_info *dev_info) > { > int irq_flag = 0; > unsigned char ISRstat; > > ISRstat = readb(isr[0]); > if(ISRstat & 0x02) > { > int_q[0][int_x[0]] = ISRstat; > int_x[0] = (int_x[0] + 1) & 0xF ; // modulo 16 > > irq_flag = 1; > } > > ISRstat = readb(isr[1]); > if(ISRstat & 0x02) > { > int_q[1][int_x[1]] = ISRstat; > int_x[1] = (int_x[1] + 1) & 0xF ; // modulo 16 > irq_flag = 1; > } > > if(irq_flag) > return(IRQ_HANDLED); > else > return(IRQ_NONE); > } > > static int jand_probe(struct device *dev) > { > info = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
info should be a local variable. If you probe the driver twice you'll overwrite the previous value of info and cause a memory leak.
> if (!info) > { > return -ENOMEM; > } > > info->mem[0].addr = base_addr; > info->mem[0].size = base_len; > info->mem[0].memtype = UIO_MEM_PHYS; > > info->mem[0].internal_addr = ioremap(info->mem[0].addr,info->mem[0].size); > if (!info->mem[0].internal_addr) > goto out_release; > > /* interrupt queue */
What does this comment mean?
> info->mem[1].addr = (unsigned long)kmalloc(64, GFP_KERNEL); > if (!info->mem[1].addr) > goto out_release1; > > int_q[0] = (unsigned char * )info->mem[1].addr; > int_q[1] = (unsigned char * )info->mem[1].addr +32; > > memset(&int_q[0], 0x00, 16); > int_x[0] = 0; > memset(&int_q[1], 0x00, 16); > int_x[1] = 0; > > info->mem[1].memtype = UIO_MEM_LOGICAL; > info->mem[1].size = 64; > > isr[0] = info->mem[0].internal_addr + 3; /* interrupt reg channel 1 */ > isr[1] = info->mem[0].internal_addr + 3 + 0x200; /* interrupt reg channel 2 */ > > info->name = "uio_jand"; > info->version = "0.0.1"; > info->irq = irq; > info->irq_flags = 0; > info->handler = int_handler; > > if (uio_register_device(dev, info)) > goto out_release2; > printk("uio_jand started!\n");
please use dev_info instead of printk
> return 0; > > > out_release2: > kfree((void *)info->mem[1].addr); > printk("uio_register_device failed!\n");
dev_err please.
> out_release1: > free_irq( irq, "uio_jand" ); > iounmap(info->mem[0].internal_addr); > release_mem_region( base_addr, base_len); > out_release: > kfree (info); > printk("Error exit ENODEV! \n");
dev_err and correct indentation, please.
> return -ENODEV; > } > > static int jand_remove(struct device *dev) > { > uio_unregister_device(info); > iounmap(info->mem[0].internal_addr); > release_mem_region( base_addr, base_len); > free_irq( irq, "uio_jand" ); > kfree((void *)info->mem[1].addr); > kfree (info); > return 0; > } > > > static int __init uio_jand_init(void) > { > uio_jand_device = platform_device_register_simple("uio_jand", -1,NULL, 0); > return driver_register(&uio_jand_driver);
Please check the return value of both *_register calls.
> } > > static void __exit uio_jand_exit(void) > { > platform_device_unregister(uio_jand_device); > driver_unregister(&uio_jand_driver); > } > > module_init( uio_jand_init ); > module_exit( uio_jand_exit ); > > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("A. Steinhoff"); > MODULE_DESCRIPTION("UIO Janus-D CAN driver");
If "CAN" means "Controller Area Network" it should probably be a socketcan driver instead of UIO...
Thanks, Hans
| |