On Wed, May 23, 2012 at 05:50:21PM +0100, Ian Abbott wrote: > As the das08_common_attach() and das08_common_detach() functions are > exported (but are only used by the das08_cs module for PCMCIA cards), > check that we support the bus type of the passed in device. > Putting all these #ifdefs in the code is kind of nasty. :/ > Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx> > --- > drivers/staging/comedi/drivers/das08.c | 55 ++++++++++++++++++++++++------- > 1 files changed, 42 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/das08.c b/drivers/staging/comedi/drivers/das08.c > index 3f81a00..829e532 100644 > --- a/drivers/staging/comedi/drivers/das08.c > +++ b/drivers/staging/comedi/drivers/das08.c > @@ -879,13 +879,30 @@ int das08_common_attach(struct comedi_device *dev, unsigned long iobase) > struct comedi_subdevice *s; > int ret; > > - /* allocate ioports for non-pcmcia, non-pci boards */ > - if ((thisboard->bustype != pcmcia) && (thisboard->bustype != pci)) { > + switch (thisboard->bustype) { > +#if IS_ENABLED(CONFIG_COMEDI_DAS08_ISA) > + case isa: > + case pc104: > + /* allocate ioports for ISA (and PC/104) boards */ > printk(KERN_INFO " iobase 0x%lx\n", iobase); > if (!request_region(iobase, thisboard->iosize, DRV_NAME)) { > printk(KERN_ERR " I/O port conflict\n"); > return -EIO; > } > + break; > +#endif > +#if IS_ENABLED(CONFIG_COMEDI_DAS08_PCI) > + case pci: > + break; > +#endif > +#if IS_ENABLED(CONFIG_COMEDI_DAS08_CS) > + case pcmcia: > + break; > +#endif > + default: > + printk(KERN_ERR " unsupported bus type\n"); All these printks have a space at the start. What's the reason for that? > + return -EIO; > + break; > } > dev->iobase = iobase; > The checking for supported bus types is really a separate thing from allocating resources. static inline bool supported_card(enum das08_bustype bustype) { switch (bustype) { #if IS_ENABLED(CONFIG_COMEDI_DAS08_ISA) case isa: case pc104: #endif #if IS_ENABLED(CONFIG_COMEDI_DAS08_PCI) case pci: #endif #if IS_ENABLED(CONFIG_COMEDI_DAS08_CS) case pcmcia: #endif return true; default: return false; } } ... We could check that in ealier in das08_attach(). Then in das08_common_attach() we wouldn't have any ifdefs. if (!supported_card(thisboard->bustype)) { printk(KERN_ERR " unsupported bus type\n"); return -EIO; } if ((thisboard->bustype == isa) || (thisboard->bustype == pc104)) { printk(KERN_INFO " iobase 0x%lx\n", iobase); if (!request_region(iobase, thisboard->iosize, DRV_NAME)) { printk(KERN_ERR " I/O port conflict\n"); return -EIO; } } > @@ -1007,9 +1024,10 @@ static int das08_attach(struct comedi_device *dev, struct comedi_devconfig *it) > return ret; > > printk(KERN_INFO "comedi%d: das08: ", dev->minor); > + switch (thisboard->bustype) > + { > #if IS_ENABLED(CONFIG_COMEDI_DAS08_PCI) > - /* deal with a pci board */ > - if (thisboard->bustype == pci) { > + case pci: > if (it->options[0] || it->options[1]) { > printk("bus %i slot %i ", > it->options[0], it->options[1]); > @@ -1058,13 +1076,13 @@ static int das08_attach(struct comedi_device *dev, struct comedi_devconfig *it) > /* Enable local interrupt 1 and pci interrupt */ > outw(INTR1_ENABLE | PCI_INTR_ENABLE, pci_iobase + INTCSR); > #endif > - } else > + break; > #endif /* IS_ENABLED(CONFIG_COMEDI_DAS08_PCI) */ > - { > + default: > iobase = it->options[0]; > + printk(KERN_INFO "\n"); > + break; > } > - printk(KERN_INFO "\n"); > - > return das08_common_attach(dev, iobase); > } How worried about we really about those extra bytes of code? Why don't we just enable the support for this stuff? The other thing is that you can use IS_ENABLED() in c code. The compiler will optimize everything away that isn't needed. if (bustype == pci && !IS_ENABLED(CONFIG_COMEDI_DAS08_PCI)) return -EIO; > #endif /* DO_COMEDI_DRIVER_REGISTER */ > @@ -1073,20 +1091,31 @@ void das08_common_detach(struct comedi_device *dev) > { > if (dev->subdevices) > subdev_8255_cleanup(dev, dev->subdevices + 4); > - if ((thisboard->bustype != pcmcia) && (thisboard->bustype != pci)) { > + switch (thisboard->bustype) { > +#if IS_ENABLED(CONFIG_COMEDI_DAS08_ISA) > + case isa: > + case pc104: > if (dev->iobase) > release_region(dev->iobase, thisboard->iosize); > - } > + break; > +#endif > #if IS_ENABLED(CONFIG_COMEDI_DAS08_PCI) > - if (devpriv) { > - if (devpriv->pdev) { > + case pci: > + if (devpriv && devpriv->pdev) { > if (devpriv->pci_iobase) > comedi_pci_disable(devpriv->pdev); > > pci_dev_put(devpriv->pdev); > } > - } > + break; > +#endif > +#if IS_ENABLED(CONFIG_COMEDI_DAS08_CS) > + case pcmcia: > + break; > #endif > + default: > + break; > + } > } It's not worth the extra mess to save these few bytes in the detach function. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel