Hi Olof.

On Mon Jun 25 11:03:11 EST 2007, Olof Johansson wrote:

Driver for the CompactFlash slot on the PA Semi Electra eval board. 
It's
a simple device sitting on localbus, with interrupts and detect/voltage
control over GPIO.

The driver is implemented as an of_platform driver, and adds localbus
as a bus being probed by the of_platform framework.

...

+
+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_size = PAGE_ALIGN(io.end - io.start);
+
+       cf->io_virt = reserve_phb_iospace(cf->io_size);
+
+       cf->gpio_base = ioremap(0xfc103000, 0x1000);
+       dev_set_drvdata(device, cf);
+
+       if (!cf->mem_base || !cf->io_virt || !cf->gpio_base) {
+               dev_err(device, "can't ioremap ranges\n");
+               status = -ENOMEM;
+               goto fail1;
+       }
+
+       __ioremap_explicit(io.start, (unsigned long)cf->io_virt, 
cf->io_size,
+                          _PAGE_NO_CACHE | _PAGE_GUARDED);
+
+       cf->io_base = (unsigned long)cf->io_virt - VMALLOC_END;
+
+       cf->iomem.start = (unsigned long)cf->mem_base;
+       cf->iomem.end = (unsigned long)cf->mem_base + (mem.end - 
mem.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;
+

get_property is now a #define for of_get_property.  I think the use of 
get_property is being deprecated.

If of_get_property fails to find the property, then you will 
dereference a NULL pointer in a probe function and will leave the 
bus_type locked.

+       cf->socket.io_offset = 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_region(cf->io_base, cf->io_size, 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;
+
+       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:
+       /* XXX No way to undo the io reservation at this time */

What io reservation is this comment referring to?


Where is the request_irq undone?

+       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);
+       free_irq(cf->irq, cf);
+       del_timer_sync(&cf->timer);
+
+       iounmap(cf->mem_base);
+       iounmap(cf->gpio_base);
+       release_mem_region(cf->mem_phys, cf->mem_size);
+       release_region(cf->io_base, cf->io_size);
+

irq_request?

+       kfree(cf);
+
+       return 0;
+}
+
+static int bus_notify(struct notifier_block *nb, unsigned long action,
+                     void *data)
+{
+       struct device *dev = data;
+
+       printk("bus notify called\n");
+
+       /* We are only intereted in device addition */
+       if (action != BUS_NOTIFY_ADD_DEVICE)
+               return 0;
+
+       /* We use the direct ops for localbus */
+       dev->archdata.dma_ops = &dma_direct_ops;
+

I'm also replying to your reply to hch's reply.  Is that enough of a 
reply? :-)

+       return 0;
+}
+
+static struct notifier_block bus_notifier = {
+       .notifier_call = bus_notify,
+};
+
+static struct of_device_id electra_cf_match[] =
+{
+       {
+               .compatible   = "electra-cf",
+       },
+       {},
+};
+
+static struct of_platform_driver electra_cf_driver =
+{
+       .name      = (char *)driver_name,
+       .match_table    = electra_cf_match,
+       .probe    = electra_cf_probe,
+       .remove   = electra_cf_remove,
+};
+
+static int __init electra_cf_init(void)
+{
+       bus_register_notifier(&pcmcia_bus_type, &bus_notifier);
+       return of_register_platform_driver(&electra_cf_driver);
+}
+module_init(electra_cf_init);
+
+static void __exit electra_cf_exit(void)
+{
+       bus_unregister_notifier(&pcmcia_bus_type, &bus_notifier);
+       of_unregister_platform_driver(&electra_cf_driver);

Should these be reversed?  (if we keep it here)

+}
+module_exit(electra_cf_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR ("Olof Johansson <olof at lixom.net>");
+MODULE_DESCRIPTION("PA Semi Electra CF driver");
+

cheers,
milton