Messages in this thread | | | From | Herton Ronaldo Krzesinski <> | Subject | Re: staging driver s626 clashes with philips SAA7146 media/dvb based cards | Date | Wed, 17 Jun 2009 21:05:56 -0300 |
| |
Em Qua 17 Jun 2009, às 20:35:13, Frank Mori Hess escreveu: > On Wednesday 17 June 2009, Herton Ronaldo Krzesinski wrote: > > Also s626_attach is modified to use now pci_get_subsys instead of > > pci_get_device as reported by Ian Abbott, additionaly ensuring that > > subvendor or subdevice id is set in pci id table entries. > > > > Reference: http://lkml.org/lkml/2009/6/16/552 > > --- > > drivers/staging/comedi/drivers/s626.c | 35 > > ++++++++++++++++++++------------ 1 files changed, 22 insertions(+), 13 > > deletions(-) > > > > diff --git a/drivers/staging/comedi/drivers/s626.c > > b/drivers/staging/comedi/drivers/s626.c index 30dec9d..3ec4407 100644 > > --- a/drivers/staging/comedi/drivers/s626.c > > +++ b/drivers/staging/comedi/drivers/s626.c > > @@ -110,9 +110,9 @@ static const struct s626_board s626_boards[] = { > > #define PCI_VENDOR_ID_S626 0x1131 > > #define PCI_DEVICE_ID_S626 0x7146 > > > > +/* sub pci id must be specified, see s626_attach for more details */ > > static DEFINE_PCI_DEVICE_TABLE(s626_pci_table) = { > > - {PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, PCI_ANY_ID, PCI_ANY_ID, 0, 0, > > - 0}, > > + {PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, 0x6000, 0x0272, 0, 0, 0}, > > {0} > > }; > > > > @@ -498,24 +498,33 @@ static int s626_attach(struct comedi_device *dev, > > struct comedi_devconfig *it) resource_size_t resourceStart; > > dma_addr_t appdma; > > struct comedi_subdevice *s; > > - struct pci_dev *pdev; > > + const struct pci_device_id *ids; > > + struct pci_dev *pdev = NULL; > > > > if (alloc_private(dev, sizeof(struct s626_private)) < 0) > > return -ENOMEM; > > > > - for (pdev = pci_get_device(PCI_VENDOR_ID_S626, PCI_DEVICE_ID_S626, > > - NULL); pdev != NULL; > > - pdev = pci_get_device(PCI_VENDOR_ID_S626, > > - PCI_DEVICE_ID_S626, pdev)) { > > + /* > > + * Require also one of sub pci ids to be defined (see check below), > > + * otherwise there will be a clash with Philips SAA7146 media/dvb > > + * based cards (they have same vendor:device == 0x1131:0x7146 pair > > + * as main S626 cards) > > + */ > > + for (ids = s626_pci_table; > > + (ids->vendor && (ids->subvendor || ids->subdevice)) && !pdev; > > + ids++) { > > + pdev = pci_get_subsys(ids->vendor, ids->device, ids->subvendor, > > + ids->subdevice, NULL); > > + if (!pdev) > > + continue; > > + > > if (it->options[0] || it->options[1]) { > > + /* matches requested bus/slot */ > > if (pdev->bus->number == it->options[0] && > > - PCI_SLOT(pdev->devfn) == it->options[1]) { > > - /* matches requested bus/slot */ > > + PCI_SLOT(pdev->devfn) == it->options[1]) > > break; > > - } > > - } else { > > - /* no bus/slot specified */ > > - break; > > + pci_dev_put(pdev); > > + pdev = NULL; > > } > > } > > devpriv->pdev = pdev; > > > This patch looks buggy. It's changing the logic beyond just checking for > subvendor/subdevice ids. >
That's the intention here, so that it avoids someone adding a new pci id without specifying either subvendor or subdevice id for 0x1131:0x7146 boards, but yes there will be a problem if boards with vendor:id not equal to 0x1131:0x7146 appear in future, as you will be obliged to add subvendor:subdevice id even if not needed.
If not wanted and it gone too far, I can revert to use the same logic as pci_match_id, or just simplify this in case it's unlikely more s626 boards appear.
The current situation is ugly, comedi subsystem could have a better way to deal with hotplug and probe of devices, without you having to reimplement what pci subsystem functions already does.
-- []'s Herton -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |