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

H Hartley Sweeten
2012-07-19T23:31:29Z

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