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

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

[H Hartley Sweeten]

Use pci_is_enabled() in the "find pci device" function to determine if
the found pci device is not in use and move the comedi_pci_enable() call
into the attach.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adv_pci1723.c b/drivers/staging/comedi/drivers/adv_pci1723.c
index f561a2a..e971fa6 100644
--- a/drivers/staging/comedi/drivers/adv_pci1723.c
+++ b/drivers/staging/comedi/drivers/adv_pci1723.c
@@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
 		}
 		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)
 			continue;
-		/*
-		 * Look for device that isn't in use.
-		 * Enable PCI device and request regions.
-		 */
-		if (comedi_pci_enable(pcidev, "adv_pci1723"))
+		if (pci_is_enabled(pcidev))
 			continue;
 		return pcidev;
 	}
@@ -335,6 +331,10 @@ static int pci1723_attach(struct comedi_device *dev,
 	if (!devpriv->pcidev)
 		return -EIO;
 
+	ret = comedi_pci_enable(devpriv->pcidev, "adv_pci1723");
+	if (ret)
+		return ret;
+
 	dev->iobase = pci_resource_start(devpriv->pcidev, 2);
 
 	dev->board_name = this_board->name;
-- 
1.7.11

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

[Ian Abbott]

On 2012-07-19 02:30, H Hartley Sweeten wrote:
> Use pci_is_enabled() in the "find pci device" function to determine if
> the found pci device is not in use and move the comedi_pci_enable() call
> into the attach.
>
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/adv_pci1723.c b/drivers/staging/comedi/drivers/adv_pci1723.c
> index f561a2a..e971fa6 100644
> --- a/drivers/staging/comedi/drivers/adv_pci1723.c
> +++ b/drivers/staging/comedi/drivers/adv_pci1723.c
> @@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
>   		}
>   		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)
>   			continue;
> -		/*
> -		 * Look for device that isn't in use.
> -		 * Enable PCI device and request regions.
> -		 */
> -		if (comedi_pci_enable(pcidev, "adv_pci1723"))
> +		if (pci_is_enabled(pcidev))
>   			continue;
>   		return pcidev;
>   	}

Just because the device is enabled doesn't mean that it is in use, so 
this change could skip over a perfectly good unused device.

If you want to move the comedi_pci_enable() call, this is a change in 
behaviour for this particular driver, but is consistent with most of the 
other Comedi PCI drivers (and the bus/slot options can be used to select 
a particular device).  This is probably a good thing, but you should 
take out the pci_is_enabled test.

> @@ -335,6 +331,10 @@ static int pci1723_attach(struct comedi_device *dev,
>   	if (!devpriv->pcidev)
>   		return -EIO;
>
> +	ret = comedi_pci_enable(devpriv->pcidev, "adv_pci1723");
> +	if (ret)
> +		return ret;
> +
>   	dev->iobase = pci_resource_start(devpriv->pcidev, 2);
>
>   	dev->board_name = this_board->name;
>


-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

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

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4274 bytes --]

On Thursday, July 19, 2012 2:38 AM, Ian Abbott wrote:
> On 2012-07-19 02:30, H Hartley Sweeten wrote:
>> Use pci_is_enabled() in the "find pci device" function to determine if
>> the found pci device is not in use and move the comedi_pci_enable() call
>> into the attach.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: Ian Abbott <abbotti@mev.co.uk>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>   drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/adv_pci1723.c b/drivers/staging/comedi/drivers/adv_pci1723.c
>> index f561a2a..e971fa6 100644
>> --- a/drivers/staging/comedi/drivers/adv_pci1723.c
>> +++ b/drivers/staging/comedi/drivers/adv_pci1723.c
>> @@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
>>   		}
>>   		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)
>>   			continue;
>> -		/*
>> -		 * Look for device that isn't in use.
>> -		 * Enable PCI device and request regions.
>> -		 */
>> -		if (comedi_pci_enable(pcidev, "adv_pci1723"))
>> +		if (pci_is_enabled(pcidev))
>>   			continue;
>>   		return pcidev;
>>   	}
>
> Just because the device is enabled doesn't mean that it is in use, so 
> this change could skip over a perfectly good unused device.

Are you sure? According to Documentation/PCI/pci.txt, the first thing
a driver "should" do when taking ownership of a PCI device is enable
the device. The last thing it "should" do when being unloaded is
disable the device.

It would seem that checking pci_is_enabled() would let us know that
the pci_dev in question is being used.

> If you want to move the comedi_pci_enable() call, this is a change in 
> behaviour for this particular driver, but is consistent with most of the 
> other Comedi PCI drivers (and the bus/slot options can be used to select 
> a particular device).  This is probably a good thing, but you should 
> take out the pci_is_enabled test.

I was curious about this.

In the original driver, doing an 'attach' with bus/slot both = 0 would result
in the pci bus being walked to find the first device that could be enabled
(i.e. a "free" device) and using that device. This allows doing the 'attach'
with more than one card installed and being able to attach to each one
by simply:

comedi_config /dev/comedi0 adv_pci1723
comedi_config /dev/comedi1 adv_pci1723
etc.

The "normal" operation for the comedi pci drivers appears to require
the bus/slot information when multiple devices are used. Or implement
the 'attach_pci' callback in the comedi_driver and allow the core to
simply pass the pci_dev directly to the driver.

What would you prefer?

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?

Regards,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[gregkh]

On Thu, Jul 19, 2012 at 12:12:02PM -0500, H Hartley Sweeten wrote:
> On Thursday, July 19, 2012 2:38 AM, Ian Abbott wrote:
> > On 2012-07-19 02:30, H Hartley Sweeten wrote:
> >> Use pci_is_enabled() in the "find pci device" function to determine if
> >> the found pci device is not in use and move the comedi_pci_enable() call
> >> into the attach.
> >>
> >> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> >> Cc: Ian Abbott <abbotti@mev.co.uk>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> ---
> >>   drivers/staging/comedi/drivers/adv_pci1723.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/staging/comedi/drivers/adv_pci1723.c b/drivers/staging/comedi/drivers/adv_pci1723.c
> >> index f561a2a..e971fa6 100644
> >> --- a/drivers/staging/comedi/drivers/adv_pci1723.c
> >> +++ b/drivers/staging/comedi/drivers/adv_pci1723.c
> >> @@ -302,11 +302,7 @@ static struct pci_dev *pci1723_find_pci_dev(struct comedi_device *dev,
> >>   		}
> >>   		if (pcidev->vendor != PCI_VENDOR_ID_ADVANTECH)
> >>   			continue;
> >> -		/*
> >> -		 * Look for device that isn't in use.
> >> -		 * Enable PCI device and request regions.
> >> -		 */
> >> -		if (comedi_pci_enable(pcidev, "adv_pci1723"))
> >> +		if (pci_is_enabled(pcidev))
> >>   			continue;
> >>   		return pcidev;
> >>   	}
> >
> > Just because the device is enabled doesn't mean that it is in use, so 
> > this change could skip over a perfectly good unused device.
> 
> Are you sure? According to Documentation/PCI/pci.txt, the first thing
> a driver "should" do when taking ownership of a PCI device is enable
> the device. The last thing it "should" do when being unloaded is
> disable the device.
> 
> It would seem that checking pci_is_enabled() would let us know that
> the pci_dev in question is being used.
> 
> > If you want to move the comedi_pci_enable() call, this is a change in 
> > behaviour for this particular driver, but is consistent with most of the 
> > other Comedi PCI drivers (and the bus/slot options can be used to select 
> > a particular device).  This is probably a good thing, but you should 
> > take out the pci_is_enabled test.
> 
> I was curious about this.
> 
> In the original driver, doing an 'attach' with bus/slot both = 0 would result
> in the pci bus being walked to find the first device that could be enabled
> (i.e. a "free" device) and using that device. This allows doing the 'attach'
> with more than one card installed and being able to attach to each one
> by simply:
> 
> comedi_config /dev/comedi0 adv_pci1723
> comedi_config /dev/comedi1 adv_pci1723
> etc.
> 
> The "normal" operation for the comedi pci drivers appears to require
> the bus/slot information when multiple devices are used. Or implement
> the 'attach_pci' callback in the comedi_driver and allow the core to
> simply pass the pci_dev directly to the driver.
> 
> What would you prefer?
> 
> 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?

thanks,

greg k-h

RE: [PATCH 15/90] staging: comedi: adv_pci1723: move comedi_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

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

[gregkh]

On Thu, Jul 19, 2012 at 06:31:23PM -0500, H Hartley Sweeten wrote:
> 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.

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.

For old ISA cards, it makes sense, but the odds that anyone uses any ISA
devices is pretty slim.

thanks,

greg k-h

RE: [PATCH 15/90] staging: comedi: adv_pci1723: move comedi_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.)

I think it's still needed for some of the devices that require external
firmware. The comedi_config utility allows the user to remove the
driver binding and then reattach to it while passing the firmware blob
into the driver.

Not saying any of this is valid... And yes, there probably is a more
"standard" way to do this. I just need a hint of what that is... ;-)

> 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 Abbott is probably the best source to answer that.

> For old ISA cards, it makes sense, but the odds that anyone uses any ISA
> devices is pretty slim.

I lot of the ISA drivers actually work for PC/104 variations of the boards. I
think they are still used somewhat. But yes, the pure ISA cards probably
are not very common. It's almost impossible to actually find a PC that has
ISA slots.

Regards,
Hartley

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

[gregkh]

On Thu, Jul 19, 2012 at 06:58:38PM -0500, H Hartley Sweeten wrote:
> 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.)
> 
> I think it's still needed for some of the devices that require external
> firmware. The comedi_config utility allows the user to remove the
> driver binding and then reattach to it while passing the firmware blob
> into the driver.

Why would the driver need to be unbound from the device to do this?

> Not saying any of this is valid... And yes, there probably is a more
> "standard" way to do this. I just need a hint of what that is... ;-)

The "bind" and "unbind" files in sysfs are for that.

thanks,

greg k-h

RE: [PATCH 15/90] staging: comedi: adv_pci1723: move comedi_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

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

[gregkh]

On Fri, Jul 20, 2012 at 11:33:27AM -0500, H Hartley Sweeten wrote:
> 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.

That "standard" way is to use the kernel's firmware layer for this :)

thanks,

greg k-h