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

[H Hartley Sweeten]

On Thursday, July 19, 2012 4:35 PM, gregkh wrote:
> On Thu, Jul 19, 2012 at 06:31:23PM -0500, H Hartley Sweeten wrote:
>> 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.
>
> The ioctl shouldn't be needed anymore for PCI or USB devices, as the
> kernel handles the matching of the driver to the device.  Even if it
> didn't, there are other more "standard" ways that you can bind devices
> to drivers (through sysfs.)
>
> So, I'd really recommend ripping all of this logic out for PCI drivers
> as odds are, it's not used, and probably doesn't really work.

Ian,

I think we could rip the PCI search stuff out of the comedi pci drivers
but I would like your opinion first.

1) Implement the "attach_pci" callback in all the pci drivers.
2) Change the "attach" callback to just return -EPERM, -EINVAL, or
    whatever seems appropriate. This has the most minimal effect
    on the comedi core. We could also just remove the "attach" from
    the pci drivers that have the "attach_pci" callback but the core
    would need some changes to account for this.

The COMEDI_DEVCONFIG ioctl, which is only used by the comedi_config
utility, would then fail on all the pci drivers but that should not be an
issue because the auto config stuff would still work correctly when the
module is loaded. You would lose the ability to specify the devnode
that the module is bound to but I think the sysfs "bind" and "unbind"
files are meant to handle that.

I think there are a couple pci drivers that would still need the old "attach"
in order to load firmware using comedi_config. We can address those
later and possibly figure out a more "standard" way to load the firmware.

What do you think?

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/