pcmcia: CompactFlash driver for PA Semi Electra boards

drivers/pcmcia/electra_cf.c new file +6 -0

+static int bus_notify(struct notifier_block *nb, unsigned long action,
+		      void *data)
+{
+	struct device *dev = data;
+
+	printk("bus notify called\n");

Quoted 6 lines

drivers/pcmcia/electra_cf.c new file +6 -0

+static int bus_notify(struct notifier_block *nb, unsigned long action,
+		      void *data)
+{
+	struct device *dev = data;
+
+	printk("bus notify called\n");

drivers/pcmcia/electra_cf.c line 343

Quoted 10 lines

drivers/pcmcia/electra_cf.c new file +10 -0

+
+	/* 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;
+
+	return 0;
+}

Sorry for not coming back to you after the last posting of the patch, but I still really this bit of the code. We don't set dma ops from a driver anywhere else in the tree, so I'd really prefer if you could handle this in architecture code somewhere. Especially as dma_direct_ops and the pcmcia_bug_type shouldn't really be something exported to users.

Also this code is unlogic if not buggy. Just because you have one electra bridge it doesn't mean all pcmcia is driven by it. In fact I'm pretty sure there are cardbus/pcmcia bridges that can be plugged into plain pci slots.

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.

Quoted 6 lines · Olof Johansson

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.

Looks good to me. The only odd thing is the inconsistant use of #ifdef CONFIG_PCMCIA in setup.c.

The ifdef is needed since for CONFIG_PCMCIA=n builds, the bus notifier isn't available. I wanted to do the bus notifier registration explicitly before the of_platform bus probe to avoid later surprises due to reordered initcalls in case it was split up in it's own initcall.

I could add the code under ifdef as well, but it didn't seem too critical. Once the second major board comes along I'll probably move it out to a per-board file, there's no real need for it just yet.

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.

Signed-off-by Olof Johansson <[email protected]>

---

Quoted 8 lines · olof

The ifdef is needed since for CONFIG_PCMCIA=n builds, the bus notifier isn't available. I wanted to do the bus notifier registration explicitly before the of_platform bus probe to avoid later surprises due to reordered initcalls in case it was split up in it's own initcall.

I could add the code under ifdef as well, but it didn't seem too critical. Once the second major board comes along I'll probably move it out to a per-board file, there's no real need for it just yet.

Alright, turns out I still need to declare the extern bus type, which would mean two #ifdefs in one function. Moving it out instead.

I've addressed Milton's comments as well.

Who's maintaining PCMCIA? MAINTAINERS only lists a mailing list, no person. Seems weird for a component that's marked as maintained.

...

(quoted diff) +1 -0

+static const char driver_name[] = "electra-cf";

Quoted 2 lines

Who's maintaining PCMCIA? MAINTAINERS only lists a mailing list, no person. Seems weird for a component that's marked as maintained.

Dominik Brodowski. He's having a bit of downtime at present (exams, I think). He expects to return. Meanwhile, cc'ing me usually has some effect.

Quoted 4 lines

+static const char driver_name[] = "electra-cf";

...

Review

Quoted 7 lines

(quoted diff) +7 -0

+static struct of_device_id electra_cf_match[] =
+{
+	{
+		.compatible   = "electra-cf",
+	},
+	{},
+};

Could have reused driver_name[] here, if that was appropriate.

arch/powerpc/platforms/pasemi/setup.c

Quoted 2 lines

arch/powerpc/platforms/pasemi/setup.c +0 -0

I never know who maintains random-scruffy-ppc code like this. From a peek in the git-whatchanged output, it appears to be yourself.

Have a few little fixies:

drivers/pcmcia/electra_cf.c renamed from drivers/pcmcia/electra_cf.c~pcmcia-compactflash-driver-for-pa-semi-electra-boards-fix +0 -0

Quoted 8 lines · Olof Johansson

Quoted 6 lines

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.

Quoted 2 lines

Looks good to me. The only odd thing is the inconsistant use of #ifdef CONFIG_PCMCIA in setup.c.

The ifdef is needed since for CONFIG_PCMCIA=n builds, the bus notifier isn't available. I wanted to do the bus notifier registration explicitly before the of_platform bus probe to avoid later surprises due to reordered initcalls in case it was split up in it's own initcall.

I could add the code under ifdef as well, but it didn't seem too critical. Once the second major board comes along I'll probably move it out to a per-board file, there's no real need for it just yet.

If CONFIG_PCMCIA=m then your notifier is not registered. The modprobe of your of_driver loads ds.ko, registers the bus, then registers your driver. When the socket driver tries to dma, the BUG in dma_64 for no archdata.dma_ops triggers.

It seems like we need

(1) a notifier that a bus is registered, run before allowing any devices, so that platforms can register bus notifiers by bus name before the devices and drivers are registered.

(2) a powerpc64 generic pcmcia bus notifier that copys the dma ops from the parent socket.

(3) something to set the dma_ops to direct_dma_ops on the of device.

If we don't want (3) to be in the driver (as Christoph previosly mentioned), then it needs to be a seperate bus that reuses the of
matching. This would be similar to how ibmebus is setup. If I
remember the discussion, ibmebus is to provide the alternate dma ops and steals match etc code from the of_platform bus type.

Oh, is this why you have depends on PCMCIA=y ?

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.

drivers/pcmcia/electra_cf.c new file +45 -0

+
+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,

drivers/pcmcia/electra_cf.c new file +6 -0

+                          _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);

drivers/pcmcia/electra_cf.c new file +12 -0

+       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;

drivers/pcmcia/electra_cf.c new file +33 -0

+       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);
+

drivers/pcmcia/electra_cf.c new file +17 -0

+       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);
+

drivers/pcmcia/electra_cf.c new file +9 -0

+       kfree(cf);
+
+       return 0;
+}
+
+static int bus_notify(struct notifier_block *nb, unsigned long action,
+                     void *data)
+{
+       struct device *dev = data;

drivers/pcmcia/electra_cf.c new file +24 -0

+       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)

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.

Quoted 1 line

+ /* 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?

Quoted 12 lines

drivers/pcmcia/electra_cf.c new file +12 -0

+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);
+

drivers/pcmcia/electra_cf.c line 322

Quoted 6 lines

drivers/pcmcia/electra_cf.c new file +5 -0

+       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?