Some random review comments on your driver, while I was passing. Spotted a few things.
On Sun, May 13, 2007 at 04:40:07PM -0500, Olof Johansson wrote: > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/sched.h> > +#include <linux/platform_device.h> > +#include <linux/platform_device.h> Silly duplicate includes. > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > + > +#include <pcmcia/ss.h> > +#include <asm/of_platform.h> > + > +static const char driver_name[] = "electra-cf"; >... > +static int electra_cf_get_status(struct pcmcia_socket *s, u_int *sp) > +{ > + struct electra_cf_socket *cf; > + > + if (!sp) > + return -EINVAL; Never called with a NULL argument, so useless check. > + > + cf = container_of(s, struct electra_cf_socket, socket); > + > + /* NOTE CF is always 3VCARD */ > + if (electra_cf_present(cf)) { > + struct electra_cf_socket *cf; > + > + *sp = SS_READY | SS_DETECT | SS_POWERON | SS_3VCARD; > + cf = container_of(s, struct electra_cf_socket, socket); > + s->pci_irq = cf->irq; > + } else > + *sp = 0; > + return 0; > +} >... > +static int electra_cf_ss_suspend(struct pcmcia_socket *s) > +{ > + pr_debug("%s: %s\n", driver_name, __FUNCTION__); > + return electra_cf_set_socket(s, &dead_socket); > +} That's already done for you. Remove this function entirely and set the ".suspend" method to NULL. > +static int electra_cf_set_io_map(struct pcmcia_socket *s, > + struct pccard_io_map *io) > +{ > + struct electra_cf_socket *cf; > + > + cf = container_of(s, struct electra_cf_socket, socket); > + io->flags &= MAP_ACTIVE|MAP_ATTRIB|MAP_16BIT; > + io->start = (unsigned long)cf->io_base; > + io->stop = (unsigned long)cf->io_base + 0x800 - 1; > + return 0; > +} PCMCIA code will ignore your writes to 'io'. Therefore, does this do anything useful? If not, an empty function will do. >... > +static int __devinit electra_cf_probe(struct of_device *ofdev, > + const struct of_device_id *match) > +{ > + struct device *device = &ofdev->dev; > + struct device_node *np = ofdev->node; > + struct electra_cf_socket *cf; > + struct resource mem, io; > + int status; > + const unsigned int *prop; > + int err; > + > + err = of_address_to_resource(np, 0, &mem); > + if (err) > + return -EINVAL; > + > + err = of_address_to_resource(np, 1, &io); > + if (err) > + return -EINVAL; > + > + cf = kzalloc(sizeof *cf, GFP_KERNEL); > + if (!cf) > + return -ENOMEM; > + > + init_timer(&cf->timer); > + cf->timer.function = electra_cf_timer; > + cf->timer.data = (unsigned long) cf; > + > + cf->ofdev = ofdev; > + cf->mem_phys = mem.start; > + cf->mem_base = ioremap(mem.start, mem.end - mem.start); > + cf->io_base = ioremap(io.start, io.end - io.start); > + cf->gpio_base = ioremap(0xfc103000, 0x1000); > + dev_set_drvdata(device, cf); > + > + if (!cf->mem_base || !cf->io_base || !cf->gpio_base) { > + dev_err(device, "can't ioremap ranges\n"); > + status = -ENOMEM; > + goto fail1; > + } > + > + cf->iomem.start = (unsigned long)cf->io_base; > + cf->iomem.end = (unsigned long)cf->io_base + (io.end - io.start); > + cf->iomem.flags = IORESOURCE_MEM; > + > + cf->irq = irq_of_parse_and_map(np, 0); > + > + status = request_irq(cf->irq, electra_cf_irq, IRQF_SHARED, > + driver_name, cf); > + if (status < 0) { > + dev_err(device, "request_irq failed\n"); > + goto fail1; > + } > + > + cf->socket.pci_irq = cf->irq; > + > + prop = get_property(np, "card-detect-gpio", NULL); > + cf->gpio_detect = *prop; > + prop = get_property(np, "card-vsense-gpio", NULL); > + cf->gpio_vsense = *prop; > + prop = get_property(np, "card-3v-gpio", NULL); > + cf->gpio_3v = *prop; > + prop = get_property(np, "card-5v-gpio", NULL); > + cf->gpio_5v = *prop; > + > + /* pcmcia layer only remaps "real" memory not iospace */ > + cf->socket.io_offset = (unsigned long)cf->io_base; > + > + /* reserve chip-select regions */ > + if (!request_mem_region(mem.start, mem.end + 1 - mem.start, > + driver_name)) { > + status = -ENXIO; > + dev_err(device, "Can't claim memory region\n"); > + goto fail1; > + } > + > + if (!request_mem_region(io.start, io.end + 1 - io.start, > + driver_name)) { > + status = -ENXIO; > + dev_err(device, "Can't claim I/O region\n"); > + goto fail2; > + } > + > + cf->socket.owner = THIS_MODULE; > + cf->socket.dev.parent = &ofdev->dev; > + cf->socket.ops = &electra_cf_ops; > + cf->socket.resource_ops = &pccard_static_ops; > + cf->socket.features = SS_CAP_PCCARD | SS_CAP_STATIC_MAP | > + SS_CAP_MEM_ALIGN; > + cf->socket.map_size = 0x800; > + cf->socket.io[0].res = &cf->iomem; Not a good idea - PCMCIA manages this resources itself and will free it when the card is removed. Use ss_cap_static_map and socket.io_offset. > + > + status = pcmcia_register_socket(&cf->socket); > + if (status < 0) { > + dev_err(device, "pcmcia_register_socket failed\n"); > + goto fail3; > + } > + > + dev_info(device, "at mem 0x%lx io 0x%lx irq %d\n", > + mem.start, io.start, cf->irq); > + > + cf->active = 1; > + electra_cf_timer((unsigned long)cf); > + return 0; > + > +fail3: > + release_mem_region(io.start, io.end + 1 - io.start); > +fail2: > + release_mem_region(mem.start, mem.end + 1 - mem.start); > +fail1: > + if (cf->io_base) > + iounmap(cf->io_base); > + if (cf->mem_base) > + iounmap(cf->mem_base); > + if (cf->gpio_base) > + iounmap(cf->gpio_base); > + device_init_wakeup(&ofdev->dev, 0); > + kfree(cf); > + return status; > + > +} > + > +static int __devexit electra_cf_remove(struct of_device *ofdev) > +{ > + struct device *device = &ofdev->dev; > + struct electra_cf_socket *cf; > + > + cf = dev_get_drvdata(device); > + > + cf->active = 0; > + pcmcia_unregister_socket(&cf->socket); > + del_timer_sync(&cf->timer); > + free_irq(cf->irq, cf); What if an IRQ occurs after the timer has been deleted? Doesn't the interrupt handler re-add the timer back? > + > + iounmap(cf->io_base); > + iounmap(cf->mem_base); > + iounmap(cf->gpio_base); > + release_mem_region(cf->mem_phys, cf->mem_size); > + release_region(cf->io_phys, cf->io_size); > + > + kfree(cf); > + > + return 0; > +} -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: _______________________________________________ Linux PCMCIA reimplementation list http://lists.infradead.org/mailman/listinfo/linux-pcmcia