On 13/03/13 16:27, H Hartley Sweeten wrote:
On Wednesday, March 13, 2013 4:10 AM, Ian Abbott wrote:
On 13/03/2013 00:41, H Hartley Sweeten wrote:
--- a/drivers/staging/comedi/comedi_pci.c
+++ b/drivers/staging/comedi/comedi_pci.c
@@ -57,14 +57,16 @@ EXPORT_SYMBOL_GPL(comedi_pci_enable);
/**
* comedi_pci_disable() - Release the regions and disable the PCI device.
- * @pcidev: pci_dev struct
- *
- * This must be matched with a previous successful call to comedi_pci_enable().
+ * @dev: comedi_device struct
*/
-void comedi_pci_disable(struct pci_dev *pcidev)
+void comedi_pci_disable(struct comedi_device *dev)
{
- pci_release_regions(pcidev);
- pci_disable_device(pcidev);
+ struct pci_dev *pcidev = comedi_to_pci_dev(dev);
+
+ if (pcidev && dev->iobase) {
+ pci_release_regions(pcidev);
+ pci_disable_device(pcidev);
+ }
}
Maybe it should set dev->iobase to 0 as well and your reworked
comedi_pci_enable() set dev->iobase to a temporary non-zero value.
Several drivers only use dev->iobase as a flag anyway. (Maybe we
shouldn't be overloading dev->iobase in this way - it seems kind of
yucky. Perhaps we should use a new member of struct comedi_device.)
This can be done in a follow-up patch.
Ian,
I need to respin these two patches to pick up the adv_pci1724 driver.
I don't think setting dev->iobase to 0 is necessary since the comedi core
always calls cleanup_device() after the (*detach). That function sets all
the dev variables to 0 or NULL.
There shouldn't be any harm in doing so anyway as there are some use
cases where more than one comedi_pci_enable()/comedi_pci_disable()
sequence could be used within a single attach()/detach() sequence. (My
recent adv_pci_dio patch to detect the PCI-1753E used such an extra
sequence, although it could have done something else to avoid it.)
I think it will be cleaner to use a separate member as a flag called
something like 'ioenabled' or some other form of dedicated flag instead
of overloading the semantics of the 'iobase' member.
After these two get accepted I plan on reworking comedi_pci_enable()
to take an additional parameter (main_pcibar) and automatically set
dev->iobase for the drivers, i.e.:
int comedi_pci_enable(struct comedi_device *dev, int main_pcibar)
{
...
If (main_pcibar >= 0) {
dev->iobase = pci_resource_start(pcidev, main_pcibar);
else
dev->iobase = 1; /* for comedi_pci_disable() */
return 0;
}
That's a good idea but may require changing the type of iobase to
resource_size_t to ensure it remains non-zero (a 64-bit PCI BAR may
start on some multiple of 2^32 which would set unsigned long iobase to 0
on a 32-bit system). If iobase is only meaningful as the start address
of a port I/O region and there is a separate flag to indicate the
success of pci_request_regions(), there is no problem keeping iobase as
an unsigned long (or even an unsigned int if none of the drivers try to
store something else in there) as there is no such thing as 64-bit port
I/O resource (I think, true for PCI at least).
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
