RE: [PATCH 8/9] staging: comedi: das08: Support mutiple PCI card types in future

[H Hartley Sweeten <hartleys@xxxxxxxxxxxxxxxxxxx>]

On Wednesday, May 23, 2012 9:50 AM, Ian Abbott wrote:
> Rename the existing board entry named "das08" for the PCI-DAS08 board to
> "pci-das08".  Add a new wildcard board entry called "das08" that will
> match any supported PCI board.  If 'dev->board_ptr' points to the
> wildcard board entry in the ->attach() routine, it will be replaced with
> a supported PCI board entry if there are any (taking into account the
> bus and slot specified by the device config options passed in to the
> function).
>
> Split the code that finds a matching PCI device into a new function
> das08_find_pci().
>
> Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
> ---

<snip>

> @@ -972,6 +977,57 @@ int das08_common_attach(struct comedi_device *dev, unsigned long iobase)
>  }
>  EXPORT_SYMBOL_GPL(das08_common_attach);
>  
> +#if IS_ENABLED(CONFIG_COMEDI_DAS08_PCI)
> +static int das08_find_pci(struct comedi_device *dev, int bus, int slot,
> +			  struct pci_dev **pci_dev_p)

A couple comments on how I think this function should work and
it's effect on the das08_attach() function. Feel free to ignore them
if you wish.

1) Change the return type to struct pci_dev *.
   Currently it only returns success (0) or -EIO. The caller can handle the
   error case simply enough and returning the pci_dev * avoids having to
   deal with the pointer to pointer stuff.

2) Just pass the struct comedi_devconfig * instead of the parsed out bus/slot.
3) Move the "pci bus %i slot %i" message from das08_attach() to this function.
   There's really no reason to parse them out in the das08_attach function.

4) Remove the thisboard = comedi_board(dev); after the das08_find_pci() call.
   It's not needed because 'thisboard' is not referenced after this point.

This makes the das08_attach() code a lot more concise:

	case pci: {
			struct pci_dev *pdev;

			pdev = das08_find_pci(dev, it);
			if (!pdev)
				return -EIO;

			return das08_pci_attach_common(dev, pdev);
		}
		break;

Regards,
Hartley

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel