[PATCH 2/9] staging: comedi: das08: Check bus type is supported.

[Dan Carpenter]

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 at mev.co.uk>
> ---
>  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