RE: [PATCH 15/90] staging: comedi: adv_pci1723: movecomedi_pci_enable() into the attach

[H Hartley Sweeten]

On Thursday, July 19, 2012 4:17 PM, gregkh wrote:
> On Thu, Jul 19, 2012 at 12:12:02PM -0500, H Hartley Sweeten wrote:
>> I was planning on making a comedi_find_pci_dev() function that the
>> drivers could call with a "match" callback. This would allow a common
>> function for most of the boilerplate code and just require the drivers
>> to check the the match against the boardinfo dev/id, etc. as required.
>> Something like:
>> 
>> struct pci_dev *comedi_find_pci_dev(struct comedi_device *dev,
>> 	struct comedi_devconfig *it,
>> 	const void *(*match)(struct comedi_device *,
>> 				struct pci_dev *))
>> {
>> 	struct pci_dev *pcidev = NULL;
>> 	int bus = it->options[0];
>> 	int slot = it->options[1];
>> 
>> 	for_each_pci_dev(pcidev) {
>> 		/* pci_is_enabled() test? */
>> 		if ((bus && bus != pcidev->bus->number) ||
>> 		    (slot && slot != PCI_SLOT(pcidev->devfn)))
>> 			continue;
>> 		dev->board_ptr = match(dev, pcidev);
>> 		if (dev->board_ptr) {
>> 			comedi_set_hw_dev(dev, &pcidev->dev);
>> 			return pcidev;
>> 		}
>> 	}
>> 	return NULL;
>> }
>> 
>> The "match" function for a driver would then just be something like:
>> 
>> const void *match_pci(struct comedi_device *dev, struct pci_dev *pcidev)
>> {
>> 	const struct boardinfo *board;
>> 	int i;
>> 
>> 	for (i = 0; i < ARRAY_SIZE(boardinfo); i++) {
>> 		board = &boardinfo[i];
>> 		if (pcidev->vendor != board->ven_id ||
>> 		    pcidev->device != board->dev_id)
>> 			continue;
>> 		return board;
>> 	}
>> 	return NULL;
>> }
>> 
>> This would require adding a dummy boardinfo to some of the drivers but
>> I think it's cleaner.
>> 
>> Comments?
>
> Ick.  What ever happened to converting these drivers to use the PCI api
> correctly and not to search the PCI space for the device, but have the
> PCI core call them if the device is found?
>
> It looks like most of these drivers have already been converted to that
> style, so these checks for "do we find a device" can all be removed
> entirely now, right?  There's no way the functions would be called if
> the device wasn't found in the first place.
> 
> Or am I missing something here?

If the comedi pci drivers have the "attach_pci" callback defined, the
PCI api does correctly probe the driver. The comedi_pci_auto_config()
then passes the pci_dev directly to the driver and the search of the
PCI space for the device is not required.

If the "attach_pci" callback is not defined, the comedi_pci_auto_config()
then falls back to passing the bus/slot information to the driver and uses
the "attach" callback. In this case we could probably fast-track the search
by using pci_get_slot() instead of doing the for_each_pci_dev() loop.

I think the problem is the COMEDI_DEVCONFIG ioctl. The userspace
utility 'comedi_config' uses that ioctl to link a device node to a
comedi driver. That utility allows passing the bus/slot information
but it's not required. This means we have to search the PCI space
for the pci_dev that matches the driver.

Not sure what to do here...

Regards,
Hartley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/